[CT-1262] add e2e tests for new auth flow failure cases #2461
Conversation
WalkthroughThe pull request introduces a comprehensive test suite for the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (15)
protocol/x/accountplus/types/message_add_authenticator.go (1)
Tests are missing coverage for
AUTHENTICATOR_DATA_MAX_LENGTH.
- Please update the following test files to include validations for
AUTHENTICATOR_DATA_MAX_LENGTH:
./protocol/x/clob/types/message_proposed_operations_test.go./protocol/x/delaymsg/types/msg_delay_message_test.go🔗 Analysis chain
Line range hint
14-30: LGTM. Consider updating related tests.The
ValidateBasicmethod correctly uses the updatedAUTHENTICATOR_DATA_MAX_LENGTHconstant. No changes are required in the method implementation.However, we should verify if any related tests need updating due to the constant change:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tests related to ValidateBasic or AUTHENTICATOR_DATA_MAX_LENGTH rg --type go "test.*ValidateBasic|test.*AUTHENTICATOR_DATA_MAX_LENGTH" .Length of output: 334
Script:
#!/bin/bash # Extract lines using AUTHENTICATOR_DATA_MAX_LENGTH in the identified test files grep "AUTHENTICATOR_DATA_MAX_LENGTH" ./protocol/x/clob/types/message_proposed_operations_test.go grep "AUTHENTICATOR_DATA_MAX_LENGTH" ./protocol/x/delaymsg/types/msg_delay_message_test.goLength of output: 187
Script:
#!/bin/bash # Use ast-grep to find any occurrences of AUTHENTICATOR_DATA_MAX_LENGTH in the identified test files ast-grep --lang go --pattern 'AUTHENTICATOR_DATA_MAX_LENGTH' ./protocol/x/clob/types/message_proposed_operations_test.go ast-grep --lang go --pattern 'AUTHENTICATOR_DATA_MAX_LENGTH' ./protocol/x/delaymsg/types/msg_delay_message_test.goLength of output: 235
protocol/x/accountplus/types/errors.go (1)
36-67: LGTM! New error definitions are well-structured and aligned with PR objectives.The new error variables for authenticator validation failures are clearly defined and logically grouped. They provide specific error messages for various validation scenarios, which will be helpful for debugging and testing the new auth flow failure cases.
For consistency, consider adding a comment above the existing errors (before line 1) to match the style of the new errors:
// Errors for account and authenticator operationsThis would improve the overall structure and readability of the file.
protocol/x/accountplus/authenticator/message_filter.go (1)
Line range hint
56-60: Approve the error type change with a minor suggestion.The change from
sdkerrors.ErrUnauthorizedtotypes.ErrMessageTypeVerificationis a good improvement. It provides a more specific error type that accurately reflects the nature of the failure in message type verification.Consider slightly modifying the error message for clarity:
return errorsmod.Wrapf( types.ErrMessageTypeVerification, - "message types do not match. Got %s, Expected %v", + "message type not in whitelist. Got %s, Whitelist: %v", sdk.MsgTypeURL(request.Msg), m.whitelist, )This change more accurately describes the validation being performed.
protocol/x/accountplus/types/iface.go (1)
13-16: LGTM. Consider adding documentation for the new struct.The addition of
SubAuthenticatorInitDatalooks good and seems to be part of a larger feature implementation for sub-authenticators. The struct is well-designed with appropriate field types and JSON tags.To improve code clarity and maintainability, consider adding documentation comments for the struct and its fields. This will help other developers understand the purpose and usage of this new type.
Example documentation:
// SubAuthenticatorInitData contains initialization data for a sub-authenticator. type SubAuthenticatorInitData struct { // Type represents the type of the sub-authenticator. Type string `json:"type"` // Config holds the configuration data for the sub-authenticator. Config []byte `json:"config"` }protocol/x/accountplus/authenticator/signature_authenticator.go (3)
Line range hint
68-74: Improved error handling and messaging.The change from
sdkerrors.ErrUnauthorizedtotypes.ErrSignatureVerificationprovides more specific error handling, which aligns well with the PR's objective of improving failure case handling in the authentication flow. The additional context in the error message (account number, sequence, and chain-id) will be valuable for debugging signature verification issues.Consider adding a log statement before returning the error to aid in debugging:
+ ctx.Logger().Info("Signature verification failed", "account_number", request.TxData.AccountNumber, "sequence", request.TxData.AccountSequence, "chain_id", request.TxData.ChainID) return errorsmod.Wrapf( types.ErrSignatureVerification, "signature verification failed; please verify account number (%d), sequence (%d) and chain-id (%s)", request.TxData.AccountNumber, request.TxData.AccountSequence, request.TxData.ChainID, )
Line range hint
46-49: Improved handling of invalid configuration.The change to set
sva.PubKeytonilwhen the config length doesn't matchsecp256k1.PubKeySizeimproves the robustness of the initialization process. This ensures that the authenticator won't use an invalid public key, which is a good defensive programming practice.For improved clarity and to make the intention more explicit, consider restructuring the condition:
- if len(config) != secp256k1.PubKeySize { - sva.PubKey = nil + if len(config) == secp256k1.PubKeySize { + sva.PubKey = &secp256k1.PubKey{Key: config} + } else { + sva.PubKey = nil } - sva.PubKey = &secp256k1.PubKey{Key: config}This makes it clear that we're only setting a valid public key when the size is correct, and explicitly setting it to
nilotherwise.
Line range hint
1-105: Overall assessment: Changes improve error handling and robustness.The modifications in this file align well with the PR objectives of improving failure case handling in the authentication flow. The changes enhance error specificity, improve error messages, and add robustness to the initialization process. These improvements will aid in debugging and make the system more resilient to invalid inputs.
Consider adding unit tests to verify the new behavior, especially:
- The error message format in the
Authenticatemethod when signature verification fails.- The handling of invalid configuration sizes in the
Initializemethod.These tests will ensure the new behavior is maintained in future updates.
protocol/x/accountplus/authenticator/composite.go (2)
95-95: LGTM. Consider adding a test for type compatibility.The change to use
types.SubAuthenticatorInitDatais consistent with the previous modification and improves code consistency. The function's logic remains intact, suggesting compatibility with the new type.To ensure the behavior remains consistent with the new type, consider adding a unit test that covers this function with various input scenarios. Would you like assistance in generating a test case for this function?
Line range hint
1-124: Summary: Type updates align with PR objectives, consider expanding test coverage.The changes in this file, while not directly adding e2e tests for auth flow failure cases, contribute to code consistency by using shared type definitions. This aligns with the broader goal of improving the authentication flow.
To fully meet the PR objectives:
- Ensure that e2e tests for auth flow failure cases are added in appropriate test files.
- Consider expanding unit test coverage for the modified functions to verify behavior with the new type.
As you continue to refactor and improve the authentication flow, consider the following:
- Document the rationale behind using
types.SubAuthenticatorInitDatato help future maintainers understand the design decision.- Review other parts of the codebase that might benefit from similar type standardization.
- Ensure that error messages and logging are consistent across all authenticator-related functions to facilitate debugging in production environments.
protocol/x/accountplus/authenticator/subaccount_filter.go (1)
❗ Undefined Error Type
ErrSubaccountVerificationDetectedThe new error type
types.ErrSubaccountVerificationis not defined in thetypespackage. This change will cause compilation failures and disrupt existing error handling mechanisms that rely onsdkerrors.ErrUnauthorized.Action Items:
- Define
ErrSubaccountVerificationin thetypespackage.- Review and update all instances where
ErrUnauthorizedis used to ensure consistency and proper error handling.🔗 Analysis chain
Line range hint
77-81: Approve the error type change with verification.The change from
sdkerrors.ErrUnauthorizedtotypes.ErrSubaccountVerificationimproves error specificity, which is good. However, we should verify a few things:
- Ensure that
types.ErrSubaccountVerificationis properly defined in thetypespackage.- Check if this change affects error handling in other parts of the codebase that might expect
ErrUnauthorized.Let's verify these points:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the error type change and its impact. # Test 1: Check if ErrSubaccountVerification is defined in the types package echo "Checking for ErrSubaccountVerification definition:" rg --type go "var ErrSubaccountVerification = " protocol/x/accountplus/types # Test 2: Look for other occurrences of ErrUnauthorized in the codebase echo "Checking for other occurrences of ErrUnauthorized:" rg --type go "ErrUnauthorized" protocol/x/accountplus # Test 3: Check for any error handling that might be affected by this change echo "Checking for potential affected error handling:" rg --type go "(?i)unauthorized.*error" protocol/x/accountplusLength of output: 2283
protocol/x/accountplus/authenticator/all_of.go (1)
102-105: Enhanced error handling inAuthenticatemethodThe modification to wrap the original error with
types.ErrAllOfVerificationimproves error context and allows for more precise error handling. This change is beneficial for debugging and maintaining the code.However, for consistency with other error wrapping in the codebase, consider using a single line for the error wrapping:
return errorsmod.Wrapf(types.ErrAllOfVerification, "sub-authenticator failed: %v", err)This suggestion maintains the improved error context while potentially improving readability.
protocol/testutil/app/app.go (1)
274-275: UpdateGenesisDocWithAppStateForModule function updated for accountplusThe function has been correctly updated to handle the
aptypes.GenesisStatetype, which is consistent with the earlier changes.Consider adding a comment explaining the purpose of the
accountplusmodule for better code documentation.protocol/x/clob/e2e/permissioned_keys_test.go (2)
29-31: Clarify the Use of Slices for AccountNum and SeqNumThe
AccountNumandSeqNumfields inTestSdkMsgare defined as slices ([]uint64). If each message is associated with a single account and sequence number, consider usinguint64instead of[]uint64for these fields to simplify the code. If multiple accounts and sequence numbers are intended, ensure that their usage is correctly handled throughout the tests.
469-480: Conditionally Assert Response Log Based on ExpectedLogIn the assertion for the response log, if
ExpectedLogis empty, therequire.Containscheck might be unnecessary or could lead to misleading test results. Consider asserting the response log only whenExpectedLogis not empty.Apply this diff to conditionally check the response log:
require.Equal( t, msg.ExpectedRespCode, resp.Code, "Response code was not as expected", ) +if msg.ExpectedLog != "" { require.Contains( t, resp.Log, msg.ExpectedLog, "Response log was not as expected", ) +}protocol/app/app.go (1)
1236-1245: Consider adding unit tests for the new authenticatorsThe newly added authenticators (
NewAllOf,NewAnyOf,NewMessageFilter,NewClobPairIdFilter,NewSubaccountFilter) are central to the authentication flow. To ensure their correctness and prevent future regressions, it's advisable to include unit tests that cover their functionality.Would you like assistance in generating unit tests for these authenticators or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
- protocol/app/app.go (1 hunks)
- protocol/testutil/app/app.go (3 hunks)
- protocol/testutil/constants/genesis.go (1 hunks)
- protocol/x/accountplus/authenticator/all_of.go (2 hunks)
- protocol/x/accountplus/authenticator/any_of.go (2 hunks)
- protocol/x/accountplus/authenticator/clob_pair_id_filter.go (1 hunks)
- protocol/x/accountplus/authenticator/clob_pair_id_filter_test.go (2 hunks)
- protocol/x/accountplus/authenticator/composite.go (2 hunks)
- protocol/x/accountplus/authenticator/composition_test.go (4 hunks)
- protocol/x/accountplus/authenticator/message_filter.go (1 hunks)
- protocol/x/accountplus/authenticator/message_filter_test.go (2 hunks)
- protocol/x/accountplus/authenticator/signature_authenticator.go (1 hunks)
- protocol/x/accountplus/authenticator/subaccount_filter.go (1 hunks)
- protocol/x/accountplus/authenticator/subaccount_filter_test.go (2 hunks)
- protocol/x/accountplus/types/errors.go (1 hunks)
- protocol/x/accountplus/types/iface.go (1 hunks)
- protocol/x/accountplus/types/message_add_authenticator.go (1 hunks)
- protocol/x/clob/e2e/permissioned_keys_test.go (1 hunks)
🔇 Additional comments (30)
protocol/x/accountplus/types/message_add_authenticator.go (1)
7-11: Verify the impact of increasing the authenticator data length limit.The increase of
AUTHENTICATOR_DATA_MAX_LENGTHfrom 256 to 1024 allows for more complex authenticators. While the added comments explain this as a spam mitigation measure, which is good, we should consider the following:
- Have we verified that this increase doesn't negatively impact system resources or security?
- Can we document the rationale for choosing 1024 as the new limit?
To assess the impact, let's check for any other occurrences of the old limit:
Also, let's verify if there are any performance tests that need updating:
protocol/x/accountplus/types/errors.go (3)
38-67: Verify intentionality of error code numbering.I noticed that the new error codes start at 100, while existing error codes end at 6. This creates a gap in the numbering sequence.
Can you confirm if this gap is intentional? It could serve the following purposes:
- Separating different categories of errors.
- Leaving room for future error codes between 7 and 99.
If this is not intentional, consider using sequential numbering (7-12) for consistency.
Line range hint
1-67: Summary and RecommendationThe changes to
errors.goare well-structured and align with the PR objectives of adding e2e tests for auth flow failure cases. The new error variables provide specific messages for various authenticator validation failures, which will be helpful for debugging and testing.Recommendations:
- Add a comment for existing errors to improve consistency.
- Verify the intentionality of the error code numbering gap.
- Ensure the new errors are properly used in the codebase and update any relevant documentation or error handling middleware.
Overall, the changes look good, but addressing these minor points will further improve the code quality and consistency.
36-67: Verify usage of new errors and potential updates in other files.The addition of these new error types suggests changes in the authentication system. To ensure completeness:
- Verify that these new errors are being used in the relevant parts of the codebase where authenticator validation occurs.
- Check if there are any other files (e.g., error handling middleware, logging, or documentation) that need to be updated to handle or describe these new errors.
Run the following script to check for usage of the new error variables:
This will help ensure that the new errors are properly integrated into the existing error handling system.
✅ Verification successful
New error variables are being utilized correctly and appropriately handled in relevant parts of the codebase.
All added error types are integrated into the existing error handling system without issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of new error variables in the codebase # Test: Search for usage of new error variables echo "Searching for usage of new error variables:" rg --type go -e "ErrSignatureVerification" -e "ErrMessageTypeVerification" -e "ErrClobPairIdVerification" -e "ErrSubaccountVerification" -e "ErrAllOfVerification" -e "ErrAnyOfVerification" # Test: Search for potential places where error handling might need to be updated echo "Searching for potential places to update error handling:" rg --type go -e "errorsmod\.Is\(" -e "errors\.Is\(" -e "switch err \{" -e "case err =="Length of output: 9028
protocol/x/accountplus/authenticator/message_filter.go (1)
Line range hint
1-93: Verify the definition oftypes.ErrMessageTypeVerificationThe change to use
types.ErrMessageTypeVerificationin theAuthenticatemethod is well-contained and doesn't affect the overall structure or functionality of theMessageFilter. This change improves error specificity without introducing any breaking changes to the public API.To ensure the new error type is properly defined, please run the following command:
This will help confirm that the new error type is correctly defined in the
typespackage.✅ Verification successful
Verification of
types.ErrMessageTypeVerificationDefinition PassedThe
types.ErrMessageTypeVerificationerror type is correctly defined in thetypes/errors.gofile. Its usage in theAuthenticatemethod is appropriate and does not introduce any issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of types.ErrMessageTypeVerification # Test: Search for the definition of ErrMessageTypeVerification in the types package rg --type go -g 'types/*.go' 'var ErrMessageTypeVerification'Length of output: 212
Script:
#!/bin/bash # Description: Verify the definition of types.ErrMessageTypeVerification by searching the correct types directory # Search for the definition of ErrMessageTypeVerification in the types package rg --type go 'var ErrMessageTypeVerification' protocol/x/accountplus/types/Length of output: 77
Script:
#!/bin/bash # Description: Broadly search for the definition of ErrMessageTypeVerification across the entire codebase # Search for the definition of ErrMessageTypeVerification in all .go files rg 'ErrMessageTypeVerification' --type goLength of output: 578
protocol/x/accountplus/authenticator/clob_pair_id_filter_test.go (3)
13-13: LGTM: Import statement updated correctly.The import statement has been updated to include the
typespackage, which is necessary for using the new error typetypes.ErrClobPairIdVerification. This change is consistent with the removal of thesdkerrorsimport (as mentioned in the AI summary) and aligns with the updated error handling in the test.
Line range hint
1-110: Overall assessment: Changes improve error handling specificity.The modifications to this test file enhance the specificity of error handling in the authentication flow tests. The changes are consistent with the PR objectives of adding e2e tests for new auth flow failure cases. The updated import and error type usage contribute to a more precise and maintainable test suite.
108-108: LGTM: Error type updated correctly. Consider verifying error message.The error type has been appropriately updated from
sdkerrors.ErrUnauthorizedtotypes.ErrClobPairIdVerification. This change aligns with the PR objectives of improving the authentication flow and its error handling.To ensure complete alignment with the new error type, please verify that the error message associated with
types.ErrClobPairIdVerificationaccurately reflects a CLOB pair ID verification failure. You can use the following script to check the error definition:✅ Verification successful
Verified: Error type and message are correctly implemented.
The
ErrClobPairIdVerificationerror is properly defined and consistently used across the codebase. The error message accurately reflects a CLOB pair ID verification failure, aligning with the PR's objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the error message for ErrClobPairIdVerification # Test: Search for the error definition rg --type go 'ErrClobPairIdVerification.*=.*errors\.Register' -A 3Length of output: 68
Script:
#!/bin/bash # Description: Locate all occurrences and definitions of ErrClobPairIdVerification # Search for all instances of ErrClobPairIdVerification in Go files rg --type go 'ErrClobPairIdVerification' -A 5Length of output: 1877
protocol/x/accountplus/authenticator/subaccount_filter_test.go (2)
13-13: LGTM: Import changes are appropriate.The addition of the
typespackage import and the removal of thesdkerrorsimport are consistent with the changes in error handling. This modification aligns well with using a custom error type for subaccount verification.
Line range hint
1-110: Summary: Improved error handling in SubaccountFilter testsThe changes in this file enhance the specificity of error handling in the SubaccountFilter tests. By replacing the generic
sdkerrors.ErrUnauthorizedwith the more specifictypes.ErrSubaccountVerification, the tests now more accurately reflect the expected behavior of the authentication flow.These modifications align well with the PR's objective of improving the testing for authentication flow failure cases. However, it's worth noting that while the PR mentions adding e2e tests, this file focuses on unit tests.
Consider the following:
- Ensure that similar changes have been made in related files for consistency.
- If e2e tests were added as part of this PR, they should be reviewed separately.
- Update any documentation that might reference the old error type.
Overall, these changes represent a positive step towards more robust and specific error handling in the authentication flow.
protocol/x/accountplus/types/iface.go (1)
13-16: Verify integration and usage of the new struct.The
SubAuthenticatorInitDatastruct is not directly used within this file. To ensure proper integration:
- Verify that this struct is used appropriately in other parts of the codebase.
- Consider if the
Authenticatorinterface or related methods need to be updated to incorporate this new struct.- Ensure that any code that will use this new struct is also updated accordingly.
To help verify the integration, you can run the following script:
This will help identify where the new struct is being used and where it might need to be integrated.
✅ Verification successful
Integration of
SubAuthenticatorInitDataVerified Successfully.The
SubAuthenticatorInitDatastruct is appropriately integrated and utilized across the codebase, as evidenced by its usage in multiple authenticator components and tests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of SubAuthenticatorInitData in the codebase # Search for SubAuthenticatorInitData usage echo "Searching for SubAuthenticatorInitData usage:" rg --type go "SubAuthenticatorInitData" -C 3 # Search for potential places where it might be used (e.g., sub-authenticator related code) echo "\nSearching for potential usage locations:" rg --type go "sub.?authenticator" -i -C 3Length of output: 44181
protocol/x/accountplus/authenticator/message_filter_test.go (3)
15-15: LGTM: New import statement is correct and necessary.The addition of the
typespackage import is appropriate for using theErrMessageTypeVerificationerror in the test case. This change aligns with the modification in error checking.
15-15: Summary: Changes improve error specificity in authentication tests.The modifications in this file, including the new import and the updated error type, enhance the specificity of error handling in the authentication process tests. These changes align well with the PR objective of adding e2e tests for new auth flow failure cases.
While the changes appear to be correct and beneficial, it's important to ensure that they don't introduce any unintended side effects in other parts of the codebase. The verification script provided earlier will help in this regard.
Overall, these changes contribute to more precise and meaningful error handling in the authentication flow tests.
Also applies to: 124-124
124-124: LGTM: Error type change is appropriate, but verify impact.The change from
sdkerrors.ErrUnauthorizedtotypes.ErrMessageTypeVerificationis more specific and aligns with the PR objective. This improves the clarity of error handling for message type verification failures.To ensure this change doesn't negatively impact other parts of the codebase, please run the following verification:
This will help ensure that the error type change is consistent across the codebase and that no unintended side effects are introduced.
✅ Verification successful
Verified: No residual uses of
ErrUnauthorizedfound in message verification contexts.The change to
types.ErrMessageTypeVerificationis consistent across the codebase and aligns with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining uses of ErrUnauthorized in message verification contexts # and for any other uses of the new ErrMessageTypeVerification error. # Search for remaining uses of ErrUnauthorized in message verification contexts echo "Searching for remaining uses of ErrUnauthorized in message verification contexts:" rg --type go "ErrUnauthorized.*message.*verif" -g '!*_test.go' # Search for other uses of the new ErrMessageTypeVerification error echo "Searching for other uses of ErrMessageTypeVerification:" rg --type go "ErrMessageTypeVerification" -g '!*_test.go'Length of output: 597
protocol/x/accountplus/authenticator/clob_pair_id_filter.go (1)
79-79: Approve the error type change, but verify its impact.The change from
sdkerrors.ErrUnauthorizedtotypes.ErrClobPairIdVerificationimproves error specificity, which is good for debugging and error handling. However, we need to ensure this doesn't break existing error handling logic elsewhere in the codebase.Please run the following script to check for any other occurrences of
sdkerrors.ErrUnauthorizedin the codebase and verify if they need similar updates:Also, please confirm that
types.ErrClobPairIdVerificationis properly defined and documented in thetypespackage.protocol/x/accountplus/authenticator/composite.go (1)
48-48: LGTM. Verify type compatibility across the codebase.The change from a local
SubAuthenticatorInitDatatype totypes.SubAuthenticatorInitDataimproves consistency. The function's logic remains intact, suggesting compatibility with the new type.To ensure full compatibility, run the following script to check for any other usages of
SubAuthenticatorInitDatathat might need updating:✅ Verification successful
Verified: Type compatibility is consistent across the codebase.
All instances of
SubAuthenticatorInitDatanow referencetypes.SubAuthenticatorInitData, ensuring consistency and compatibility throughout the codebase. No further issues detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other usages of SubAuthenticatorInitData # Test: Search for SubAuthenticatorInitData usage rg --type go 'SubAuthenticatorInitData'Length of output: 1725
protocol/x/accountplus/authenticator/all_of.go (2)
52-52: Improved type clarity forinitDatasThe change from
[]SubAuthenticatorInitDatato[]types.SubAuthenticatorInitDataenhances code clarity by explicitly referencing thetypespackage. This modification ensures consistency with the package structure and reduces potential naming conflicts.
Line range hint
1-164: Summary of changes inall_of.goThe modifications in this file enhance the
AllOfauthenticator implementation by:
- Improving type clarity in the
Initializemethod.- Enhancing error handling in the
Authenticatemethod.These changes align with the PR objectives of improving the authentication flow and don't introduce any breaking changes. The code maintains its overall structure and functionality while making these beneficial improvements.
protocol/x/accountplus/authenticator/any_of.go (3)
63-63: LGTM: Improved type specificityThe change from
[]SubAuthenticatorInitDatato[]types.SubAuthenticatorInitDataenhances code clarity by explicitly specifying the namespace of theSubAuthenticatorInitDatatype. This modification aligns with best practices for type declarations and improves code readability.
Line range hint
1-210: Summary: Improvements in type specificity and error handlingThe changes in this file enhance the code quality by:
- Improving type specificity with the use of
types.SubAuthenticatorInitData.- Introducing a more specific error type
types.ErrAnyOfVerificationfor better error handling.These modifications align with best practices and should improve code clarity and maintainability. Ensure that the new error type is properly defined and consistently used throughout the codebase.
129-129: LGTM: Improved error specificityThe change from
sdkerrors.ErrUnauthorizedtotypes.ErrAnyOfVerificationenhances error handling by using a more specific error type for the AnyOf authenticator's verification process. This improvement allows for more precise error handling and debugging.To ensure consistency and proper implementation, please run the following script to verify the new error type:
✅ Verification successful
Verification Successful:
ErrAnyOfVerificationis properly defined and utilizedThe
ErrAnyOfVerificationerror type is correctly defined inprotocol/x/accountplus/types/errors.goand appropriately used inprotocol/x/accountplus/authenticator/any_of.go. This ensures enhanced error specificity and consistency within the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of the new ErrAnyOfVerification error type # Test 1: Check if ErrAnyOfVerification is defined in the types package echo "Checking ErrAnyOfVerification definition:" rg --type go "var ErrAnyOfVerification = " protocol/x/accountplus/types # Test 2: Check for other usages of ErrAnyOfVerification in the codebase echo "Checking ErrAnyOfVerification usage:" rg --type go "ErrAnyOfVerification" protocol/x/accountplusLength of output: 469
protocol/x/accountplus/authenticator/composition_test.go (5)
190-192: LGTM: Type update is consistent with refactoring.The change from
authenticator.SubAuthenticatorInitDatatotypes.SubAuthenticatorInitDatais consistent with the refactoring effort to centralize type definitions. This should improve the overall structure of the codebase.
347-349: LGTM: Consistent type update.This change is consistent with the previous update, changing
authenticator.SubAuthenticatorInitDatatotypes.SubAuthenticatorInitData. It's part of the same refactoring effort to centralize type definitions.
565-572: LGTM: Consistent type updates in CompositeSpyAuth.These changes in the
buildInitDatamethod ofCompositeSpyAuthare consistent with the previous updates, changingauthenticator.SubAuthenticatorInitDatatotypes.SubAuthenticatorInitData. The method logic remains unchanged, and these updates align with the overall refactoring effort to centralize type definitions.Also applies to: 580-587
904-911: LGTM: Consistent type update in marshalAuth function.This change in the
marshalAuthfunction is consistent with all previous updates, changingauthenticator.SubAuthenticatorInitDatatotypes.SubAuthenticatorInitData. The function logic remains unchanged, and this update aligns with the overall refactoring effort to centralize type definitions.
Line range hint
1-915: Summary: Consistent type updates improve code structure.The changes in this file are part of a larger refactoring effort to move type definitions from the
authenticatorpackage to a more generaltypespackage. All instances ofauthenticator.SubAuthenticatorInitDatahave been updated totypes.SubAuthenticatorInitData. These changes are consistent throughout the file and should improve the overall structure of the codebase by centralizing type definitions.The refactoring doesn't alter any logic or functionality, making it a low-risk change that enhances code organization. Great job on maintaining consistency across all affected areas!
protocol/testutil/app/app.go (2)
52-52: New import added for accountplus typesThe addition of
aptypes "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/types"suggests that theaccountplusmodule is being integrated into the testing framework.
211-212: GenesisStates interface updated to include accountplusThe
GenesisStatesinterface has been expanded to includeaptypes.GenesisState. This change allows the testing framework to handle genesis state for theaccountplusmodule.protocol/testutil/constants/genesis.go (2)
Line range hint
1-1037: LGTM, pending clarification on thedydxaccountplussectionThe addition of the
dydxaccountplussection to the genesis state appears to be properly integrated into the existing structure. The rest of theGenesisStateconstant remains unchanged, which is good for maintaining consistency with the previous version.However, this approval is contingent on receiving satisfactory answers to the questions raised about the
dydxaccountplussection in the previous comment. Once those points are clarified, we can consider this change fully reviewed and approved.
445-452: Newdydxaccountplussection added: Please provide additional contextThe new
dydxaccountplussection has been added to the genesis state. While it appears to be properly structured, there are a few points that require clarification:
- What is the purpose of the
is_smart_account_activeparameter, and what are the implications of setting it tofalsein the genesis state?- The
next_authenticator_idis initialized as a string "0". Is this intentional, or should it be an integer?- Both
accountsandauthenticator_dataare initialized as empty arrays. Is this the expected initial state, or should there be any pre-populated data?Could you please provide more context on these points and confirm if the current implementation aligns with the intended functionality of the
dydxaccountplusmodule?To ensure this new section doesn't conflict with existing configurations, let's check for any other occurrences of similar fields or structures:
| @@ -105,7 +105,7 @@ func (s *SubaccountFilterTest) TestFilter() { | |||
| if tt.match { | |||
| s.Require().NoError(err) | |||
| } else { | |||
| s.Require().ErrorIs(err, sdkerrors.ErrUnauthorized) | |||
| s.Require().ErrorIs(err, types.ErrSubaccountVerification) | |||
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Inconsistent Usage of Error Types
While the change from sdkerrors.ErrUnauthorized to types.ErrSubaccountVerification enhances error specificity, there are remaining instances of sdkerrors.ErrUnauthorized across the codebase that may need to be updated for consistency.
Please review and address the following files to ensure uniform error handling:
- protocol/x/ratelimit/util/ibc.go
- protocol/x/subaccounts/keeper/subaccount_test.go
- protocol/x/subaccounts/keeper/transfer_test.go
- protocol/x/sending/e2e/isolated_subaccount_transfers_test.go
- protocol/x/clob/keeper/liquidations_test.go
- protocol/x/clob/e2e/isolated_subaccount_orders_test.go
- protocol/x/clob/ante/clob_test.go
- protocol/x/clob/ante/clob.go
- protocol/testing/e2e/authz/authz_test.go
- protocol/x/accountplus/testutils/spy_authenticator.go
- protocol/x/accountplus/testutils/generic_authenticator.go
- protocol/x/accountplus/lib/authentication_request.go
- protocol/x/accountplus/keeper/authenticators.go
- protocol/x/accountplus/keeper/msg_server.go
- protocol/x/accountplus/ante/timestampnonce.go
- protocol/x/accountplus/authenticator/signature_authenticator.go
- protocol/x/accountplus/authenticator/composite.go
- protocol/x/accountplus/authenticator/any_of.go
- protocol/x/accountplus/authenticator/all_of.go
- protocol/x/accountplus/ante/ante.go
- protocol/app/msgs/app_test.go
- protocol/app/ante.go
- protocol/app/ante/gas_test.go
- protocol/app/ante/sigverify.go
- protocol/app/ante/msg_type_test.go
- protocol/app/ante/replay_protection.go
- protocol/app/ante/msg_type.go
- protocol/app/ante/basic_test.go
- protocol/app/ante/market_update.go
Ensuring that all instances are consistently updated will maintain code quality and prevent potential errors.
🔗 Analysis chain
LGTM: Error type change improves specificity.
The change from sdkerrors.ErrUnauthorized to types.ErrSubaccountVerification is an improvement as it provides a more specific error type for subaccount verification failures. This aligns well with best practices for error handling.
To ensure consistency across the codebase, please run the following script:
This script will help identify:
- Where the new
ErrSubaccountVerificationis being used. - If there are any remaining uses of
ErrUnauthorizedthat might need to be updated. - Any other files using sdkerrors that might need review.
Please review the results to ensure consistency across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of ErrSubaccountVerification and potential remaining uses of ErrUnauthorized
# Test 1: Check for uses of ErrSubaccountVerification
echo "Occurrences of ErrSubaccountVerification:"
rg --type go "ErrSubaccountVerification"
# Test 2: Check for any remaining uses of ErrUnauthorized
echo "\nRemaining occurrences of ErrUnauthorized:"
rg --type go "ErrUnauthorized"
# Test 3: Check for any files that might need updating
echo "\nPotential files that might need updating:"
rg --type go -l "sdkerrors\.Err.*"
Length of output: 3989
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Replace panic with require.NoError for Better Error Handling
Using panic(err) in test code can abruptly terminate the test suite and may not provide as much context as assertion helpers. It's recommended to use require.NoError(t, err) to assert that no error occurred, which provides better test failure messages and continues execution of other tests.
Apply this diff to improve error handling:
bytes, err := tApp.App.TxConfig().TxEncoder()(tx)
-if err != nil {
- panic(err)
-}
+require.NoError(t, err)📝 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.
| if err != nil { | |
| panic(err) | |
| } | |
| bytes, err := tApp.App.TxConfig().TxEncoder()(tx) | |
| require.NoError(t, err) |
| Sender: constants.BobAccAddress.String(), | ||
| AuthenticatorType: "AnyOf", | ||
| Data: compositeAuthenticatorConfig, | ||
| }, | ||
|
|
||
| Fees: constants.TestFeeCoins_5Cents, | ||
| Gas: 300_000, | ||
| AccountNum: []uint64{1}, | ||
| SeqNum: []uint64{1}, | ||
| Signers: []cryptotypes.PrivKey{constants.BobPrivateKey}, | ||
|
|
||
| ExpectedRespCode: 0, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Block: 4, | ||
| Msgs: []TestSdkMsg{ | ||
| { | ||
| Msg: clobtypes.NewMsgPlaceOrder( | ||
| testapp.MustScaleOrder( | ||
| constants.Order_Bob_Num0_Id11_Clob1_Buy5_Price40_GTB20, | ||
| testapp.DefaultGenesis(), | ||
| ), | ||
| ), | ||
| Authenticators: []uint64{0}, | ||
|
|
||
| Fees: constants.TestFeeCoins_5Cents, | ||
| Gas: 0, | ||
| AccountNum: []uint64{1}, | ||
| SeqNum: []uint64{1}, | ||
| Signers: []cryptotypes.PrivKey{constants.BobPrivateKey}, | ||
|
|
||
| ExpectedRespCode: aptypes.ErrAnyOfVerification.ABCICode(), | ||
| ExpectedLog: aptypes.ErrAnyOfVerification.Error(), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| expectedOrderIdsInMemclob: map[clobtypes.OrderId]bool{ | ||
| constants.Order_Bob_Num0_Id11_Clob1_Buy5_Price40_GTB20.OrderId: false, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| tApp := testapp.NewTestAppBuilder(t).WithGenesisDocFn(func() (genesis types.GenesisDoc) { | ||
| genesis = testapp.DefaultGenesis() | ||
| testapp.UpdateGenesisDocWithAppStateForModule( | ||
| &genesis, | ||
| func(genesisState *aptypes.GenesisState) { | ||
| genesisState.Params.IsSmartAccountActive = tc.smartAccountEnabled | ||
| }, | ||
| ) | ||
| return genesis | ||
| }).Build() | ||
| ctx := tApp.InitChain() | ||
|
|
||
| for _, block := range tc.blocks { | ||
| for _, msg := range block.Msgs { | ||
| tx, err := testtx.GenTx( | ||
| ctx, | ||
| tApp.App.TxConfig(), | ||
| []sdk.Msg{msg.Msg}, | ||
| msg.Fees, | ||
| msg.Gas, | ||
| tApp.App.ChainID(), | ||
| msg.AccountNum, | ||
| msg.SeqNum, | ||
| msg.Signers, | ||
| msg.Signers, | ||
| msg.Authenticators, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| bytes, err := tApp.App.TxConfig().TxEncoder()(tx) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| checkTxReq := abcitypes.RequestCheckTx{ | ||
| Tx: bytes, | ||
| Type: abcitypes.CheckTxType_New, | ||
| } | ||
|
|
||
| resp := tApp.CheckTx(checkTxReq) | ||
| require.Equal( | ||
| t, | ||
| msg.ExpectedRespCode, | ||
| resp.Code, | ||
| "Response code was not as expected", | ||
| ) | ||
| require.Contains( | ||
| t, | ||
| resp.Log, | ||
| msg.ExpectedLog, | ||
| "Response log was not as expected", | ||
| ) | ||
| } | ||
| ctx = tApp.AdvanceToBlock(block.Block, testapp.AdvanceToBlockOptions{}) |
There was a problem hiding this comment.
Ensure Proper Sequence Number Incrementation in Test Messages
The SeqNum (sequence numbers) in your test messages do not increment between transactions from the same account (AccountNum: []uint64{1}). In practice, sequence numbers should increment with each transaction to reflect their order and prevent nonce reuse. Failing to increment sequence numbers may cause transactions to be rejected or not accurately simulate real-world scenarios.
Please update the SeqNum for each transaction to increment appropriately. For example:
SeqNum: []uint64{1}, // First transaction
+// Next transaction from the same account
+SeqNum: []uint64{2},
+// Subsequent transactions increment accordingly
+SeqNum: []uint64{3},Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
protocol/x/accountplus/types/message_add_authenticator_test.go (1)
Line range hint
31-75: "Failure: Data exceeds max length" test case is comprehensive but could be improved.This test case effectively checks the behavior when the authenticator data exceeds the maximum allowed length:
- It uses a complex JSON structure that includes multiple
SignatureVerificationconfigurations, which is good for thorough testing.- The expected error type
types.ErrAuthenticatorDataExceedsMaximumLengthis appropriate for this scenario.- The use of constants for private keys (e.g.,
constants.AlicePrivateKey) is a good practice for consistency and maintainability.However, there's room for a minor improvement:
Consider extracting the long JSON string into a separate constant or helper function. This would improve readability and make it easier to maintain or modify the test data in the future.
For example:
func getTestDataExceedingMaxLength() string { return fmt.Sprintf( `[ {"Type":"MessageFilter","Config":"%s"}, { "Type":"AnyOf", "Config":" [ {"Type":"SignatureVerification","Config":"%s"}, // ... (rest of the configurations) ] " }, ]`, messageFilterData, constants.AlicePrivateKey.PubKey().String(), // ... (rest of the arguments) ) } // In the test case Data: []byte(getTestDataExceedingMaxLength()),This change would make the test case more concise and easier to read.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- protocol/x/accountplus/types/message_add_authenticator_test.go (3 hunks)
🔇 Additional comments (3)
protocol/x/accountplus/types/message_add_authenticator_test.go (3)
12-12: Function name change accurately reflects the test purpose.The updated function name
TestMsgAddAuthenticator_ValidateBasiccorrectly represents the test's focus on validating theMsgAddAuthenticatormessage. This change aligns well with the PR objective of adding e2e tests for the new auth flow.
Line range hint
18-24: "Success" test case is well-structured and appropriate.The test case for successful validation is correctly implemented:
- It uses a constant (
AliceAccAddress) for the sender address, which is a good practice.- The
AuthenticatorTypeis set to "MessageFilter", which seems appropriate for the test scenario.- The
Datafield contains a valid string of message types.This test case effectively covers a successful validation scenario for the
MsgAddAuthenticatormessage.
Line range hint
25-30: "Failure: Not an address" test case correctly handles invalid input.This test case effectively checks the behavior when an invalid sender address is provided:
- It uses "invalid" as the sender address, which should trigger a validation error.
- The expected error type
types.ErrInvalidAccountAddressis appropriate for this scenario.This test ensures that the
ValidateBasicmethod correctly identifies and reports invalid addresses.
| func TestPlaceOrder_PermissionedKeys_Failures(t *testing.T) { | ||
| config := []aptypes.SubAuthenticatorInitData{ | ||
| { | ||
| Type: "SignatureVerification", |
There was a problem hiding this comment.
Nit (Tangential to this PR): did we consider using defining an enum type for the authenticator types? We could save plenty of payloads in gossiped messages.
At the very least, feels like the strings could be shorter
There was a problem hiding this comment.
let me address this separately. created this ticket to keep track
| testapp.DefaultGenesis(), | ||
| ), | ||
| ), | ||
| Authenticators: []uint64{0}, |
There was a problem hiding this comment.
Question (tangential to PR): is it correct that AuthenticatorId is global, but authenticators are always stored under account prefix. Is the main motivation behind global ID so that we just need one ID counter?
There was a problem hiding this comment.
yeah this decision was from x/smart-account, and I think it was mostly just for simplicity (at least from what I've seen in the code)
| Signers []cryptotypes.PrivKey | ||
|
|
||
| ExpectedRespCode uint32 | ||
| ExpectedLog string |
There was a problem hiding this comment.
Could we add an ExpectedGas field?
There was a problem hiding this comment.
hmm these are short term orders with infinite gas meters and also IIUC if a transaction fails check tx, it doesn't incur any gas?
I can add expected gas for success cases, for stateful orders
There was a problem hiding this comment.
Oh I was referring to failed transactions that are not ST orders (e.g. transfer/send) since those should still incur gas cost even if transaction failed.
Up to you - not necessary if too much to set up
| expectedOrderIdsInMemclob: map[clobtypes.OrderId]bool{ | ||
| constants.Order_Bob_Num0_Id11_Clob1_Buy5_Price40_GTB20.OrderId: false, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
I think (?) we support multi-msg transaction + authenticator. Should we add a test where one of the messages were not authenticated successfully?
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
PlaceOrderfunctionality within the permissioned context of the Central Limit Order Book (CLOB) module.Bug Fixes
Documentation