Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(x/bank): swap sendrestriction check in InputOutputCoins #21976

Merged
merged 5 commits into from
Oct 3, 2024

Conversation

julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented Sep 30, 2024

Description

This PR switches the subUnlockedCoins and send restrictions checks in InputOutputCoins.
Users should always use a cache context anyway, but this resolves a footgun if someone forgets to.
I thought we already swapped those, as it was suggested once by Injective (cc @kakysha), and it came up again in a report.

As it is still a behavior change, we'll only backport this to 0.52. The current behavior shouldn't pose any issue to 0.50.


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

    • Introduced MsgBurn for burning coins.
    • Added support for application wiring with SendRestrictionFn.
  • Improvements

    • Enhanced transaction methods to return errors instead of panicking.
    • Improved validation checks for coin transactions.
  • Bug Fixes

    • Corrected SpendableBalances and SpendableCoins queries for accurate balance reporting.
  • API Breaking Changes

    • Updated method signatures for several functions, including BurnCoins and various balance query requests.
  • Tests

    • Expanded test coverage for the bank module, focusing on send and mint restrictions.

Copy link
Contributor

coderabbitai bot commented Sep 30, 2024

📝 Walkthrough

Walkthrough

The pull request introduces significant updates to the Cosmos SDK's bank module, including the addition of a new message type MsgBurn, enhancements to error handling in coin transaction methods, and various improvements to the testing framework for the bank's keeper. Notable changes include a restructuring of the InputOutputCoins method to improve transaction validation, updates to the SpendableBalances query, and modifications to the API that remove certain methods and alter others. The changes also include several new test cases to ensure robust coverage of the bank module's functionality.

Changes

File Path Change Summary
x/bank/CHANGELOG.md Updated changelog to include new features, improvements, bug fixes, and breaking changes.
x/bank/keeper/keeper_test.go Added multiple new test cases and enhanced existing tests for the bank keeper's functionality and error handling.
x/bank/keeper/send.go Modified logic in InputOutputCoins method for better transaction handling; no signature changes.
x/group/keeper/abci.go Updated import statement and modified telemetry reference in EndBlocker method.

Possibly related PRs

Suggested labels

C:x/bank/v2

Suggested reviewers

  • sontrinh16
  • akhilkumarpilli
  • hieuvubk

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

@julienrbrt julienrbrt added the backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release label Sep 30, 2024
@julienrbrt julienrbrt marked this pull request as ready for review October 2, 2024 11:26
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (6)
x/bank/CHANGELOG.md (2)

38-38: Consider clarifying the term "footgun".

The new entry effectively communicates the change in the order of operations. However, the term "footgun" might not be universally understood. Consider rephrasing to make it more explicit, for example:

