[CT-1262] add e2e tests for permissioned keys success cases #2479
Conversation
WalkthroughThis pull request introduces significant changes to the transaction processing logic within the application. Key modifications include restructuring the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 4
🧹 Outside diff range and nitpick comments (2)
protocol/x/clob/e2e/permissioned_keys_test.go (2)
666-666: Add a Comment for the New Test FunctionConsider adding a comment above the new test function
TestPlaceOrder_PermissionedKeys_Successto describe its purpose. This enhances code readability and helps future maintainers understand the test scenarios covered.Apply this diff to add the comment:
+// TestPlaceOrder_PermissionedKeys_Success tests successful order placements using permissioned keys. func TestPlaceOrder_PermissionedKeys_Success(t *testing.T) {
1379-1382: Avoid Usingpanicin Test CodeUsing
panic(err)in test code can halt the entire test suite on failure. Instead, userequire.NoError(t, err)to fail the specific test case gracefully without affecting others.Apply this diff to replace
panicwithrequire.NoError:bytes, err := tApp.App.TxConfig().TxEncoder()(tx) -if err != nil { - panic(err) -} +require.NoError(t, err, "Failed to encode transaction")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- protocol/app/ante.go (1 hunks)
- protocol/app/app.go (1 hunks)
- protocol/testutil/constants/stateful_orders.go (1 hunks)
- protocol/x/accountplus/ante/circuit_breaker.go (2 hunks)
- protocol/x/accountplus/ante/circuit_breaker_test.go (1 hunks)
- protocol/x/clob/e2e/permissioned_keys_test.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (13)
protocol/x/accountplus/ante/circuit_breaker.go (3)
21-22: LGTM! Verify function calls in codebase.The parameter type changes from
sdk.AnteDecoratortosdk.AnteHandlerin theNewCircuitBreakerDecoratorfunction are consistent with the struct field modifications. This ensures type consistency when creating new instances ofCircuitBreakerDecorator.To ensure this change doesn't introduce any issues, please run the following script to check for any calls to
NewCircuitBreakerDecoratorin the codebase:#!/bin/bash # Description: Check for calls to NewCircuitBreakerDecorator # Test: Search for NewCircuitBreakerDecorator function calls rg --type go 'NewCircuitBreakerDecorator\('
47-47: LGTM! Verify correct behavior of AnteHandle method.The changes to the
AnteHandlemethod are consistent with the type changes fromsdk.AnteDecoratortosdk.AnteHandler. The removal of thenextparameter and the inclusion of thesimulateparameter in both method calls align with the new handler type's expected signature.To ensure this change doesn't introduce any unexpected behavior, please run the following script to check for any tests related to the
AnteHandlemethod:#!/bin/bash # Description: Check for tests related to AnteHandle method # Test: Search for test functions that might be testing AnteHandle rg --type go 'func.*Test.*AnteHandle'Additionally, please ensure that the
simulateparameter is being used correctly in both the authenticator and original ante handler flows.Also applies to: 51-51
14-15: Overall changes look good. Ensure comprehensive update across codebase.The modifications to
CircuitBreakerDecoratorand its associated methods consistently update the ante handler types fromsdk.AnteDecoratortosdk.AnteHandler. This change simplifies the interface and aligns with the SDK'sAnteHandlertype, which is a positive improvement.To ensure a smooth transition:
- Update all relevant parts of the application that interact with
CircuitBreakerDecorator.- Modify any existing tests to reflect these changes.
- Update documentation to reflect the new method signatures and usage patterns.
- Consider running integration tests to verify that the changes don't introduce any unexpected behavior in the transaction processing flow.
Run the following script to identify potential areas that might need updates:
#!/bin/bash # Description: Identify potential areas needing updates due to AnteDecorator changes # Test 1: Search for AnteDecorator usages echo "Searching for AnteDecorator usages:" rg --type go 'AnteDecorator' # Test 2: Search for CircuitBreakerDecorator usages echo "Searching for CircuitBreakerDecorator usages:" rg --type go 'CircuitBreakerDecorator' # Test 3: Search for NewCircuitBreakerDecorator usages echo "Searching for NewCircuitBreakerDecorator usages:" rg --type go 'NewCircuitBreakerDecorator' # Test 4: Search for AnteHandle method usages echo "Searching for AnteHandle method usages:" rg --type go '\.AnteHandle\('Review the output of this script to identify any areas of the codebase that might need updates to align with the new changes.
Also applies to: 21-22, 47-47, 51-51
protocol/x/accountplus/ante/circuit_breaker_test.go (3)
Line range hint
86-94: LGTM: New AnteHandle method enhances test capabilitiesThe addition of the
AnteHandlemethod toMockAnteDecoratoris a positive change. It allows for more nuanced testing of the CircuitBreaker's behavior by simulating different decorator responses based on theCalledfield.This enhancement will provide more comprehensive test coverage for the CircuitBreaker functionality.
Line range hint
1-194: Overall assessment: Improved test structure with potential for wider impactThe changes to this test file, particularly in the
TestCircuitBreakerAntefunction and the addition of theAnteHandlemethod toMockAnteDecorator, represent an improvement in the test structure and capabilities. The use ofsdk.ChainAnteDecoratorsaligns the test more closely with SDK patterns, while the new mock method allows for more nuanced testing.However, these changes, especially the modification to the decorator chaining, could potentially impact the behavior of the CircuitBreaker or how it's tested. To ensure the integrity of the tests:
- Verify that all test cases still pass and behave as expected.
- Consider adding additional test cases that specifically target the new chaining behavior.
- Review any other tests or code that may rely on the previous CircuitBreaker instantiation method to ensure consistency across the codebase.
To assist in this verification, please run the following script:
#!/bin/bash # Description: Run the CircuitBreaker tests and check for any failures or unexpected behaviors # Test: Run the CircuitBreaker tests go test -v ./protocol/x/accountplus/ante -run TestCircuitBreakerAnte # Additional check: Look for any other files that might be affected by the changes rg --type go "CircuitBreakerDecorator" ./protocolReview the test results and any files highlighted by the script to ensure full compatibility with the changes.
154-155: Verify the impact of chaining ante decoratorsThe modification to wrap
mockTestAuthenticatorandmockTestClassicwithsdk.ChainAnteDecoratorschanges how these decorators are chained together. While this change aligns with the SDK's decorator chaining pattern, it's important to ensure that this doesn't alter the expected behavior of the CircuitBreaker.To confirm that this change doesn't introduce any unintended side effects, please run the following verification script:
If the script returns results, review those occurrences to ensure consistency across all tests.
✅ Verification successful
No other tests rely on the previous decorator chaining method
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the CircuitBreaker behavior remains consistent with the new decorator chaining. # Test: Check if there are any other tests relying on the previous decorator chaining method rg --type go "NewCircuitBreakerDecorator\s*\(" ./protocol/x/accountplus/anteLength of output: 1181
protocol/testutil/constants/stateful_orders.go (1)
297-308: New long-term order added for Bob with ClobPairId 1The new order
LongTermOrder_Bob_Num0_Id0_Clob1_Buy25_Price30_GTBT10has been added, which is similar to existing orders but uses ClobPairId 1 instead of 0. This addition expands the test coverage to include orders for different CLOB pairs.A few observations:
- The order is consistent with the naming convention used for other long-term orders.
- It uses the same SubaccountId (Bob_Num0) and ClientId (0) as some existing Bob orders, which is fine for testing purposes.
- The order parameters (Side, Quantums, Subticks, GoodTilBlockTime) are within the range of values used in other test orders.
This addition will allow for more comprehensive testing of the system's behavior with multiple CLOB pairs.
To ensure consistency and completeness, let's verify if there are other orders for ClobPairId 1:
protocol/app/ante.go (2)
127-133: Good refactoring: ChainingNewAuthenticatorDecoratorwithsdk.ChainAnteDecoratorsThe introduction of
sdk.ChainAnteDecoratorsto chain theNewAuthenticatorDecoratorenhances code clarity and maintainability by simplifying the ante decorator chain.
135-141: Streamlined ante handlers usingsdk.ChainAnteDecoratorsChaining
SetPubKeyDecorator,SigGasConsumeDecorator, andSigVerificationDecoratorwithsdk.ChainAnteDecoratorssimplifies the ante handler logic and improves readability.protocol/x/clob/e2e/permissioned_keys_test.go (3)
670-670: Verify Usage of Alice's Public Key in Authenticator ConfigThe
SignatureVerificationauthenticator is configured withconstants.AlicePrivateKey.PubKey().Bytes(). Verify that using Alice's public key here is intentional and aligns with the test scenarios. This ensures that only transactions signed by Alice's private key are accepted.
848-848: Confirm Account Number and Sequence Number for AliceIn this test case, the transaction is signed using Alice's private key, but
AccountNumis set to[]uint64{1}andSeqNumis[]uint64{0}. Ensure that these values correspond to Alice's account. If Alice's account number is0, theAccountNumshould be updated to reflect this.Apply this diff if an update is needed:
Fees: constants.TestFeeCoins_5Cents, Gas: 0, -AccountNum: []uint64{1}, +AccountNum: []uint64{0}, SeqNum: []uint64{0}, // Sign using Alice's private key. Signers: []cryptotypes.PrivKey{constants.AlicePrivateKey},
1395-1400: Check for Empty Expected Log MessagesIn the assertion for the response log,
require.Containsis used withmsg.ExpectedLog, which might be empty in some test cases. Consider adding a check to ensure thatExpectedLogis not empty before using it in the assertion to avoid misleading test results.Apply this diff to add the check:
+if msg.ExpectedLog != "" { require.Contains( t, resp.Log, msg.ExpectedLog, "Response log was not as expected", ) +}protocol/app/app.go (1)
1115-1136: LGTM: Initialization of AuthenticatorManager and AccountPlusKeeperThe initialization of the
AuthenticatorManagerand theAccountPlusKeeperis correctly implemented. The code is clear and aligns with best practices.
| authenticatorAnteHandlerFlow sdk.AnteHandler | ||
| originalAnteHandlerFlow sdk.AnteHandler |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Address Remaining sdk.AnteDecorator Usages
The recent changes in circuit_breaker.go simplify the interface by transitioning from sdk.AnteDecorator to sdk.AnteHandler. However, the following files still utilize sdk.AnteDecorator:
protocol/x/clob/ante/rate_limit.goprotocol/x/clob/ante/clob.goprotocol/x/accountplus/ante/circuit_breaker_test.goprotocol/mocks/AnteDecorator.goprotocol/app/ante.go- Multiple test files within
protocol/app/ante/
To ensure full consistency and prevent potential integration issues, please update these usages to sdk.AnteHandler where appropriate.
🔗 Analysis chain
LGTM! Verify impact on codebase.
The change from sdk.AnteDecorator to sdk.AnteHandler for both authenticatorAnteHandlerFlow and originalAnteHandlerFlow fields simplifies the interface and aligns with the modifications in the rest of the file. This is a good improvement in terms of code clarity and consistency.
To ensure this change doesn't introduce any issues, please run the following script to check for any remaining usages of sdk.AnteDecorator in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for remaining usages of sdk.AnteDecorator
# Test: Search for sdk.AnteDecorator usages
rg --type go 'sdk\.AnteDecorator'
Length of output: 10145
| _, fillAmount, _ := tApp.App.ClobKeeper.GetOrderFillAmount(ctx, orderId) | ||
| require.Equal(t, expectedFillAmount, fillAmount.ToUint64()) | ||
| } |
There was a problem hiding this comment.
Handle Possible Errors from GetOrderFillAmount
The error returned by GetOrderFillAmount is currently ignored. To ensure test accuracy and prevent false positives, it's important to check and handle this error.
Apply this diff to handle the error:
for orderId, expectedFillAmount := range tc.expectedOrderFillAmounts {
- _, fillAmount, _ := tApp.App.ClobKeeper.GetOrderFillAmount(ctx, orderId)
+ _, fillAmount, err := tApp.App.ClobKeeper.GetOrderFillAmount(ctx, orderId)
+ require.NoError(t, err, "Failed to get fill amount for order %v", orderId)
require.Equal(t, expectedFillAmount, fillAmount.ToUint64())
}📝 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.
| _, fillAmount, _ := tApp.App.ClobKeeper.GetOrderFillAmount(ctx, orderId) | |
| require.Equal(t, expectedFillAmount, fillAmount.ToUint64()) | |
| } | |
| _, fillAmount, err := tApp.App.ClobKeeper.GetOrderFillAmount(ctx, orderId) | |
| require.NoError(t, err, "Failed to get fill amount for order %v", orderId) | |
| require.Equal(t, expectedFillAmount, fillAmount.ToUint64()) | |
| } |
| { | ||
| Type: "SignatureVerification", | ||
| Config: constants.AlicePrivateKey.PubKey().Bytes(), | ||
| }, | ||
| { | ||
| Type: "MessageFilter", | ||
| Config: []byte("/dydxprotocol.clob.MsgPlaceOrder"), | ||
| }, | ||
| { | ||
| Type: "ClobPairIdFilter", | ||
| Config: []byte("0,1"), | ||
| }, | ||
| { | ||
| Type: "SubaccountFilter", | ||
| Config: []byte("0,1"), | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Authenticator Configuration into a Helper Function
The authenticator configuration setup is repeated across multiple test cases. Consider refactoring this code into a helper function to adhere to the DRY (Don't Repeat Yourself) principle and enhance maintainability.
Apply this diff to create a helper function:
+func createCompositeAuthenticatorConfig() []byte {
+ config := []aptypes.SubAuthenticatorInitData{
+ {
+ Type: "SignatureVerification",
+ Config: constants.AlicePrivateKey.PubKey().Bytes(),
+ },
+ {
+ Type: "MessageFilter",
+ Config: []byte("/dydxprotocol.clob.MsgPlaceOrder"),
+ },
+ {
+ Type: "ClobPairIdFilter",
+ Config: []byte("0,1"),
+ },
+ {
+ Type: "SubaccountFilter",
+ Config: []byte("0,1"),
+ },
+ }
+ compositeAuthenticatorConfig, err := json.Marshal(config)
+ if err != nil {
+ panic(err)
+ }
+ return compositeAuthenticatorConfig
+}
+
func TestPlaceOrder_PermissionedKeys_Success(t *testing.T) {
- config := []aptypes.SubAuthenticatorInitData{
- {
- Type: "SignatureVerification",
- Config: constants.AlicePrivateKey.PubKey().Bytes(),
- },
- {
- Type: "MessageFilter",
- Config: []byte("/dydxprotocol.clob.MsgPlaceOrder"),
- },
- {
- Type: "ClobPairIdFilter",
- Config: []byte("0,1"),
- },
- {
- Type: "SubaccountFilter",
- Config: []byte("0,1"),
- },
- }
- compositeAuthenticatorConfig, err := json.Marshal(config)
- require.NoError(t, err)
+ compositeAuthenticatorConfig := createCompositeAuthenticatorConfig()Committable suggestion was skipped due to low confidence.
| }, | ||
| { | ||
| Type: "MessageFilter", | ||
| Config: []byte("/dydxprotocol.clob.MsgPlaceOrder"), |
There was a problem hiding this comment.
Ensure Correct Message Type in MessageFilter
The MessageFilter authenticator uses the message type /dydxprotocol.clob.MsgPlaceOrder. Verify that this string correctly matches the fully-qualified type name of the MsgPlaceOrder message. If the actual message type includes a version suffix (e.g., /dydxprotocol.clob.v1.MsgPlaceOrder), update the filter accordingly to prevent mismatches.
Apply this diff if an update is needed:
{
Type: "MessageFilter",
- Config: []byte("/dydxprotocol.clob.MsgPlaceOrder"),
+ Config: []byte("/dydxprotocol.clob.v1.MsgPlaceOrder"),
},Committable suggestion was skipped due to low confidence.
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
AuthenticatorManagerfor enhanced account management and authentication.Bug Fixes
Tests