Check for blocked address before withdrawing to it #2709
Conversation
WalkthroughThis pull request introduces changes to address validation and transfer restrictions across multiple files in the subaccounts module. The modifications focus on enhancing the bank keeper's functionality to block specific addresses during fund transfers. The changes include updating the bank keeper initialization, adding a new method to check blocked addresses, and extending test cases to validate the new address restriction logic. Changes
Sequence DiagramsequenceDiagram
participant Sender
participant SubaccountKeeper
participant BankKeeper
participant Recipient
Sender->>SubaccountKeeper: Initiate Fund Transfer
SubaccountKeeper->>BankKeeper: Check Recipient Address
BankKeeper-->>SubaccountKeeper: Is Address Blocked?
alt Address Not Blocked
SubaccountKeeper->>Recipient: Transfer Funds
else Address Blocked
SubaccountKeeper-->>Sender: Return Unauthorized Error
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
roy-dydx
force-pushed
the
roy/withdraw
branch
from
08b49b6 to
1f3787f
Compare
roy-dydx
force-pushed
the
roy/withdraw
branch
from
1f3787f to
e1dc900
Compare
![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
🧹 Nitpick comments (1)
protocol/testutil/keeper/bank.go (1)
10-13: LGTM! Good security practice to block the distribution module address.The changes correctly set up the bank keeper to block transfers to the distribution module address in tests. This aligns with the PR's objective of implementing address blocking and helps prevent accidental transfers to system module accounts.
Consider documenting why the distribution module address is blocked, either in code comments or in the test descriptions, to help future maintainers understand the security implications.
Also applies to: 30-32
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
protocol/testutil/keeper/bank.go(2 hunks)protocol/x/subaccounts/keeper/transfer.go(2 hunks)protocol/x/subaccounts/keeper/transfer_test.go(5 hunks)protocol/x/subaccounts/types/expected_keepers.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- protocol/x/subaccounts/types/expected_keepers.go
- protocol/x/subaccounts/keeper/transfer.go
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: test-sim-import-export
- GitHub Check: test-sim-multi-seed-short
- GitHub Check: test-sim-after-import
- GitHub Check: unit-end-to-end-and-integration
- GitHub Check: test-race
- GitHub Check: liveness-test
- GitHub Check: container-tests
- GitHub Check: test-coverage-upload
- GitHub Check: Summary
🔇 Additional comments (4)
protocol/x/subaccounts/keeper/transfer_test.go (4)
334-336: LGTM! Good addition of the optional recipient field.The new
optionalRecipientfield with a default value mechanism enhances test flexibility by allowing custom recipient addresses while maintaining backward compatibility.
340-353: LGTM! Well-structured test case for blocked address.The test case effectively verifies that withdrawals to blocked addresses (e.g., distribution module address) are rejected with an unauthorized error. The test setup includes:
- Appropriate asset positions and perpetual positions
- Correct error expectation (
sdkerrors.ErrUnauthorized)- Use of a known blocked address (
distrtypes.ModuleName)
488-493: LGTM! Clean implementation of the optional recipient logic.The implementation properly handles the optional recipient by:
- Using a default test account if none is provided
- Properly handling any potential errors from address conversion
502-502: LGTM! Consistent usage of optional recipient.The test setup correctly uses the optional recipient across all relevant operations:
- Account funding
- Withdrawal testing
- Deposit testing
- Balance verification
Also applies to: 554-554, 561-561, 590-590
Changelist
Test Plan
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Tests