feat(x/bank): Placing SendRestriction before Deduction of Coins in SendCoins #20517
Conversation
WalkthroughWalkthroughThe primary change involves updating the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant BankModule
participant SendRestriction
participant CoinKeeper
User->>+BankModule: Initiate SendCoins
BankModule->>+SendRestriction: Apply send restriction
SendRestriction-->>-BankModule: Restriction result
alt Restriction Passed
BankModule->>+CoinKeeper: Deduct coins
CoinKeeper-->>-BankModule: Deduction result
BankModule->>User: SendCoins Success
else Restriction Failed
BankModule->>User: SendCoins Failed
end
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
wr1159
changed the title
wr1159
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
x/bank/keeper/keeper_test.go (1)
Line range hint
1183-1197: Ensure proper error handling inTestSendCoinsWithRestrictions
The test caserestriction returns errorinTestSendCoinsWithRestrictionsis designed to simulate a scenario where the send restriction function returns an error. However, the test does not assert that the error message is as expected. It's crucial to verify that the error message matches the expected output to ensure that the system behaves as intended under error conditions.- suite.Assert().EqualError(err, tc.expErr, "SendCoins error") + suite.Assert().EqualError(err, tc.expErr, "SendCoins error: expected error message to match")
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- x/bank/README.md (1 hunks)
- x/bank/keeper/keeper_test.go (2 hunks)
- x/bank/keeper/send.go (1 hunks)
Additional context used
Path-based instructions (3)
x/bank/keeper/send.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/bank/README.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"
LanguageTool
x/bank/README.md
[uncategorized] ~12-~12: Possible missing comma found.
Context: ...counts and tracking special-case pseudo-transfers which must work differently with partic...
[uncategorized] ~15-~15: Possible missing comma found.
Context: ...ities for secure interaction with other modules which must alter user balances. In add...
[grammar] ~24-~24: Adverb repetition.
Context: ...sed in the Cosmos Hub. ## Contents * Supply * Total Supply * [Module Accounts](#mo...
[typographical] ~62-~62: It appears that a comma is missing.
Context: ...al cases mint or burn tokens. At a base level these module accounts are capable of se...
[style] ~66-~66: Consider a shorter alternative to avoid wordiness.
Context: ...n track those tokens internally. Later, in order to send tokens, the module would need to e...
[style] ~87-~87: Consider a shorter alternative to avoid wordiness.
Context: ...rthat are related toModuleAccounts in order to be able to: * Get and setModuleAccou...
[grammar] ~100-~100: The word “lookup” is a noun. The verb is spelled with a space.
Context: ...the allowed functions, theKeepercan lookup the permissions to that specific accoun...
[uncategorized] ~105-~105: Loose punctuation mark.
Context: ...e available permissions are: *Minter: allows for a module to mint a specific ...
[uncategorized] ~105-~105: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ... allows for a module to mint a specific amount of coins. *Burner: allows for a modu...
[uncategorized] ~106-~106: Loose punctuation mark.
Context: ...t a specific amount of coins. *Burner: allows for a module to burn a specific ...
[uncategorized] ~106-~106: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ... allows for a module to burn a specific amount of coins. *Staking: allows for a mod...
[uncategorized] ~107-~107: Loose punctuation mark.
Context: ... a specific amount of coins. *Staking: allows for a module to delegate and und...
[uncategorized] ~107-~107: ‘Amount of’ should usually only be used with uncountable or mass nouns. Consider using “number” if this is not the case.
Context: ...e to delegate and undelegate a specific amount of coins. ## State Thex/bankmodul...
[grammar] ~278-~278: The preposition ‘from’ is repeated. Make sure that it is correct.
Context: ...ion is applied before coins are removed from the from address and adding them to the to addre...
[grammar] ~590-~590: The past participle is required after “to be”, alternatively you could omit the “be”.
Context: ... parameter is now deprecated and not to be use. It is replaced with state store record...
[grammar] ~596-~596: There seems to be a noun/verb agreement error. Did you mean “sends” or “sent”?
Context: .... ### DefaultSendEnabled The default send enabled value controls send transfer ca...
Markdownlint
x/bank/README.md
25-25: Expected: 2; Actual: 4
Unordered list indentation
27-27: Expected: 2; Actual: 4
Unordered list indentation
33-33: Expected: 2; Actual: 4
Unordered list indentation
34-34: Expected: 2; Actual: 4
Unordered list indentation
36-36: Expected: 2; Actual: 4
Unordered list indentation
37-37: Expected: 2; Actual: 4
Unordered list indentation
39-39: Expected: 2; Actual: 4
Unordered list indentation
40-40: Expected: 2; Actual: 4
Unordered list indentation
41-41: Expected: 2; Actual: 4
Unordered list indentation
398-398: Expected: 0 or 2; Actual: 1
Trailing spaces
423-423: Expected: 0 or 2; Actual: 1
Trailing spaces
289-289: Column: 1
Hard tabs
290-290: Column: 1
Hard tabs
291-291: Column: 1
Hard tabs
292-292: Column: 1
Hard tabs
294-294: Column: 1
Hard tabs
295-295: Column: 1
Hard tabs
303-303: Column: 1
Hard tabs
304-304: Column: 1
Hard tabs
305-305: Column: 1
Hard tabs
316-316: Column: 1
Hard tabs
321-321: Column: 1
Hard tabs
326-326: Column: 1
Hard tabs
327-327: Column: 1
Hard tabs
328-328: Column: 1
Hard tabs
329-329: Column: 1
Hard tabs
330-330: Column: 1
Hard tabs
331-331: Column: 1
Hard tabs
339-339: Column: 1
Hard tabs
593-593: Expected: 1; Actual: 2
Multiple consecutive blank lines
Additional comments not posted (2)
x/bank/keeper/send.go (1)
192-197: The change correctly implements the PR objective of applying send restrictions before coin deduction.This adjustment ensures that the send restriction logic is evaluated before any modification to the account balances, which is crucial for maintaining consistency and preventing errors in non-transactional flows.
x/bank/README.md (1)
278-278: The documentation accurately reflects the updated behavior in theSendCoinsmethod.It's crucial that the documentation stays in sync with the code changes to ensure that developers and users have a correct understanding of the module's functionality.
Tools
LanguageTool
[grammar] ~278-~278: The preposition ‘from’ is repeated. Make sure that it is correct.
Context: ...ion is applied before coins are removed from the from address and adding them to the to addre...
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
x/bank/CHANGELOG.md (1)
Line range hint
41-44: Adjust the indentation of the unordered list items to match the expected format.- * Changed `NewInput`, `NewOutput`, `NewQueryBalanceRequest`, `NewQueryAllBalancesRequest`, `NewQuerySpendableBalancesRequest` to accept a string instead of an `AccAddress`. - * Added an address codec as an argument to `NewSendAuthorization`. - * Added an address codec as an argument to `SanitizeGenesisBalances` which also returns an error. - * (simulation) `RandomGenesisBalances` also returns an error. + * Changed `NewInput`, `NewOutput`, `NewQueryBalanceRequest`, `NewQueryAllBalancesRequest`, `NewQuerySpendableBalancesRequest` to accept a string instead of an `AccAddress`. + * Added an address codec as an argument to `NewSendAuthorization`. + * Added an address codec as an argument to `SanitizeGenesisBalances` which also returns an error. + * (simulation) `RandomGenesisBalances` also returns an error.Tools
LanguageTool
[uncategorized] ~35-~35: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...MintCoinsandBurnCoinsmethods now returns an error instead of panicking if any mo...
[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...
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/bank/CHANGELOG.md (1 hunks)
Additional context used
Path-based instructions (1)
x/bank/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
x/bank/CHANGELOG.md
[uncategorized] ~35-~35: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...MintCoinsandBurnCoinsmethods now returns an error instead of panicking if any mo...
[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...
Markdownlint
x/bank/CHANGELOG.md
41-41: Expected: 2; Actual: 4
Unordered list indentation
42-42: Expected: 2; Actual: 4
Unordered list indentation
43-43: Expected: 2; Actual: 4
Unordered list indentation
44-44: Expected: 2; Actual: 4
Unordered list indentation
Additional comments not posted (1)
x/bank/CHANGELOG.md (1)
36-36: The changelog entry clearly and accurately describes the changes made in PR #20517 regarding theSendCoinsmethod.
| @@ -33,6 +33,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
| ### Improvements | |||
|
|
|||
| * [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `SendCoinsFromModuleToAccount`, `SendCoinsFromModuleToModule`, `SendCoinsFromAccountToModule`, `DelegateCoinsFromAccountToModule`, `UndelegateCoinsFromModuleToAccount`, `MintCoins` and `BurnCoins` methods now returns an error instead of panicking if any module accounts does not exist or unauthorized. | |||
There was a problem hiding this comment.
Correct the verb form to match the plural subject.
- methods now returns an error instead of panicking if any module accounts does not exist or unauthorized.
+ methods now return an error instead of panicking if any module accounts do not exist or are unauthorized.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.
| * [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `SendCoinsFromModuleToAccount`, `SendCoinsFromModuleToModule`, `SendCoinsFromAccountToModule`, `DelegateCoinsFromAccountToModule`, `UndelegateCoinsFromModuleToAccount`, `MintCoins` and `BurnCoins` methods now returns an error instead of panicking if any module accounts does not exist or unauthorized. | |
| * [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `SendCoinsFromModuleToAccount`, `SendCoinsFromModuleToModule`, `SendCoinsFromAccountToModule`, `DelegateCoinsFromAccountToModule`, `UndelegateCoinsFromModuleToAccount`, `MintCoins` and `BurnCoins` methods now return an error instead of panicking if any module accounts do not exist or are unauthorized. |
Tools
LanguageTool
[uncategorized] ~35-~35: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...MintCoinsandBurnCoinsmethods now returns an error instead of panicking if any mo...
[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...
Correct the verb form and add the missing verb for clarity.
- methods now returns an error instead of panicking if any module accounts does not exist or unauthorized.
+ methods now return an error instead of panicking if any module accounts do not exist or are unauthorized.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.
| * [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `SendCoinsFromModuleToAccount`, `SendCoinsFromModuleToModule`, `SendCoinsFromAccountToModule`, `DelegateCoinsFromAccountToModule`, `UndelegateCoinsFromModuleToAccount`, `MintCoins` and `BurnCoins` methods now returns an error instead of panicking if any module accounts does not exist or unauthorized. | |
| * [#18636](https://github.com/cosmos/cosmos-sdk/pull/18636) `SendCoinsFromModuleToAccount`, `SendCoinsFromModuleToModule`, `SendCoinsFromAccountToModule`, `DelegateCoinsFromAccountToModule`, `UndelegateCoinsFromModuleToAccount`, `MintCoins` and `BurnCoins` methods now return an error instead of panicking if any module accounts do not exist or are unauthorized. |
Tools
LanguageTool
[uncategorized] ~35-~35: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...MintCoinsandBurnCoinsmethods now returns an error instead of panicking if any mo...
[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...
julienrbrt
changed the title
* main: docs: add docs on permissions (#20526) refactor(x/gov): set environment in context for legacy proposals (#20521) docs: migrate diagrams to mermaidjs (#20503) refactor(tools/hubl): don't use nil panic (#20515) refactor(x/authz): set environment in context (#20502) build(deps): Bump github.com/spf13/viper from 1.18.2 to 1.19.0 (#20519) feat(x/bank): Placing SendRestriction before Deduction of Coins in SendCoins (#20517) chore: sonar ignore directories with their own go.mods (#20509) ci: run action in merge queue (#20508)
* main: refactor(x/feegrant): set environment in context (#20529) refactor(x/accounts)!: accounts and auth module use the same account number tracking (#20405) chore: remove sonar from simapp (#20528) docs: add docs on permissions (#20526) refactor(x/gov): set environment in context for legacy proposals (#20521) docs: migrate diagrams to mermaidjs (#20503) refactor(tools/hubl): don't use nil panic (#20515) refactor(x/authz): set environment in context (#20502) build(deps): Bump github.com/spf13/viper from 1.18.2 to 1.19.0 (#20519) feat(x/bank): Placing SendRestriction before Deduction of Coins in SendCoins (#20517) chore: sonar ignore directories with their own go.mods (#20509) ci: run action in merge queue (#20508) refactor(server/v2/cometbft): update function comments (#20506)
Description
Closes: #20516
Reorders
SendRestrictionto beforesubUnlockedCoins. Relevant discussionAuthor 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
Bug Fixes
SendCoinsprocess to ensure restrictions are checked before coin transfer.Tests
SendCoinsto reflect new coin values and revised error handling logic.Documentation
SendCoinsmethod and send restriction application timing.