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

Fix: Error wrap may panic by nil error #179

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

fyInALT
Copy link
Contributor

@fyInALT fyInALT commented Apr 4, 2024

Motivation

Note in:

func (ar *AvsRegistryServiceChainCaller) getOperatorPubkeys(ctx context.Context, operatorId types.OperatorId) (types.OperatorPubkeys, error) {
	operatorAddr, err := ar.AvsRegistryReader.GetOperatorFromId(&bind.CallOpts{Context: ctx}, operatorId)
	if err != nil {
		return types.OperatorPubkeys{}, types.WrapError(errors.New("Failed to get operator address from pubkey hash"), err)
	}
	pubkeys, ok := ar.operatorPubkeysService.GetOperatorPubkeys(ctx, operatorAddr)
	if !ok {
		return types.OperatorPubkeys{}, fmt.Errorf("Failed to get operator pubkeys from pubkey compendium service (operatorAddr: %v, operatorId: %v)", operatorAddr, operatorId)
	}
	return pubkeys, nil
}

if operatorPubkeysService not found the key, it will return false but the err is nil, it will cause a panic by read nil point by wrapper:

fmt.Errorf("%s: %s", mainErr.Error(), subErr.Error())

Solution

  • we need add checks in wrapper, even use nil error, will not cause a panic
  • fix the operatorPubkeysService error wrapper, the wrap is useless by the err will always nil

Copy link
Collaborator

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

LGTM. Been thinking we should just use https://pkg.go.dev/errors#Join instead of our own WrapError function. That one takes a vararg list of errors and just filters out the nil errors passed. But it prints using '\n' separators between the errors which some people don't like... not sure what's the best way here, but at least thanks for fixing these errors. :)

@samlaf samlaf merged commit 42449b9 into Layr-Labs:master Apr 4, 2024
2 of 3 checks passed
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.

2 participants