fix(erc20): validation considering EIP-55 (#2696) #2913
Conversation
* fix(erc20): validation considering EIP-55 * add changelog entry * fix chlog * update var name
WalkthroughThe changes in this pull request primarily involve updates to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
|
https://github.com/Mergifyio backport rama/release-v20 |
✅ Backports have been created
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
CHANGELOG.md (3)
52-52: Significant changes in the upcoming releaseThe "Unreleased" section contains numerous important updates, including state machine breaking changes and improvements. It's crucial to carefully review these changes, especially the state machine breaking ones, as they may require special handling during the upgrade process.
Consider adding a note at the beginning of the "Unreleased" section to highlight any specific upgrade considerations or prerequisites for node operators and developers.
Line range hint
54-65: Well-documented state machine breaking changesThe state machine breaking changes are clearly documented, providing a good overview of the updates to various modules and dependencies. This level of detail is helpful for developers and node operators preparing for the upgrade.
For complex changes, such as the new governance precompile or the refactoring to use
sdk.BaseDenom, consider adding brief explanations of the impact or reasons for these changes. This additional context could help users better understand the motivations behind the updates.🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
Line range hint
67-86: Comprehensive list of improvementsThe "Improvements" subsection provides a thorough list of enhancements, bug fixes, and new features across various modules and components. These changes appear to significantly improve the overall functionality and performance of the system.
Given the extent of these improvements, it would be beneficial to ensure that all related documentation, user guides, and API references are updated to reflect these changes. Consider adding a task or note to review and update the project's documentation in line with these improvements.
🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- x/erc20/keeper/precompiles.go (1 hunks)
🔇 Additional comments (3)
x/erc20/keeper/precompiles.go (2)
51-52: LGTM: Improved implementation ofIsAvailableERC20PrecompileThe changes to the
IsAvailableERC20Precompilemethod look good. By delegating the checks toparams.IsNativePrecompileandparams.IsDynamicPrecompile, the code becomes more modular and easier to maintain. This change aligns well with the principle of separation of concerns.
51-52: Clarification needed: EIP-55 validation implementationThe PR objectives mention implementing validation changes in accordance with EIP-55 (Ethereum address formatting). However, the changes in this file don't seem to directly address EIP-55 validation. Could you please clarify where the EIP-55 validation is implemented, or if it's planned to be implemented in a different part of the codebase?
To help verify the implementation of EIP-55 validation, you can run the following script:
CHANGELOG.md (1)
Line range hint
1-86: Overall approval with minor suggestionsThe CHANGELOG.md file is well-structured and follows the Keep a Changelog format, providing valuable information about the changes in each release. The categorization of changes into "State Machine Breaking" and "Improvements" helps users quickly identify the impact of each update.
To further enhance the changelog:
- Consider adding links to relevant issues or pull requests for each change, allowing users to easily find more context if needed.
- For major changes or new features, a brief example or use case could be helpful to illustrate the impact or benefits.
- Ensure consistency in the level of detail provided for each entry across all sections.
These minor improvements could make the changelog even more informative and user-friendly.
🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
|
Thanks Tom! |
…2915) * fix(erc20): validation considering EIP-55 (#2696) (#2913) * fix(erc20): validation considering EIP-55 (#2696) * fix(erc20): validation considering EIP-55 * add changelog entry * fix chlog * update var name * add changelog entry (cherry picked from commit 82e0ff4) # Conflicts: # CHANGELOG.md * fix chlog --------- Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com> Co-authored-by: tom <tomasguerraalda@hotmail.com>
fix(erc20): validation considering EIP-55
add changelog entry
fix chlog
update var name
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...
Reviewers 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...
Unreleasedsection inCHANGELOG.mdSummary by CodeRabbit
Improvements
eth_headerByHashand fixes for community pool issues.Bug Fixes