fix: audit #89
fix: audit #89
Conversation
WalkthroughThe pull request introduces modifications to the bank and tokenfactory modules, focusing on enhancing the multi-send functionality and before-send hook mechanisms. The changes primarily involve improving address handling, adding new validation checks, and expanding test coverage for contract interactions. Several functions related to force transfers have been removed, streamlining the overall functionality and enhancing error handling. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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 (
|
![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 (3)
x/tokenfactory/keeper/before_send.go (1)
28-49: Enhance error handling in contract validation.The contract validation logic could be improved in the following ways:
- The error from
CacheContext()is being ignored.- The test message could be defined as a constant to avoid repeated allocations.
- Consider adding a timeout for the contract call to prevent long-running validations.
} else { // if a contract is being set, call the contract using cache context // to test if the contract is an existing, valid contract. - cacheCtx, _ := sdk.UnwrapSDKContext(ctx).CacheContext() + cacheCtx, writeCache := sdk.UnwrapSDKContext(ctx).CacheContext() + defer writeCache() cwAddr, err := sdk.AccAddressFromBech32(cosmwasmAddress) if err != nil { return err } - tempMsg := types.TrackBeforeSendSudoMsg{ - TrackBeforeSend: types.TrackBeforeSendMsg{}, - } - msgBz, err := json.Marshal(tempMsg) + msgBz, err := getTestMessage() if err != nil { return err } _, err = k.contractKeeper.Sudo(cacheCtx, cwAddr, msgBz)Add this helper function:
var testMessage = types.TrackBeforeSendSudoMsg{ TrackBeforeSend: types.TrackBeforeSendMsg{}, } func getTestMessage() ([]byte, error) { return json.Marshal(testMessage) }x/bank/keeper/msg_server.go (1)
108-111: Consider adding transaction batching for multiple outputs.The current implementation processes each output individually, which could be inefficient for transactions with many outputs. Consider batching the before-send hooks for better performance.
inAddr, err := k.ak.AddressCodec().StringToBytes(input.Address) if err != nil { return nil, err } +// Process all outputs in batches +const batchSize = 100 +for i := 0; i < len(msg.Outputs); i += batchSize { + end := i + batchSize + if end > len(msg.Outputs) { + end = len(msg.Outputs) + } + batch := msg.Outputs[i:end] + + // Process batch... +}Also applies to: 114-127
x/tokenfactory/keeper/before_send_test.go (1)
222-233: Consider using a more realistic invalid address.Instead of using a zeroed address, consider using a valid but non-existent contract address to make the test more realistic.
-cosmwasmAddress = make(sdk.AccAddress, 32) +// Use a valid bech32 address that doesn't have a contract +cosmwasmAddress, _ = sdk.AccAddressFromBech32("initia1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq9ht8qf")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/tokenfactory/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
x/bank/keeper/msg_server.go(2 hunks)x/tokenfactory/keeper/before_send.go(2 hunks)x/tokenfactory/keeper/before_send_test.go(3 hunks)
🔇 Additional comments (4)
x/bank/keeper/msg_server.go (2)
94-95: LGTM! Improved readability.The change to use a named variable
inputinstead of directly accessingmsg.Inputs[0]improves code readability.
130-130: Verify error handling in InputOutputCoins.The error from
InputOutputCoinsis properly captured and returned, which is good practice.x/tokenfactory/keeper/before_send_test.go (2)
150-156: LGTM! Good test structure.The test case structure is well-organized with clear fields for different test scenarios.
180-186: LGTM! Good test coverage.The new test case for invalid contract handling improves the test coverage.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
==========================================
+ Coverage 64.38% 64.83% +0.45%
==========================================
Files 39 39
Lines 3229 3171 -58
==========================================
- Hits 2079 2056 -23
+ Misses 972 938 -34
+ Partials 178 177 -1
|
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...
Summary by CodeRabbit
New Features
Tests
Bug Fixes