[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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
protocol/x/affiliates/e2e/register_affiliate_test.go (1)
45-47: Improved test isolation by moving app initialization inside the test loop.This change enhances the isolation between test cases by initializing a new test application (
tApp) and context (ctx) for each iteration. This approach prevents potential state leakage between test cases, aligning with best practices for test design.Consider the following points:
- Improved reliability: Each test case now runs in a completely fresh environment, reducing the risk of inter-test dependencies.
- Slight performance impact: Initializing the app for each test case might increase the overall test execution time. However, the benefits of improved isolation often outweigh this minor performance cost.
- Consistency: Ensure this pattern is applied consistently across similar test files for uniformity in the testing approach.
To further improve the test suite:
- Consider extracting the app initialization into a helper function to reduce code duplication if this pattern is used across multiple test files.
- If performance becomes a concern, you might want to implement parallel test execution using
t.Parallel(), which could offset the increased initialization time.
| options.SignModeHandler, | ||
| sdk.ChainAnteDecorators( | ||
| ante.NewSetPubKeyDecorator(options.AccountKeeper), | ||
| ante.NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer), |
There was a problem hiding this comment.
Could you remind how how gas consumption is done for accountplus flow? A quick search of StaticGas on authenticators suggests they are all 0?
There was a problem hiding this comment.
Added some gas here. we only support secp256k1 so it's a static value.
| customante.NewSigVerificationDecorator( | ||
| options.AccountKeeper, | ||
| options.SignModeHandler, | ||
| sdk.ChainAnteDecorators( |
There was a problem hiding this comment.
For my understanding why was it necessary to move SetPubKey and SigGas under this chain and thus not applicable to smart account flow?
There was a problem hiding this comment.
| constants.Order_Alice_Num0_Id1_Clob1_Sell5_Price15_GTB20_IOC.OrderId: 0, | ||
| }, | ||
| }, | ||
| "Stateful maker order can be matched even if permissioned key is removed": { |
There was a problem hiding this comment.
Could we provide some context on these behaviors when the order is removed? My understanding (please CWIW):
- For ST orders, we HAVE to remove the maker order because we can't trust BP on this maker order being valid
- For stateful orders, we don't HAVE TO remove the maker order because we can trust the onchain state. We MAY choose to remove the maker stateful order, but needs additional work (e.g. prune orders when removing authenticators)
There was a problem hiding this comment.
yeah that's correct. for ST orders, it's not necessarily about trust, it's more about maintaining the invariant that operations queue is always valid.
There was a problem hiding this comment.
Thanks, to clarify I was suggesting to add some comment on the test cases about this difference in expected behavior between ST and stateful order, for the benefit of reader
| // First consume gas for verifying the signature | ||
| params := sva.ak.GetParams(ctx) | ||
| ctx.GasMeter().ConsumeGas(params.SigVerifyCostSecp256k1, "secp256k1 signature verification") |
There was a problem hiding this comment.
added gas here @teddyding .
we currently only support secp256k1 signature verification so charging some static gas here.
There was a problem hiding this comment.
Could we add a short comment that that the signature authenticator only accepts secp256k1?
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
protocol/x/accountplus/ante/circuit_breaker.go (2)
21-22: LGTM! Consider a minor naming improvement.The changes to the
NewCircuitBreakerDecoratorfunction are well-implemented and consistent with the updates to theCircuitBreakerDecoratorstruct. The parameter types have been correctly updated tosdk.AnteHandler, and the struct initialization reflects the new field names and types.Consider shortening the parameter name
authenticatorAnteHandlerFlowtoauthenticatorFlowfor better readability, as theAnteHandlerpart is already implied by the type:func NewCircuitBreakerDecorator( cdc codec.BinaryCodec, authenticatorFlow sdk.AnteHandler, defaultAnteHandlerFlow sdk.AnteHandler, ) CircuitBreakerDecorator { // ... }Also applies to: 26-27
Line range hint
39-43: LGTM! Update method comment.The removal of the
next sdk.AnteHandlerparameter from theAnteHandlemethod signature is correct and consistent with the changes to usesdk.AnteHandlerinstead ofsdk.AnteDecorator.Please update the method comment to reflect the removal of the
nextparameter:// AnteHandle checks if a tx is a smart account tx and routes it through the correct series of ante handlers. // // Note that whether or not to use the new authenticator flow is determined by the presence of the `TxExtension`. // This is different from the Osmosis's implementation, which falls back to the original flow if // smart account is disabled. // The reason for this is because only minimal validation is done on resting maker orders when they get matched // and this approach mitigates an issue for maker orders when smart account gets disabled through governance. func (ad CircuitBreakerDecorator) AnteHandle( ctx sdk.Context, tx sdk.Tx, simulate bool, ) (newCtx sdk.Context, err error) { // ... }protocol/x/accountplus/authenticator/signature_authenticator.go (2)
Line range hint
36-41: Prevent invalid public key assignment inInitializemethodIn the
Initializemethod, the current logic assignssva.PubKeyregardless of the validity ofconfig. Ifconfiglength does not matchsecp256k1.PubKeySize,sva.PubKeyis set tonilbut then immediately overwritten with an invalid key. This can lead to unexpected behavior when the public key is used elsewhere.Apply this diff to ensure that
sva.PubKeyis only set whenconfigis valid:func (sva SignatureVerification) Initialize(config []byte) (types.Authenticator, error) { if len(config) != secp256k1.PubKeySize { sva.PubKey = nil + return sva, nil } sva.PubKey = &secp256k1.PubKey{Key: config} return sva, nil }
Line range hint
68-69: Correct typo in error messageThe error message "pubkey on not set on account or authenticator" contains a typo. It should read "pubkey not set on account or authenticator" for clarity.
Apply this diff to fix the typo:
if sva.PubKey == nil { - return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "pubkey on not set on account or authenticator") + return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "pubkey not set on account or authenticator") }protocol/app/ante/pubkey.go (1)
88-89: Remove unnecessary re-declaration oferr.At line 88,
var err erroris declared within the function scope whereerrhas already been defined. This re-declaration is unnecessary and could cause confusion.Apply this diff to remove the redundant declaration:
sigs := [][]byte{} -var err error
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- protocol/app/ante.go (1 hunks)
- protocol/app/ante/pubkey.go (1 hunks)
- protocol/x/accountplus/ante/circuit_breaker.go (2 hunks)
- protocol/x/accountplus/authenticator/signature_authenticator.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protocol/app/ante.go
🧰 Additional context used
🔇 Additional comments (2)
protocol/x/accountplus/ante/circuit_breaker.go (1)
14-15: LGTM! Consistent with previous recommendations.The changes to the
CircuitBreakerDecoratorstruct are well-implemented. Updating the types fromsdk.AnteDecoratortosdk.AnteHandlerand renamingoriginalAnteHandlerFlowtodefaultAnteHandlerFlowimproves consistency and clarity. These modifications align with the previous review comments suggesting to address remainingsdk.AnteDecoratorusages.protocol/app/ante/pubkey.go (1)
98-105: Verify the correctness ofcryptotypes.MultiSignatureusage andMarshalmethod.The
cryptotypes.MultiSignaturestruct is used to aggregate signatures, and itsMarshalmethod is called to obtain the aggregated signature. Ensure thatMultiSignatureand itsMarshalmethod are correctly implemented and compatible with the intended cryptographic operations.Run the following script to check the existence and usage of
MultiSignatureand itsMarshalmethod:
protocol/x/accountplus/authenticator/signature_authenticator.go
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,110 @@ | |||
| package ante | |||
There was a problem hiding this comment.
@teddyding I added a new decorator that looks identical to SetPubKey but only emits events
reference from Osmosis (link)
There was a problem hiding this comment.
Nice, how did you figure this was necessary?
There was a problem hiding this comment.
Was just looking at the SetPubKey decorator and also using Osmosis as reference. Wanted to keep the behavior consistent
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
protocol/app/ante/pubkey.go (1)
16-18: Adjust the comment to match the type definitionThe comment above
EmitPubKeyEventsDecoratorrefers toNewEmitPubKeyEventsDecoratorbut is placed above the type definition. To enhance clarity, please adjust the comment to accurately describe theEmitPubKeyEventsDecoratortype.Apply this diff to correct the comment:
-// NewEmitPubKeyEventsDecorator emits events for each signer's public key. +// EmitPubKeyEventsDecorator emits events for each signer's public key.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- protocol/app/ante/pubkey.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
protocol/app/ante/pubkey.go (2)
24-71: Proper implementation ofAnteHandlemethodThe
AnteHandlemethod is well-structured and correctly processes the transaction to emit the necessary events. It appropriately handles errors and maintains the flow to the next ante handler.
77-110: Efficient handling of signature dataThe
signatureDataToBzfunction effectively converts both single and multi-signature data into raw byte slices. The recursive approach ensures all nested signatures are correctly processed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
protocol/x/clob/e2e/permissioned_keys_test.go (2)
688-694: Add documentation for the test structure.Consider adding documentation for the test struct fields to improve readability and maintainability. This will help other developers understand the purpose of each field and how to use them in new test cases.
Add this documentation above the struct:
// TestCase represents a scenario for testing permissioned key functionality. // Fields: // - smartAccountEnabled: Controls whether smart account features are active // - blocks: Sequence of blocks containing test messages // - expectedOrderIdsInMemclob: Maps order IDs to whether they should exist in memclob // - expectedOrderFillAmounts: Maps order IDs to their expected fill amounts
1394-1407: Enhance test failure messages for better debugging.The current error messages "Response code was not as expected" and "Response log was not as expected" could be more informative.
Consider enhancing the error messages with more context:
require.Equal( t, msg.ExpectedRespCode, resp.Code, - "Response code was not as expected", + "Unexpected response code for msg type %T. Expected %d, got %d", + msg.Msg[0], + msg.ExpectedRespCode, + resp.Code, ) require.Contains( t, resp.Log, msg.ExpectedLog, - "Response log was not as expected", + "Response log did not contain expected message for msg type %T", + msg.Msg[0], )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- protocol/app/app.go (1 hunks)
- protocol/x/accountplus/authenticator/signature_authenticator.go (1 hunks)
- protocol/x/clob/e2e/permissioned_keys_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protocol/x/accountplus/authenticator/signature_authenticator.go
🧰 Additional context used
🔇 Additional comments (3)
protocol/x/clob/e2e/permissioned_keys_test.go (1)
695-1351: LGTM! Comprehensive test coverage for permissioned keys.The test cases thoroughly cover the key scenarios:
- Short term and stateful order placement
- Order matching in various combinations
- Key removal behavior with clear explanation of the differences between short term and stateful orders
protocol/app/app.go (2)
1116-1127: Correct Initialization of AuthenticatorManager and AuthenticatorsThe
AuthenticatorManageris properly instantiated, and the authenticators are correctly initialized. The use ofNewAllOf,NewAnyOf, and other authenticators with the appropriate dependencies ensures a flexible authentication mechanism.
1128-1137: Proper Setup of AccountPlusKeeper with AuthenticatorManagerThe
AccountPlusKeeperis correctly initialized with theAuthenticatorManagerand the required parameters. The integration appears seamless, and the creation of theaccountplusModuleis appropriate.
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
AuthenticatorManager.Bug Fixes
Tests