- * [#21976](https://github.com/cosmos/cosmos-sdk/pull/21976) Resolve a footgun by swapping send restrictions check in `InputOutputCoins` before coin deduction.
+ * [#21976](https://github.com/cosmos/cosmos-sdk/pull/21976) Enhance safety by swapping send restrictions check in `InputOutputCoins` to occur before coin deduction, preventing potential user errors.

This clarification maintains the essence of the improvement while making it more accessible to all readers.


42-45: Consider improving the formatting of the Bug Fixes section.

The bug fixes are well-described, but the formatting could be enhanced for better readability. Consider using sub-bullets for each specific fix:

### Bug Fixes

* [#21407](https://github.com/cosmos/cosmos-sdk/pull/21407) Fix handling of negative spendable balances:
  * The `SpendableBalances` query now correctly reports spendable balances when one or more denoms are negative (used to report all zeros). Also, this query now looks up only the balances for the requested page.
  * The `SpendableCoins` keeper method now returns the positive spendable balances even when one or more denoms have more locked than available (used to return an empty `Coins`).
  * The `SpendableCoin` keeper method now returns a zero coin if there's more locked than available (used to return a negative coin).

This format maintains the association with the single issue while clearly delineating each specific fix.

x/bank/keeper/send.go (2)

Line range hint 159-174: Ensure Consistency of Recipient Address in Event Logs

The sendRestriction.apply function may modify the recipient address (outAddress). However, AddressStr is currently assigned using the original out.Address before applying the send restriction. This could lead to inconsistencies in the event logs if the recipient address is altered by the restriction, as the emitted event will display the original address instead of the modified one.

To maintain consistency, consider updating AddressStr after applying the send restriction by converting the potentially modified outAddress back to a string.

Apply this diff to fix the inconsistency:

 			outAddress, err := k.sendRestriction.apply(ctx, inAddress, outAddress, out.Coins)
 			if err != nil {
 				return err
 			}

+			// Convert the potentially modified outAddress back to string
+			outAddressStr, err := k.ak.AddressCodec().BytesToString(outAddress)
+			if err != nil {
+				return err
+			}

 			sending = append(sending, toSend{
-				AddressStr: out.Address,
+				AddressStr: outAddressStr,
 				Address:    outAddress,
 				Coins:      out.Coins,
 			})

157-157: Preallocate 'sending' Slice for Performance Improvement

To optimize memory allocation and improve performance, consider preallocating the sending slice with the capacity equal to the length of outputs. This reduces the number of memory reallocations during the append operations in the loop.

Apply this diff to preallocate the slice:

-	sending := make([]toSend, 0)
+	sending := make([]toSend, 0, len(outputs))
x/bank/keeper/keeper_test.go (2)

793-793: Consider specifying expected call count instead of AnyTimes()

Using .AnyTimes() in the mock allows any number of calls, which might mask unexpected behavior. Specifying the exact number of expected calls can make tests stricter and help catch unintended calls.


882-882: Consider calculating expected balances dynamically in tests

In the test cases, the expected balances are hard-coded using magic numbers, which can be error-prone and hard to maintain. Calculating expected balances based on initial balances and transaction amounts can improve readability and reduce the chance of mistakes.

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between bfdd954 and a95e85c.

📒 Files selected for processing (4)
  • x/bank/CHANGELOG.md (1 hunks)
  • x/bank/keeper/keeper_test.go (5 hunks)
  • x/bank/keeper/send.go (2 hunks)
  • x/group/keeper/abci.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
x/bank/CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

x/bank/keeper/keeper_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"

x/bank/keeper/send.go (1)

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

x/group/keeper/abci.go (1)

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

🔇 Additional comments (9)
x/group/keeper/abci.go (3)

15-15: LGTM: Telemetry measurement updated correctly.

The telemetry measurement has been appropriately updated to use group.ModuleName. This change is consistent with the import statement modification and ensures correct attribution of telemetry data to the group module.


Line range hint 1-23: Overall assessment: Changes are appropriate and well-implemented.

The modifications in this file are minimal, focused, and align well with the PR objectives. The import statement and telemetry measurement have been correctly updated to use the group module instead of the gov module. These changes improve the module-specific nature of the code without altering the core functionality of the EndBlocker method.


6-6: LGTM: Import statement updated correctly.

The import statement has been appropriately updated to use the group module instead of the gov module. This change aligns with the file's context and the PR objectives.

To ensure all references have been updated correctly, please run the following command:

x/bank/CHANGELOG.md (3)

38-40: LGTM: Features section is well-formatted and informative.

The Features section correctly lists two new additions with their corresponding GitHub issue links. The descriptions are concise and clear, adhering to changelog best practices.


Line range hint 47-57: LGTM: API Breaking Changes section is comprehensive and well-formatted.

The API Breaking Changes section effectively communicates the significant changes to the API. Each entry is clearly described and includes the corresponding GitHub issue link. The use of sub-bullets for related changes enhances readability.

🧰 Tools
🪛 LanguageTool

[grammar] ~35-~35: The verb form ‘does’ does not seem to match the subject ‘accounts’.
Context: ...ead of panicking if any module accounts does not exist or unauthorized. * [#20517](h...

(SUBJECT_VERB_AGREEMENT_PLURAL)


Line range hint 59-60: LGTM: Consensus Breaking Changes section is clear and concise.

The Consensus Breaking Changes section effectively communicates a significant change in the system's behavior. The entry is well-formatted, includes the corresponding GitHub issue link, and clearly describes the change.

🧰 Tools
🪛 LanguageTool

[grammar] ~35-~35: The verb form ‘does’ does not seem to match the subject ‘accounts’.
Context: ...ead of panicking if any module accounts does not exist or unauthorized. * [#20517](h...

(SUBJECT_VERB_AGREEMENT_PLURAL)

x/bank/keeper/keeper_test.go (3)

911-911: Consider calculating expected balances dynamically in tests

As previously mentioned, using calculated expected balances in test cases can enhance readability and reduce potential errors due to hard-coded values.


941-942: Consider calculating expected balances dynamically in tests

Repeated suggestion to compute expected balances based on transactions instead of hard-coding to improve test reliability.


970-971: Consider calculating expected balances dynamically in tests

Similarly, computing expected balances programmatically enhances test maintainability.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (3)
x/bank/CHANGELOG.md (1)

38-38: Improve clarity and specificity of the changelog entry.

While the entry follows the correct format and aligns with the PR objectives, consider the following improvements:

  1. Replace the informal term "footgun" with a more professional description.
  2. Provide more specific details about the change.

Here's a suggested revision:

-* [#21976](https://github.com/cosmos/cosmos-sdk/pull/21976) Resolve a footgun by swapping send restrictions check in `InputOutputCoins` before coin deduction.
+* [#21976](https://github.com/cosmos/cosmos-sdk/pull/21976) Enhance transaction safety by reordering checks in `InputOutputCoins`: send restrictions are now verified before coin deduction.

This revision maintains the essential information while improving clarity and professionalism.

x/bank/keeper/keeper_test.go (2)

Line range hint 1768-1828: Consider asserting error messages in negative test cases

In TestMintCoinRestrictions, when expectPass is false, you check that an error occurred using require.Error(). To strengthen the test, consider using require.ErrorContains() or require.EqualError() to assert that the error message matches the expected error. This ensures that the test fails only for the intended reasons.


Line range hint 2073-2095: Avoid suppressing linter warnings where possible

In TestSetParams, the comment //nolint:staticcheck is used to suppress a linter warning. Consider refactoring the test to comply with the linter's expectations, or if suppression is necessary, ensure that it's clearly documented why it's appropriate in this context.

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between bfdd954 and a95e85c.

📒 Files selected for processing (4)
  • x/bank/CHANGELOG.md (1 hunks)
  • x/bank/keeper/keeper_test.go (5 hunks)
  • x/bank/keeper/send.go (2 hunks)
  • x/group/keeper/abci.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
x/bank/CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

x/bank/keeper/keeper_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"

x/bank/keeper/send.go (1)

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

x/group/keeper/abci.go (1)

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

🔇 Additional comments (12)
x/group/keeper/abci.go (2)

15-15: LGTM. Telemetry measurement updated correctly.

The change from types.ModuleName to group.ModuleName in the ModuleMeasureSince call is consistent with the import statement modification. This ensures that the telemetry measurement is correctly associated with the group module.


6-6: LGTM. Verify type usage throughout the file.

The import change from "cosmossdk.io/x/gov/types" to "cosmossdk.io/x/group" aligns with the refactoring objective. This change is appropriate as it now correctly imports the group module.

Please run the following script to verify that all references to types from the previous gov module have been updated to use the group module:

x/bank/keeper/send.go (1)

187-190: Confirm correct address encoding in event emission

When emitting the EventTypeTransfer event, verify that out.AddressStr correctly represents the recipient's address in string format according to the expected address encoding. This ensures that event logs contain accurate and consistent address representations.

x/bank/keeper/keeper_test.go (9)

Line range hint 793-1012: TestInputOutputCoinsWithRestrictions is comprehensive and well-structured

The test TestInputOutputCoinsWithRestrictions thoroughly tests various scenarios of input and output coins with send restrictions, effectively verifying the expected behaviors.


Line range hint 1013-1105: TestSendCoinsWithRestrictions effectively validates send restrictions

The test TestSendCoinsWithRestrictions covers different cases of send restrictions, including error handling and address modifications, ensuring robustness.


Line range hint 1829-1849: TestIsSendEnabledDenom covers scenarios effectively

The test TestIsSendEnabledDenom adequately covers different scenarios for IsSendEnabledDenom, including default values and specific overrides, ensuring correctness.


Line range hint 1850-1874: Well-structured test for GetSendEnabledEntry

The test TestGetSendEnabledEntry effectively verifies the retrieval of send enabled entries, testing both found and not found cases, enhancing reliability.


Line range hint 1875-1906: TestSetSendEnabled verifies send enabled settings accurately

The test TestSetSendEnabled ensures that setting send enabled statuses for different denominations works as expected across various scenarios.


Line range hint 1907-1959: Effective testing of SetAllSendEnabled functionality

The test TestSetAllSendEnabled thoroughly checks setting send enabled statuses for multiple denominations, verifying that the function operates correctly with different inputs.


Line range hint 1960-1980: TestDeleteSendEnabled functions as expected

The test TestDeleteSendEnabled correctly verifies the deletion of send enabled entries and confirms that denominations revert to default behavior after deletion.


Line range hint 1981-2029: Comprehensive test for IterateSendEnabledEntries

The test TestIterateSendEnabledEntries covers scenarios with zero and multiple send enabled entries, ensuring the iteration over entries works correctly.


Line range hint 2030-2072: TestGetAllSendEnabledEntries validates retrieval of all entries

The test TestGetAllSendEnabledEntries accurately tests the retrieval of all send enabled entries in various situations, confirming expected behavior.

Comment on lines +176 to +178
if err := k.subUnlockedCoins(ctx, inAddress, input.Coins); err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure transactional integrity when subtracting coins

By moving subUnlockedCoins after processing outputs, there is a potential risk that if an error occurs during addCoins, the sender's coins have already been subtracted, but not all recipients have received their coins. Ensure that the entire operation is atomic and that any errors during addCoins will result in a full rollback to maintain balance consistency.

}

var outAddress sdk.AccAddress
sending := make([]toSend, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Optimize 'sending' slice allocation

To improve performance by reducing memory allocations, consider preallocating the sending slice with the capacity of len(outputs).

You can update the code as follows:

-sending := make([]toSend, 0)
+sending := make([]toSend, 0, len(outputs))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
sending := make([]toSend, 0)
sending := make([]toSend, 0, len(outputs))

Copy link
Member

@sontrinh16 sontrinh16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@julienrbrt julienrbrt added this pull request to the merge queue Oct 3, 2024
Merged via the queue into main with commit bb7d11d Oct 3, 2024
79 of 80 checks passed
@julienrbrt julienrbrt deleted the julien/bank branch October 3, 2024 11:23
mergify bot pushed a commit that referenced this pull request Oct 3, 2024
julienrbrt added a commit that referenced this pull request Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release C:x/bank C:x/group
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants