fix(bank): apply send restriction in InputOutputCoins#528
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughInputOutputCoins now applies the keeper's sendRestriction to each decoded recipient, aborting on error, caches the post-restriction recipient address for account creation and MultiSend, and updates transfer events; a new test asserts restriction calls for SendCoins and InputOutputCoins. ChangesSend Restriction Enforcement in InputOutputCoins
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
x/bank/keeper/send_test.go (1)
39-44: ⚡ Quick winStrengthen the multi-send assertion to validate “once per output.”
Line 42 builds only one output, so
calls == 1cannot distinguish per-output vs per-transaction restriction behavior. Use at least two outputs and assertcalls == len(outputs).✅ Suggested test adjustment
// InputOutputCoins (MsgMultiSend) must apply the restriction once per output calls = 0 in := banktypes.Input{Address: from.String(), Coins: coins} - outputs := []banktypes.Output{{Address: to.String(), Coins: coins}} + to2 := input.Faucet.NewFundedAccount(ctx, sdk.NewInt64Coin(denom, 1)) + outputs := []banktypes.Output{ + {Address: to.String(), Coins: sdk.NewCoins(sdk.NewInt64Coin(denom, 400))}, + {Address: to2.String(), Coins: sdk.NewCoins(sdk.NewInt64Coin(denom, 600))}, + } require.NoError(t, bk.InputOutputCoins(ctx, in, outputs)) - require.Equal(t, 1, calls) + require.Equal(t, len(outputs), calls)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@x/bank/keeper/send_test.go` around lines 39 - 44, The test currently only uses one output so asserting calls == 1 doesn't prove the restriction is applied per output; update the test in send_test.go around the InputOutputCoins call to create at least two banktypes.Output entries (e.g., outputs := []banktypes.Output{{...}, {...}}) and then assert require.Equal(t, len(outputs), calls) after invoking bk.InputOutputCoins(ctx, in, outputs) so the assertion verifies the restriction ran once for each output; reference the variables/functions InputOutputCoins, bk, calls, in, and outputs when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@x/bank/keeper/send.go`:
- Around line 143-150: The code currently applies k.sendRestriction.apply which
can rewrite the recipient address into the local variable addr, but then caches
and emits the original output.Address, causing event/recipient desyncs and
possible map collisions; change the logic so that after calling
k.sendRestriction.apply you store and emit the rewritten addr (use
addrMap[output.Address] = addr but ensure events use addr.String()/addr.Bytes()
or the resolved addr variable instead of output.Address) and update the same fix
pattern in the other affected blocks (the sections around the 158-163 and
174-177 ranges) so all caching and event emission consistently use the
post-restriction recipient (addr) rather than the original output.Address.
---
Nitpick comments:
In `@x/bank/keeper/send_test.go`:
- Around line 39-44: The test currently only uses one output so asserting calls
== 1 doesn't prove the restriction is applied per output; update the test in
send_test.go around the InputOutputCoins call to create at least two
banktypes.Output entries (e.g., outputs := []banktypes.Output{{...}, {...}}) and
then assert require.Equal(t, len(outputs), calls) after invoking
bk.InputOutputCoins(ctx, in, outputs) so the assertion verifies the restriction
ran once for each output; reference the variables/functions InputOutputCoins,
bk, calls, in, and outputs when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6df85a24-3b15-4dfb-9e89-a9d0f3cc1360
📒 Files selected for processing (2)
x/bank/keeper/send.gox/bank/keeper/send_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #528 +/- ##
==========================================
+ Coverage 38.49% 38.51% +0.02%
==========================================
Files 327 327
Lines 30693 30697 +4
==========================================
+ Hits 11814 11823 +9
+ Misses 16980 16973 -7
- Partials 1899 1901 +2
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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...
!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...