feat(x/bank/cli)!: Allow MultiSendTx to Support Transactions with a Single Recipient

[meetrick]

This PR modifies the existing MultiSendTx functionality to support sending funds to a single recipient without returning an error. Previously, the MultiSendTx command required at least two recipient addresses, and attempting to send tokens to only one account would result in an error.

The changes in this PR adjust the cobra.MinimumNArgs from 4 to 3, allowing the multi-send command to handle a single recipient as well as multiple recipients. Additionally, when the --split flag is used, the specified amount will be split evenly among recipients. If only one recipient is provided, the entire amount will be sent to that recipient, making the functionality more flexible and user-friendly.

Description

Closes: #XXXX

---

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...

• included the correct type prefix in the PR title, you can find examples of the prefixes below:
• confirmed ! in the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• reviewed "Files changed" and left comments if necessary
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• updated the relevant documentation or specification, including comments for documenting Go code
• confirmed all CI checks have passed

Reviewers 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...

• confirmed the correct type prefix in the PR title
• confirmed all author checklist items have been addressed
• reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

• New Features

  • Enhanced multi-send transaction command to allow sending funds from one account to a single recipient, reducing the minimum required arguments from four to three.
  • Retained the ability to split amounts equally among recipients.

• Bug Fixes

  • Improved error handling for various transaction scenarios including invalid addresses and amounts.

• Tests

  • Added new test cases for single recipient transactions to enhance testing coverage of the multi-send functionality.

[coderabbitai]

📝 Walkthrough

📝 Walkthrough

📝 Walkthrough

📝 Walkthrough

📝 Walkthrough

📝 Walkthrough

📝 Walkthrough

Walkthrough

The pull request introduces a modification to the NewMultiSendTxCmd function, enabling transactions to a single recipient, thereby reducing the minimum required arguments from four to three. Additionally, it adds new test cases to validate this functionality within the testing suite, enhancing the overall coverage of multi-send transactions. The command's logic and comments have been updated to reflect these changes.

Changes

Files	Change Summary
x/bank/client/cli/tx.go	Updated NewMultiSendTxCmd to allow sending funds to one or more accounts, reducing required arguments and adjusting logic for recipient processing.
x/bank/client/cli/tx_test.go	Added TestMultiSendTxCmd_SingleRecipient for testing transactions to a single recipient; updated structure of test suite to include this new test case.

Possibly related PRs

• test: migrate e2e/bank to system tests #21607: This PR introduces system tests for the MultiSendTxCmd, which directly relates to the new feature in the main PR that allows MultiSendTx to support transactions with a single recipient.
• fix(cli): avoid id isn't supported error when query address-by-acc-num #21919: This PR modifies the CLI functionality for querying addresses, which may indirectly relate to the changes in transaction handling and command structure introduced in the main PR, particularly in the context of sending transactions.

Suggested labels

C:x/tx

Suggested reviewers

• julienrbrt
• sontrinh16
• aaronc
• testinginprod
• kocubinski

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)

x/bank/client/cli/tx.go (3)

44-50: LGTM: Detailed description updated with clear explanations

The Long description has been comprehensively updated to reflect the new functionality, including clear explanations of the --split flag behavior and a note about the --from flag. This provides valuable guidance to users.

Consider adding an example of using the --split flag in the Long description to further clarify its usage.

---

50-51: LGTM: Updated argument count, but example could be improved

The minimum argument count has been correctly updated to 3, aligning with the new functionality.

Consider updating the Example to showcase both single-recipient and multi-recipient usage, as well as the --split flag. For instance:

Example: fmt.Sprintf(`
  %[1]s tx bank multi-send cosmos1... cosmos1... 10stake
  %[1]s tx bank multi-send cosmos1... cosmos1... cosmos1... 30stake --split
`, version.AppName),

This would provide a more comprehensive illustration of the command's capabilities.

---

76-84: LGTM: Recipient processing and amount splitting logic implemented correctly

