Skip to content
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

Open
wants to merge 28 commits into
base: dev
Choose a base branch
from

Conversation

maximopalopoli
Copy link
Contributor

@maximopalopoli maximopalopoli commented Jan 27, 2025

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:

type Error struct {
	code        int
	message     string
	description string
	cause       error
}

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:

func (r *ChainReader) IsOperatorRegistered(
	ctx context.Context,
	operator types.Operator,
) (bool, error) {
	if r.delegationManager == nil {
		wrappedError := CreateErrorForMissingContract("DelegationManager")
		return false, wrappedError
	}
...
}

func CreateErrorForMissingContract(contractName string) Error {
	errDescription := fmt.Sprintf("%s contract not provided", contractName)
	return Error{1, "Missing needed contract", errDescription, nil}
}

So, in this use example the error message would be the following:

"Missing needed contract(1) - DelegationManager contract not provided"

Reviewer Checklist

  • Code is well-documented
  • Code adheres to Go naming conventions
  • Code deprecates any old functionality before removing it

@maximopalopoli maximopalopoli marked this pull request as ready for review January 28, 2025 16:18
@maximopalopoli maximopalopoli changed the title feat: common error interface feat: common error interface for elcontracts chainReader Jan 28, 2025
@maximopalopoli maximopalopoli changed the title feat: common error interface for elcontracts chainReader refactor: common error interface for elcontracts chainReader Jan 28, 2025
maximopalopoli added a commit to lambdaclass/eigensdk-go that referenced this pull request Jan 28, 2025
import "fmt"

type Error struct {
code int
Copy link
Contributor

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.

Copy link
Contributor Author

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)

message string
description string
cause error
// metadata map[string]interface{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

Copy link
Contributor Author

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

chainio/clients/elcontracts/error.go Outdated Show resolved Hide resolved
@damiramirez
Copy link
Contributor

LGTM! Just left some comments

return e.cause
}

func CreateErrorForMissingContract(contractName string) Error {
Copy link
Contributor

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.

Copy link
Contributor Author

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}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Comment on lines +31 to +34
0,
"Binding error",
errDescription,
errorCause,
Copy link
Contributor

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:

Suggested change
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)
Copy link
Contributor

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.

Comment on lines +855 to +859
assert.Equal(
t,
err.Error(),
"Binding error(0) - Error happened while calling token contract: no contract code at given address",
)
Copy link
Contributor

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")
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants