feat(x/auth/ante): allow custom verifyIsOnCurve when validate tx for public key #23128
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new feature in the Cosmos SDK that allows for a custom Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
mmsqe
requested review from
facundomedica,
testinginprod,
aaronc and
a team
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
x/auth/ante/sigverify.go (3)
72-76: Consider renaming or expanding comment for clarity.
Having a field namedextraVerifyIsOnCurvemight cause confusion as to when and how this verification is applied or bypassed. Consider adding a clarifying comment or renaming to something likecustomVerifyIsOnCurveto highlight its optional usage.
83-89: Ensure function pointer usage is documented.
While this constructor clearly injects the optional verification function, consider adding a doc comment explaining the expected return contract for(bool, error)—particularly howboolindicates whether the check is “handled” or should fall back to the default.
159-168: Validate behavior when the custom function partially handles the check.
This logic bypasses the built-inverifyIsOnCurveonly if the custom function returns(true, err), which might be easy to misunderstand. Adding a doc comment or an explicit code comment clarifying the expected usage and meaning ofhandledin the returned tuple can help future maintainers.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)x/auth/ante/sigverify.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
x/auth/ante/sigverify.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (3)
x/auth/ante/sigverify.go (2)
80-82: Reusing the new constructor is good.
This approach avoids code duplication by simply redirecting to the more general constructor. No issues found here.
436-436: Good to see centralized curve verification.
Invoking svd.VerifyIsOnCurve(txPubKey) ensures all relevant checks, including any custom logic, are properly applied. This approach is consistent with the single-responsibility principle.
CHANGELOG.md (1)
Line range hint 1-2000: LGTM! The CHANGELOG.md follows best practices
The changelog is well-structured and follows best practices:
- Clear categorization of changes (Features, Improvements, Bug Fixes etc.)
- Consistent formatting throughout
- Proper linking to PRs and issues
- Detailed descriptions of changes
- Good organization by version numbers
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/auth/ante/sigverify.go (1)
79-90: Consider adding validation for the verifyFn parameter.While the implementation is correct, it would be beneficial to add validation for the
verifyFnparameter to ensure it behaves consistently when called.func NewSigVerificationDecoratorWithVerifyOnCurve(ak AccountKeeper, signModeHandler *txsigning.HandlerMap, sigGasConsumer SignatureVerificationGasConsumer, aaKeeper AccountAbstractionKeeper, verifyFn func(pubKey cryptotypes.PubKey) (bool, error)) SigVerificationDecorator { + if verifyFn != nil { + // Validate the function with a nil pubkey to ensure it handles edge cases + if _, err := verifyFn(nil); err != nil && !errors.Is(err, sdkerrors.ErrInvalidPubKey) { + panic(fmt.Sprintf("verifyFn must handle nil pubkey gracefully: %v", err)) + } + } return SigVerificationDecorator{ aaKeeper: aaKeeper, ak: ak, signModeHandler: signModeHandler, sigGasConsumer: sigGasConsumer, extraVerifyIsOnCurve: verifyFn, } }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)x/auth/ante/sigverify.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (1)
x/auth/ante/sigverify.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (3)
x/auth/ante/sigverify.go (3)
72-76: LGTM! Well-structured decorator with clear field declarations.The addition of
extraVerifyIsOnCurveas an optional function field provides good flexibility for custom curve verification while maintaining backward compatibility.
Line range hint
432-436: LGTM! Proper integration of curve verification in setPubKey.The
VerifyIsOnCurvecheck is correctly placed before setting the public key in the account.
Line range hint
114-163: Verify error handling in recursive multisig verification.The implementation of
VerifyIsOnCurveis thorough, but there's a potential issue in the multisig handling where we break early on the first error without collecting all validation errors.Consider collecting all validation errors for better debugging:
case multisig.PubKey: pubKeysObjects := typedPubKey.GetPubKeys() - ok := true + var errs []error for _, pubKeyObject := range pubKeysObjects { if err := svd.VerifyIsOnCurve(pubKeyObject); err != nil { - ok = false - break + errs = append(errs, err) } } - if !ok { - return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "some keys are not on curve") + if len(errs) > 0 { + return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, fmt.Sprintf("invalid keys: %v", errs)) }
Description
make verify with ethsecp256k1 easier
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
verifyIsOnCurvefunction when validating transactions for public keys likeethsecp256k1Improvements