-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: common error interface for elcontracts chainReader #477
base: dev
Are you sure you want to change the base?
refactor: common error interface for elcontracts chainReader #477
Conversation
import "fmt" | ||
|
||
type Error struct { | ||
code int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about creating a type of "enum" for the errors codes? Maybe it would be more expressive?
I think that we can do something like this:
type ErrorCode int
const (
ErrCodeBindingError ErrorCode = 0
ErrCodeMissingContract
ErrCodeNestedError
)
Do you think this is a good option? If not, we can use the current implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought about leaving a comment or something in the code that clarifies what type of error each ID is equivalent to, but I would prefer not to do so until I clearly define what error each one is equivalent to (it could change)
chainio/clients/elcontracts/error.go
Outdated
message string | ||
description string | ||
cause error | ||
// metadata map[string]interface{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I left it just in case but could not find any use. Removed in 6978c58
LGTM! Just left some comments |
Co-authored-by: Damian Ramirez <daramirez@frba.utn.edu.ar>
chainio/clients/elcontracts/error.go
Outdated
return e.cause | ||
} | ||
|
||
func CreateErrorForMissingContract(contractName string) Error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename these functions to MissingContractError
, BindingError
, XError
, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -40,7 +38,7 @@ type ChainReader struct { | |||
ethClient eth.HttpBackend | |||
} | |||
|
|||
var errLegacyAVSsNotSupported = errors.New("method not supported for legacy AVSs") | |||
var errLegacyAVSsNotSupported = Error{3, "Other errors", "Method not supported for legacy AVSs", nil} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's introduce another function for this kind of "Other errors", although we could probably find a category to move it to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I thought about renaming some of them to "logic errors", but preferred to keep "others" for now.
0, | ||
"Binding error", | ||
errDescription, | ||
errorCause, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the full syntax here:
0, | |
"Binding error", | |
errDescription, | |
errorCause, | |
code: 0, | |
message: "Binding error", | |
description: errDescription, | |
cause: errorCause, |
@@ -140,9 +166,11 @@ func (r *ChainReader) GetOperatorDetails( | |||
) | |||
// This call should not fail since it's a getter | |||
if err != nil { | |||
return types.Operator{}, err | |||
wrappedError := CreateForBindingError("delegationManager.DelegationApprover", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make it "DelegationManager.delegationApprover" following the solidity casing.
assert.Equal( | ||
t, | ||
err.Error(), | ||
"Binding error(0) - Error happened while calling token contract: no contract code at given address", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use ErrorContains
here.
|
||
_, err = chainReader.GetStrategiesForOperatorSet(ctx, operatorSet) | ||
require.Error(t, err) | ||
assert.Equal(t, err.Error(), "Other errors(3) - Method not supported for legacy AVSs") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should expose this as a variable.
Do not merge
What Changed?
The idea is to create a common interface to all SDK errors. The structure of this common interface is currently:
The code is associated with the message, which could change in the future, such that one of both must be kept and the other must be replaced or transformed.
One use example could be the following:
So, in this use example the error message would be the following:
"Missing needed contract(1) - DelegationManager contract not provided"
Reviewer Checklist