[CT-1202] logic to handle unauthorized maker orders when authenticato… #2412
Conversation
WalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 7
🧹 Outside diff range and nitpick comments (7)
protocol/x/clob/types/mem_clob_keeper.go (1)
118-118: Consider architectural implications of the new methodWhile the new
MaybeValidateAuthenticatorsmethod addresses the PR objective, it raises some architectural concerns:
This method introduces authentication responsibilities to the
MemClobKeeper, which may violate the single responsibility principle. Consider if this functionality belongs in a separate authentication service or middleware.The "Maybe" prefix suggests optional execution, which could lead to inconsistent usage across the codebase. It might be better to have a clear, always-executed validation method, with the conditional logic handled by the caller.
It's not clear how this method integrates with the existing methods in the interface. Consider adding documentation or reorganizing the interface to show the relationship between methods.
Consider the following alternatives:
- Create a separate
AuthenticatorValidatorinterface and inject it intoMemClobKeeper.- Rename the method to
ValidateAuthenticatorsand handle the conditional execution in the implementation or caller.- Add documentation to clarify the method's role in the overall process flow of the keeper.
protocol/x/clob/types/expected_keepers.go (1)
189-196: LGTM: New AccountPlusKeeper interface. Consider adding documentation.The new
AccountPlusKeeperinterface is well-structured and aligns with the PR objectives. The method names are clear and their signatures are appropriate for their purposes.Consider adding documentation for the interface and its methods to improve maintainability. For example:
// AccountPlusKeeper defines the expected account plus keeper used for handling smart accounts and their authenticators. type AccountPlusKeeper interface { // GetIsSmartAccountActive returns whether smart accounts are currently active in the system. GetIsSmartAccountActive(ctx sdk.Context) bool // GetInitializedAuthenticatorForAccount retrieves an initialized authenticator for the specified account and selected authenticator index. GetInitializedAuthenticatorForAccount( ctx sdk.Context, account sdk.AccAddress, selectedAuthenticator uint64, ) (aptypes.InitializedAuthenticator, error) }protocol/x/clob/keeper/keeper.go (1)
47-47: Consider broader impact ofaccountPlusKeeperaddition.The changes in this file are minimal and focused on adding the
accountPlusKeeperfunctionality to the CLOB module'sKeeper. This suggests that this change is part of a larger feature implementation or refactoring related to account management.To ensure a comprehensive implementation:
- Review the
AccountPlusKeeperinterface definition and implementation.- Check for any necessary updates to the CLOB module's logic that might benefit from the new
accountPlusKeeperfunctionality.- Ensure that all relevant tests have been updated or added to cover the new functionality.
- Update the module's documentation to reflect the addition of
accountPlusKeeperand its purpose within the CLOB module.#!/bin/bash # Search for AccountPlusKeeper interface definition rg --type go 'type AccountPlusKeeper interface' protocol # Search for potential usage of accountPlusKeeper in the CLOB module rg --type go 'accountPlusKeeper\.' protocol/x/clob # Search for test files that might need updates rg --type go 'func Test.*Keeper' protocol/x/clobprotocol/x/clob/types/errors.go (1)
228-232: LGTM with a minor suggestion for consistency.The addition of
ErrTxnHasMultipleSignersis appropriate and well-placed within the existing error definitions. The error code (49) follows the sequence, and the message clearly describes the issue.For consistency with other error messages in this file, consider capitalizing the first letter of the error message:
ErrTxnHasMultipleSigners = errorsmod.Register( ModuleName, 49, - "The transaction has multiple signers", + "Transaction has multiple signers", )protocol/testutil/memclob/keeper.go (1)
532-534: LGTM: Placeholder implementation for testing purposes.The
MaybeValidateAuthenticatorsmethod is correctly implemented as a placeholder for theFakeMemClobKeeper. It always returnsnil, which is appropriate for testing scenarios where actual authentication validation is not needed.Consider adding a comment to explain that this is a placeholder implementation for testing purposes. This would improve code readability and make the intent clear for other developers. For example:
func (f *FakeMemClobKeeper) MaybeValidateAuthenticators(ctx sdk.Context, txBytes []byte) error { + // Placeholder implementation for testing purposes. + // In a real implementation, this method would validate authenticators. return nil }protocol/x/clob/types/operations_to_propose.go (1)
479-501: LGTM! Consider enhancing the error message.The implementation of
MustGetShortTermOrderTxBytesis correct and consistent with the existing codebase. It properly validates the input, retrieves the data, and handles errors by panicking, which is expected for a "Must" prefixed method.Consider enhancing the error message to include the order hash for easier debugging:
"MustGetShortTermOrderTxBytes: Order (%s) does not exist in "+ - "`ShortTermOrderHashToTxBytes`.", + "`ShortTermOrderHashToTxBytes`. Order hash: %s", order.GetOrderTextString(), + orderHash, ),protocol/x/clob/keeper/authenticators.go (1)
16-73: Add unit tests forMaybeValidateAuthenticatorsfunctionTo ensure the correctness and reliability of the
MaybeValidateAuthenticatorsfunction, consider adding unit tests that cover various scenarios:
- Transactions with no authenticators specified.
- Smart account flow inactive.
- Invalid transaction types.
- Transactions with multiple signers.
- Invalid or removed authenticators.
Do you need assistance in creating unit tests for this function? I can help generate test cases or open a GitHub issue to track this task.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- protocol/mocks/MemClobKeeper.go (2 hunks)
- protocol/testutil/memclob/keeper.go (1 hunks)
- protocol/x/clob/keeper/authenticators.go (1 hunks)
- protocol/x/clob/keeper/keeper.go (1 hunks)
- protocol/x/clob/memclob/memclob.go (1 hunks)
- protocol/x/clob/types/errors.go (1 hunks)
- protocol/x/clob/types/expected_keepers.go (2 hunks)
- protocol/x/clob/types/mem_clob_keeper.go (1 hunks)
- protocol/x/clob/types/operations_to_propose.go (1 hunks)
🔇 Additional comments (15)
protocol/x/clob/types/mem_clob_keeper.go (2)
118-118: Final thoughts and remindersThe addition of the
MaybeValidateAuthenticatorsmethod to theMemClobKeeperinterface addresses the PR objective of handling unauthorized maker orders. However, please ensure the following:
- Update any relevant documentation to reflect this new method and its purpose in the overall system architecture.
- Add or update unit tests to cover the new method and its various use cases, including edge cases and error scenarios.
- Consider the architectural and security implications raised in the previous comments.
To check for updated documentation and tests, run:
118-118: Address potential security implications of the new methodThe
MaybeValidateAuthenticatorsmethod introduces some security considerations that should be addressed:
Operating on raw transaction bytes could expose the system to malformed or malicious input. Ensure that proper input sanitization and validation are performed before processing these bytes.
Consider adding logging or alerting mechanisms for failed validations to aid in detecting potential security issues or attacks.
The placement of this authentication-related method in the
MemClobKeeperinterface might not provide the best separation of concerns for security-critical operations. Consider moving this functionality to a dedicated security or authentication module.To ensure proper error handling and logging are implemented, run the following script:
protocol/x/clob/types/expected_keepers.go (1)
10-10: LGTM: Import statement for accountplus types.The addition of the import statement for the
accountplustypes package is necessary for the newAccountPlusKeeperinterface. The use of the aliasaptypesis consistent with the naming convention used in other imports.protocol/x/clob/keeper/keeper.go (1)
47-47: LGTM. Verify usage of the newaccountPlusKeeperfield.The addition of the
accountPlusKeeperfield to theKeeperstruct is appropriate. This change suggests that the CLOB module now requires additional account-related functionality.To ensure proper integration, please verify the usage of
accountPlusKeeperthroughout the module:#!/bin/bash # Search for usage of accountPlusKeeper in the CLOB module rg --type go 'accountPlusKeeper' protocol/x/clobprotocol/x/clob/types/errors.go (1)
228-232: Verify the usage and impact of the new error.The introduction of
ErrTxnHasMultipleSignerssuggests new logic for handling transactions with multiple signers. To ensure this change doesn't introduce unexpected behavior:
- Confirm where and how this error is being used in the codebase.
- Verify that existing transaction processing flows properly handle this new error case.
- Ensure that appropriate tests have been added to cover scenarios where this error might be thrown.
To help verify the usage of this new error, you can run the following script:
This will help identify where the new error is being used and any related changes in transaction processing logic.
protocol/testutil/memclob/keeper.go (2)
Line range hint
526-530: LGTM: Placeholder implementation for testing purposes.The
AddOrderToOrderbookSubaccountUpdatesCheckmethod is correctly implemented as a placeholder for theFakeMemClobKeeper. It satisfies the interface requirement and always returnssatypes.Success, which is appropriate for testing scenarios where actual validation logic is not needed.
Line range hint
526-534: Overall assessment: Changes are appropriate for testing purposes.The additions to
FakeMemClobKeeperare well-implemented placeholders that satisfy interface requirements. These changes enhance the fake keeper's ability to mimic the real keeper's behavior in test scenarios without introducing actual logic. This approach is suitable for unit testing and allows for flexible test setups.protocol/mocks/MemClobKeeper.go (2)
291-307: LGTM: New methodMaybeValidateAuthenticatorsaddedThe new method
MaybeValidateAuthenticatorshas been correctly implemented following the established pattern for mock methods in this file. This addition aligns with the PR objective of handling unauthorized maker orders.
436-439: Verify usage of updatedSendOrderbookFillUpdatemethodThe
SendOrderbookFillUpdatemethod has been updated to handle a singleorderbookFillinstead of multipleorderbookFills. This change might impact how the method is used in other parts of the codebase.Please run the following script to check for any potential impacts:
✅ Verification successful
Verified: All usages of
SendOrderbookFillUpdateconform to the updated singleorderbookFillparameter signature. No issues detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of SendOrderbookFillUpdate method # and verify if they align with the new signature. # Search for method calls rg --type go "SendOrderbookFillUpdate\(" -A 3Length of output: 3568
protocol/x/clob/keeper/authenticators.go (6)
14-15: Well-documented function purposeThe comments clearly explain the purpose and behavior of the
MaybeValidateAuthenticatorsfunction, adhering to good Go documentation practices.
18-21: Proper error handling when decoding the transactionThe transaction is correctly decoded from
txBytes, and errors are appropriately handled.
23-27: Efficient check for specified authenticatorsThe code efficiently checks if the transaction has authenticators specified and returns early if not, improving performance by avoiding unnecessary processing.
31-33: Correct verification of smart account activationThe function appropriately checks if the smart account flow is active and returns an error if it is not, ensuring that the subsequent logic is only executed when valid.
35-39: Proper type assertion forSigVerifiableTxThe transaction is correctly asserted to be of type
authsigning.SigVerifiableTx, and an error is returned if the assertion fails, preventing potential runtime panics.
41-44: Appropriate retrieval and handling of signersThe signers are retrieved using
GetSigners(), and errors are properly handled, ensuring that the function operates on valid signer information.
| @@ -115,4 +115,5 @@ type MemClobKeeper interface { | |||
| subaccountId satypes.SubaccountId, | |||
| order PendingOpenOrder, | |||
| ) satypes.UpdateResult | |||
| MaybeValidateAuthenticators(ctx sdk.Context, txBytes []byte) error | |||
There was a problem hiding this comment.
💡 Codebase verification
Missing MaybeValidateAuthenticators Implementation in MemClobKeeper Interface
The MaybeValidateAuthenticators method has been added to the MemClobKeeper interface, but it appears that many implementations do not include this method. Please ensure that all concrete types implementing MemClobKeeper provide an implementation for MaybeValidateAuthenticators to maintain interface compliance.
- protocol/streaming/full_node_streaming_manager.go
- protocol/streaming/noop_streaming_manager.go
- protocol/streaming/ws/websocket_message_sender.go
- protocol/streaming/ws/websocket_server.go
- ... (additional files as identified)
🔗 Analysis chain
New method added to MemClobKeeper interface
The addition of MaybeValidateAuthenticators method aligns with the PR objective of handling unauthorized maker orders. However, there are a few points to consider:
- The method name uses "Maybe", suggesting conditional execution. Consider documenting when this validation occurs.
- The method operates on raw transaction bytes. Ensure that this level of access is necessary and doesn't bypass any security measures.
- As this is an interface method, all implementations of
MemClobKeeperwill need to be updated to include this method.
To ensure all implementations are updated, run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of MemClobKeeper and check for MaybeValidateAuthenticators method
# Find all Go files that might implement MemClobKeeper
implementers=$(fd -e go | xargs grep -l "type.*struct.*{")
# Check each potential implementer for the new method
for file in $implementers; do
if grep -q "func.*MaybeValidateAuthenticators" "$file"; then
echo "Implementation found in $file"
else
echo "Potential missing implementation in $file"
fi
done
Length of output: 148215
| type AccountPlusKeeper interface { | ||
| GetIsSmartAccountActive(ctx sdk.Context) bool | ||
| GetInitializedAuthenticatorForAccount( | ||
| ctx sdk.Context, | ||
| account sdk.AccAddress, | ||
| selectedAuthenticator uint64, | ||
| ) (aptypes.InitializedAuthenticator, error) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
No implementations of the AccountPlusKeeper interface were found.
While the AccountPlusKeeper interface is defined and its methods are being utilized in the codebase, there are currently no concrete implementations of this interface. This could lead to potential runtime issues or indicate an incomplete integration of the new interface.
- Action Items:
- Implement the
AccountPlusKeeperinterface: Ensure that at least one struct in the codebase implements this interface to fulfill its contract. - Review usage points: Verify that all usages of the interface's methods correspond to an actual implementation to prevent potential runtime errors.
- Implement the
🔗 Analysis chain
Verify usage of the new AccountPlusKeeper interface.
The new AccountPlusKeeper interface looks good and aligns with the PR objectives. To ensure its proper integration, please verify that this interface is implemented and used correctly in other parts of the codebase.
Run the following script to check the usage of the new interface:
This script will help ensure that the new interface is properly implemented and its methods are being used in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation and usage of the AccountPlusKeeper interface.
# Test 1: Check for the interface implementation
echo "Checking for AccountPlusKeeper implementation:"
rg --type go "type \w+ struct" -A 10 | rg "AccountPlusKeeper"
# Test 2: Check for the usage of GetIsSmartAccountActive method
echo "Checking for usage of GetIsSmartAccountActive method:"
rg --type go "GetIsSmartAccountActive\("
# Test 3: Check for the usage of GetInitializedAuthenticatorForAccount method
echo "Checking for usage of GetInitializedAuthenticatorForAccount method:"
rg --type go "GetInitializedAuthenticatorForAccount\("
Length of output: 1975
| @@ -44,6 +44,7 @@ type ( | |||
| rewardsKeeper types.RewardsKeeper | |||
| affiliatesKeeper types.AffiliatesKeeper | |||
| revshareKeeper types.RevShareKeeper | |||
| accountPlusKeeper types.AccountPlusKeeper | |||
There was a problem hiding this comment.
💡 Codebase verification
Issues Found: Incomplete Implementation of accountPlusKeeper in NewKeeper
The NewKeeper function signature in protocol/x/clob/keeper/keeper.go has been updated to include the accountPlusKeeper parameter. However, the implementation does not initialize this new field within the Keeper struct. Additionally, multiple calls to NewKeeper across the project are missing the required accountPlusKeeper parameter.
Affected Areas:
protocol/x/clob/keeper/keeper.go:NewKeeperimplementation missing initialization ofaccountPlusKeeper.- Numerous instances across the project where
NewKeeperis called without theaccountPlusKeeperparameter, including but not limited to:protocol/x/stats/keeper/keeper.goprotocol/x/subaccounts/keeper/keeper.goprotocol/testutil/keeper/assets.goprotocol/app/app.go
Please address these issues to ensure that the accountPlusKeeper is properly integrated and initialized throughout the codebase.
🔗 Analysis chain
LGTM. Verify NewKeeper implementation and usage.
The addition of the accountPlusKeeper parameter to the NewKeeper function signature is consistent with the struct change. However, we need to ensure that the implementation properly initializes this new field.
Please verify the following:
-
The
NewKeeperfunction implementation: -
All calls to
NewKeeperhave been updated:
Action Required: Update All NewKeeper Calls to Include accountPlusKeeper
The NewKeeper function signature has been updated to include the accountPlusKeeper parameter. However, the implementation in protocol/x/clob/keeper/keeper.go does not initialize this new field. Additionally, ensure that all calls to NewKeeper across the project are updated to include the accountPlusKeeper parameter to maintain consistency and prevent potential issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Show the NewKeeper function implementation
rg --type go -A 30 'func NewKeeper\(' protocol/x/clob/keeper/keeper.go
Length of output: 1216
Script:
#!/bin/bash
# Find all calls to NewKeeper in the project
rg --type go 'NewKeeper\(' protocol
Length of output: 5220
Script:
#!/bin/bash
# Identify NewKeeper calls missing the 'accountPlusKeeper' parameter
rg --type go 'NewKeeper\(' protocol | grep -v 'accountPlusKeeper'
Length of output: 5248
| _, err := k.accountPlusKeeper.GetInitializedAuthenticatorForAccount( | ||
| ctx, | ||
| account, | ||
| authenticatorId, | ||
| ) | ||
| if err != nil { | ||
| return errorsmod.Wrapf( | ||
| err, | ||
| "selected authenticator (%s, %d) is not registered or removed from state", | ||
| account.String(), | ||
| authenticatorId, | ||
| ) | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Ensure error messages do not disclose sensitive information
When wrapping errors that include account addresses and authenticator IDs, consider whether including this information is appropriate. Exposing sensitive details in error messages may pose security risks.
If necessary, you might limit the amount of detail in the error messages or ensure that such messages are only logged internally and not exposed to end-users.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- protocol/x/clob/abci_test.go (1 hunks)
- protocol/x/clob/keeper/liquidations_test.go (2 hunks)
- protocol/x/clob/keeper/orders_test.go (2 hunks)
🔇 Additional comments (2)
protocol/x/clob/abci_test.go (1)
1215-1222: Order placement test setup correctly initializes transaction contextThe code properly creates a temporary context with
TxBytesto simulate a transaction during order placement in testing. Error handling is appropriately performed usingrequire.NoError(t, err).protocol/x/clob/keeper/liquidations_test.go (1)
1318-1327: Refactor duplicated code for settingTxBytesinto a helper functionThis code block is identical to the one at lines 355-364. Refactoring into a helper function as suggested earlier will reduce code duplication and improve maintainability.
| txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder() | ||
| err := txBuilder.SetMsgs(operation.GetShortTermOrderPlacement()) | ||
| require.NoError(t, err) | ||
| bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx()) | ||
| require.NoError(t, err) | ||
| tempCtx = tempCtx.WithTxBytes(bytes) | ||
|
|
||
| _, _, err = ks.ClobKeeper.PlaceShortTermOrder( |
There was a problem hiding this comment.
Ensure writeCache() is called to commit state changes
After successfully placing the short-term order with PlaceShortTermOrder, you should call writeCache() to commit the changes from the cached context to the main context. Otherwise, the modifications in tempCtx won't persist, and the order placement might not have the intended effect in the test.
| msg := &types.MsgPlaceOrder{Order: order} | ||
|
|
||
| txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder() | ||
| err := txBuilder.SetMsgs(msg) | ||
| require.NoError(t, err) | ||
| bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx()) | ||
| require.NoError(t, err) | ||
| ctx = ctx.WithTxBytes(bytes) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code into a helper function
The code in lines 626-634 and 644-651 is duplicated. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider extracting this repeated code into a helper function.
Apply this diff to refactor the code:
- msg := &types.MsgPlaceOrder{Order: order}
-
- txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
- err := txBuilder.SetMsgs(msg)
- require.NoError(t, err)
- bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
- require.NoError(t, err)
- ctx = ctx.WithTxBytes(bytes)
+ ctx, err = prepareTxWithMsg(ctx, &types.MsgPlaceOrder{Order: order})
+ require.NoError(t, err)- msg := &types.MsgPlaceOrder{Order: tc.order}
-
- txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
- err = txBuilder.SetMsgs(msg)
- require.NoError(t, err)
- bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
- require.NoError(t, err)
- ctx = ctx.WithTxBytes(bytes)
+ ctx, err = prepareTxWithMsg(ctx, &types.MsgPlaceOrder{Order: tc.order})
+ require.NoError(t, err)Define the helper function outside the selected line ranges:
func prepareTxWithMsg(ctx sdk.Context, msg sdk.Msg) (sdk.Context, error) {
txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
err := txBuilder.SetMsgs(msg)
if err != nil {
return ctx, err
}
bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
if err != nil {
return ctx, err
}
return ctx.WithTxBytes(bytes), nil
}Also applies to: 644-651
| msg := &types.MsgPlaceOrder{Order: order} | ||
|
|
||
| txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder() | ||
| err := txBuilder.SetMsgs(msg) | ||
| require.NoError(t, err) | ||
| bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx()) | ||
| require.NoError(t, err) | ||
| ctx = ctx.WithTxBytes(bytes) | ||
|
|
||
| _, _, err = ks.ClobKeeper.PlaceShortTermOrder(ctx, msg) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code for setting TxBytes into a helper function
The code for creating a transaction builder, setting messages, encoding the transaction, and updating ctx.TxBytes is duplicated. Consider refactoring this code into a helper function to improve maintainability and reduce code duplication.
You can create a helper function like this:
func setContextTxBytes(ctx sdk.Context, msg sdk.Msg) (sdk.Context, error) {
txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
err := txBuilder.SetMsgs(msg)
if err != nil {
return ctx, err
}
bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
if err != nil {
return ctx, err
}
return ctx.WithTxBytes(bytes), nil
}Then, replace the duplicated code with:
- txBuilder := constants.TestEncodingCfg.TxConfig.NewTxBuilder()
- err := txBuilder.SetMsgs(msg)
- require.NoError(t, err)
- bytes, err := constants.TestEncodingCfg.TxConfig.TxEncoder()(txBuilder.GetTx())
- require.NoError(t, err)
- ctx = ctx.WithTxBytes(bytes)
+ ctx, err = setContextTxBytes(ctx, msg)
+ require.NoError(t, err)
|
|
||
| // MaybeValidateAuthenticators checks if the transaction has authenticators specified and if so, | ||
| // validates them. It returns an error if the authenticators are not valid or removed from state. | ||
| func (k Keeper) MaybeValidateAuthenticators(ctx sdk.Context, txBytes []byte) error { |
There was a problem hiding this comment.
None of the logic here is specific to x/clob - should this function be a method in CLOB or Accountplus?
| @@ -0,0 +1,73 @@ | |||
| package keeper | |||
There was a problem hiding this comment.
For my understanding, this PR doesn't need to touch ProcessOperations explicitly, because it's already taken care of by calling AnteHandler on the orders right?
There was a problem hiding this comment.
correct.
again, the assumption here is that the light weight validation is sufficient because all current validations are stateless
There was a problem hiding this comment.
Correct, and ProcessOperations do run the anthenticators explicitly since we cannot trust the proposer
| // validates them. It returns an error if the authenticators are not valid or removed from state. | ||
| func (k Keeper) MaybeValidateAuthenticators(ctx sdk.Context, txBytes []byte) error { | ||
| // Decode the tx from the tx bytes. | ||
| tx, err := k.txDecoder(txBytes) |
There was a problem hiding this comment.
We need to decode maker orders for all fills (regardless of smart account) - do we think this is okay because there are not many fews?
There was a problem hiding this comment.
I was planning to look at frame graphs after this is merged and deployed and optimize only if needed. I feel like this might be okay since like you said, only a very small percentage of the orders get matched.
note that all crossing maker orders get decoded, not just those that generate a fill
| // are registered and not removed from state. | ||
| // | ||
| // Note that we only verify the existence of the authenticators here without actually | ||
| // runnning them. This is because all current authenticators are stateless and do not read/modify any states. |
There was a problem hiding this comment.
Could we document the we are not running the authenticators due to deadlock concerns
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
protocol/x/accountplus/keeper/authenticators.go (3)
13-13: Organize Imports for Better ReadabilityThe imports at lines 13 and 16 introduce new packages. Consider grouping imports into standard library packages, third-party packages, and local packages, and separating them with blank lines to improve readability.
Also applies to: 16-16
46-48: Consider Supporting Multiple Signers if ApplicableAt lines 46-48, the code returns an error if there are multiple signers, enforcing that only one signer is allowed.
- Architecture Advice: Evaluate if there are valid cases where multiple signers should be permitted, such as multi-signature transactions. If so, consider updating the logic to handle multiple signers appropriately.
50-51: Simplify Conversion tosdk.AccAddressAt line 50, the code converts the first signer to an
sdk.AccAddress:account := sdk.AccAddress(signers[0])
- Suggestion: Ensure that
signers[0]is already of typesdk.AccAddressto avoid unnecessary conversions. Ifsignersis not guaranteed to be of this type, add appropriate checks or handle potential conversion errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- protocol/x/accountplus/keeper/authenticators.go (1 hunks)
- protocol/x/accountplus/types/errors.go (1 hunks)
- protocol/x/clob/keeper/authenticators.go (1 hunks)
- protocol/x/clob/types/expected_keepers.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- protocol/x/clob/keeper/authenticators.go
- protocol/x/clob/types/expected_keepers.go
🔇 Additional comments (4)
protocol/x/accountplus/types/errors.go (2)
Line range hint
1-35: Overall, the change looks good and aligns with the PR objectives.The addition of
ErrTxnHasMultipleSignersenhances the error handling capabilities of theaccountplusmodule, specifically addressing scenarios where unauthorized maker orders might occur due to multiple signers. This change is consistent with the existing error definitions and contributes to the PR's goal of improving the handling of unauthorized maker orders.
31-35: New error definition looks good, but consider improving the error message.The addition of
ErrTxnHasMultipleSignersis consistent with the existing error definitions in terms of structure and naming convention. The error code (6) is unique within this file, which is correct.However, the error message could be more specific to provide better context:
Consider updating the error message to be more descriptive:
ErrTxnHasMultipleSigners = errorsmod.Register( ModuleName, 6, - "The transaction has multiple signers", + "Unauthorized: Transaction has multiple signers when only one is expected", )This change would provide more context about why multiple signers are problematic in this scenario.
To ensure this error is used appropriately, let's check for its usage:
This will help verify that the error is being used in the correct contexts related to handling unauthorized maker orders.
✅ Verification successful
Verified:
ErrTxnHasMultipleSignersis used appropriatelyThe error
ErrTxnHasMultipleSignersis correctly utilized within theauthenticators.gofile to enforce the single signer constraint. The current implementation ensures that transactions with multiple signers are properly handled.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of ErrTxnHasMultipleSigners rg --type go "ErrTxnHasMultipleSigners"Length of output: 257
protocol/x/accountplus/keeper/authenticators.go (2)
57-72: Optimize Authenticator Retrieval LoopIn the loop from lines 57-72, each selected authenticator is retrieved individually.
- Performance Consideration: If the number of selected authenticators is large, consider batch retrieval methods or caching mechanisms to minimize state reads and improve performance.
[performance]
31-33: Verify the Smart Account Activation CheckAt lines 31-33, the code checks if the smart account flow is active using
k.GetIsSmartAccountActive(ctx). Ensure that the smart account activation state is correctly managed elsewhere in the codebase and that this check aligns with the intended system behavior.Run the following script to verify the usage of
GetIsSmartAccountActive:✅ Verification successful
Smart Account Activation Check Verified
The
GetIsSmartAccountActivemethod correctly retrieves the activation state from the parameters. TheIsSmartAccountActiveparameter is properly defined and managed within the codebase, ensuring that the activation check aligns with the intended system behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify where and how `GetIsSmartAccountActive` is set. # Test: Search for all occurrences of `SetIsSmartAccountActive`. Expect: Places where the smart account flow is activated or deactivated. rg --type go 'SetIsSmartAccountActive'Length of output: 38
Script:
#!/bin/bash # Description: Locate the definition and implementation of `GetIsSmartAccountActive`. # Search for the definition of the method `GetIsSmartAccountActive` within the Go codebase. ast-grep --lang go --pattern $'func (k *Keeper) GetIsSmartAccountActive($_) bool { $$$ }'Length of output: 332
Script:
#!/bin/bash # Description: Locate the definition of the parameters struct containing `IsSmartAccountActive` and all its assignments. # Search for the struct definition that includes `IsSmartAccountActive`. ast-grep --lang go --pattern $'type $_ struct { $$$ IsSmartAccountActive $_ $$$ }' # Search for all assignments to `IsSmartAccountActive`. ast-grep --lang go --pattern $'$_.IsSmartAccountActive = $_'Length of output: 498
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
protocol/testutil/keeper/accountplus.go (2)
Line range hint
19-41: LGTM! Consider updating the comment for clarity.The renaming from
TimestampNonceKeeperstoAccountPlusKeepersaligns well with the shift towards AccountPlus functionality. The changes are consistent and maintain the existing logic.Consider adding a brief comment above the function to describe its purpose, e.g.:
// AccountPlusKeepers initializes and returns the necessary keepers and context for AccountPlus module testing. func AccountPlusKeepers(t testing.TB) ( // ... (existing parameters) ) { // ... (existing function body) }
Line range hint
44-68: LGTM! Consider updating import alias for consistency.The renaming from
createTimestampNonceKeepertocreateAccountPlusKeeperis consistent with the overall changes and maintains the existing logic.For better consistency and readability, consider updating the import alias for the keeper package:
- keeper "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/keeper" + accountpluskeeper "github.com/dydxprotocol/v4-chain/protocol/x/accountplus/keeper"Then update the usage:
- k := keeper.NewKeeper( + k := accountpluskeeper.NewKeeper( // ... (existing parameters) )This change would make it clearer that we're dealing with the AccountPlus keeper specifically.
protocol/testutil/keeper/listing.go (2)
61-65: Update tests and documentation foraccountPlusKeeper.With the introduction of
accountPlusKeeper:
- Ensure that relevant test cases have been updated or added to cover the new functionality.
- Update any affected documentation, including API docs and README files, to reflect this change.
- Consider adding comments explaining the purpose and importance of
accountPlusKeeperin the CLOB module.Also applies to: 163-163
Line range hint
1-224: Summary: NewaccountPlusKeeperadded to keeper initialization.The changes introduce
accountPlusKeeperto the keeper initialization process, specifically integrating it with the CLOB module. While the implementation looks correct, please provide more context on:
- The purpose and functionality of
accountPlusKeeper.- Why it was necessary to add this keeper to the CLOB module.
- Any expected changes in system behavior due to this addition.
This information will help ensure that the changes align with the PR objectives and that all potential impacts have been considered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- protocol/testutil/keeper/accountplus.go (3 hunks)
- protocol/testutil/keeper/clob.go (6 hunks)
- protocol/testutil/keeper/listing.go (2 hunks)
🔇 Additional comments (8)
protocol/testutil/keeper/accountplus.go (1)
Line range hint
1-68: Overall, the changes look good and align with the PR objectives.The renaming of functions from TimestampNonce to AccountPlus is consistent throughout the file and reflects the shift in focus towards AccountPlus functionality. The logic remains intact, ensuring that the existing behavior is preserved while improving clarity.
A few minor suggestions have been made to further enhance readability and consistency. Great job on maintaining code quality while refactoring!
protocol/testutil/keeper/listing.go (1)
59-60: LGTM: Minor formatting improvements.The alignment of closing parentheses enhances code readability.
protocol/testutil/keeper/clob.go (6)
20-20: ImportingaccountpluskeeperPackageThe import statement for
accountpluskeeperis correctly added, which integrates the AccountPlus functionality into the module.
55-55: AddingAccountPlusKeeperField toClobKeepersTestContextThe addition of the
AccountPlusKeeperfield to theClobKeepersTestContextstruct is appropriate for incorporating AccountPlus functionality into the test context.
96-102: InitializingAccountPlusKeeperin Test ContextThe
AccountPlusKeeperis properly initialized using thecreateAccountPlusKeeperfunction, ensuring it's ready for use in the test context.
202-202: PassingAccountPlusKeepertocreateClobKeeperThe
AccountPlusKeeperis correctly passed to thecreateClobKeeperfunction, integrating it into the Clob keeper's initialization.
243-243: UpdatingcreateClobKeeperFunction SignatureThe
createClobKeeperfunction signature now includes theaccountplusKeeperparameter, which is necessary for the integration with the AccountPlus module.
275-275: Verifykeeper.NewKeeperAcceptsaccountplusKeeperParameterEnsure that the
keeper.NewKeeperfunction has been updated to accept theaccountplusKeeperparameter and that all instances wherekeeper.NewKeeperis called include this new parameter.Run the following script to confirm:
#!/bin/bash # Description: Verify `keeper.NewKeeper` function definition and its usage. # Test 1: Check the function definition includes `accountplusKeeper`. Expect: Function definition should have `accountplusKeeper` as a parameter. rg --type go 'func NewKeeper\(' -A 10 | rg 'accountplusKeeper' # Test 2: Verify all calls to `keeper.NewKeeper` include `accountplusKeeper`. Expect: All calls should pass `accountplusKeeper` as an argument. rg --type go 'keeper\.NewKeeper\(' -A 10 | rg 'accountplusKeeper'
…rs are removed
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
AccountPlusKeeperfor improved account management across modules.Improvements
Bug Fixes
Documentation