feat(accounts): implement account abstraction execution #18499
Conversation
WalkthroughWalkthroughThe changes involve enhancements to the Cosmos SDK, focusing on message routing, account abstraction, and protobuf compatibility. A new Changes
TipsChat with CodeRabbit Bot (
|
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/mint
C:x/auth
and removed
C:x/staking
C:CLI
C:x/gov
C:x/mint
C:x/auth
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 54
Configuration used: CodeRabbit UI
Files ignored due to filter (7)
- testutil/testdata/query.pb.go
- testutil/testdata/testdata.pb.go
- testutil/testdata/testpb/query_grpc.pb.go
- testutil/testdata/testpb/tx_grpc.pb.go
- testutil/testdata/tx.pb.go
- testutil/testdata/unknonwnproto.pb.go
- x/accounts/v1/account_abstraction.pb.go
Files selected for processing (30)
- CHANGELOG.md (1 hunks)
- api/cosmos/accounts/interfaces/account_abstraction/v1/interface.pulsar.go (43 hunks)
- api/cosmos/accounts/v1/account_abstraction.pulsar.go (40 hunks)
- baseapp/internal/protocompat/protocompat.go (1 hunks)
- baseapp/msg_service_router.go (3 hunks)
- proto/cosmos/accounts/interfaces/account_abstraction/v1/interface.proto (3 hunks)
- proto/cosmos/accounts/v1/account_abstraction.proto (3 hunks)
- simapp/app.go (2 hunks)
- tests/e2e/accounts/account_abstraction_test.go (1 hunks)
- tests/e2e/accounts/setup_test.go (1 hunks)
- testutil/testdata/testpb/query.pulsar.go (1 hunks)
- testutil/testdata/testpb/testdata.pulsar.go (1 hunks)
- testutil/testdata/testpb/tx.pulsar.go (1 hunks)
- testutil/testdata/testpb/unknonwnproto.pulsar.go (1 hunks)
- x/accounts/accountstd/exports.go (2 hunks)
- x/accounts/genesis_test.go (2 hunks)
- x/accounts/internal/implementation/api_builder.go (1 hunks)
- x/accounts/internal/implementation/context.go (2 hunks)
- x/accounts/internal/implementation/context_test.go (2 hunks)
- x/accounts/internal/implementation/proto_util.go (1 hunks)
- x/accounts/keeper.go (12 hunks)
- x/accounts/keeper_account_abstraction.go (1 hunks)
- x/accounts/keeper_test.go (6 hunks)
- x/accounts/module.go (2 hunks)
- x/accounts/msg_server.go (1 hunks)
- x/accounts/msg_server_test.go (2 hunks)
- x/accounts/query_server_test.go (2 hunks)
- x/accounts/testing/account_abstraction/full.go (1 hunks)
- x/accounts/testing/account_abstraction/minimal.go (1 hunks)
- x/accounts/utils_test.go (1 hunks)
Files not summarized due to errors (2)
- api/cosmos/accounts/interfaces/account_abstraction/v1/interface.pulsar.go: Error: Message exceeds token limit
- api/cosmos/accounts/v1/account_abstraction.pulsar.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (5)
- testutil/testdata/testpb/query.pulsar.go
- testutil/testdata/testpb/testdata.pulsar.go
- testutil/testdata/testpb/tx.pulsar.go
- testutil/testdata/testpb/unknonwnproto.pulsar.go
- x/accounts/msg_server_test.go
Additional comments: 47
tests/e2e/accounts/setup_test.go (1)
- 9-13: The
setupAppfunction correctly sets up asimapp.SimAppinstance for testing purposes. It's good practice to callt.Helper()to mark the function as a test helper function. However, ensure that thesimapp.Setupfunction does not require any additional parameters or configuration changes to support the new account abstraction features mentioned in the pull request summary.x/accounts/module.go (1)
- 14-20: The addition of the new import statement for "github.com/cosmos/cosmos-sdk/types/address" is appropriate given the use of the
addresspackage on line 28. This change is necessary for the new functionality being introduced.baseapp/internal/protocompat/protocompat.go (1)
- 225-235: The new function
ResponseFullNameFromMethodDescis a good addition for symmetry withRequestFullNameFromMethodDescand will be useful for retrieving the response type associated with a gRPC method. However, ensure that there are corresponding tests for this new function to verify its behavior and handle edge cases.x/accounts/query_server_test.go (2)
6-11: The removal of the import "google.golang.org/protobuf/runtime/protoiface" is appropriate given the change in the function signature from
protoiface.MessageV1toproto.Message. This change aligns with the updated usage within the test function.17-21: The change from
k.queryModuleFunctok.queryRouterand the corresponding function signature update fromprotoiface.MessageV1toproto.Messageis consistent with the broader updates to the Cosmos SDK's account abstraction feature. This change simplifies the interface and aligns with the updated protobuf definitions. Ensure that all references to the old function and its signature are updated across the codebase to prevent any breakage.proto/cosmos/accounts/interfaces/account_abstraction/v1/interface.proto (3)
8-18: The addition of the
bundlerfield toMsgAuthenticateis consistent with the change summary. However, ensure that all services that construct or handleMsgAuthenticatemessages are updated to include and process this new field. Also, verify that the documentation and comments are updated to reflect this change.27-37: Similar to the
MsgAuthenticate, thebundlerfield has been added toMsgPayBundler. Ensure that the corresponding handling logic and documentation are updated accordingly. Additionally, verify that the security implications of allowing the account to modify thebundler_payment_messagesare thoroughly reviewed and documented.48-58: The
bundlerfield has been added toMsgExecuteas well. Ensure that the handling logic for this message type is updated to accommodate the new field. It's also important to ensure that theexecution_messagescan be safely modified by the account, as this could have security implications.simapp/app.go (2)
22-28: The import statement for
"cosmossdk.io/x/accounts/testing/account_abstraction"has been added to include the account abstraction testing accounts. This is part of the feature update to implement account abstraction execution in the Cosmos SDK. Ensure that this new dependency is properly managed and that the package is used elsewhere in the code as expected.285-296: The
accountsKeeperis being initialized with new account types "counter", "aa_minimal", and "aa_full". This is part of the setup for account abstraction testing. Ensure that the corresponding functionscounter.NewAccount,account_abstraction.NewMinimalAbstractedAccount, andaccount_abstraction.NewFullAbstractedAccountare correctly implemented and that they adhere to the expected interface for account types. Additionally, verify that error handling is correctly implemented for the case whereaccounts.NewKeeperreturns an error, as the application will panic in such a scenario.x/accounts/genesis_test.go (2)
5-11: The import of
"google.golang.org/protobuf/runtime/protoiface"has been correctly replaced with"google.golang.org/protobuf/proto". This change aligns with the usage ofproto.Messagein the test file. Ensure that there are no remaining references toprotoifacethat would require this import.17-18: The assignment of
k.queryRouterhas been updated to use amockQueryfunction. This is a good practice for unit testing as it allows for the isolation of the test environment from external dependencies. However, ensure that themockQueryfunction is defined somewhere in the test file or test helpers, as it is not shown in the provided code.x/accounts/internal/implementation/context_test.go (2)
6-11: The import
google.golang.org/protobuf/runtime/protoifacehas been removed, which is good if it's no longer used. However, ensure that no other parts of the codebase are affected by this removal. If this import was used elsewhere, those parts of the code will need to be updated accordingly.43-74: The refactoring of
MakeAccountContextto accept different function signatures for module execution and querying is a good approach to enhance flexibility and maintainability. It allows for more granular control over the behavior of the account context in different scenarios. The tests are well-structured and ensure that the context is correctly set up and that the execution and querying of modules work as expected. The use of generics withExecModuleandExecModuleUntypedis a modern feature of Go that improves type safety and reduces the need for type assertions.x/accounts/accountstd/exports.go (3)
2-21: The introduction of a new global variable
accountsModuleAddressis a significant change. Ensure that this variable is used consistently across the codebase and that its introduction does not introduce any security or logical issues, such as unauthorized access to the accounts module address or conflicts with other modules.72-78: The function
SenderIsAccountsModuleis a security-sensitive check. It is crucial to ensure that the comparison between the sender's address and theaccountsModuleAddressis done securely and correctly. Usingbytes.Equalis appropriate for this purpose, but ensure that theSenderfunction is reliable and cannot be spoofed or manipulated.80-126: The
ExecModuleAnysfunction is performing multiple operations that could fail, such as unpacking messages, executing them, and packing responses. It is important to ensure that the error handling is robust and that any errors are logged or handled appropriately. Additionally, consider the implications of a partial failure where some messages are executed successfully while others fail. The current implementation will return on the first error, which may or may not be the desired behavior depending on the use case.proto/cosmos/accounts/v1/account_abstraction.proto (3)
18-26: The reassignment of field numbers in the
UserOperationmessage is a breaking change. Ensure that all serialized data and client code that interacts with these messages are updated to reflect the new field numbers. This is critical for maintaining compatibility and preventing potential data corruption or misinterpretation of message fields.30-44: The addition of
bundler_payment_messagesandexecution_messagesasrepeated google.protobuf.Anyfields allows for flexibility in the types of messages that can be included. However, it's important to ensure that the code handling these messages is equipped to safely unpack theAnytype and handle the dynamic message types appropriately. This is crucial for preventing runtime errors and ensuring the robustness of the message handling logic.58-66: The addition of the
errorfield to theUserOperationResponsemessage is a good practice for error handling. It allows clients to understand the nature of the failure without having to infer it from other fields. However, ensure that the error messages are clear, actionable, and properly documented so that clients can handle them effectively.x/accounts/internal/implementation/context.go (5)
13-13: The
AccountStatePrefixis being created with a fixed value of 255. This could potentially lead to conflicts if other parts of the system use the same prefix value. It's important to ensure that this prefix is unique across the system to avoid key collisions in the store.23-30: The
contextValuestruct has been updated to includemoduleExecUntyped. This change should be reflected in all parts of the code wherecontextValueis used to ensure that the new functionality is properly integrated and that there are no missing or misaligned usages of the struct.42-57: The
MakeAccountContextfunction has been updated to include a new parametermoduleExecUntyped. Ensure that all invocations of this function are updated to pass the new argument. Additionally, consider if the function signature change could break any existing code that relies on the previous signature.60-71: The new
ExecModuleUntypedfunction has been added to execute a module message when the response type is unknown. This is a significant addition and should be thoroughly tested to ensure that it handles all possible error conditions and edge cases correctly. It's also important to ensure that the function is thread-safe if it's going to be used in a concurrent environment.73-83: The
ExecModulefunction has been updated to include the newmoduleExecUntypedparameter. This change should be carefully reviewed to ensure that it doesn't introduce any regressions or bugs, especially since it affects the execution of module messages. Additionally, the type assertion on line 76 should be accompanied by a check to handle the case where the assertion fails, to prevent potential panics at runtime.x/accounts/utils_test.go (1)
- 1-80: The test utilities introduced in this hunk seem to be well-structured and follow the interface definitions required for the Cosmos SDK's account abstraction feature. The
addressCodec,eventService,mockQuery,mockSigner, andmockExecare all mock implementations of their respective interfaces, which is a common pattern for unit testing. ThenewKeeperfunction is a helper that sets up a newKeeperinstance for testing, and the use oft.Helper()is appropriate for indicating that this is a test helper function.However, there are a few points to consider:
- The
addressCodecimplementation is very simplistic and may not reflect the actual complexity of address encoding and decoding in a real-world scenario. Ensure that this simplicity is sufficient for the tests being written.- The
eventServiceand its methods do not perform any actions. If event emission is a critical part of the functionality being tested, you may need to implement a more sophisticated mock that can track emitted events.- The
mockQuery,mockSigner, andmockExectypes are defined as function types, which is a flexible way to create mocks that can have different behaviors for different tests. Just ensure that the tests using these mocks are providing the necessary function implementations to simulate the desired behavior.- The
newKeeperfunction does not allow for customization of theeventService,addressCodec, or other dependencies beyond theAccountCreatorFunc. If tests require more control over these dependencies, consider adding parameters tonewKeeperto allow for this customization.Overall, the test utilities appear to be correctly implemented for the purpose of unit testing within the Cosmos SDK's account abstraction feature. Just ensure that the simplicity of the mocks aligns with the requirements of the tests they are intended to support.
x/accounts/testing/account_abstraction/full.go (2)
3-9: The import statements are clear and well-organized. However, ensure that the new dependency
account_abstractionv1is properly managed in the project's dependency management files (e.g.,go.mod) and that the correct version is being used.65-77: The
RegisterInitHandler,RegisterExecuteHandlers, andRegisterQueryHandlersmethods delegate to the embeddedMinimalAbstractedAccount. This is a good use of composition to avoid code duplication. However, ensure that the delegation toMinimalAbstractedAccountis intentional and that there are no additional handlers thatFullAbstractedAccountshould be registering.x/accounts/testing/account_abstraction/minimal.go (3)
3-14: The import section looks clean and well-organized. However, ensure that all imported packages are used within the file to avoid unnecessary dependencies.
23-28: The
NewMinimalAbstractedAccountfunction correctly initializes a newMinimalAbstractedAccountwith a public key and sequence. Ensure that error handling is implemented if any of the initialization steps can fail.59-70: The registration of handlers for initialization, execution, and querying is done correctly. However, ensure that the handlers being registered (e.g.,
RotatePubKey,Authenticate,QueryAuthenticateMethods) are fully implemented and tested, as some are currently returning "not implemented" errors.baseapp/msg_service_router.go (6)
31-36: The addition of the
responseByRequestmap to theMsgServiceRouterstruct is a significant change. It's important to ensure that all parts of the code that interact withMsgServiceRouterare aware of this new map and handle it appropriately. This change could potentially break existing functionality if not integrated correctly. Make sure to update any relevant documentation and consider the impact on serialization ifMsgServiceRouteris ever persisted or sent over the network.40-47: The constructor
NewMsgServiceRouterhas been updated to initialize the newresponseByRequestmap. This is a good practice as it ensures that the map is nevernil, preventing potentialnilpointer dereferences when the map is used later on.93-95: The new method
ResponseByRequestNameprovides a way to retrieve the response type based on the request name. It's important to ensure that this method is used consistently across the codebase wherever such a mapping is required. Additionally, consider adding error handling for cases where the request name is not found in the map to prevent unexpected behavior.97-115: The
registerHybridHandlermethod has been modified to include the mapping of request types to their corresponding response types. This is a critical change that enhances the message routing capabilities of theMsgServiceRouter. However, it's important to ensure that theinputNameandoutputNameare always valid and that the mapping does not introduce any conflicts or overwrite existing entries without proper checks. Also, consider what should happen if a mapping already exists for a giveninputName. Should it be overwritten, ignored, or should an error be raised? This needs to be clearly defined to avoid unexpected behavior.112-112: The mapping of
inputNametooutputNameis done by casting them tostring. Ensure that theprotocompat.RequestFullNameFromMethodDescandprotocompat.ResponseFullNameFromMethodDescmethods guarantee that the returned names are unique and valid for casting tostring. If there's any possibility of non-unique names, this could lead to incorrect behavior in message routing.114-115: The check for
msr.circuitBreaker == nilis used to decide whether to decorate the handler with a circuit breaker. It's important to ensure that the circuit breaker is correctly initialized before this method is called. If the circuit breaker is intended to be optional, then this logic is correct. However, if a circuit breaker is expected to be present, then the absence of it should be handled as an error condition.x/accounts/keeper_account_abstraction.go (1)
- 3-12: The import section is well-organized and follows the convention of grouping standard library imports separately from third-party libraries. However, it's important to ensure that all imported packages are used within the file to avoid unnecessary dependencies. If any of the imported packages are not used, they should be removed to keep the code clean and maintainable.
x/accounts/keeper.go (4)
3-22: The new imports introduced here should be reviewed to ensure they are all necessary for the functionality being added. Unused imports should be removed to keep the code clean and maintainable.
28-32: The error
errAccountTypeNotFoundis defined but not used in the provided code. If it is not used elsewhere in the codebase, it should be removed to avoid dead code.74-91: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [65-89]
The
NewKeeperfunction has been updated to take an additionalBranchExecutorparameter and new fields have been added to theKeeperstruct. Ensure that all instances whereNewKeeperis called have been updated to pass the new parameter. Additionally, verify that the new fields are properly initialized and used throughout the codebase.
- 140-153: The
Initmethod now takes an additionalctxparameter. Ensure that all calls to this method have been updated accordingly. Also, verify that the context is being used correctly within the method.x/accounts/keeper_test.go (5)
6-26: The test
TestKeeper_Inithas been updated to use amockQueryfunction for thequeryRouter. This mock function is specific to aQueryBalanceRequestandQueryBalanceResponsefrom thebankv1beta1package. Ensure that this mock is sufficient for all tests withinTestKeeper_Initand that it does not need to handle other request types. If other request types are needed, the mock function should be adjusted accordingly.53-58: The
TestKeeper_Executefunction setup includes creating a new account. It's important to ensure that the account creation process is successful before proceeding with the rest of the tests. The test should include assertions to verify that the account was created correctly and that the initial state is as expected.73-86: The
TestKeeper_Executefunction includes a subtest "exec module" that uses amockExecfunction for themsgRouterand amockSignerfor thesignerProvider. This test is specifically checking the execution of aMsgSendfrom thebankv1beta1package. Ensure that themockExecandmockSignerfunctions are correctly simulating the expected behavior of themsgRouterandsignerProviderin the context of theExecutemethod. If there are other message types that need to be tested, consider adding additional cases or a more generic mock function.94-98: In the
TestKeeper_Queryfunction, a genericmockQueryfunction is set up for thequeryRouterthat does not perform any specific checks or actions. This might be intentional for certain tests, but if specific behavior is expected from thequeryRouterduring the tests, the mock function should be updated to reflect that behavior.116-122: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [116-130]
The subtest "query module" within
TestKeeper_Queryis using amockQueryfunction that is specific to aQueryBalanceRequestand sets a fixed response. This is a good practice for unit testing as it isolates the test scenario. However, ensure that the test is comprehensive and covers all necessary aspects of theQuerymethod's behavior, including error handling and edge cases.
| BundlerPaymentMessages: intoAny(t, &bankv1beta1.MsgSend{ | ||
| FromAddress: aaFullAddrStr, | ||
| ToAddress: bundlerAddrStr, | ||
| Amount: coins(t, "1stake"), // we expect this to fail since the account is implement in such a way not to allow bank sends. | ||
| }), | ||
| BundlerPaymentGasLimit: 50000, | ||
| ExecutionMessages: intoAny(t, &bankv1beta1.MsgSend{ | ||
| FromAddress: aaFullAddrStr, | ||
| ToAddress: aliceAddrStr, | ||
| Amount: coins(t, "2000stake"), | ||
| }), | ||
| ExecutionGasLimit: 36000, | ||
| }) | ||
| // in order to assert the call we expect an error to be returned. | ||
| require.Contains(t, resp.Error, accounts.ErrBundlerPayment.Error()) // error is bundler payment | ||
| require.Contains(t, resp.Error, "this account does not allow bank send messages") // error is bundler payment | ||
| }) | ||
|
|
||
| t.Run("implements execution - fail", func(t *testing.T) { | ||
| resp := ak.ExecuteUserOperation(ctx, bundlerAddrStr, &accountsv1.UserOperation{ | ||
| Sender: aaFullAddrStr, | ||
| AuthenticationMethod: "secp256k1", | ||
| AuthenticationData: []byte("signature"), | ||
| AuthenticationGasLimit: 10000, | ||
| BundlerPaymentMessages: nil, | ||
| BundlerPaymentGasLimit: 50000, | ||
| ExecutionMessages: intoAny(t, &stakingv1beta1.MsgDelegate{ | ||
| DelegatorAddress: aaFullAddrStr, | ||
| ValidatorAddress: "some-validator", | ||
| Amount: coins(t, "2000stake")[0], | ||
| }), | ||
| ExecutionGasLimit: 36000, | ||
| }) | ||
| // in order to assert the call we expect an error to be returned. | ||
| require.Contains(t, resp.Error, accounts.ErrExecution.Error()) // error is in execution | ||
| require.Contains(t, resp.Error, "this account does not allow delegation messages") | ||
| }) | ||
|
|
||
| t.Run("implements bundler payment and execution - success", func(t *testing.T) { | ||
| // we simulate the abstracted account pays the bundler using an NFT. | ||
| require.NoError(t, app.NFTKeeper.SaveClass(ctx, nft.Class{ | ||
| Id: "omega-rare", | ||
| })) | ||
| require.NoError(t, app.NFTKeeper.Mint(ctx, nft.NFT{ | ||
| ClassId: "omega-rare", | ||
| Id: "the-most-rare", | ||
| }, aaFullAddr)) | ||
|
|
||
| resp := ak.ExecuteUserOperation(ctx, bundlerAddrStr, &accountsv1.UserOperation{ | ||
| Sender: aaFullAddrStr, | ||
| AuthenticationMethod: "secp256k1", | ||
| AuthenticationData: []byte("signature"), | ||
| AuthenticationGasLimit: 10000, | ||
| BundlerPaymentMessages: intoAny(t, &nftv1beta1.MsgSend{ | ||
| ClassId: "omega-rare", | ||
| Id: "the-most-rare", | ||
| Sender: aaFullAddrStr, | ||
| Receiver: bundlerAddrStr, | ||
| }), | ||
| BundlerPaymentGasLimit: 50000, | ||
| ExecutionMessages: intoAny(t, &bankv1beta1.MsgSend{ | ||
| FromAddress: aaFullAddrStr, | ||
| ToAddress: aliceAddrStr, | ||
| Amount: coins(t, "2000stake"), | ||
| }), | ||
| ExecutionGasLimit: 36000, | ||
| }) | ||
| require.Empty(t, resp.Error) // no error | ||
| }) | ||
| } | ||
|
|
||
| func intoAny(t *testing.T, msgs ...proto.Message) (anys []*anypb.Any) { | ||
| t.Helper() | ||
| for _, msg := range msgs { | ||
| any, err := anyutil.New(msg) | ||
| require.NoError(t, err) | ||
| anys = append(anys, any) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| func coins(t *testing.T, s string) []*v1beta1.Coin { | ||
| t.Helper() | ||
| coins, err := sdk.ParseCoinsNormalized(s) | ||
| require.NoError(t, err) | ||
| coinsv2 := make([]*v1beta1.Coin, len(coins)) | ||
| for i, coin := range coins { | ||
| coinsv2[i] = &v1beta1.Coin{ | ||
| Denom: coin.Denom, | ||
| Amount: coin.Amount.String(), | ||
| } | ||
| } | ||
| return coinsv2 | ||
| } | ||
|
|
||
| func balanceIs(t *testing.T, ctx context.Context, app *simapp.SimApp, addr sdk.AccAddress, s string) { | ||
| t.Helper() | ||
| balance := app.BankKeeper.GetAllBalances(ctx, addr) | ||
| require.Equal(t, s, balance.String()) | ||
| } |
There was a problem hiding this comment.
The test suite for account abstraction seems comprehensive and covers a variety of scenarios. However, there are a few points that could be improved for better maintainability and readability:
- The
intoAnyandcoinshelper functions are used to convert messages and coin amounts to the required types. These functions are called multiple times with similar parameters. Consider refactoring these functions to reduce redundancy and improve code clarity. - The test cases use hardcoded strings for error messages, such as
"this account does not allow bank send messages". It's better to use constants for such strings to avoid duplication and ease future changes. - The test cases use hardcoded coin denominations like
"stake"and"atom". It would be more maintainable to define these as constants at the beginning of the test file. - The
balanceIsfunction asserts the balance of an account. It would be beneficial to include a comment explaining the expected state of the balance for clarity. - The test cases are well-named, indicating the scenario being tested. However, adding comments to each test case explaining the scenario in more detail would be helpful for future maintainers or other developers looking at the tests.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (3)
- baseapp/msg_service_router.go (3 hunks)
- x/accounts/keeper.go (12 hunks)
- x/accounts/utils_test.go (1 hunks)
Additional comments: 14
baseapp/msg_service_router.go (1)
- 97-115: The
registerHybridHandlermethod has been updated to populate theresponseByRequestmap and to potentially wrap the hybrid handler with a circuit breaker. This is a critical section of code that needs to ensure that the mapping and the circuit breaker logic are correctly implemented. It's important to verify that theinputNameandoutputNameare being correctly determined and that the circuit breaker is being applied as intended. Additionally, the error handling within this method should be reviewed to ensure that it is consistent and that any errors are properly reported and handled.x/accounts/keeper.go (6)
103-114: The
Keeperstruct has been extended with new fields. Ensure that these fields are properly encapsulated and that their access is controlled to maintain the integrity of theKeeperstate.163-170: The
Initmethod sets up mappings between account addresses and types, as well as account numbers and addresses. Ensure that these mappings are consistent and that there are no potential conflicts or overwrites that could occur.246-252: The
makeAccountContextfunction creates a context with various capabilities based on whether it is a query or not. Ensure that the permissions and capabilities granted by the context are appropriate and secure for the intended operations.272-291: The
sendAnyMessageshelper function executes a series ofanypb.Anymessages. Ensure that the messages are validated and that the execution is secure, especially since the messages can belong to any module.313-335: The
sendModuleMessagefunction sends a message to a module and expects the caller to know the response type. Ensure that the sender verification logic is robust and that there is no way for a sender to bypass the authorization checks.337-355: The
queryModulefunction andgetMessageNameutility function are introduced. Ensure that the query routing logic is secure and that there is no ambiguity in the handling of multiple query handlers.x/accounts/utils_test.go (7)
3-15: The import section is well-organized and follows the convention of grouping standard library imports separately from third-party libraries. This is good for readability and maintainability.
17-22: The
addressCodecstruct correctly implements theaddress.Codecinterface. The methodsStringToBytesandBytesToStringare simple and appear to be correct, assuming that the string representation of the address is the same as its byte representation. However, in a real-world scenario, address encoding and decoding might require more complex logic to handle different address formats and potential validation.24-36: The
eventServicestruct implements theevent.Managerinterface with stub methods that do nothing and return nil. This is acceptable for testing purposes, but it's important to ensure that these stubs are not used in production code where actual event handling logic is required.38-43: The
newKeeperfunction is a test helper that creates aKeeperinstance for testing. It uses thecolltest.MockStore()to create a mock store and context, which is good for isolating tests. The use ofrequire.NoErrorensures that the test will fail immediately if an error occurs during the creation of theKeeper, which is a good practice for test clarity and debugging.46-54: The
mockQuerytype is a function type that implements theQueryRouterinterface. TheHybridHandlerByRequestNamemethod returns a slice of handler functions that cast theprotoiface.MessageV1toproto.Messageand call themockQueryfunction. This is a clever way to mock the query routing for testing purposes, but it assumes that thereqandresparguments can always be safely cast toproto.Message. This should be fine for tests, but it's important to ensure that the actual implementation handles type assertions safely.56-66: The
mockSignertype is a function type that implements theSignerProviderinterface. TheGetSignersmethod calls themockSignerfunction and wraps the result in a slice of byte slices. This is a simple and effective way to mock the signer for testing. However, it's important to ensure that the actual signer implementation handles errors and edge cases appropriately.68-80: The
mockExectype is a function type that implements theMsgRouterinterface. TheHybridHandlerByMsgNamemethod returns a handler function that casts theprotoiface.MessageV1toproto.Messageand calls themockExecfunction. TheResponseNameByRequestNamemethod appends "Response" to the request name to generate the response name. This is a simple and effective way to mock message execution for testing. However, it's important to ensure that the actual message execution logic handles message routing and response generation correctly.
| func (k Keeper) makeAddress(accNum uint64) ([]byte, error) { | ||
| // TODO: better address scheme, ref: https://github.com/cosmos/cosmos-sdk/issues/17516 | ||
| addr := sha256.Sum256(append([]byte("x/accounts"), binary.BigEndian.AppendUint64(nil, num)...)) | ||
| addr := sha256.Sum256(append([]byte("x/accounts"), binary.BigEndian.AppendUint64(nil, accNum)...)) | ||
| return addr[:], nil |
There was a problem hiding this comment.
The makeAddress function uses a SHA-256 hash to generate an address. This approach may not be collision-resistant if the account number space is large. Consider using a more robust scheme for address generation, as noted in the TODO comment.
There was a problem hiding this comment.
left two comments but the design is confusing me a bit. What is the difference between aa and accounts now? why does aa have to be native to accounts? i thought the point of accounts is that nothing needs to be native? what are the tradeoffs?
Could a README be added to accounts explaining the design and some docs on it
| @@ -221,3 +221,16 @@ func RequestFullNameFromMethodDesc(sd *grpc.ServiceDesc, method grpc.MethodDesc) | |||
| } | |||
| return methodDesc.Input().FullName(), nil | |||
| } | |||
|
|
|||
| @@ -31,6 +31,7 @@ type MsgServiceRouter struct { | |||
| interfaceRegistry codectypes.InterfaceRegistry | |||
| routes map[string]MsgServiceHandler | |||
| hybridHandlers map[string]func(ctx context.Context, req, resp protoiface.MessageV1) error | |||
| responseByRequest map[string]string | |||
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- baseapp/internal/protocompat/protocompat.go (2 hunks)
Additional comments: 2
baseapp/internal/protocompat/protocompat.go (2)
26-30: The implementation of
MakeHybridHandlerlooks correct and handles both gogo and protov2 messages as intended.223-224: The changes to
RequestFullNameFromMethodDesclook correct and should properly return the fully-qualified name of the request message.
| if err := k.AccountsByType.Set(ctx, accountAddr, accountType); err != nil { | ||
| return nil, nil, err | ||
| } | ||
| // map account number to account address | ||
| if err := k.AccountByNumber.Set(ctx, accountAddr, num); err != nil { |
There was a problem hiding this comment.
nit: would it make sense to merge these 2 into a single map?
|
Question, in practice what are the cases in which the response can be known/unknown? ( I need to go over the entire thing again as I don't fully understand it 😅. |
Description
BaseApp.MsgRouter changes
We add a new method in the router to extract the response type given the request name, this is needed since the Handler API follows gRPC semantics, in which both request and response are well-known types as can be seen by the following function signature: https://github.com/cosmos/cosmos-sdk/blob/main/baseapp/internal/protocompat/protocompat.go#L24 (response is passed in and not returned, whihc assumes we know the response).
Problem is that we cannot assume to always know how request and responses map together. So this method simply allows a consumer to know which is the response type expected from a handler.
Introduction of account abstraction execution logic in x/accounts
This PR also introduces the execution logic of a user operation, the execution logic can either be executed in a single step, so with the execution of
ExecuteUserOperation, or each method can be executed in separate steps.Specifically:
Authenticate
Takes care of running authentication in an x/account which implements the AA interface.
Execute Messages
Runs the Execution step of TX messages, in case it's not implemented by an x/account, then it defaults to simply sending the messages to the router after verifying that no impersonation has happened.
Pay Bundler
Behaves the same as Execute Messages.
x/accounts refactor
This PR also adds a small cleanup on x/accounts in:
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
Improvements
MsgServiceRouterwith additional methods for response type retrieval.Keeperstructure with new methods for message and query handling.Refactor
Bug Fixes
ExecuteBundlefunction.Documentation
MsgRouterresponse type enhancements.