feat(bank/v2): Add MsgSend handler #21736
Conversation
WalkthroughWalkthroughThe changes in this pull request primarily enhance the registration and handling of services within the Cosmos SDK's banking module. Key modifications include updates to service registration methods, the introduction of new message types for transactions, and enhancements to query capabilities. Additionally, the changes facilitate the transition to a new version of the bank module, ensuring compatibility with updated Genesis states and improving the overall functionality of the banking system. Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Recent review detailsConfiguration used: .coderabbit.yml Files selected for processing (5)
Files skipped from review as they are similar to previous changes (5)
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
Documentation and Community
|
| @@ -102,6 +104,12 @@ func (am AppModule) RegisterMsgHandlers(router appmodulev2.MsgRouter) { | |||
| errs = errors.Join(errs, err) | |||
| } | |||
|
|
|||
| if err := appmodulev2.RegisterHandler( | |||
There was a problem hiding this comment.
@testinginprod, imho we should improve the API to register multiple messages and handlers in that method. Otherwise it is imho way too verbose.
like
appmodulev2.RegisterHandlers(router, "cosmos.bank.v2.MsgSend": handlers.SendHandler, "cosmos.bank.v2.MsgUpdateParams": handlers.ParamsHandler)
It will need to be a bit less type safe I think, but still a better UX.
x/bank/v2/client/cli/tx.go
Outdated
|
|
||
| var FlagSplit = "split" | ||
|
|
||
| // NewTxCmd returns a root CLI command handler for all x/bank transaction commands. |
There was a problem hiding this comment.
Can we add a TODO to remove and link the gRPC / AutoCLI issue ?
x/bank/proto/cosmos/bank/v2/tx.proto
Outdated
| message MsgSend { | ||
| option (cosmos.msg.v1.signer) = "from_address"; | ||
| option (amino.name) = "cosmos-sdk/MsgSend"; |
runtime/v2/manager.go
Outdated
| @@ -480,6 +480,14 @@ func (m *MM[T]) RegisterServices(app *App[T]) error { | |||
| if module, ok := module.(appmodulev2.HasPostMsgHandlers); ok { | |||
| module.RegisterPostMsgHandlers(app.msgRouterBuilder) | |||
| } | |||
|
|
|||
| if module, ok := module.(appmodulev2.HasMsgHandlers); ok { | |||
There was a problem hiding this comment.
We should add a check in registerService to avoid the double registration.
…os-sdk into hieu/bankv2/handlers
runtime/v2/manager.go
Outdated
| } | ||
|
|
||
| if c.err != nil { | ||
| app.logger.Warn("error registering services", "error", c.err) | ||
| } | ||
|
|
||
| // TODO: query regist by RegisterQueryHandlers not in grpcQueryDecoders |
There was a problem hiding this comment.
Do some hacky here cause RegisterQueryHandlers not add query decoder to grpcQueryDecoders so I have to add again.
Must be some way better but idk yet.
| if errs != nil { | ||
| panic(errs) | ||
| } | ||
| } | ||
|
|
||
| // GetQueryDecoders registers the query handlers for the bank module. | ||
| func (am AppModule) GetQueryDecoders() map[string]func() gogoproto.Message { |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| decoderMap := module.GetQueryDecoders() | ||
| for path, decoder := range decoderMap { | ||
| app.GRPCMethodsToMessageMap[path] = decoder |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| @@ -5,11 +5,14 @@ | |||
| "encoding/json" | |||
| "errors" | |||
| "fmt" | |||
| "reflect" | |||
Check notice
Code scanning / CodeQL
Sensitive package import Note
hieuvubk
requested review from
akhilkumarpilli,
a team,
sontrinh16 and
kocubinski
as code owners
|
@hieuvubk your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
x/bank/v2/keeper/handlers.go (1)
79-80: Reminder: Address the TODO comment.The TODO comment indicates that the denom enable check is not yet implemented. Please ensure that this check is added to prevent potential issues.
Do you want me to generate the code for the denom enable check or open a GitHub issue to track this task?
x/bank/v2/module.go (2)
108-119: Consider improving the API to register multiple messages and handlers.The changes to register handlers for
MsgSendandMsgMintmessages are crucial for the bank module's functionality. However, as suggested in the past review comments, the registration process could be improved to be less verbose.Consider updating the API to allow registering multiple messages and handlers in a single call, like:
appmodulev2.RegisterHandlers(router, "cosmos.bank.v2.MsgSend": handlers.SendHandler, "cosmos.bank.v2.MsgUpdateParams": handlers.ParamsHandler, "cosmos.bank.v2.MsgMint": handlers.MintHandler, )This would improve the developer experience while maintaining type safety.
136-141: Consider applying the same improvement asRegisterMsgHandlers.The changes to register a handler for
QueryBalanceRequestare important for the bank module's query functionality. However, similar toRegisterMsgHandlers, the registration process could be improved to be less verbose.Consider updating the API to allow registering multiple queries and handlers in a single call, like:
appmodulev2.RegisterHandlers(router, "cosmos.bank.v2.QueryParamsRequest": handlers.QueryParamsHandler, "cosmos.bank.v2.QueryBalanceRequest": handlers.QueryBalanceHandler, )This would maintain consistency with the suggested improvement for
RegisterMsgHandlers.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (3)
x/bank/v2/types/genesis.pb.gois excluded by!**/*.pb.gox/bank/v2/types/query.pb.gois excluded by!**/*.pb.gox/bank/v2/types/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (15)
- runtime/v2/manager.go (4 hunks)
- simapp/v2/simdv2/cmd/testnet.go (3 hunks)
- tests/systemtests/bankv2_test.go (1 hunks)
- tests/systemtests/testnet_init.go (3 hunks)
- x/bank/proto/cosmos/bank/v2/genesis.proto (1 hunks)
- x/bank/proto/cosmos/bank/v2/query.proto (2 hunks)
- x/bank/proto/cosmos/bank/v2/tx.proto (2 hunks)
- x/bank/v2/client/cli/query.go (1 hunks)
- x/bank/v2/client/cli/tx.go (1 hunks)
- x/bank/v2/keeper/genesis.go (2 hunks)
- x/bank/v2/keeper/handlers.go (3 hunks)
- x/bank/v2/module.go (3 hunks)
- x/bank/v2/types/codec.go (1 hunks)
- x/bank/v2/types/msgs.go (1 hunks)
- x/bank/v2/types/query.go (1 hunks)
Additional context used
Path-based instructions (12)
runtime/v2/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/testnet.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/systemtests/bankv2_test.go (3)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"tests/systemtests/testnet_init.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"x/bank/v2/client/cli/query.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/client/cli/tx.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/keeper/handlers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/codec.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/msgs.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/v2/types/query.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (40)
x/bank/v2/types/query.go (1)
4-6: LGTM!The
NewQueryBalanceRequestfunction is a well-implemented constructor for theQueryBalanceRequeststruct. It follows the Golang naming convention, has appropriately named parameters with specified types, and initializes the struct fields correctly. The function body is concise and readable.x/bank/v2/types/codec.go (1)
11-11: LGTM!The addition of
&MsgSend{}to theRegisterInterfacesfunction is a straightforward change that enhances the functionality of thetransaction.Msginterface by allowing it to handle a new message type. This change conforms to the Uber Golang style guide and does not alter existing logic or control flow.x/bank/v2/types/msgs.go (2)
16-18: LGTM!The
NewMsgSendconstructor function is implemented correctly. It initializes a newMsgSendinstance with the providedfromAddr,toAddr, andamountparameters.
21-39: LGTM!The
ValidateBasicmethod performs important validations on theMsgSendfields:
It checks if
FromAddressandToAddressare valid Bech32 addresses usingsdk.AccAddressFromBech32. If invalid, it wraps the error with appropriate messages usingsdkerrors.ErrInvalidAddress.It checks if the
Amountis valid using theIsValidmethod. If invalid, it wraps the error with an appropriate message usingsdkerrors.ErrInvalidCoins.It checks if the
Amountis positive using theIsAllPositivemethod. If not positive, it wraps the error with an appropriate message usingsdkerrors.ErrInvalidCoins.The error handling and wrapping is done correctly using the
errorsmodandsdkerrorspackages. The method is implemented correctly and follows the expected validation logic.x/bank/proto/cosmos/bank/v2/query.proto (4)
7-7: LGTM!The import statement is necessary and correctly added.
19-20: LGTM!The closing brace and empty line are correctly added, improving the structure and readability of the code.
21-31: LGTM!The
QueryBalanceRequestmessage is well-defined and includes the necessary fields to query balances for a specific address and denomination. Theaddressfield annotation provides clarity on the expected format.
33-36: LGTM!The
QueryBalanceResponsemessage is well-defined and includes thebalancefield of typecosmos.base.v1beta1.Coin, which accurately represents the balance of the coin for the queried address and denomination. The usage of theCointype from the Cosmos SDK ensures consistency.tests/systemtests/bankv2_test.go (1)
13-47: Excellent system test for the bank module's send transaction functionality!This test is well-structured, covering all the essential aspects of sending a transaction using the bank module in version 2 of the application. It follows a clear setup, execution, and assertion pattern, making it easy to understand and maintain.
Some key strengths of this test:
- It sets up the necessary accounts and balances before executing the transaction.
- It constructs the send command correctly and waits for the transaction to be committed.
- It uses appropriate assertions to validate the transaction's success.
- It is well-documented with comments explaining each step.
- It skips execution if the application is not running in version 2, ensuring that it only runs in the appropriate context.
Overall, this test provides a comprehensive check of the transaction sending capabilities in the bank module, enhancing the testing suite and helping ensure the correctness and reliability of the module's functionality.
x/bank/v2/keeper/genesis.go (5)
18-19: LGTM!The
totalSupplyMapis initialized correctly usingsdk.NewMapCoinswith an emptysdk.Coinsslice.
20-35: LGTM!The code correctly iterates over the
state.Balancesslice, sets the balances in thebalancescollection using a joined key of the address bytes and coin denomination, and updates thetotalSupplyMapby adding the balance coins. Error handling is in place for the address conversion and balance setting operations.
36-37: LGTM!The
totalSupplyMapis correctly converted to asdk.Coinsslice using theToCoinsmethod, and thetotalSupplyvariable is assigned the value of the converted coins.
38-40: LGTM!The code correctly checks if the
state.Supplyis not empty and not equal to thetotalSupply. If the condition is true, it returns an error indicating that the genesis supply is incorrect, providing the expected and actual values. The error is returned using thefmt.Errorffunction.
42-44: LGTM!The code correctly iterates over the
totalSupplycoins and calls thesetSupplymethod of the keeper with the context and supply coin for each supply coin.x/bank/proto/cosmos/bank/v2/genesis.proto (5)
16-18: LGTM!The addition of the
repeated Balance balancesfield to theGenesisStatemessage enhances the flexibility of the genesis state by allowing the inclusion of multiple account balances during initialization. The(gogoproto.nullable) = falseand(amino.dont_omitempty) = trueoptions ensure that the field is always present and not omitted when empty, providing a consistent structure for the genesis state.
20-27: LGTM!The addition of the
repeated cosmos.base.v1beta1.Coin supplyfield to theGenesisStatemessage provides flexibility in defining the total supply of coins in the genesis state. It allows for explicit definition of the total supply or automatic calculation based on the provided account balances. The(gogoproto.nullable) = falseand(amino.dont_omitempty) = trueoptions ensure that the field is always present and not omitted when empty, maintaining a consistent structure for the genesis state.
32-37: LGTM!The introduction of the
Balancemessage provides a structured way to represent account balances in the genesis state. Theaddressfield holds the address of the balance holder as a string, and the(cosmos_proto.scalar) = "cosmos.AddressString"option ensures that it is properly validated and serialized as a Cosmos address string. This improves the organization and readability of account balances in the genesis state.
39-45: LGTM!The addition of the
repeated cosmos.base.v1beta1.Coin coinsfield to theBalancemessage enables the representation of multiple coin balances for each account in the genesis state. The(gogoproto.nullable) = falseand(amino.dont_omitempty) = trueoptions ensure that the field is always present and not omitted when empty, maintaining a consistent structure for the account balances.
7-7: LGTM!The inclusion of the
cosmos/base/v1beta1/coin.protoimport ensures that the necessaryCoinmessage type is available for use in theGenesisStateandBalancemessages. It maintains the dependency on thecosmos.base.v1beta1package for theCointype, which is used in thesupplyandcoinsfields.x/bank/v2/client/cli/tx.go (3)
14-15: Address the TODO comment.The TODO comment indicates that AutoCLI commands should be used in the future. Please ensure that this is tracked and addressed in the linked issue (#21682).
16-30: LGTM!The implementation of the root command for the bank module is well-structured and follows the expected pattern. The usage, description, and validation properties are set appropriately, and the
NewSendTxCmdsubcommand is added correctly.
33-63: LGTM!The implementation of the
NewSendTxCmdfunction is well-structured and follows the expected pattern for creating a CLI command. The usage, description, and argument validation are set appropriately. TheRunEfunction handles the command execution correctly by parsing the arguments, creating aMsgSendmessage, and generating or broadcasting the transaction using theGenerateOrBroadcastTxCLIutility. The standard transaction flags are also added to the command.x/bank/v2/client/cli/query.go (2)
22-36: LGTM!The
GetQueryCmdfunction is implemented correctly and follows the Uber Golang style guide. It sets up the parent command for bank queries with appropriate metadata and adds theGetBalanceCmdsubcommand.
38-78: LGTM!The
GetBalanceCmdfunction is implemented correctly and follows the Uber Golang style guide. It sets up the CLI command to query account balances with the following key points:
- Validates input arguments for address and denom.
- Converts the address from Bech32 to
sdk.AccAddress.- Constructs a new
QueryBalanceRequestwith the provided arguments.- Invokes the query through the client context.
- Prints the response in proto format.
The function also sets up appropriate flags for the denom and pagination.
x/bank/proto/cosmos/bank/v2/tx.proto (4)
9-9: LGTM!The import statement is necessary and correctly added.
27-28: LGTM!The
MsgUpdateParamsResponsemessage is correctly defined as an empty message, following the common practice for gRPC services.
29-42: LGTM!The
MsgSendmessage is well-defined and includes all necessary fields for sending coins between accounts. The use ofcosmos_proto.scalar,amino, andgogoproto.castrepeatedoptions ensures compatibility with the Cosmos SDK's messaging framework and type safety.
47-62: LGTM!The
MsgMintmessage is well-defined and includes all necessary fields for minting new coins. The use ofcosmos_proto.scalar,amino, andgogoproto.castrepeatedoptions ensures compatibility with the Cosmos SDK's messaging framework and type safety.x/bank/v2/keeper/handlers.go (3)
55-99: LGTM!The
MsgSendfunction correctly implements the following:
- Validation of sender and receiver addresses.
- Checking the validity and positivity of coin amounts.
- Processing the coin transfer using the
SendCoinsmethod.- Recording telemetry metrics for the transferred amounts.
101-136: LGTM!The
MsgMintfunction correctly implements the following:
- Verification of the authority of the minting request.
- Checking the validity and positivity of coin amounts.
- Performing the minting operation using the
MintCoinsmethod.
152-170: LGTM!The
QueryBalancefunction correctly implements the following:
- Validation of the request and checking for nil.
- Validation of the denom using the
sdk.ValidateDenomfunction.- Validation of the address using the
addressCodec.StringToBytesmethod.- Retrieval of the balance using the
GetBalancemethod.x/bank/v2/module.go (3)
148-172: LGTM!The
GetQueryDecodersfunction is a good addition to the bank module. It provides a flexible way to register query decoders by creating a map of message names to decoder functions.The use of reflection to create instances of the query request types is a clever solution, but it's worth noting that it could have some performance implications. However, given the benefits of flexibility and maintainability, it seems like a reasonable trade-off.
Overall, the function is well-structured and follows good practices.
175-177: LGTM!The
GetTxCmdfunction is a simple yet useful addition to the bank module. By leveraging thecli.NewTxCmd()function, it promotes code reuse and provides a convenient way to access the root transaction command.The function is concise and follows good practices.
179-181: LGTM!The
GetQueryCmdfunction is another simple yet useful addition to the bank module. Similar toGetTxCmd, it leverages theclipackage, specifically thecli.GetQueryCmd()function, to promote code reuse and provide a convenient way to access the root query command.The function is concise and follows good practices.
tests/systemtests/testnet_init.go (2)
17-20: LGTM!The
isV2function is well-named, has a clear purpose, and follows the Golang naming convention. The logic is straightforward and easy to understand.
66-70: Approve the conditional argument appending based onisV2().The changes introduce a conditional check to append the appropriate minimum gas price argument based on the simapp version. This ensures compatibility with both simapp v1 and v2. The logic is clear and the changes are well-integrated into the existing code.
simapp/v2/simdv2/cmd/testnet.go (1)
514-522: LGTM!The
getBankV2GenesisFromV1function correctly converts the bank v1 genesis state to v2 genesis state by iterating over the balances, converting each balance to the v2 format, and aggregating the total supply. The implementation follows the expected logic and ensures a smooth transition from v1 to v2 genesis state.runtime/v2/manager.go (3)
462-463: LGTM!The changes to
registerServicessimplify the service registration process and make it more adaptable to different module types. The function is now more resilient and flexible in handling various module implementations.
604-608: Fallback mechanism for registering message and query handlers.The changes provide a fallback mechanism to register message and query handlers if the module does not implement
RegisterServices. This ensures that the necessary handlers are registered even if the module does not fully implement thehasServicesV1interface.
616-624: Verify the query registration process.The TODO comment indicates that there might be an issue with query registration not being added to
grpcQueryDecoders. Please verify the query registration process and ensure that the registered queries are correctly added togrpcQueryDecoders.Run the following script to verify the query registration:
| // TODO: should mint to mint module then transfer? | ||
| err = h.MintCoins(ctx, to, msg.Amount) |
There was a problem hiding this comment.
Consider the approach mentioned in the TODO comment.
The TODO comment suggests minting to the mint module and then transferring to the recipient, instead of minting directly to the recipient. This approach might be a better practice for maintaining a clear separation of concerns and improving auditability.
Please consider implementing the suggested approach or provide a rationale for the current implementation.
| if isV2() { | ||
| args = append(args, "--server.minimum-gas-prices="+s.minGasPrice) | ||
| } else { | ||
| args = append(args, "--minimum-gas-prices="+s.minGasPrice) |
There was a problem hiding this comment.
Approve the conditional argument appending based on isV2(), but consider refactoring to remove duplication.
The changes introduce a conditional check to append the appropriate minimum gas price argument based on the simapp version, ensuring compatibility with both simapp v1 and v2. The logic is clear and the changes are well-integrated into the existing code.
However, the same conditional logic is present in both SingleHostTestnetCmdInitializer and ModifyConfigYamlInitializer. Consider extracting this logic into a separate function to avoid duplication and improve maintainability.
| @@ -657,6 +675,7 @@ func (c *configurator) registerQueryHandlers(sd *grpc.ServiceDesc, ss interface{ | |||
| for _, md := range sd.Methods { | |||
| // TODO(tip): what if a query is not deterministic? | |||
| requestFullName, err := registerMethod(c.stfQueryRouter, sd, md, ss) | |||
| fmt.Println("requestFullName", requestFullName) | |||
There was a problem hiding this comment.
| fmt.Println("requestFullName", requestFullName) |
| @@ -402,6 +403,11 @@ func initGenFiles[T transaction.Tx]( | |||
| } | |||
| appGenState[banktypes.ModuleName] = clientCtx.Codec.MustMarshalJSON(&bankGenState) | |||
|
|
|||
| var bankV2GenState bankv2types.GenesisState | |||
| clientCtx.Codec.MustUnmarshalJSON(appGenState[bankv2types.ModuleName], &bankV2GenState) | |||
There was a problem hiding this comment.
Why we need this step as you are assigning bankV2GenState a new value in next step?
| rsp := cli.Run(append(bankSendCmdArgs, "--fees=1stake")...) | ||
| txResult, found := cli.AwaitTxCommitted(rsp) | ||
| require.True(t, found) | ||
| RequireTxSuccess(t, txResult) |
There was a problem hiding this comment.
check balances after tx to confirm transfer done successfully
| } | ||
| } | ||
|
|
||
| // GetQueryDecoders registers the query handlers for the bank module. |
There was a problem hiding this comment.
please update godoc
Description
Closes: #21705
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation