feat!: refactor with account number fix on shorthand account #132
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Poem
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
==========================================
+ Coverage 56.93% 57.04% +0.11%
==========================================
Files 110 110
Lines 10063 10045 -18
==========================================
+ Hits 5729 5730 +1
+ Misses 3508 3493 -15
+ Partials 826 822 -4
|
![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 (3)
x/evm/keeper/msg_server.go (2)
236-257: Enhance error handling and input validation invalidateArgumentsThe
validateArgumentsmethod effectively consolidates validation, but consider the following improvements:
- Consistent Error Messages: Ensure all error messages provide clear context to aid debugging.
- Input Data Length Check: For non-creation transactions, validate that
dataBzis not excessively large to prevent potential performance issues.
259-283: Optimize publisher authorization lookupCurrently, the authorization check iterates over
params.AllowedPublishers. To improve performance:
Use a Map for Efficient Lookups: Convert
AllowedPublishersto a map to achieve constant-time lookups, which is beneficial if the list grows.allowedPublishers := make(map[string]struct{}, len(params.AllowedPublishers)) for _, publisher := range params.AllowedPublishers { allowedPublishers[publisher] = struct{}{} } if _, allowed := allowedPublishers[sender]; !allowed { return sdkerrors.ErrUnauthorized.Wrapf("`%s` is not allowed to deploy a contract", sender) }x/evm/keeper/address_test.go (1)
77-111: Strengthen assertions inTest_AllowLongCosmosAddress_ConvertEmptyAccountThe new test effectively verifies the account number consistency. To enhance the test:
Assert Account Creation: After minting to
evmAddr2, add an assertion to confirm the account exists.account := input.AccountKeeper.GetAccount(ctx, addr2) require.NotNil(t, account)Check Account Number Consistency: Ensure the account number remains the same after all operations to verify that account numbers are managed correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
x/evm/keeper/address.go(2 hunks)x/evm/keeper/address_test.go(1 hunks)x/evm/keeper/msg_server.go(5 hunks)
🔇 Additional comments (8)
x/evm/keeper/msg_server.go (5)
45-49: Consolidate argument validation for improved maintainability
The use of ms.validateArguments centralizes the argument validation logic, reducing code duplication across message handlers. This enhances maintainability and ensures consistent validation.
51-54: Standardize publisher authorization checks
By introducing ms.assertAllowedPublishers, the code now consistently enforces publisher authorization. This improves code clarity and security by keeping the authorization logic in one place.
83-87: Reuse validation logic in Create2 method
Using ms.validateArguments in the Create2 method promotes code reuse and ensures that argument validation is consistent with the Create method.
89-92: Apply consistent authorization in Create2 method
Applying ms.assertAllowedPublishers in Create2 ensures that only authorized publishers can deploy contracts using this method as well.
126-130: Leverage centralized validation in Call method
Integrating ms.validateArguments into the Call method maintains consistency across all message handlers and simplifies validation.
x/evm/keeper/address.go (3)
Line range hint 21-41: Verify thread safety when handling account numbers
The addition of accountNumber management improves account handling for shorthand accounts. Ensure that when retrieving and assigning accountNumber, the operations are thread-safe to prevent race conditions in concurrent environments.
45-49: Avoid unnecessary account number increments
The check if accountNumber == 0 before calling NextAccountNumber prevents overwriting existing account numbers, ensuring that only new accounts receive a new account number.
57-58: Assign account number before setting the account
Assigning shorthandAccount.AccountNumber = accountNumber prior to SetAccount ensures that the account number is correctly associated with the account upon creation.
Description
Refactor to reduce redundant code and keep account number at shorthand account creation.
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
Bug Fixes
Tests
Refactor