[CT-1161] add signature verification authenticator #2183
Conversation
WalkthroughThe changes introduced a new authentication system within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Authenticator
participant SignatureVerifier
participant TransactionManager
User->>Authenticator: Generate Authentication Request
Authenticator->>TransactionManager: Retrieve Signers and Signatures
TransactionManager-->>Authenticator: Return Signers and Signatures
Authenticator->>SignatureVerifier: Verify Signatures
SignatureVerifier-->>Authenticator: Return Verification Result
Authenticator-->>User: Return Authentication Result
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
CodeRabbit Configuration File (
|
| if txSigners[i].Equals(account) { | ||
| msgSignature = single.Signature | ||
| } |
There was a problem hiding this comment.
Note that I am skipping ReplayProtection here.
Osmosis's implementation only compares sequence numbers. since we have our own timestamp nonce logic, probably better to leave it out of the smart account's authenticator flows
There was a problem hiding this comment.
Does GenerateAuthenticationRequest flow get invoked in a new antehandler? And is the regular sigVerficationHandler bypassed when this authenticator is in effect? I assume yes since the regular sigVerificationHandler doesn't understand the alternative authenticator? And if yes, wouldn't we need to protect against replay here?
Lmk if I'm misunderstanding sth
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
protocol/x/accountplus/authenticator/requests.go (1)
7-16: LGTM!The
TrackRequeststruct contains all the necessary fields to track an authentication request. The field names are clear and self-explanatory.Consider adding a comment to explain the purpose of the
LocalAnytype for clarity. For example:// LocalAny represents a custom message type used for authentication requests. Msg LocalAny `json:"msg"`
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- protocol/x/accountplus/authenticator/authentication_request.go (1 hunks)
- protocol/x/accountplus/authenticator/base_test.go (1 hunks)
- protocol/x/accountplus/authenticator/iface.go (1 hunks)
- protocol/x/accountplus/authenticator/requests.go (1 hunks)
- protocol/x/accountplus/authenticator/signature_authenticator.go (1 hunks)
- protocol/x/accountplus/authenticator/signature_authenticator_test.go (1 hunks)
Additional comments not posted (26)
protocol/x/accountplus/authenticator/requests.go (1)
29-49: LGTM!The
AuthenticationRequeststruct contains all the necessary fields to sign and process an authentication request. The field names are clear and self-explanatory, and the comments provide additional clarity on the purpose of some fields.protocol/x/accountplus/authenticator/iface.go (2)
8-11: LGTM!The
InitializedAuthenticatorstruct is well-defined and serves its purpose of wrapping anAuthenticatorinstance with a unique identifier.
15-51: LGTM!The
Authenticatorinterface is well-designed and comprehensively covers the essential methods required for authentication processes within a blockchain context. The interface methods enable robust transaction verification, management of gas consumption, and ensure the integrity and security of account-authenticator relationships.protocol/x/accountplus/authenticator/signature_authenticator.go (7)
42-44: LGTM!The function correctly initializes a new
SignatureVerificationinstance with the providedAccountKeeper.
47-53: LGTM!The function correctly initializes the
PubKeyfield based on the providedconfigbyte slice. It handles the case when theconfiglength is not equal to the expectedsecp256k1.PubKeySizeby setting thePubKeytonil.
57-80: LGTM!The function correctly verifies the signature against the transaction data. It consumes gas for the signature verification and handles various scenarios such as simulations, re-checks, and missing public keys. The error messages provide useful information for debugging signature verification failures.
90-105: LGTM!The function correctly validates the public key size when an authenticator is added. It allows users to pass no data or a valid public key for signature verification. The error message provides clear information about the expected and actual public key sizes.
32-34: LGTM!The function correctly returns the type of the authenticator as
SignatureVerificationType.
36-39: LGTM!The function correctly returns 0 as the static gas cost. The comment provides clarity on how the gas is consumed based on the public key type in the
Authenticatefunction.
27-30: LGTM!The
SignatureVerificationstruct correctly defines the necessary fields for a signature authenticator. TheAccountKeeperfield is used for managing account-related operations, and thePubKeyfield is used for signature verification. The struct implements theAuthenticatorinterface, ensuring compatibility with the authentication system.protocol/x/accountplus/authenticator/base_test.go (5)
25-35: LGTM!The
BaseAuthenticatorSuitestruct is well-defined and contains all the necessary fields for setting up and running tests. The field names and types are appropriate, making the purpose of each field clear.
37-80: LGTM!The
SetupKeysmethod correctly sets up the necessary test accounts and keys. It follows a clear sequence of steps for initializing the test environment, including decoding private keys, generating public keys, populating the account keeper, and initializing the test app with a custom genesis state. The implementation is well-structured and free of any apparent issues.
82-122: LGTM!The
GenSimpleTxmethod correctly generates a simple transaction with the provided messages and signers. It retrieves the account numbers and sequences for the signers from the account keeper, handling the case where an account does not exist by creating a new base account. The transaction is properly generated using theGenTxfunction with the provided parameters. The implementation is well-structured and free of any apparent issues.
124-174: LGTM!The
GenSimpleTxWithSelectedAuthenticatorsmethod correctly generates a simple transaction with the provided messages, signers, and selected authenticators. It retrieves the account numbers and sequences for the signers from the account keeper and creates a base transaction builder using theMakeTxBuilderfunction with the provided parameters. The selected authenticators are properly set as a non-critical extension option in the transaction builder. The implementation is well-structured and free of any apparent issues.
176-180: LGTM!The
FundAccmethod correctly funds a target account with the specified amount using thetestutil.FundAccountfunction. It properly asserts that no error occurs during the funding process. The function is concise and serves its purpose well, with no apparent issues in its logic or implementation.protocol/x/accountplus/authenticator/authentication_request.go (4)
59-98: LGTM!The function correctly retrieves signers and signatures from the transaction and validates their lengths. The error handling looks good.
101-120: LGTM!The function correctly retrieves the signer data for the given account. The logic for handling the genesis block and retrieving the account number and sequence looks good.
124-156: LGTM!The function correctly extracts the explicit transaction data from the provided transaction and signer data. The error handling for the type assertions and message encoding looks good.
192-284: LGTM!The function correctly generates an
AuthenticationRequestfor the provided transaction. The logic for retrieving the signers, signatures, signer data, explicit transaction data, and sign bytes looks good. The handling of thesimulateflag and the building of theAuthenticationRequeststruct are also correct.protocol/x/accountplus/authenticator/signature_authenticator_test.go (7)
1-23: LGTM!The import statements are correctly formatted, organized, and relevant to the functionality of the test file. There are no unused imports.
25-29: LGTM!The test suite struct definition is correctly formatted and follows the common pattern of embedding a base suite. The
SigVerificationAuthenticatorfield is of the correct type for testing.
31-49: LGTM!The test function and setup/teardown methods are correctly defined. The
SetupTestmethod properly initializes theSigVerificationAuthenticatorfor testing, and theTearDownTestmethod ensures cleanup of the test environment.
51-66: LGTM!The struct definitions for
SignatureVerificationTestDataandSignatureVerificationTestare correctly formatted. The fields inSignatureVerificationTestDatacover various scenarios for testing signature verification, and theSignatureVerificationTeststruct provides a convenient way to define test cases.
68-314: LGTM!The test function
TestSignatureAuthenticatoris well-structured and follows the table-driven testing approach. The test cases cover various scenarios for signature verification, including successful and unsuccessful cases, single and multiple signers, and different combinations of messages and signatures.The test logic is comprehensive and checks for the expected outcomes based on the test case data. The use of helper functions
GenTxandGenerateAuthenticationRequestenhances the readability and maintainability of the test code.Overall, the test function provides thorough coverage for the signature verification functionality.
316-386: LGTM!The helper function
MakeTxBuilderis well-implemented and correctly creates a transaction builder with the provided messages, fees, gas, and signatures. The function properly sets up the signature data and populates the transaction builder with the necessary information.The function also handles the signing of the transaction using the provided private keys, ensuring that the transaction is properly signed. The function returns the transaction builder and any error encountered during the process, providing a convenient way to create signed transactions for testing purposes.
Overall, the
MakeTxBuilderfunction is a useful utility for generating signed transactions in the test suite.
388-406: LGTM!The helper function
GenTxis a convenient utility for generating signed mock transactions. It leverages theMakeTxBuilderfunction to create a transaction builder with the provided parameters, ensuring that the transaction is properly constructed and signed.The function retrieves the signed transaction from the transaction builder using the
GetTxmethod and returns it along with any error encountered during the process. This makes it easy to generate signed transactions for testing purposes.Overall, the
GenTxfunction is a useful addition to the test suite, simplifying the process of creating signed mock transactions.
| type ConfirmExecutionRequest struct { | ||
| AuthenticatorId string `json:"authenticator_id"` | ||
| Account sdk.AccAddress `json:"account"` | ||
| FeePayer sdk.AccAddress `json:"fee_payer"` | ||
| FeeGranter sdk.AccAddress `json:"fee_granter,omitempty"` | ||
| Fee sdk.Coins `json:"fee"` | ||
| Msg LocalAny `json:"msg"` | ||
| MsgIndex uint64 `json:"msg_index"` | ||
| AuthenticatorParams []byte `json:"authenticator_params,omitempty"` | ||
| } |
There was a problem hiding this comment.
Consider merging TrackRequest and ConfirmExecutionRequest into a single struct.
The ConfirmExecutionRequest struct is identical to TrackRequest in terms of fields and their types. Having two identical structs with different names may lead to confusion and duplication of code.
Consider merging TrackRequest and ConfirmExecutionRequest into a single struct to avoid duplication. For example:
type AuthenticationRequest struct {
AuthenticatorId string `json:"authenticator_id"`
Account sdk.AccAddress `json:"account"`
FeePayer sdk.AccAddress `json:"fee_payer"`
FeeGranter sdk.AccAddress `json:"fee_granter,omitempty"`
Fee sdk.Coins `json:"fee"`
Msg LocalAny `json:"msg"`
MsgIndex uint64 `json:"msg_index"`
AuthenticatorParams []byte `json:"authenticator_params,omitempty"`
RequestType string `json:"request_type"` // "track" or "confirm_execution"
}If there are specific reasons for keeping them separate, add comments to explain the difference in their purpose.
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
TrackRequest,ConfirmExecutionRequest, andAuthenticationRequest.SignatureVerificationauthenticator to validate transaction signatures.Bug Fixes
Tests
authenticatormodule andSignatureVerificationfunctionality, ensuring reliability and robustness in authentication processes.