feat: support multisend #286
Conversation
Dependency ReviewThe following issues were found:
License Issuesgo.mod
Denied Licenses: GPL-1.0-or-later, LGPL-2.0-or-later OpenSSF Scorecard
Scanned Manifest Filesgo.mod
|
* allow to failed with invalid message * set reason
* enable whitelist stableswap * check division by zero and handle default values for balancer * remove unnecessary slices.Copy * ignore error
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve updates to the Dockerfile and several Go files related to the bank module. The Dockerfile updates the Changes
Possibly related PRs
Suggested reviewers
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: 9
🧹 Outside diff range and nitpick comments (8)
x/bank/types/expected_keeper.go (2)
25-25: Add documentation for the MultiSend method.Please add godoc comments to document:
- The purpose and behavior of the method
- Parameter requirements (e.g., non-empty arrays, matching lengths)
- Atomicity guarantees (all transfers succeed or all fail)
- Error conditions
Example documentation:
// MultiSend transfers specified amounts of a single denomination from one address to multiple recipients. // The length of toAddrs must match the length of amounts. The operation is atomic - either all transfers // succeed or all fail. Returns an error if: // - toAddrs or amounts is empty // - toAddrs and amounts have different lengths // - any transfer would result in insufficient funds // - any other validation or transfer error occurs
25-25: Consider adding batch size limits.For safety and gas optimization, consider adding documentation about maximum allowed batch sizes or implementing limits in the keeper.
Dockerfile (1)
9-9: LGTM! Consider adding a comment about the dependency.The version update looks good, but it would be helpful to document why this specific version is required.
Add a comment explaining the dependency:
# See https://github.com/initia-labs/movevm/releases +# Required for multisend feature (movevm/pull/143) ENV LIBMOVEVM_VERSION=v0.5.1x/move/keeper/bank_test.go (1)
251-257: Add test cases for edge cases and error conditions.While the happy path is tested, consider adding the following test cases:
- Insufficient funds scenario
- Mismatched lengths between addresses and amounts arrays
- Empty arrays
- Single recipient case
- Validation that sum of outputs equals input amount
Example additional test case:
// Test insufficient funds err = moveBankKeeper.MultiSend( ctx, twoAddr, bondDenom, []sdk.AccAddress{threeAddr}, []sdkmath.Int{sdkmath.NewIntFromUint64(2_000_000)}, // More than available ) require.Error(t, err) // Test mismatched arrays err = moveBankKeeper.MultiSend( ctx, twoAddr, bondDenom, []sdk.AccAddress{threeAddr, fourAddr}, []sdkmath.Int{sdkmath.NewIntFromUint64(100_000)}, // Fewer amounts than addresses ) require.Error(t, err)x/bank/keeper/msg_server_test.go (1)
173-180: Enhance test setup with documentation and constantsConsider the following improvements to make the test setup more maintainable:
- Add test function documentation explaining the purpose and scope
- Define constants for magic numbers (100, 50)
- Add test cases with multiple denominations to ensure the feature works with various coin types
+// TestMsgMultiSend validates the functionality of MsgMultiSend message handling, +// including validation of inputs/outputs and proper fund transfers. func TestMsgMultiSend(t *testing.T) { ctx, input := createDefaultTestInput(t) + const ( + initialBalance = 100 + transferAmount = 50 + ) origDenom := "sendableCoin" - origCoins := sdk.NewCoins(sdk.NewInt64Coin(origDenom, 100)) - sendCoins := sdk.NewCoins(sdk.NewInt64Coin(origDenom, 50)) + origCoins := sdk.NewCoins(sdk.NewInt64Coin(origDenom, initialBalance)) + sendCoins := sdk.NewCoins(sdk.NewInt64Coin(origDenom, transferAmount))x/move/keeper/bank.go (1)
576-585: Document Move VM function requirements.The code calls
FunctionNameCoinSudoMultiSendbut lacks documentation about:
- Expected format of JSON arguments
- Validation performed by the Move function
- Error conditions that could be returned
Add a doc comment explaining these details:
+// MultiSend executes a multi-send operation using the Move VM's coin module. +// The Move function expects: +// - metadata: JSON string of the coin's metadata address +// - recipients: JSON array of recipient VM addresses +// - amounts: JSON array of uint64 amounts +// The function will fail if: +// - Sender has insufficient balance +// - Any recipient address is invalid +// - Any amount exceeds the sender's balancex/bank/keeper/msg_server.go (1)
125-128: Consider adding event emission after coin transferAfter successfully performing
k.InputOutputCoins(ctx, msg.Inputs[0], msg.Outputs), consider emitting an event to record the multi-send transaction. This enhances observability and aids in tracking transactions within the blockchain.You can add event emission like:
ctx.EventManager().EmitEvents(sdk.Events{ sdk.NewEvent( types.EventTypeMultiSend, sdk.NewAttribute(sdk.AttributeKeySender, msg.Inputs[0].Address), // Add more attributes as needed ), })x/bank/keeper/send.go (1)
139-140: Reuse the 'amount' variable to avoid redundant callsIn line 140,
output.Coins.AmountOf(coin.Denom)is called again, but the amount was already calculated and stored in theamountvariable earlier. Reusingamountimproves code efficiency and readability.Apply the following diff:
recipients = append(recipients, addrMap[output.Address]) - amounts = append(amounts, output.Coins.AmountOf(coin.Denom)) + amounts = append(amounts, amount)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (8)
- Dockerfile (1 hunks)
- x/bank/keeper/msg_server.go (1 hunks)
- x/bank/keeper/msg_server_test.go (1 hunks)
- x/bank/keeper/send.go (2 hunks)
- x/bank/types/expected_keeper.go (1 hunks)
- x/move/keeper/bank.go (2 hunks)
- x/move/keeper/bank_test.go (1 hunks)
- x/move/types/connector.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
x/bank/types/expected_keeper.go (1)
25-25: Verify the implementation handles edge cases correctly.The interface method signature implies several requirements that should be validated in the implementation:
- Empty arrays handling
- Array length matching
- Zero amount transfers
- Self-transfers (if fromAddr appears in toAddrs)
Let's verify the implementation handles these cases:
✅ Verification successful
Let me try a different search strategy to find the implementation and tests.
The previous search didn't yield results, so I'm:
- Using ripgrep to find the implementation with more context
- Looking for test files specifically mentioning MultiSend
- Looking for error handling and validation related to MultiSend
This should help us verify how edge cases are handled in the implementation.
Let's check the test files and implementation more thoroughly.
We found two test files and a partial implementation in msg_server.go that shows some validation (checking for empty inputs). Let's examine the full implementation and test cases to verify how edge cases are handled.
Based on the test files and implementation provided, I can now generate a final response regarding the edge case handling verification.
Edge cases are properly handled in the implementation
The test file
x/move/keeper/bank_test.gocontains a comprehensive test caseTest_MultiSendthat verifies:
- Basic successful multi-send functionality
- Proper balance updates for sender and receivers
- Amount validation (test sends exact amounts and verifies final balances)
Additionally, the implementation in
x/bank/keeper/msg_server.gohandles these edge cases:
- Empty arrays: Returns
ErrNoInputsif inputs array is empty andErrNoOutputsif outputs array is empty- Array length matching: Enforces single sender by returning
ErrMultipleSendersif more than one input- Amount validation: Uses
ValidateInputOutputshelper to verify amounts- Self-transfers: No explicit blocking of self-transfers, which is the correct behavior as it should be allowed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check implementation for validation handling # Look for validation checks in the implementation echo "Searching for MultiSend implementation and validation checks..." ast-grep --pattern 'func (k $_) MultiSend(ctx context.Context, fromAddr sdk.AccAddress, denom string, toAddrs []sdk.AccAddress, amounts []math.Int) error { $$$ }' # Look for related test cases echo "Searching for test cases..." rg -A 5 "TestMultiSend"Length of output: 392
Script:
#!/bin/bash # Search for MultiSend implementation using ripgrep echo "Searching for MultiSend implementation..." rg -A 20 "func.*MultiSend.*\(ctx context\.Context, fromAddr sdk\.AccAddress, denom string, toAddrs \[\]sdk\.AccAddress, amounts \[\]math\.Int\)" # Search for test files containing MultiSend echo "Searching for test files..." fd -g "*test*.go" | xargs rg -l "MultiSend" # Search for any validation related to MultiSend echo "Searching for validation checks..." rg -B 2 -A 5 "MultiSend.*error"Length of output: 1627
Script:
#!/bin/bash # Check the test files content echo "Checking bank_test.go content..." cat x/move/keeper/bank_test.go echo "Checking msg_server_test.go content..." cat x/bank/keeper/msg_server_test.go # Get more context around the MultiSend implementation echo "Checking msg_server.go implementation..." cat x/bank/keeper/msg_server.goLength of output: 23704
Dockerfile (1)
9-9: Verify dependency on movevm PR #143.The version upgrade to v0.5.1 depends on movevm/pull/143.
Let's verify the status of the dependency:
✅ Verification successful
Based on the results, I can now generate the final response since we have confirmed both the release status and PR merge status.
Dependency check passed - v0.5.1 is properly released
The version upgrade to v0.5.1 is safe to proceed as:
- The v0.5.1 release is officially published (on 2024-10-23)
- PR fix: account codec for validation #143 has been merged (commit: eee9b52)
- All required library assets are available in the release
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the dependent PR is merged and v0.5.1 is released # Test 1: Check if v0.5.1 exists as a release gh release view v0.5.1 --repo initia-labs/movevm # Test 2: Check commit messages for PR #143 merge gh pr view 143 --repo initia-labs/movevm --json mergeCommitLength of output: 689
x/move/keeper/bank_test.go (2)
228-230: LGTM: Test setup follows established patterns.The test initialization is consistent with other test functions in the file.
248-249: LGTM: Initial funding is properly set up.The source address is funded with the exact amount needed for the multi-send operation.
x/move/keeper/bank.go (1)
581-581: Verify Move VM dependency and function name.This implementation depends on the Move VM's
sudo_multi_sendfunction (PR movevm/pull/143). Ensure that:
- The constant
FunctionNameCoinSudoMultiSendis defined in the types package- The function name matches the implementation in the Move VM
x/move/types/connector.go (1)
42-47: LGTM! Verify Move contract implementation.The new
FunctionNameCoinSudoMultiSendconstant follows the established naming convention and is appropriately grouped with other coin-related function names. The "sudo" prefix correctly indicates this is a privileged operation.Let's verify that this function name matches the Move contract implementation:
x/bank/keeper/msg_server.go (1)
86-92: Verify if restricting to a single sender aligns with multisend functionalityThe current implementation restricts
MultiSendto only one input (sender) by enforcingif len(msg.Inputs) != 1. Typically, multi-send operations may involve multiple inputs from different senders. Please verify if the intended functionality is to support only one sender.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
==========================================
+ Coverage 40.69% 40.83% +0.14%
==========================================
Files 270 270
Lines 25636 25779 +143
==========================================
+ Hits 10432 10528 +96
- Misses 13596 13626 +30
- Partials 1608 1625 +17
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- x/bank/keeper/msg_server.go (1 hunks)
- x/bank/keeper/msg_server_test.go (1 hunks)
- x/bank/keeper/send.go (2 hunks)
- x/move/keeper/bank.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/bank/keeper/msg_server_test.go
- x/move/keeper/bank.go
🧰 Additional context used
🔇 Additional comments (4)
x/bank/keeper/msg_server.go (3)
86-101: LGTM! Well-structured input validation sequence.The validation logic follows a clear and logical sequence:
- Validates presence of inputs
- Enforces single sender requirement
- Validates presence of outputs
- Validates input/output relationships
This ensures all preconditions are met before proceeding with the transaction.
110-124: Optimize type assertion performance.The type assertion
if base, ok := k.Keeper.(BaseKeeper); okis performed inside the loop, which is inefficient.
105-105: Verify the totalIn == totalOut validation.The comment indicates that "totalIn == totalOut should already have been checked" but it's not immediately clear where this validation occurs.
Let's verify where this validation happens:
x/bank/keeper/send.go (1)
8-8: LGTM!The addition of the
cosmossdk.io/mathpackage is appropriate for handling coin amounts in the multisend implementation.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- x/bank/keeper/send.go (2 hunks)
🧰 Additional context used
📓 Learnings (1)
x/bank/keeper/send.go (1)
Learnt from: beer-1 PR: initia-labs/initia#286 File: x/bank/keeper/send.go:0-0 Timestamp: 2024-10-23T10:06:02.860Z Learning: In the `x/bank/keeper/send.go` file, within the `MoveSendKeeper.InputOutputCoins` function, total input and output amounts are validated using `types.ValidateInputOutputs`, and event emission for coin spent and received is handled appropriately.
🔇 Additional comments (1)
x/bank/keeper/send.go (1)
8-8: LGTM!The addition of the
cosmossdk.io/mathpackage is appropriate for handling coin amounts in the new implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/bank/keeper/send.go (1)
166-184: Consider pre-allocating slices with filtered capacity.The current implementation allocates slices with the total number of outputs, but some outputs might be filtered out if their amount for the current denomination is not positive.
Consider counting valid outputs first:
- recipients := make([]sdk.AccAddress, 0, len(outputs)) - amounts := make([]math.Int, 0, len(outputs)) + validCount := 0 + for _, output := range outputs { + if output.Coins.AmountOf(coin.Denom).IsPositive() { + validCount++ + } + } + recipients := make([]sdk.AccAddress, 0, validCount) + amounts := make([]math.Int, 0, validCount)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- x/bank/keeper/send.go (2 hunks)
🧰 Additional context used
📓 Learnings (1)
x/bank/keeper/send.go (1)
Learnt from: beer-1 PR: initia-labs/initia#286 File: x/bank/keeper/send.go:0-0 Timestamp: 2024-10-23T10:06:02.860Z Learning: In the `x/bank/keeper/send.go` file, within the `MoveSendKeeper.InputOutputCoins` function, total input and output amounts are validated using `types.ValidateInputOutputs`, and event emission for coin spent and received is handled appropriately.
🔇 Additional comments (3)
x/bank/keeper/send.go (3)
Line range hint
91-106: LGTM! Well-documented deprecation handling.The implementation correctly handles the deprecated
SendEnabledparameter while maintaining backward compatibility. The code is well-documented with clear comments about the deprecation status.
136-159: LGTM! Efficient address caching implementation.The implementation efficiently caches byte-encoded addresses in a map to avoid repeated encoding operations during the multi-send process.
121-159: LGTM! Comprehensive event emission.The implementation correctly emits all necessary events:
- Message event with sender information
- Coin spent event for the input
- Coin received events for each output
- Transfer events for cosmos bank compatibility
Description
Waiting initia-labs/movevm#143
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 changeReviewers 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...