chore(werc20): Add wrapper ERC20 precompile #2943
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant changes across various modules, including the addition of a new Wrapped ERC20 token interface ( Changes
Possibly related PRs
Suggested labels
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 (
|
…varadinov/re-add-werc20
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
precompiles/werc20/tx.go (3)
6-13: LGTM: Constants are well-defined and documented.The constants
DepositMethodandWithdrawMethodare appropriately named and documented. Using constants for method names is a good practice for maintainability.Consider adding a package-level comment explaining the purpose of these constants in the context of the WERC20 implementation. This would provide more context for developers unfamiliar with the WERC20 concept.
15-20: LGTM: Deposit method implemented as described.The
Depositmethod is correctly implemented as a no-op function, consistent with the comment's explanation.Consider expanding the comment to explain why a no-op implementation is sufficient in this context. For example:
// Deposit is a no-op mock function that provides the same interface as the // WETH contract. It supports equality between the native coin and its wrapped // ERC-20 (e.g., EVMOS and WEVMOS). A no-op is sufficient because the actual // wrapping logic is handled elsewhere in the Evmos system, and this precompile // serves as an interface compatibility layer.This additional context would help developers understand the design decision behind this implementation.
22-27: LGTM: Withdraw method implemented as described, with suggestions for improvement.The
Withdrawmethod is correctly implemented as a no-op function, consistent with the comment's explanation and theDepositmethod's implementation.
Consider updating the comment to be specific to the
Withdrawmethod, similar to the suggestion for theDepositmethod.Since both
DepositandWithdrawmethods have identical implementations, consider refactoring to reduce duplication:type noOpFunc func() ([]byte, error) func noOp() ([]byte, error) { return nil, nil } // Deposit is a no-op mock function that provides the same interface as the // WETH contract. It supports equality between the native coin and its wrapped // ERC-20 (e.g., EVMOS and WEVMOS). A no-op is sufficient because the actual // wrapping logic is handled elsewhere in the Evmos system, and this precompile // serves as an interface compatibility layer. func (p Precompile) Deposit() noOpFunc { return noOp } // Withdraw is a no-op mock function with the same purpose and behavior as Deposit, // but for the withdrawal operation. func (p Precompile) Withdraw() noOpFunc { return noOp }This refactoring would make the code more DRY and easier to maintain if the implementation needs to change in the future.
precompiles/werc20/IWERC20.sol (2)
4-9: LGTM: Well-documented interface with a minor suggestion.The interface is well-documented using NatSpec comments, providing clear information about its purpose and authorship. This follows good practices for code documentation.
Consider adding a
@noticetag to provide a brief, one-line description of the interface. This can improve documentation generation tools' output. For example:/** * @notice Interface for a wrapped ERC20 token representing the native EVM token * @author Evmos Team * @title Wrapped ERC20 Interface * @dev Interface for representing the native EVM token as ERC20 standard. */
26-37: LGTM: Well-defined deposit and withdraw functions with a minor suggestion.The
depositandwithdrawfunctions are properly defined with appropriate visibility and parameters. The documentation provides clear information about their expected behavior and effects.For consistency, consider aligning the documentation style of both functions. The
withdrawfunction includes a@paramtag, while thedepositfunction doesn't have any parameters to document. You could add a@noticetag to thedepositfunction for consistency:/// @notice Deposits native tokens in exchange for wrapped ERC20 token. /// @dev After execution of this function the SetBalance function /// @dev burns the tokens and increases the contract balance of the ERC20 tokens. /// @dev Emits a Deposit Event. function deposit() external payable;This change would make the documentation style more consistent across both functions.
precompiles/werc20/werc20.go (5)
38-41: Define gas cost constants with consistent naming.Consider following a consistent naming convention for the gas cost constants to improve code readability. For example, you can use either
DepositRequiredGasandWithdrawRequiredGasorRequiredGasDepositandRequiredGasWithdraw.Apply this diff to update the constants:
- DepositRequiredGas uint64 = 23_878 - WithdrawRequiredGas uint64 = 9207 + RequiredGasDeposit uint64 = 23_878 + RequiredGasWithdraw uint64 = 9_207
46-48: Handle error when loading ABI more precisely.In the
LoadABIfunction, consider wrapping the error with additional context to make debugging easier.Modify the function to include context in the error:
func LoadABI() (abi.ABI, error) { - return cmn.LoadABI(f, abiPath) + abi, err := cmn.LoadABI(f, abiPath) + if err != nil { + return abi, fmt.Errorf("failed to load ABI from %s: %w", abiPath, err) + } + return abi, nil }
58-61: Check fornilbefore returning error.After calling
LoadABI(), it's good practice to check if the returned ABI is valid before proceeding.Ensure the ABI is valid:
if err != nil { return nil, err } +if newABI == nil { + return nil, fmt.Errorf("ABI loaded is nil") +}
68-70: Assign ABI directly to the ERC20 precompile.Since
erc20PrecompileembedsPrecompile, you can assign the ABI directly toerc20Precompile.ABIfor clarity.Apply this diff:
- // use the IWERC20 ABI - erc20Precompile.Precompile.ABI = newABI + // Use the IWERC20 ABI + erc20Precompile.ABI = newABI
146-154: Leverage embeddedIsTransactionmethod consistently.In the
IsTransactionmethod, after checking forDepositMethodandWithdrawMethod, the default case callsp.Precompile.IsTransaction(methodName). SincePrecompileembedserc20.Precompile, which may already handle these methods, consider whether the explicit checks are necessary.If
erc20.Precompiledoes not handleDepositMethodandWithdrawMethod, it's fine to keep the explicit checks. Otherwise, you can simplify the method:func (p Precompile) IsTransaction(methodName string) bool { - switch methodName { - case DepositMethod, WithdrawMethod: - return true - default: - return p.Precompile.IsTransaction(methodName) - } + return p.Precompile.IsTransaction(methodName) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- precompiles/werc20/IWERC20.sol (1 hunks)
- precompiles/werc20/abi.json (1 hunks)
- precompiles/werc20/tx.go (1 hunks)
- precompiles/werc20/werc20.go (1 hunks)
- x/evm/keeper/static_precompiles.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (17)
precompiles/werc20/tx.go (1)
1-4: LGTM: Copyright, license, and package declaration are correct.The copyright notice, SPDX license identifier, and package declaration are properly formatted and appropriate for this file.
precompiles/werc20/IWERC20.sol (2)
1-2: LGTM: Appropriate license and Solidity version.The LGPL-3.0-only license is suitable for an open-source project, and the Solidity version (>=0.8.18) is recent, ensuring access to the latest language features and security improvements.
10-18: LGTM: Well-defined and documented events.The
DepositandWithdrawalevents are properly defined with appropriate parameters. The use ofindexedfor address parameters is a good practice for efficient event filtering. The documentation for each event is clear and informative, following good NatSpec conventions.precompiles/werc20/abi.json (5)
1-5: LGTM: Contract metadata is correctly defined.The contract metadata is properly structured and includes the correct contract name "IWERC20" and source file path. This follows the standard format for Hardhat artifacts.
6-43: LGTM: Event definitions are correct and follow best practices.The "Deposit" and "Withdrawal" events are properly defined with appropriate parameters:
- Both events use an indexed address parameter for efficient event filtering.
- The "wad" parameter (representing the amount) is correctly defined as a non-indexed uint256.
These definitions align with standard practices for wrapped token contracts.
44-71: LGTM: Function definitions are correct and follow WERC20 standards.The function definitions are properly structured and include:
- Payable fallback and receive functions, allowing the contract to accept native tokens.
- A payable
deposit()function with no parameters, enabling users to wrap native tokens.- A nonpayable
withdraw(uint256 wad)function, allowing users to unwrap tokens.These definitions align with the expected functionality of a wrapped token contract.
73-76: LGTM: Bytecode and link references are correctly empty.The bytecode and link references are empty, which is expected for an interface contract like IWERC20. This is correct because:
- Interface contracts don't contain implementation code.
- There are no external libraries to link for this contract.
1-77: LGTM: WERC20 ABI is correctly defined and follows best practices.The
abi.jsonfile successfully defines the Application Binary Interface (ABI) for the IWERC20 contract. It includes:
- Correct contract metadata.
- Properly defined "Deposit" and "Withdrawal" events.
- Appropriate function definitions for
deposit(),withdraw(uint256), and payable fallback/receive functions.- Correctly empty bytecode and link references.
This ABI definition provides a solid foundation for interacting with the WERC20 precompile in the Evmos ecosystem.
CHANGELOG.md (5)
Line range hint
42-47: State Machine Breaking changes look significant and well-documented.The changes for v16.0.0 include several state machine breaking updates, such as:
- Adding a new precompile for the secp256r1 curve
- Introducing new custom transactions for distribution and staking modules
- Adding a bech32 conversion precompile
- Restricting transaction fee tokens
These changes are clearly explained and include references to the relevant pull requests, which is good practice for changelogs.
🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
49-77: Improvements section is comprehensive and well-organized.The improvements section for v16.0.0 covers a wide range of updates, including:
- Refactoring of various modules (vesting, inflation, incentives, claims, revenue)
- Updates to dependencies and Go version
- Additions to CLI functionality
- Improvements to testing and CI processes
The entries are concise yet informative, and include pull request references. This follows good changelog practices.
🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
79-85: Bug Fixes section is present and well-documented.The bug fixes section for v16.0.0 includes several important fixes, such as:
- Updating Ledger supported algorithms
- Fixing CLI issues
- Addressing performance concerns
Each fix is clearly described and includes a reference to the relevant pull request.
🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
1-85: Overall changelog structure and formatting is consistent and follows best practices.The changelog:
- Uses reverse chronological order
- Categorizes changes (State Machine Breaking, API Breaking, Improvements, Bug Fixes)
- Includes version numbers and release dates
- Provides concise descriptions of changes
- Includes pull request references
This structure makes it easy for users and developers to understand the evolution of the project.
🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
1-1161: The CHANGELOG.md file is well-maintained and informative.This changelog effectively documents the evolution of the Evmos project:
- It follows a consistent structure across versions
- Changes are categorized clearly (State Machine Breaking, API Breaking, Improvements, Bug Fixes)
- Entries are concise yet informative
- Pull request references are included for traceability
- The most recent changes (v16.0.0) are well-documented, highlighting significant updates and breaking changes
The maintenance of this changelog demonstrates a commitment to clear communication with users and developers about the project's progress and changes.
🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
precompiles/werc20/werc20.go (1)
83-85:⚠️ Potential issueVerify and update the hardcoded gas values.
The gas values for
DepositRequiredGasandWithdrawRequiredGasare currently placeholders obtained from Remix. The TODO comment indicates that we should execute the transactions on the Evmos testnet to ensure parity in the values.Please verify these gas values by executing the transactions on the Evmos testnet and update the constants accordingly.
x/evm/keeper/static_precompiles.go (3)
25-27: Proper Addition of Imports for WERC20 PrecompileThe new imports for
werc20anderc20typesare correctly added to support the WERC20 precompile integration.
100-110: Correct Initialization of Wevmos PrecompileThe
wevmosPrecompileis properly instantiated with thetokenPairand necessary keepers (bankKeeper,authzKeeper,transferKeeper). Error handling is consistent with existing patterns, panicking on instantiation failure.
122-122: Ensure Unique Address for wevmosPrecompileVerify that
wevmosPrecompile.Address()does not conflict with existing precompile addresses in theprecompilesmap to prevent unintended overwriting.Run the following script to check for duplicate addresses:
✅ Verification successful
Verification Successful:
wevmosPrecompile.Address()is UniqueThe address for
wevmosPrecompileis added only once to theprecompilesmap, ensuring no conflicts with existing precompile addresses.
static_precompiles.go: precompiles[wevmosPrecompile.Address()]🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure that wevmosPrecompile's address is unique in the precompiles map. # Command: Search for all addresses added to the precompiles map. rg --type go 'precompiles\[.*\.Address\(\)\]' -o # Expected Result: Each address should be unique. Confirm that `wevmosPrecompile.Address()` is not duplicated elsewhere.Length of output: 731
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (12)
precompiles/werc20/tx.go (3)
6-13: LGTM: Well-defined constants with clear documentation.The constants
DepositMethodandWithdrawMethodare well-named and their purpose is clearly documented. Using constants for method names is a good practice for maintainability.Consider adding a brief comment above the
constblock to explain the purpose of these constants in the context of the WERC20 precompile. This would provide a quick overview for developers new to the file.
15-20: LGTM: Deposit method implemented as described.The
Depositmethod is correctly implemented as a no-op function, matching the description in the comments. It provides the expected interface compatibility with the WETH contract.Consider adding a TODO comment within the function body to indicate that this is a placeholder implementation. This can help future developers understand that the function might need to be updated in the future. For example:
func (p Precompile) Deposit() ([]byte, error) { // TODO: Implement actual deposit logic if needed in the future return nil, nil }
22-27: LGTM: Withdraw method implemented as described.The
Withdrawmethod is correctly implemented as a no-op function, matching the description in the comments. It provides the expected interface compatibility with the WETH contract.Similar to the
Depositmethod, consider adding a TODO comment within the function body:func (p Precompile) Withdraw() ([]byte, error) { // TODO: Implement actual withdraw logic if needed in the future return nil, nil }Additionally, since both
DepositandWithdrawmethods have identical implementations, you might want to consider adding a comment explaining why they are separate methods despite having the same behavior. This could help prevent future confusion or accidental merging of the methods.precompiles/werc20/IWERC20.sol (1 10000 )
4-9: LGTM: Well-documented interface with a minor suggestion.The interface is well-documented using NatSpec comments, providing clear information about its purpose. This follows good practices for code documentation.
Consider adding a
@noticetag to provide a brief explanation of the interface's functionality, which can be useful for automatic documentation generation:/** * @author Evmos Team * @title Wrapped ERC20 Interface * @dev Interface for representing the native EVM token as ERC20 standard. * @notice This interface defines the contract for wrapping and unwrapping native EVM tokens to ERC20 tokens. */precompiles/werc20/abi.json (2)
5-43: LGTM: Event definitions are correct and follow best practices.The "Deposit" and "Withdrawal" events are properly defined with appropriate parameters. The use of indexed addresses allows for efficient event filtering.
Consider adding a comment in the source Solidity file to explain that "wad" represents the amount in the smallest unit of the token (e.g., Wei for Ether).
44-71: LGTM: Function definitions are correct and cover essential WERC20 operations.The fallback, deposit, withdraw, and receive functions are properly defined with appropriate mutability. This allows for wrapping and unwrapping of native tokens as expected in a WERC20 contract.
Consider adding a
balanceOffunction to allow users to check their WERC20 token balance. This is a common feature in WERC20 implementations and improves usability.{ "inputs": [{"internalType": "address", "name": "account", "type": "address"}], "name": "balanceOf", "outputs": [{"internalType": "uint256", "name": "", "type": "uint256"}], "stateMutability": "view", "type": "function" }CHANGELOG.md (4)
Line range hint
42-93: Comprehensive "Unreleased" section with room for improvementThe "Unreleased" section provides a detailed overview of upcoming changes, which is excellent for keeping users informed. It's well-organized into "State Machine Breaking", "Improvements", and "Bug Fixes" subsections.
To further enhance this section:
- Consider adding a brief summary at the beginning of the "Unreleased" section to highlight the most significant changes.
- For each entry, it would be helpful to add a short explanation of the impact or benefit of the change, especially for non-technical users.
🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
95-160: Well-documented v15.0.0 release with significant changesThe v15.0.0 release section provides a comprehensive list of changes, including API breaking and state machine breaking changes. This level of detail is crucial for users upgrading from previous versions.
Suggestion for improvement:
Consider adding a migration guide or linking to one for this major version change. This would greatly assist users in navigating the breaking changes and ensuring a smooth upgrade process.🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
162-235: Contrast in detail between v14.1.0 and v14.0.0 releasesThe changelog effectively distinguishes between the minor v14.1.0 patch release and the major v14.0.0 release. The v14.0.0 section provides a comprehensive list of changes across different categories, which is commendable.
Suggestion for improvement:
For the v14.1.0 release, consider adding a brief note explaining why this specific bug fix warranted a new release. This context can help users understand the importance of the patch.🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
237-1115: Comprehensive historical changelog with room for improved navigationThe inclusion of detailed changelogs for all previous versions, dating back to v0.1.0, is exemplary. This level of detail allows users to understand the project's evolution and helps with troubleshooting version-specific issues.
Suggestion for improvement:
To enhance readability and navigation, consider adding a table of contents at the beginning of the file with links to each version. This would allow users to quickly jump to the version they're interested in, especially useful in such a long changelog.🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
precompiles/werc20/werc20.go (2)
22-22: Ensure consistent naming of WERC20 throughout the codeThere is an inconsistency in naming: both "WERC-20" and "WERC20" are used in comments. For clarity and consistency, please choose one naming convention and use it consistently throughout the code and comments.
Also applies to: 33-33
83-85: Address TODO comment regarding gas valuesThe TODO comment indicates that the gas values for
DepositRequiredGasandWithdrawRequiredGaswere obtained from Remix usingWEVMOS9.sol. It is important to verify these gas values on the Evmos testnet to ensure they are accurate and prevent any mismatches in gas estimation.Would you like assistance in verifying these gas values on the Evmos testnet? I can help outline the steps or open a GitHub issue to track this task.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- precompiles/werc20/IWERC20.sol (1 hunks)
- precompiles/werc20/abi.json (1 hunks)
- precompiles/werc20/tx.go (1 hunks)
- precompiles/werc20/werc20.go (1 hunks)
- x/evm/keeper/static_precompiles.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (13)
precompiles/werc20/tx.go (2)
1-4: LGTM: Copyright, license, and package declaration are correct.The file starts with the appropriate copyright notice, SPDX license identifier, and package declaration. This follows best practices for code organization and licensing.
1-27: Overall assessment: Well-structured and documented WERC20 precompile implementation.This file introduces the necessary constants and methods for the WERC20 precompile. The code is well-structured, with clear documentation for each component. The no-op implementations of
DepositandWithdrawmethods are consistent with the described behavior, providing interface compatibility with the WETH contract.To ensure that this implementation is consistent with other parts of the codebase, please run the following verification script:
This script will help identify any potential inconsistencies or areas that might need attention in relation to the WERC20 precompile implementation.
precompiles/werc20/IWERC20.sol (3)
1-2: LGTM: Appropriate license and Solidity version.The LGPL-3.0-only license is suitable for an open-source project, and the Solidity version (>=0.8.18) is recent, ensuring access to the latest language features and security improvements.
10-18: LGTM: Well-defined and documented events.The
DepositandWithdrawalevents are well-defined and follow standard naming conventions. The use of indexed parameters for addresses is appropriate for efficient event filtering. The NatSpec documentation clearly explains the purpose and parameters of each event.
1-38: Overall, well-structured interface with minor improvements suggested.The
IWERC20interface provides a solid foundation for implementing a Wrapped ERC20 token. It's well-documented and follows good Solidity practices. To further improve the interface:
- Clarify the relationship with the
SetBalancefunction mentioned in comments.- Consider adding return values to
depositandwithdrawfunctions.- Ensure implementing contracts handle the behavior of fallback and receive functions as intended.
These improvements will enhance the clarity and usability of the interface for developers implementing or interacting with WERC20 tokens.
precompiles/werc20/abi.json (3)
1-4: LGTM: Contract metadata is correctly defined.The contract metadata is properly structured and includes the necessary information such as the contract name and source file location.
73-76: LGTM: Bytecode and link references are correctly empty.For an interface contract like IWERC20, it's expected and correct to have empty bytecode and link references.
1-77: Overall assessment: The IWERC20 ABI is well-defined and follows best practices.The ABI correctly defines the interface for a WERC20 token, including essential functions (deposit, withdraw) and events (Deposit, Withdrawal). The contract metadata, function mutability, and event indexing are all properly set.
While the current implementation is solid, consider the following suggestions for further improvement:
- Add comments in the Solidity source file to explain terminology (e.g., "wad").
- Consider adding a
balanceOffunction to enhance usability.These suggestions aside, the ABI is ready for use in its current form.
CHANGELOG.md (2)
Line range hint
1-1115: Well-structured and consistent changelogThe overall structure and consistency of the CHANGELOG.md file are commendable. Key strengths include:
- Consistent formatting across all versions
- Clear categorization of changes (e.g., "State Machine Breaking", "API Breaking", "Improvements", "Bug Fixes")
- Inclusion of links to relevant issues and pull requests for most entries
This structure makes it easy for users and developers to understand the evolution of the project and find specific changes they're interested in.
🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
1-1115: Summary: Well-maintained CHANGELOG.md with minor suggestions for improvementOverall, the CHANGELOG.md file is exemplary in its detail, consistency, and structure. It provides a comprehensive history of the project's evolution, which is invaluable for users and developers alike. The suggested improvements, such as adding a table of contents and providing more context for certain changes, are minor and aimed at further enhancing an already strong document.
Key strengths:
- Detailed documentation of changes for each version
- Consistent structure and categorization
- Inclusion of links to relevant issues and pull requests
Suggested improvements:
- Add a table of contents for easier navigation
- Provide more context for significant changes, especially in major releases
- Consider adding brief summaries for "Unreleased" sections
These enhancements would further improve the user experience and the overall value of the changelog.
🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
precompiles/werc20/werc20.go (1)
98-104:⚠️ Potential issueDefine constants
DepositMethodandWithdrawMethodThe constants
DepositMethodandWithdrawMethodare used but not defined in this file. This will result in compilation errors. Please define these constants with the appropriate string values corresponding to the method names in the ABI.You can run the following script to check if
DepositMethodandWithdrawMethodare defined elsewhere in the codebase:Also applies to: 122-123, 125-126, 149-150
x/evm/keeper/static_precompiles.go (2)
25-27: Approved: Necessary imports addedThe new import statements for
werc20,erc20Keeper, anderc20typesare appropriate and necessary for the added functionality.
122-122: Approved: AddedwevmosPrecompiletoprecompilesmapThe
wevmosPrecompileis correctly added to theprecompilesmap, ensuring it's available for use in the EVM module.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
precompiles/werc20/tx.go (2)
6-13: LGTM: Well-defined constants with clear comments.The constants
DepositMethodandWithdrawMethodare appropriately named and documented. Using constants for method names is a good practice for maintainability.Consider using backticks for the constant values to prevent any potential escaping issues:
- DepositMethod = "deposit" + DepositMethod = `deposit` - WithdrawMethod = "withdraw" + WithdrawMethod = `withdraw`
15-27: LGTM: Well-implemented mock functions with clear explanations.The
DepositandWithdrawmethods are correctly implemented as no-op functions, aligning with their intended purpose as mock functions. The comments provide clear explanations of their role in supporting equality between the native coin and its wrapped ERC-20.For consistency with Go's standard library and common practices, consider returning
nilexplicitly for both return values:func (p Precompile) Deposit() ([]byte, error) { - return nil, nil + return nil, nil } func (p Precompile) Withdraw() ([]byte, error) { - return nil, nil + return nil, nil }This makes it clear that we're intentionally returning
nilfor both the byte slice and the error.precompiles/werc20/IWERC20.sol (3)
4-9: LGTM: Well-documented interface with a minor suggestion.The interface is well-documented using NatSpec comments, providing clear information about its purpose and authorship. This follows good documentation practices.
Consider adding a
@noticetag to provide a brief, one-line description of the interface. This can improve documentation generation and readability. For example:/** * @notice Interface for a Wrapped ERC20 token representing the native EVM token * @author Evmos Team * @title Wrapped ERC20 Interface * @dev Interface for representing the native EVM token as ERC20 standard. */
20-24: LGTM: Fallback and receive functions with a suggestion for clarity.The inclusion of both
fallback()andreceive()functions ensures compatibility with different methods of sending Ether to the contract. This is a good practice for handling incoming transactions.For improved clarity, consider updating the documentation to reflect that the interface doesn't implement the actual call to
deposit(). You could modify the comments as follows:/// @dev Default fallback payable function. Should call the deposit method in the implementation. fallback() external payable; /// @dev Default receive payable function. Should call the deposit method in the implementation. receive() external payable;This clarifies that the actual implementation of these functions should include the call to
deposit().
26-37: LGTM: Well-defined deposit and withdraw functions with a minor suggestion.The
deposit()andwithdraw(uint256)functions are well-defined and properly documented. The function signatures are appropriate for their intended use, and the documentation clearly explains the expected behavior, including interactions with theSetBalancefunction and event emissions.For consistency with the
Depositevent, consider renaming thewadparameter in theWithdrawalevent toamount. This would make the terminology more uniform across the interface. For example:event Withdrawal(address indexed src, uint256 amount);And update the
withdrawfunction accordingly:function withdraw(uint256 amount) external;This change would improve the consistency of the terminology used throughout the interface.
precompiles/werc20/abi.json (1)
44-71: LGTM: Functions are correctly defined, with a minor suggestion.The fallback, receive, deposit, and withdraw functions are correctly defined and match the expected behavior of a Wrapped Ether (WETH) contract. The payable and non-payable modifiers are appropriately used.
Consider adding a
balanceOf(address)function to the ABI to allow querying of wrapped token balances, which is a standard feature in WETH contracts.CHANGELOG.md (1)
Line range hint
42-93: Consider adding more context to some changelog entriesThe "Unreleased" section provides a good overview of the upcoming changes. However, some entries could benefit from additional context or explanation:
In the "State Machine Breaking" subsection:
- For the WERC20 precompile re-addition, it would be helpful to briefly explain why it was removed and why it's being re-added.
- For the removal of automatic withdrawal of staking rewards, consider explaining the rationale behind this change and its potential impact on users.
In the "Improvements" subsection:
- For entries related to outposts (Stride and Osmosis), it would be beneficial to provide a brief explanation of what outposts are and their significance in the Evmos ecosystem.
In the "Bug Fixes" subsection:
- For the fix related to burned cosmos transactions fees metric, consider explaining the importance of this metric and how the fix improves it.
Adding this additional context would make the changelog more informative for users and developers.
🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
precompiles/werc20/werc20.go (2)
23-23: Consider renamingabiPathfor clarityThe constant
abiPathis set to"abi.json", which is specific to the WERC20 precompile. Renaming it towerc20ABIPathcan improve code readability and maintainability.Apply this diff:
-const abiPath = "abi.json" +const werc20ABIPath = "abi.json"Remember to update all references to this constant within the file.
83-85: Update TODO comment to reference the correct contractThe TODO comment mentions
WEVMOS9.sol, which may not be relevant for WERC20. Clarify the comment to reference the appropriate contract or file related to WERC20.Apply this diff:
-// TODO: these values were obtained from Remix using the WEVMOS9.sol. +// TODO: These gas values were obtained from Remix using the WERC20 contract. // We should execute the transactions from Evmos testnet // to ensure parity in the values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- CHANGELOG.md (1 hunks)
- precompiles/werc20/IWERC20.sol (1 hunks)
- precompiles/werc20/abi.json (1 hunks)
- precompiles/werc20/tx.go (1 hunks)
- precompiles/werc20/werc20.go (1 hunks)
- x/evm/keeper/static_precompiles.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (14)
precompiles/werc20/tx.go (2)
1-5: LGTM: Proper license and package declaration.The file correctly starts with the copyright notice, SPDX license identifier, and appropriate package declaration.
1-27: Overall: Well-implemented WERC20 boilerplate.This file successfully reintroduces the WERC20 boilerplate as intended in the PR objectives. The constants and mock functions provide a clear interface for interacting with a wrapped ERC-20 token, specifically designed to support equality between the native coin (EVMOS) and its wrapped counterpart (WEVMOS).
The code is well-structured, properly documented, and aligns with the goal of enhancing the functionality of available static precompiles in the Evmos blockchain framework.
To ensure this implementation doesn't conflict with any existing code, let's verify that these method names are unique within the project:
This will help us confirm that we're not overriding any existing functionality.
✅ Verification successful
Verification Complete:
DepositandWithdrawmethods are unique.The
DepositandWithdrawmethods inprecompiles/werc20/tx.godo not conflict with any existing implementations within the project. There are no duplicate or conflicting method names found, ensuring that the new methods function as intended without overriding existing functionalities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any existing implementations of Deposit and Withdraw methods echo "Checking for existing Deposit methods:" rg --type go -A 5 'func.*Deposit\(' echo "Checking for existing Withdraw methods:" rg --type go -A 5 'func.*Withdraw\('Length of output: 2443
precompiles/werc20/IWERC20.sol (3)
8000
1-2: LGTM: Appropriate license and Solidity version.The LGPL-3.0-only license is suitable for an open-source project, and the Solidity version (>=0.8.18) is recent, ensuring access to the latest language features and security improvements.
10-18: LGTM: Well-defined and documented events.The
DepositandWithdrawalevents are well-defined and properly documented. The use of indexed parameters for addresses allows for efficient event filtering, which is crucial for tracking token movements. The documentation clearly explains the purpose and parameters of each event.
1-38: Overall: Well-designed WERC20 interface with minor suggestions for improvement.The
IWERC20interface is well-structured and provides a solid foundation for implementing a Wrapped ERC20 token. It includes all necessary functions and events, with clear documentation. The suggestions made throughout the review are minor and focus on improving clarity, consistency, and documentation. These changes would enhance the overall quality of the interface without altering its core functionality.Great job on creating a comprehensive and well-documented interface for the WERC20 token!
precompiles/werc20/abi.json (3)
1-4: LGTM: Metadata section is correct and consistent.The metadata section correctly defines the contract name as "IWERC20" and provides the appropriate source file path. The format "hh-sol-artifact-1" indicates this is a Hardhat artifact, which is suitable for defining the ABI of a precompiled contract.
5-43: LGTM: Events are well-defined and follow standard patterns.The Deposit and Withdrawal events are correctly defined with appropriate parameters. The use of indexed addresses allows for efficient filtering of events. The naming convention (dst/src for addresses, wad for amounts) is consistent with common Ethereum practices.
73-77: LGTM: Bytecode and link references are correctly empty.The empty bytecode and link references are appropriate for an interface or precompiled contract. This indicates that the actual implementation will be provided by the blockchain's precompiled contracts rather than deployed as a separate smart contract.
CHANGELOG.md (4)
Line range hint
95-139: LGTM: v15.0.0 release section is well-documentedThe v15.0.0 release section is comprehensive and well-organized. It clearly outlines the API Breaking changes, State Machine Breaking changes, Improvements, and Bug Fixes. The information provided is sufficient for users and developers to understand the significant changes in this release.
🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
141-211: LGTM: v14.1.0 and v14.0.0 release sections are well-documentedThe v14.1.0 and v14.0.0 release sections are well-organized and provide clear information about the changes:
- v14.1.0 correctly identifies a specific bug fix.
- v14.0.0 comprehensively covers State Machine Breaking changes, API Breaking changes, Improvements, and Bug Fixes.
The level of detail provided is appropriate for users and developers to understand the scope and impact of these releases.
🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
213-1034: LGTM: Historical release sections are well-maintainedThe changelog sections for releases from v13.0.2 down to v0.1.0 are well-maintained and provide a comprehensive history of the project's evolution:
- Consistent structure across all releases, categorizing changes into appropriate sections (e.g., "State Machine Breaking", "Improvements", "Bug Fixes").
- Clear and concise descriptions of changes, often with links to relevant pull requests.
- Proper versioning, allowing users to easily track changes between releases.
This historical record is valuable for users and developers who need to understand the project's development over time or track down when specific features or fixes were introduced.
🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
1-1034: LGTM: CHANGELOG.md structure and formatting follow best practicesThe overall structure and formatting of the CHANGELOG.md file adhere to best practices:
- Follows the "Keep a Changelog" format, which is a widely accepted standard.
- Includes a helpful comment at the top explaining the principles and usage of the changelog.
- Releases are listed in reverse chronological order, making it easy to find the most recent changes.
- Each release section is clearly marked with a version number and release date.
- Changes are consistently categorized (e.g., "State Machine Breaking", "Improvements", "Bug Fixes").
This structure makes the changelog easy to read and maintain, benefiting both users and contributors of the project.
🧰 Tools
🪛 Markdownlint
45-45: Expected: 120; Actual: 136
Line length(MD013, line-length)
precompiles/werc20/werc20.go (1)
117-117: Confirm correct usage of deferredHandleGasErrorThe
defer cmn.HandleGasError(ctx, contract, initialGas, &err)()statement ensures that gas errors are handled gracefully. Verify that this usage aligns with the intended error handling strategy.x/evm/keeper/static_precompiles.go (1)
122-122: WEVMOS precompile added to available precompilesThe
wevmosPrecompileis correctly added to theprecompilesmap, making it available as a static precompiled contract.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- precompiles/werc20/IWERC20.sol (1 hunks)
- precompiles/werc20/tx.go (1 hunks)
- precompiles/werc20/werc20.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- precompiles/werc20/IWERC20.sol
- precompiles/werc20/werc20.go
🧰 Additional context used
🔇 Additional comments (2)
precompiles/werc20/tx.go (2)
1-21: LGTM: Package structure and imports are well-definedThe package declaration, imports, and constants are appropriately structured and documented. The use of relevant Cosmos SDK and Ethereum packages aligns with the expected functionality of a WERC20 implementation.
23-41: 🛠️ Refactor suggestionVerify the intended behavior of the
DepositfunctionThe current implementation of
Depositsends the deposited coins back to the sender, which seems to contradict typical deposit behavior. While this aligns with a previous review comment, it's worth confirming if this is the intended functionality.Additionally, consider emitting an event to track deposit attempts, which could be useful for monitoring and auditing purposes.
If the current behavior is intended, consider adding a comment explaining the rationale. If not, here's a suggested modification:
func (p Precompile) Deposit(ctx sdk.Context, contract *vm.Contract) ([]byte, error) { // Get the deposited amount from the contract value depositedAmount := contract.Value() // Get the sender's address sender := common.BytesToAddress(contract.Caller().Bytes()) senderAccAddress := sdk.AccAddress(sender.Bytes()) precompileAccAddr := sdk.AccAddress(p.Address().Bytes()) - // Send the coins back to the sender - err := p.bankKeeper.SendCoins(ctx, precompileAccAddr, senderAccAddress, sdk.NewCoins(sdk.NewCoin(types.GetEVMCoinDenom(), math.NewIntFromBigInt(depositedAmount)))) + // Transfer the coins from the sender to the contract + err := p.bankKeeper.SendCoins(ctx, senderAccAddress, precompileAccAddr, sdk.NewCoins(sdk.NewCoin(types.GetEVMCoinDenom(), math.NewIntFromBigInt(depositedAmount)))) if err != nil { return nil, err } + // Emit a deposit event + ctx.EventManager().EmitEvent( + sdk.NewEvent( + "werc20_deposit", + sdk.NewAttribute("sender", senderAccAddress.String()), + sdk.NewAttribute("amount", depositedAmount.String()), + ), + ) + return nil, nil }This modification would make the
Depositfunction behave more like a typical deposit operation and include event emission for tracking.
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com> Signed-off-by: stepit <48993133+0xstepit@users.noreply.github.com>
Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com> Signed-off-by: stepit <48993133+0xstepit@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
x/erc20/keeper/precompiles.go (2)
Line range hint
42-56: Critical: Parameter name mismatch causing compilation errorThe function parameter is named
hasWrappedMethodsbut the code usesisNativein the conditional check, causing compilation errors. The parameter name should be consistent throughout the function.Apply this fix:
-func (k Keeper) InstantiateERC20Precompile(ctx sdk.Context, contractAddr common.Address, hasWrappedMethods bool) (vm.PrecompiledContract, error) { +func (k Keeper) InstantiateERC20Precompile(ctx sdk.Context, contractAddr common.Address, isNative bool) (vm.PrecompiledContract, error) {Also update the documentation to reflect the correct parameter name:
-// If the `hasWrappedMethods` boolean is true, the ERC20 instance returned -// exposes methods for `withdraw` and `deposit` as it is common for wrapped tokens. +// If the `isNative` boolean is true, the ERC20 instance returned will be +// a wrapped token implementation with additional `withdraw` and `deposit` methods.🧰 Tools
🪛 GitHub Check: build
[failure] 54-54:
undefined: isNative🪛 GitHub Check: test-unit-cover
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative🪛 GitHub Check: test-solidity
[failure] 54-54:
undefined: isNative🪛 GitHub Check: dependency-review
[failure] 54-54:
undefined: isNative🪛 GitHub Check: Run golangci-lint
[failure] 54-54:
undefined: isNative (typecheck)
[failure] 54-54:
undefined: isNative) (typecheck)
[failure] 54-54:
undefined: isNative) (typecheck)
[failure] 54-54:
undefined: isNative) (typecheck)
61-64: Enhance documentation clarityThe documentation could be more explicit about the types of precompiles. Consider clarifying the distinction between native and dynamic precompiles.
Suggested improvement:
-// The available ERC-20 precompiles consist of the dynamic precompiles and the native -// ones. +// The available ERC-20 precompiles are of two types: +// 1. Native precompiles: Built-in token contracts that support wrapping functionality +// 2. Dynamic precompiles: User-registered token contracts with standard ERC20 functionalityprecompiles/werc20/tx.go (3)
36-38: Consider handling zero-value depositsThe function accepts deposits of any value, including zero. Consider adding a validation check for zero-value deposits to prevent unnecessary state updates and gas consumption.
caller := contract.Caller() depositedAmount := contract.Value() +if depositedAmount.Sign() == 0 { + return nil, fmt.Errorf("cannot deposit zero amount") +}
71-74: Enhance mock implementation transparencyWhile the mock implementation is intentional (as documented), it would be beneficial to add logging to make it clear to users that this is a no-op function.
// Withdraw is a no-op and mock function that provides the same interface as the // WETH contract to support equality between the native coin and its wrapped // ERC-20 (e.g. EVMOS and WEVMOS). -func (p Precompile) Withdraw(ctx sdk.Context, contract *vm.Contract, stateDB vm.StateDB, args []interface{}) ([]byte, error) { +func (p Precompile) Withdraw(ctx sdk.Context, contract *vm.Contract, stateDB vm.StateDB, args []interface{}) ([]byte, error) { + // Log that this is a mock implementation + p.Logger.Debug("executing mock withdraw function - no actual withdrawal will occur")
84-86: Optimize error message constructionThe error message uses string formatting even when the error condition isn't met. Consider using
errors.Newfor static messages or wrapping the formatting in a separate function to avoid unnecessary string concatenation.-if nativeBalance.Amount.LT(amountInt) { - return nil, fmt.Errorf("account balance %v is lower than withdraw balance %v", nativeBalance.Amount, amountInt) +if nativeBalance.Amount.LT(amountInt) { + return nil, p.insufficientBalanceError(nativeBalance.Amount, amountInt) } +// Add this helper method to the Precompile struct +func (p Precompile) insufficientBalanceError(balance, amount math.Int) error { + return fmt.Errorf("account balance %v is lower than withdraw balance %v", balance, amount) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
precompiles/werc20/tx.go(1 hunks)precompiles/werc20/werc20.go(1 hunks)x/erc20/keeper/precompiles.go(3 hunks)x/erc20/types/params.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- precompiles/werc20/werc20.go
- x/erc20/types/params.go
🧰 Additional context used
🪛 GitHub Check: build
x/erc20/keeper/precompiles.go
[failure] 54-54:
undefined: isNative
🪛 GitHub Check: test-unit-cover
x/erc20/keeper/precompiles.go
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
[failure] 54-54:
undefined: isNative
🪛 GitHub Check: test-solidity
x/erc20/keeper/precompiles.go
[failure] 54-54:
undefined: isNative
🪛 GitHub Check: dependency-review
x/erc20/keeper/precompiles.go
[failure] 54-54:
undefined: isNative
🪛 GitHub Check: Run golangci-lint
x/erc20/keeper/precompiles.go
[failure] 54-54:
undefined: isNative (typecheck)
[failure] 54-54:
undefined: isNative) (typecheck)
[failure] 54-54:
undefined: isNative) (typecheck)
[failure] 54-54:
undefined: isNative) (typecheck)
🔇 Additional comments (3)
x/erc20/keeper/precompiles.go (3)
13-13: LGTM: Import added for wrapped ERC20 functionality
The werc20 import is correctly added to support the new wrapped token functionality.
24-35: LGTM: Improved control flow with early returns
The refactoring enhances readability and follows good practices:
- Early return pattern for unavailable precompiles
- Clear separation of concerns with proper vertical spacing
- Explicit error wrapping for initialization failures
Line range hint 44-50: Verify token pair validation completeness
Let's ensure the token pair validation is comprehensive and secure.
✅ Verification successful
Based on the search results, I can see comprehensive validation of token pairs across the codebase. The key findings show:
-
<
10000
p dir="auto">Token pair validation includes multiple checks:
- ID existence check (
if len(id) == 0) - Pair existence check (
if !okorif !found) - Error handling with detailed messages
- Additional validation in IBC callbacks and proposals
- ID existence check (
-
The precompile code in question follows the standard validation pattern used throughout the codebase:
- Checks token pair ID existence
- Verifies token pair existence
- Returns appropriate error messages
The validation is consistent with other critical parts of the system like IBC callbacks, proposals, and queries. The implementation is secure and follows the established patterns.
Token pair validation is comprehensive and secure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for additional validation on token pairs in the codebase
# Look for token pair validation patterns
rg -A 5 "GetTokenPair|ValidateTokenPair"
# Check for potential security checks on token pairs
ast-grep --pattern 'func $_(pair $_, $_) {
if $_ {
return $_, $_
}
$$$
}'
Length of output: 25568
43ddaa6 to
9f5d77d
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
x/erc20/keeper/precompiles.go (1)
Line range hint
38-57: LGTM: Clear separation of wrapped and regular token handlingThe implementation correctly differentiates between wrapped and regular ERC20 tokens. The parameter name
hasWrappedMethodsis clear and self-documenting.Consider a future refactoring to introduce an interface or abstract factory pattern for different token types, which could make it easier to add new token variants in the future.
precompiles/werc20/tx.go (1)
55-60: Optimize journal entries for contract callsThe balance change entries are only necessary when the precompile is called from another contract, not from an EOA. Consider optimizing this by checking the caller type.
+ // Add journal entries only for contract calls + if !stateDB.IsOrigin(caller) { p.SetBalanceChangeEntries( cmn.NewBalanceChangeEntry(caller, depositedAmount, cmn.Add), cmn.NewBalanceChangeEntry(p.Address(), depositedAmount, cmn.Sub), ) + }precompiles/werc20/integration_test.go (4)
485-487: Correct grammatical errors in test descriptionsIn the
DescribeTableentries, there are grammatical errors that can be improved for clarity:
- Use "don't" instead of "dont"
- Rephrase the descriptions for better readability
Apply this diff to correct the descriptions:
-Entry("it should not move funds and dont emit the event reverting before changing state", true, false), +Entry("it should not move funds and doesn't emit the event when reverting before changing state", true, false), -Entry("it should not move funds and dont emit the event reverting after changing state", false, true), +Entry("it should not move funds and doesn't emit the event when reverting after changing state", false, true),
67-67: Consider using realistic gas limits in testsThe gas limit is set to an extremely high value (
1_000_000_000_000). While this ensures that gas is not a bottleneck in tests, it may mask potential issues with gas consumption. Consider setting a realistic gas limit to help catch unexpected gas usage.
174-174: Use appropriate gas limits for contract deploymentSimilarly, the gas limit for deploying contracts is set to a very high value. Adjusting it to a more practical value can improve test accuracy regarding gas consumption.
69-70: Check theRetfield inethResfor execution resultsAfter calling
CallContractAndCheckLogs, it's good practice to check theRetfield inethResto verify that the contract execution returned the expected data.
🛑 Comments failed to post (2)
precompiles/werc20/tx.go (1)
30-35: 🛠️ Refactor suggestion
Remove unused parameter 'origin'
The
originparameter is not used in the function implementation. Consider removing it or documenting why it's needed for future use.func (p Precompile) Deposit( ctx sdk.Context, - origin common.Address, contract *vm.Contract, stateDB vm.StateDB, ) ([]byte, error) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (p Precompile) Deposit( ctx sdk.Context, contract *vm.Contract, stateDB vm.StateDB, ) ([]byte, error) {🧰 Tools
🪛 GitHub Check: Run golangci-lint
[failure] 32-32:
unused-parameter: parameter 'origin' seems to be unused, consider removing or renaming it as _ (revive)precompiles/werc20/integration_test.go (1)
498-498:
⚠️ Potential issueSafely convert
big.Inttosdk.Intwithout overflowUsing
transferAmount.Int64()may cause overflow iftransferAmountis larger than the maximum value ofint64. This could lead to incorrect token amounts being transferred.Apply this diff to handle large transfer amounts safely:
-transferCoins := sdk.Coins{sdk.NewInt64Coin(is.wrappedCoinDenom, transferAmount.Int64())} +transferCoins := sdk.Coins{sdk.NewCoin(is.wrappedCoinDenom, sdk.NewIntFromBigInt(transferAmount))}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.transferCoins := sdk.Coins{sdk.NewCoin(is.wrappedCoinDenom, sdk.NewIntFromBigInt(transferAmount))}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
x/erc20/keeper/precompiles.go (1)
Line range hint
44-59: Consider consistent error message formattingThe error messages use different formats:
"precompile id not found: %s""token pair not found: %s"Consider standardizing the error message format for consistency.
- return nil, fmt.Errorf("precompile id not found: %s", address) + return nil, fmt.Errorf("token pair ID not found: %s", address)precompiles/werc20/tx.go (2)
27-65: Consider optimizing journal entries for EOA callsThe journal entries are only necessary when the precompile is called from another contract. Consider adding an optimization to skip these entries for EOA calls.
+ // Add the entries to the statedb journal only if caller is a contract + // since these entries are only needed for proper revert handling + if !stateDB.IsOrigin(caller) { p.SetBalanceChangeEntries( cmn.NewBalanceChangeEntry(caller, depositedAmount, cmn.Add), cmn.NewBalanceChangeEntry(p.Address(), depositedAmount, cmn.Sub), ) + }
67-88: Enhance error message for insufficient balanceThe error message for insufficient balance could be more user-friendly by including the denomination.
- return nil, fmt.Errorf("account balance %v is lower than withdraw balance %v", nativeBalance.Amount, amountInt) + return nil, fmt.Errorf("insufficient balance: account has %v%s, but tried to withdraw %v%s", + nativeBalance.Amount, evmtypes.GetEVMCoinDenom(), + amountInt, evmtypes.GetEVMCoinDenom())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
precompiles/werc20/integration_test.go(1 hunks)precompiles/werc20/tx.go(1 hunks)precompiles/werc20/werc20.go(1 hunks)x/erc20/keeper/precompiles.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- precompiles/werc20/integration_test.go
- precompiles/werc20/werc20.go
🔇 Additional comments (6)
x/erc20/keeper/precompiles.go (5)
13-13: LGTM: Import added for WERC20 precompile
The new import is correctly added and necessary for the wrapped ERC20 functionality.
24-35: LGTM: Improved control flow and readability
The refactoring enhances code clarity through:
- Early return pattern for unavailable precompiles
- Clear separation between availability check and instantiation
- Proper vertical spacing
38-42: LGTM: Clear documentation and parameter naming
The method signature and documentation clearly indicate the purpose of the hasWrappedMethods parameter, making the code's intent obvious.
61-64: LGTM: Clear and accurate documentation
The documentation clearly explains the purpose and scope of available ERC20 precompiles.
54-56: Verify WERC20 precompile integration
Let's verify that all necessary WERC20 methods are properly exposed through the precompile.
precompiles/werc20/tx.go (1)
18-25: LGTM! Constants are well-defined and documented.
The ABI method names are clearly defined with appropriate documentation.
Pull request was converted to draft
It's been reviewed by others already
Description
This PR introduces the werc20 precompile to be used with registered native precompiles addresses. The implementation extend the already existing erc20 precompile adding the methods for the wrapping logic.
Te instantiation logic of the dynamic precompiles has been updated to load werc20 or erc20 depending on the address.
Other minor changes introduced in the PR are:
/contractsfolder and keep it only in the precompiles directory.Summary by CodeRabbit
New Features
IWERC20) for managing native EVM tokens.WEVMOS9contract, detailing its functions and events.WEVMOS9TestCaller) for testing deposit functionalities with revert scenarios.WEVMOS9TestCallercontract from JSON.Bug Fixes
Documentation
CHANGELOG.mdto reflect recent changes and improvements.Tests
werc20precompile, focusing on event emissions and contract behavior.abi.Methodtype for improved type safety.Summary by CodeRabbit
New Features
IWERC20) for managing native EVM tokens.WEVMOS9smart contract, including its ABI and metadata.WEVMOS9TestCaller) for interacting with the WERC20 token.Improvements
Bug Fixes
evmosd testnet init-filescommand.Documentation