The changes accurately implement the functionality for single or multiple recipients, including the correct calculation of recipient count and amount splitting. The loop for processing recipients is also implemented correctly.

Consider adding a check to ensure that totalAddrs is not zero before performing the division in the split case. Although this scenario is unlikely due to the cobra.MinimumNArgs(3) check, it would add an extra layer of safety:

if split {
    if totalAddrs.IsZero() {
        return errors.New("no recipients specified")
    }
    sendCoins = coins.QuoInt(totalAddrs)
}

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3856e77 and e9b0b74.

📒 Files selected for processing (2)

• x/bank/client/cli/tx.go (4 hunks)
• x/bank/client/cli/tx_test.go (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

x/bank/client/cli/tx.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

x/bank/client/cli/tx_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

---

Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

🔇 Additional comments (9)

x/bank/client/cli/tx.go (4)

39-40: LGTM: Updated function comment accurately reflects new functionality

The comment has been appropriately updated to reflect the new capability of sending funds to one or more accounts, which aligns with the PR objectives.

---

42-44: LGTM: Command usage and description updated correctly

The Use field and Short description have been appropriately updated to reflect the new functionality of sending funds to one or more accounts. This change aligns with the PR objectives and provides clear guidance to users.

---

Line range hint 53-62: LGTM: Sender address and amount parsing logic updated correctly

The changes accurately implement the new argument structure, with the first argument set as the sender and the last argument parsed as the amount. The added comment enhances code readability.

---

93-98: LGTM: Total amount calculation logic updated correctly

The changes accurately implement the calculation of the total amount to be sent by the sender, handling both split and non-split cases correctly. This logic aligns well with the PR objectives and the previous changes.

x/bank/client/cli/tx_test.go (5)

54-72: LGTM: Setup for single recipient test cases looks good.

The setup for the TestMultiSendTxCmd_SingleRecipient function is well-structured. It correctly creates two accounts (one sender and one recipient) and sets up the necessary command and arguments.

---

74-143: Comprehensive test cases for single recipient scenarios.

The test cases cover various scenarios including:

1. Valid transaction with a single recipient
2. Invalid sender address
3. Invalid recipient address
4. Invalid amount

This comprehensive set of test cases ensures robust testing of the single recipient functionality.

---

146-171: LGTM: Test execution logic is sound.

The test execution logic is well-implemented. It correctly sets up the context, builds the command arguments, and checks for expected errors or successful responses.

---

174-174: LGTM: Multi-recipient test function placement is appropriate.

The TestMultiSendTxCmd_MultiRecipient function is correctly placed after the new single recipient test function. This arrangement provides a logical flow from testing single recipient scenarios to multi-recipient scenarios, enhancing the overall test coverage for the MultiSendTxCmd functionality.

---

54-174: Changes align well with PR objectives.

The addition of the TestMultiSendTxCmd_SingleRecipient function directly addresses the PR objective of allowing MultiSendTx to support transactions with a single recipient. The new test cases thoroughly cover various scenarios for single recipient transactions, complementing the existing multi-recipient tests.

These changes enhance the test coverage and ensure that the new functionality works as expected without breaking existing features.

[julienrbrt]

If you send to one recipient, then you should just use bank send.
Why would you use multi send for that??

[meetrick]

If you send to one recipient, then you should just use bank send. Why would you use multi send for that??

@julienrbrt , You’re absolutely right that for a single recipient, the bank send command is typically the most straightforward approach. However, the reason for allowing MultiSendTx to support a single recipient is to enhance flexibility and consistency across use cases where users may want to programmatically or operationally use the same command, regardless of whether they are sending to one or multiple recipients.

For example:

• Some scripts or automated tools might generate transactions dynamically based on varying recipient counts. By allowing MultiSendTx to handle both single and multiple recipients, we simplify the logic for such scripts, as they no longer need to switch between bank send and MultiSendTx depending on the number of recipients.
• MultiSendTx offers the ability to split amounts between multiple recipients with the --split flag. Users may prefer to use this functionality consistently, even when the recipient count is just one.

In short, this change is about offering flexibility and making the multi-send command more adaptable without forcing users to switch between commands based on recipient count. Of course, users can still use bank send if they prefer, but this change provides a more consistent and versatile option.

Would love to hear your thoughts on this!

[julienrbrt]

I think it makes the concept of multi-send CLI unclear if it can take one recipient. Your script could just have an if condition to use bank send instead.

While MsgMultiSend does accept one recipient, I don't think we should add ambiguity about which command to use for the user. There is a comment in the command that says this: // For a better UX this command is limited to send funds from one account to two or more accounts.. I, personally, agree with it.

[meetrick]

@julienrbrt ,Thank you for sharing your perspective! I understand the concern about maintaining clarity in the multi-send CLI and the potential confusion between bank send and multi-send. However, I believe allowing MultiSendTx to support a single recipient adds significant value in terms of flexibility and user experience, especially for developers who are automating or scripting transactions.

Key Reasons for Supporting Single Recipient in MultiSendTx:

1. Consistency in Transaction Workflow:
   By allowing MultiSendTx to handle both single and multiple recipients, we provide users with a unified transaction workflow. This eliminates the need for conditionals in scripts or automated systems, where the logic would otherwise have to switch between bank send for a single recipient and multi-send for multiple recipients. Reducing the number of different commands users need to handle can streamline the overall development process, reducing potential points of failure and simplifying the user experience.

2. Reducing Complexity in Automation:
   Many users are automating their Cosmos SDK operations. In such cases, having to decide which command to use (based on the number of recipients) introduces unnecessary complexity. Allowing MultiSendTx to handle both scenarios helps avoid branching logic in scripts and makes transaction handling more straightforward. This is particularly beneficial when the number of recipients is dynamically determined.

3. Leverage of --split Functionality:
   The --split functionality is a key feature of MultiSendTx, allowing the automatic splitting of token amounts across recipients. Even in cases where there is only one recipient, users may want to consistently use this feature for future-proofing their scripts, or simply to keep their code uniform, regardless of the number of recipients. Allowing MultiSendTx to support a single recipient gives users the flexibility to use this flag without needing separate logic for bank send.

4. Avoiding Redundancy:
   While it's true that bank send works perfectly for single recipients, requiring users to switch between the two commands for what is essentially the same type of transaction could introduce redundancy and confusion in itself. Instead of forcing users to remember which command to use based on recipient count, we can allow MultiSendTx to handle all cases. This offers a cleaner, simpler interface, which can ultimately be more intuitive for developers.

5. Existing MsgMultiSend Flexibility:
   As you pointed out, MsgMultiSend already accepts a single recipient at the message level. Extending this flexibility to the CLI command aligns with the underlying message structure and leverages its full potential without introducing complexity or deviating from its core functionality.

Conclusion:

To summarize, while I fully understand the importance of maintaining clear distinctions between commands, I believe the benefits of extending MultiSendTx to support a single recipient—consistency, reduced complexity in automation, and leveraging existing functionality—outweigh the concerns of potential ambiguity. Rather than introducing confusion, this change simplifies and unifies the user experience, especially for developers working with dynamic or automated transaction flows.

I would love to discuss this further and explore ways to address any potential UX concerns while still offering this enhanced flexibility.

---

And, Do you see any potential functional concerns with this?

[middthe]

To be honest, the reply contents from @meetrick are indeed look like generated from AI :)

[meetrick]

To be honest, the reply contents from @meetrick are indeed look like generated from AI :)

@middthe used AI to help organize my response, but it doesn’t seem convincing enough.
What do you think about my PR?

[julienrbrt]

Hi, discussed with the codeowners of bank, and we came to the same conclusion.
I'll close this. Thank you for your contribution!