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

Make the signer constructor return an error if it fails #1190

Merged
merged 5 commits into from
Jan 31, 2025

Conversation

litt3
Copy link
Contributor

@litt3 litt3 commented Jan 30, 2025

Why are these changes needed?

  • The constructor for the LocalSigner indicates an error through a fatal log instead of an error, for some reason
  • This PR modifies the constructor to follow our standard pattern of returning an error if something fails

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@litt3 litt3 self-assigned this Jan 30, 2025
@litt3 litt3 marked this pull request as ready for review January 30, 2025 20:45
Copy link
Contributor

@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 after you change the %v to %w (those have different semantics)

privateKeyBytes := common.FromHex(privateKeyHex)
privateKey, err := crypto.ToECDSA(privateKeyBytes)
if err != nil {
log.Fatalf("Failed to parse private key: %v", err)
return nil, fmt.Errorf("create ECDSA private key: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("create ECDSA private key: %v", err)
return nil, fmt.Errorf("create ECDSA private key: %w", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gah, once again! Fixed fc4e34ebc

return nil, fmt.Errorf("invalid length for signer private key")
signer, err := auth.NewLocalBlobRequestSigner(payloadDispCfg.SignerPaymentKey)
if err != nil {
return nil, fmt.Errorf("new local blob request signer: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("new local blob request signer: %v", err)
return nil, fmt.Errorf("new local blob request signer: %w", err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@litt3 litt3 merged commit a1fceb9 into Layr-Labs:master Jan 31, 2025
8 checks passed
@litt3 litt3 deleted the signer-constructor-error branch January 31, 2025 18:32
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