[CT-1198] Add smart account and circuit break ante handler #2325
Conversation
WalkthroughThe changes introduce several new components and functionalities to the authentication system within the protocol. Key additions include new constants for tracking authenticator metrics, the implementation of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 18
Outside diff range and nitpick comments (13)
protocol/x/accountplus/types/errors.go (1)
21-25: LGTM! Consider a minor improvement to the error message.The new error
ErrSmartAccountNotActiveis well-defined and consistent with the existing error structure. The error code (4) correctly follows the sequence of existing error codes.Consider slightly modifying the error message for improved clarity:
- "Smart account is not active", + "Smart account is not active or not properly initialized",This change provides more context about potential reasons for the error, which could be helpful for debugging.
protocol/x/accountplus/types/codec.go (1)
10-13: LGTM: New interface looks good, but needs documentation.The
AuthenticatorTxOptionsinterface is well-defined and aligns with the PR objectives. However, it would be beneficial to add documentation comments explaining the purpose of this interface and its method.Consider adding documentation comments like this:
// AuthenticatorTxOptions defines the interface for transaction options related to authenticators. type AuthenticatorTxOptions interface { // GetSelectedAuthenticators returns a slice of authenticator IDs that are selected for the transaction. GetSelectedAuthenticators() []uint64 }protocol/x/accountplus/lib/lib.go (1)
10-15: LGTM: Function signature is well-defined. Consider enhancing documentation.The function signature is clear and appropriate. However, to improve maintainability and usability, consider adding a more detailed godoc comment that includes:
- A brief description of each parameter
- An explanation of the return values
- Any potential error cases or side effects
Here's a suggested enhancement for the documentation:
// HasSelectedAuthenticatorTxExtensionSpecified checks if the transaction has the correct // extension for the authenticator flow. // // Parameters: // - tx: The transaction to check // - accountPlusKeeper: The keeper for accessing account-related data // // Returns: // - bool: true if the transaction has the correct extension, false otherwise // - types.AuthenticatorTxOptions: The authenticator options if present, nil otherwise // // Note: This function returns false to indicate that the authenticator flow should continue.protocol/lib/metrics/metric_keys.go (1)
92-96: Enhance clarity and consistency of new authenticator metricsThe new constants for authenticator metrics are a good addition and align with the PR objectives. However, there are a few suggestions for improvement:
The comment "Account plus" could be more descriptive. Consider changing it to "Authenticator metrics" or "Smart account metrics" for clarity.
For consistency with the guidelines mentioned at the top of the file, consider adding a unit suffix to the
AuthenticatorDecoratorAnteHandleLatencyconstant. For example:AuthenticatorDecoratorAnteHandleLatencyMs = "authenticator_decorator_ante_handle_latency_ms"To provide more context, you might want to add brief comments explaining the purpose of each new metric, especially for
MissingRegisteredAuthenticatorandAuthenticatorTrackFailed.Here's a suggested revision:
// Authenticator metrics AuthenticatorDecoratorAnteHandleLatencyMs = "authenticator_decorator_ante_handle_latency_ms" MissingRegisteredAuthenticator = "missing_registered_authenticator" AuthenticatorTrackFailed = "authenticator_track_failed"Consider adding brief comments above each constant to explain their specific purposes.
protocol/x/accountplus/ante/circuit_breaker_test.go (3)
77-77: Correct the application name in the commentThe comment mentions "Initialize the Osmosis application," which seems out of context for this codebase. Update the comment to accurately reflect the application being initialized.
Apply this diff to correct the comment:
-// Initialize the Osmosis application +// Initialize the test application
174-174: Fix grammatical error in commentThe comment should read "there is no selected authenticator" instead of "there is not selected authenticator".
Apply this diff to correct the grammar:
-// Here we test when smart accounts are active and there is not selected authenticator +// Here we test when smart accounts are active and there is no selected authenticator
190-190: Correct the comment for clarityThe comment has a minor grammatical error. It should read "Test if smart accounts are active and the authenticator flow is selected".
Apply this diff to improve the comment:
-// Test is smart accounts are active and the authenticator flow is selected +// Test if smart accounts are active and the authenticator flow is selectedprotocol/x/accountplus/keeper/authenticators.go (2)
116-117: Improve clarity and fix grammatical error in comment.The comment on line 116 has a grammatical error and could be rephrased for better clarity. It currently reads:
// GetAuthenticatorExtension unpacks the extension for the transaction, this is used with transactions specify // an authenticator to useConsider revising it as:
-// GetAuthenticatorExtension unpacks the extension for the transaction, this is used with transactions specify -// an authenticator to use +// GetAuthenticatorExtension unpacks the transaction extensions to find an AuthenticatorTxOptions. +// This is used with transactions that specify an authenticator to use.
174-174: Correct typo in log message for professionalism.There's a typo in the log message on line 174. The word "asscoicated" should be "associated".
- "account asscoicated authenticator not registered in manager", + "account associated authenticator not registered in manager",protocol/x/accountplus/ante/ante_test.go (4)
70-82: Initialization of Slice Fields Before AppendingThe slices
s.TestPrivKeysands.TestAccAddressare being appended to without prior initialization, which is acceptable but may reduce code readability.Consider initializing the slices explicitly before appending:
// Set up test accounts -accounts := make([]sdk.AccountI, 0) +accounts := []sdk.AccountI{} +s.TestPrivKeys = []*secp256k1.PrivKey{} +s.TestAccAddress = []sdk.AccAddress{} for _, key := range s.TestKeys {
108-110: Unnecessary Use ofHomeDirand Cleanup inTearDownTestThe
HomeDirfield is assigned but not utilized elsewhere in the code. Additionally,os.RemoveAll(s.HomeDir)may not be necessary.If
HomeDiris not required, consider removing it and the associated cleanup:-type AuthenticatorAnteSuite struct { - // ... - HomeDir string -} +type AuthenticatorAnteSuite struct { + // ... +} func (s *AuthenticatorAnteSuite) TearDownTest() { - os.RemoveAll(s.HomeDir) + // No cleanup necessary }
232-233: Typographical Error in Method NameTestFeePayerGasComsumptionThe method name
TestFeePayerGasComsumptioncontains a typo: "Comsumption" should be "Consumption".Correct the method name for clarity:
-func (s *AuthenticatorAnteSuite) TestFeePayerGasComsumption() { +func (s *AuthenticatorAnteSuite) TestFeePayerGasConsumption() {
295-380: RedundantnameField in Test Cases ofTestSpecificAuthenticatorThe
namefield inside each test case is not utilized since the test name is derived from the map keys.Remove the unnecessary
namefield to simplify the test cases:testCases := map[string]struct { - name string senderKey cryptotypes.PrivKey // ... }{ - "Correct authenticator 0": { - name: "Correct authenticator 0", + "Correct authenticator 0": { senderKey: s.TestPrivKeys[0], // ... }, // Other test cases... }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- protocol/lib/metrics/metric_keys.go (1 hunks)
- protocol/x/accountplus/ante/ante.go (1 hunks)
- protocol/x/accountplus/ante/ante_test.go (1 hunks)
- protocol/x/accountplus/ante/circuit_breaker.go (1 hunks)
- protocol/x/accountplus/ante/circuit_breaker_test.go (1 hunks)
- protocol/x/accountplus/keeper/authenticators.go (2 hunks)
- protocol/x/accountplus/lib/lib.go (1 hunks)
- protocol/x/accountplus/types/codec.go (1 hunks)
- protocol/x/accountplus/types/errors.go (1 hunks)
Additional comments not posted (6)
protocol/x/accountplus/types/codec.go (3)
6-7: LGTM: New imports are appropriate for the changes.The added imports for
msgserviceandtxfrom the Cosmos SDK are necessary and correctly used in the subsequent code changes.
24-24: LGTM: Message service descriptor registration added.The addition of
msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc)is correct and follows Cosmos SDK module practices. However, we should verify the existence and correct definition of_Msg_serviceDesc.Please run the following script to verify the
_Msg_serviceDescvariable:#!/bin/bash # Description: Verify the existence and correct definition of _Msg_serviceDesc # Test 1: Search for the definition of _Msg_serviceDesc echo "Searching for _Msg_serviceDesc definition:" rg --type go "_Msg_serviceDesc\s*=\s*" # Test 2: Check if _Msg_serviceDesc is of the correct type echo "Checking _Msg_serviceDesc type:" ast-grep --lang go --pattern 'var _Msg_serviceDesc = $$$grpc.ServiceDesc$$$'
18-23: LGTM: Interface registrations look correct, but need verification.The registrations for
tx.TxExtensionOptionIandAuthenticatorTxOptionsare correctly implemented. However, we should verify that theTxExtensionstruct properly implements both interfaces.Please run the following script to verify the
TxExtensionstruct implementation:protocol/x/accountplus/lib/lib.go (1)
3-8: LGTM: Imports are appropriate and concise.The imported packages are relevant to the function's purpose, including SDK types, auth ante, and local project packages.
protocol/x/accountplus/ante/circuit_breaker_test.go (1)
106-115: Consider invoking thenextAnteHandler inMockAnteDecoratorIn the
AnteHandlemethod ofMockAnteDecorator, thenextAnteHandler is not called. Typically, an AnteDecorator should invoke thenexthandler to ensure the entire chain executes properly. If skippingnextis intentional for this test, please ensure that it's documented.If it's appropriate to call
next, apply this diff:func (m MockAnteDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler, ) (newCtx sdk.Context, err error) { prio := ctx.Priority() if m.Called == 1 { - return ctx.WithPriority(prio + 1), nil + ctx = ctx.WithPriority(prio + 1) } else { - return ctx.WithPriority(prio + 2), nil + ctx = ctx.WithPriority(prio + 2) } + return next(ctx, tx, simulate) }protocol/x/accountplus/ante/ante_test.go (1)
382-412: Potential Misalignment Between Test Case Descriptions and AssertionsEnsure that each test case in
TestSpecificAuthenticatorclearly reflects its purpose and that assertions align with the expected outcomes.Review the test cases to confirm they accurately test the specified scenarios and adjust descriptions if necessary.
| func (s *AuthenticatorAnteSuite) SetupTest() { | ||
| // Test data for authenticator signature verification | ||
| TestKeys := []string{ | ||
| "6cf5103c60c939a5f38e383b52239c5296c968579eec1c68a47d70fbf1d19159", | ||
| "0dd4d1506e18a5712080708c338eb51ecf2afdceae01e8162e890b126ac190fe", | ||
| "49006a359803f0602a7ec521df88bf5527579da79112bb71f285dd3e7d438033", | ||
| } | ||
|
|
||
| s.HomeDir = fmt.Sprintf("%d", rand.Int()) |
There was a problem hiding this comment.
Variable Shadowing of TestKeys in SetupTest
The local variable TestKeys declared on line 56 shadows the TestKeys field of the AuthenticatorAnteSuite struct. This could lead to confusion or unintended behavior.
Consider renaming the local variable or assigning the keys directly to s.TestKeys:
-func (s *AuthenticatorAnteSuite) SetupTest() {
- // Test data for authenticator signature verification
- TestKeys := []string{
+func (s *AuthenticatorAnteSuite) SetupTest() {
+ // Test data for authenticator signature verification
+ s.TestKeys = []string{Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *AuthenticatorAnteSuite) SetupTest() { | |
| // Test data for authenticator signature verification | |
| TestKeys := []string{ | |
| "6cf5103c60c939a5f38e383b52239c5296c968579eec1c68a47d70fbf1d19159", | |
| "0dd4d1506e18a5712080708c338eb51ecf2afdceae01e8162e890b126ac190fe", | |
| "49006a359803f0602a7ec521df88bf5527579da79112bb71f285dd3e7d438033", | |
| } | |
| s.HomeDir = fmt.Sprintf("%d", rand.Int()) | |
| func (s *AuthenticatorAnteSuite) SetupTest() { | |
| // Test data for authenticator signature verification | |
| s.TestKeys = []string{ | |
| "6cf5103c60c939a5f38e383b52239c5296c968579eec1c68a47d70fbf1d19159", | |
| "0dd4d1506e18a5712080708c338eb51ecf2afdceae01e8162e890b126ac190fe", | |
| "49006a359803f0602a7ec521df88bf5527579da79112bb71f285dd3e7d438033", | |
| } | |
| s.HomeDir = fmt.Sprintf("%d", rand.Int()) |
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
<details>
Files selected for processing (3)
- protocol/x/accountplus/ante/circuit_breaker.go (1 hunks)
- protocol/x/accountplus/ante/circuit_breaker_test.go (1 hunks)
- protocol/x/accountplus/lib/lib.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/accountplus/ante/circuit_breaker.go
- protocol/x/accountplus/ante/circuit_breaker_test.go
Additional comments not posted (2)
protocol/x/accountplus/lib/lib.go (2)
3-9: LGTM: Import statements are appropriate and concise.The import statements are well-organized and include all necessary packages for the implemented functionality. There are no unused imports.
1-43: Overall assessment: Good implementation with room for minor improvements.The new
lib.gofile introduces two well-structured functions that correctly implement the logic for handling authenticator extensions in transactions. The code is generally well-written and follows good practices.Key points:
- Both functions,
HasSelectedAuthenticatorTxExtensionSpecifiedandGetAuthenticatorExtension, are logically sound and serve their intended purposes.- The suggested improvements focus on enhancing error logging and adding inline comments, which will improve the code's maintainability and debuggability.
- Once these minor enhancements are implemented, the code will be more robust and easier to maintain.
Great job on the implementation! Please consider the suggested improvements to further enhance the code quality.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
protocol/x/accountplus/ante/ante.go (1)
48-230: Consider refactoring AnteHandle for improved readabilityThe
AnteHandlemethod is quite long and complex. Consider breaking it down into smaller, well-named helper functions to improve readability and maintainability. This would make the overall flow easier to understand and test.LGTM: Consistent error handling
The error handling in this method is now consistent, addressing a previous review comment. Good job on maintaining consistency in returning the context with errors.
Verification: Osmosis reference removed
The comment referring to Osmosis has been removed as suggested in a previous review, improving the clarity and relevance of the documentation.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- protocol/x/accountplus/ante/ante.go (1 hunks)
- protocol/x/accountplus/ante/ante_test.go (1 hunks)
- protocol/x/accountplus/ante/circuit_breaker.go (1 hunks)
- protocol/x/accountplus/ante/circuit_breaker_test.go (1 hunks)
- protocol/x/accountplus/authenticator/signature_authenticator.go (0 hunks)
- protocol/x/accountplus/keeper/authenticators.go (2 hunks)
Files not reviewed due to no reviewable changes (1)
- protocol/x/accountplus/authenticator/signature_authenticator.go
Files skipped from review as they are similar to previous changes (4)
- protocol/x/accountplus/ante/ante_test.go
- protocol/x/accountplus/ante/circuit_breaker.go
- protocol/x/accountplus/ante/circuit_breaker_test.go
- protocol/x/accountplus/keeper/authenticators.go
Additional comments not posted (4)
protocol/x/accountplus/ante/ante.go (4)
1-31: LGTM: Package declaration, imports, and struct definitionThe package declaration, imports, and
AuthenticatorDecoratorstruct definition look appropriate for the intended functionality.
33-46: LGTM: NewAuthenticatorDecorator functionThe
NewAuthenticatorDecoratorfunction correctly initializes and returns a new instance ofAuthenticatorDecoratorwith all necessary fields.
232-262: LGTM: ValidateAuthenticatorFeePayer methodThe
ValidateAuthenticatorFeePayermethod correctly enforces that the transaction fee payer is not manually set and is the first signer of the transaction. The error messages are clear and informative.
264-299: LGTM: GetSelectedAuthenticators methodThe
GetSelectedAuthenticatorsmethod correctly retrieves the selected authenticators from the transaction extension and ensures that their number matches the number of messages. The error handling is appropriate, with clear and informative error messages.
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
Release Notes
New Features
AuthenticatorDecoratorto manage authentication logic during transaction processing.CircuitBreakerDecoratorfor dynamic transaction routing based on circuit breaker status.Bug Fixes
Tests
AuthenticatorDecoratorandCircuitBreakerDecorator, ensuring robust functionality and error handling.Documentation