feat: erc20 wrapper #75
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve extensive updates to the documentation and code related to the execution of EVM contracts through IBC hooks, including the introduction of an Changes
Possibly related PRs
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 #75 +/- ##
==========================================
- Coverage 29.04% 28.21% -0.83%
==========================================
Files 122 124 +2
Lines 13103 13544 +441
==========================================
+ Hits 3806 3822 +16
- Misses 8751 9168 +417
- Partials 546 554 +8
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (12)
proto/minievm/evm/v1/genesis.proto (1)
33-35: LGTM! Consider enhancing the comment for clarity.The addition of the
erc20_wrapperfield is well-implemented and aligns with the PR objectives. The field type and numbering are consistent with existing fields.Consider enhancing the comment to provide more context:
- // erc20 wrapper contract address + // Address of the ERC20 wrapper contract used for IBC-compatible token transfers bytes erc20_wrapper = 6;x/evm/types/keys.go (1)
45-45: LGTM! Consider enhancing the documentation.The addition of
ERC20WrapperAddrKeyis consistent with the existing codebase and aligns with the PR objectives. The constant follows the established naming convention and value format.To improve clarity, consider expanding the inline comment to briefly explain the purpose of the ERC20 wrapper in the context of IBC transfers. For example:
// key of erc20 wrapper contract address used for IBC-compatible token transfersx/evm/keeper/query_server.go (1)
159-169: Approve implementation with minor suggestionsThe
ERC20Wrappermethod is well-implemented and follows the established pattern for query methods in this file. However, there are a couple of minor points to consider:
The response type
QueryERC20FactoryResponseseems inconsistent with the method nameERC20Wrapper. Consider renaming the response type toQueryERC20WrapperResponsefor better clarity and consistency.The
reqparameter is currently unused. If it's not needed, consider removing it to simplify the method signature:func (qs *queryServerImpl) ERC20Wrapper(ctx context.Context) (*types.QueryERC20WrapperResponse, error) { // ... (rest of the implementation remains the same) }app/ibc-hooks/README.md (5)
Line range hint
5-14: LGTM! Consider adding a brief mention of the ERC20 wrapper.The updated introduction clearly explains the purpose of EVM hooks and their role in cross-chain contract calls. This aligns well with the PR objectives.
Consider adding a brief mention of the ERC20 wrapper in this introduction, as it's a key component of the PR objectives. This would provide a more complete overview of the changes.
Line range hint
15-79: LGTM! Consider clarifying the purpose of the 'Value' field.The addition of the 'Value' field to the MsgCall struct and its inclusion in the constructed evm call message is well-documented and aligns with the PR objectives.
Consider adding a brief explanation of why the 'Value' field was introduced and how it enhances the functionality of the ERC20 wrapper. This would provide more context for developers using this feature.
Line range hint
80-119: LGTM! Ensure consistency in field naming.The updated ICS20 packet structure and formatting criteria accurately reflect the addition of the 'value' field, which is consistent with the changes in the MsgCall struct.
For consistency, consider using either camelCase or snake_case for all field names in the JSON structure. Currently, 'contract_addr' uses snake_case while other fields use camelCase. Standardizing the naming convention would improve readability and maintain consistency throughout the documentation.
🧰 Tools
🪛 LanguageTool
[style] ~122-~122: Consider removing “of” to be more concise
Context: ...packet as directed towards evmhooks iff all of the following hold: -memois not blank ...(ALL_OF_THE)
Line range hint
120-170: LGTM! Consider adding an example usage of async callbacks.The introduction of async callbacks with the
ibc_ackandibc_timeoutfunctions, along with the updated memo field structure, enhances the functionality of the EVM hooks and aligns well with the PR objectives.To further improve the documentation, consider adding a brief example of how a contract might use these async callbacks in practice. This would help developers better understand the practical application of this feature.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~165-~165: Possible missing comma found.
Context: ...ut(uint64 callback_id) external; } ``` Also when a contract make IBC transfer reque...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~166-~166: Possibly, a comma is missing after an introductory clause, the verb inflection is not correct, or a question mark is missing. Did you mean “contract, make” or “contract makes”?
Context: ...llback_id) external; } ``` Also when a contract make IBC transfer request, it should provide...(IF_DT_NN_VBZ)
[uncategorized] ~168-~168: Loose punctuation mark.
Context: ... -memo['evm']['async_callback']['id']: the async callback id is assigned from ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~169-~169: Loose punctuation mark.
Context: ...vm']['async_callback']['contract_addr']`: The address of contract which defines t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~173-~173: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: use the ERC20Wrapper contract to wrap a...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: unwrapped the wrapped token by execute ...(UNLIKELY_OPENING_PUNCTUATION)
171-202: Great addition! Consider enhancing the examples for clarity.The new section on IBC Transfer using ERC20Wrapper is a valuable addition that directly addresses the PR objectives. The JSON examples for both transfer directions provide clear guidance on how to structure the IBC transfer data.
To further improve this section:
- Consider adding a brief explanation of the
fc078758function selector and how it relates to the ERC20Wrapper contract.- Provide more context on how to obtain the ERC20 wrapper contract address mentioned in the comment.
- Add a note explaining the significance of the
denomfield in the context of the ERC20Wrapper.These additions would make the documentation more comprehensive and easier for developers to implement.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~173-~173: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: use the ERC20Wrapper contract to wrap a...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: unwrapped the wrapped token by execute ...(UNLIKELY_OPENING_PUNCTUATION)
x/evm/keeper/keeper.go (1)
258-265: LGTM with a minor suggestion: GetERC20WrapperAddr method implemented correctly.The
GetERC20WrapperAddrmethod is well-implemented and consistent with other getter methods in the Keeper. It properly retrieves the wrapper address and handles errors appropriately.For consistency with other similar methods in the file (e.g.,
GetERC20FactoryAddr), consider adding a comment describing the method's purpose. For example:// GetERC20WrapperAddr returns the address of the ERC20 wrapper contract. func (k Keeper) GetERC20WrapperAddr(ctx context.Context) (common.Address, error) { // ... (existing implementation) }x/evm/contracts/util/Strings.sol (1)
5-5: Consider renamingHEX_DIGITStoDIGITSand trimming unused charactersThe constant
HEX_DIGITSincludes hexadecimal characters'a'to'f', but thetoStringfunction only uses decimal digits'0'to'9'. Renaming it toDIGITSand limiting it to decimal digits can improve clarity and slightly reduce the constant size.Apply this diff to make the change:
-bytes16 private constant HEX_DIGITS = "0123456789abcdef"; +bytes10 private constant DIGITS = "0123456789";Also, update the reference in the assembly code:
- mstore8(ptr, byte(mod(value, 10), HEX_DIGITS)) + mstore8(ptr, byte(mod(value, 10), DIGITS))x/evm/keeper/genesis.go (1)
25-25: Align step numbering in comments for consistencyIn the
InitializeWithDecimalsmethod, the steps are numbered starting from 1, which may cause confusion when compared to theInitializemethod where steps are numbered 1 to 3. For consistency and to aid understanding, consider aligning the step numbering across both methods.Also applies to: 46-46
x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (1)
26-28: Enhance documentation for thewrapfunctionWhile a brief notice is provided, consider adding detailed NatSpec comments to the
wrapfunction. This will improve code readability and help users understand the purpose of each parameter and the function's behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/evm/types/genesis.pb.gois excluded by!**/*.pb.gox/evm/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
- app/ibc-hooks/README.md (7 hunks)
- proto/minievm/evm/v1/genesis.proto (1 hunks)
- proto/minievm/evm/v1/query.proto (1 hunks)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (1 hunks)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (1 hunks)
- x/evm/contracts/util/Strings.go (1 hunks)
- x/evm/contracts/util/Strings.sol (1 hunks)
- x/evm/keeper/erc721_test.go (1 hunks)
- x/evm/keeper/genesis.go (4 hunks)
- x/evm/keeper/genesis_test.go (2 hunks)
- x/evm/keeper/keeper.go (3 hunks)
- x/evm/keeper/query_server.go (1 hunks)
- x/evm/types/address.go (1 hunks)
- x/evm/types/errors.go (1 hunks)
- x/evm/types/keys.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/evm/contracts/util/Strings.go
🧰 Additional context used
🪛 LanguageTool
app/ibc-hooks/README.md
[uncategorized] ~168-~168: Loose punctuation mark.
Context: ... -memo['evm']['async_callback']['id']: the async callback id is assigned from ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~169-~169: Loose punctuation mark.
Context: ...vm']['async_callback']['contract_addr']`: The address of contract which defines t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~173-~173: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: use the ERC20Wrapper contract to wrap a...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: unwrapped the wrapped token by execute ...(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (19)
x/evm/keeper/genesis_test.go (1)
15-15: LGTM! Consider enhancing the test coverage.The addition of the
Erc20Wrapperfield to thegenStateis consistent with the changes in theGenesisStatestruct. This change ensures that the new field is included in genesis initialization and export tests.To improve test coverage, consider adding a specific assertion to verify the contents of the
Erc20Wrapperfield after callingExportGenesis. This would ensure that the field is correctly preserved during the export process. For example:_genState := input.EVMKeeper.ExportGenesis(ctx) require.Equal(t, genState, _genState) require.Equal(t, []byte{5, 6, 7, 8}, _genState.Erc20Wrapper)To ensure that the
Erc20Wrapperfield has been properly added to theGenesisStatestruct, let's verify its presence in the types package:✅ Verification successful
Verified the presence of
Erc20Wrapperin theGenesisStatestruct.The
Erc20Wrapperfield is correctly added to theGenesisStatestruct, and the test appropriately initializes it. No further issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the presence of Erc20Wrapper field in GenesisState struct # Test: Search for the Erc20Wrapper field in the GenesisState struct rg --type go 'type GenesisState struct' -A 20 x/evm/typesLength of output: 1873
proto/minievm/evm/v1/genesis.proto (1)
Line range hint
1-35: Verify integration with related components.The addition of the
erc20_wrapperfield to theGenesisStatemessage is a focused change that complements the existingerc20_factoryfield. This change maintains backward compatibility and aligns well with the PR objectives.To ensure proper integration, please verify:
- The genesis initialization logic in the module to handle the new
erc20_wrapperfield.- Any CLI commands or API endpoints that interact with the genesis state.
- Unit tests covering the new field in the genesis state.
Run the following script to help with verification:
x/evm/types/address.go (2)
17-17: New ERC20WrapperSalt variable addedThe addition of
ERC20WrapperSaltis consistent with the existing pattern of defining salt values for different contract types. The naming convention and type (uint64) match the existingERC20FactorySalt.However, to improve clarity and maintainability:
- Consider adding a comment explaining the purpose of this salt, similar to the comment for
ERC20FactorySalt.- Verify that the value
2is intentional and doesn't conflict with any existing salt values.Consider adding a comment to explain the purpose of
ERC20WrapperSalt:+// ERC20WrapperSalt is the salt used to create the ERC20 wrapper address var ERC20WrapperSalt = uint64(2)To ensure the new salt value doesn't conflict with existing ones, let's search for other salt definitions:
✅ Verification successful
Salt value is unique and does not conflict
The new
ERC20WrapperSaltvalue of2does not conflict with existing salt values.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for salt definitions in Go files rg --type go 'var \w+Salt = uint64\(' -g '!vendor/'Length of output: 165
Line range hint
1-43: Verify usage of ERC20WrapperSalt in other filesThe addition of
ERC20WrapperSaltlooks good in this file. To ensure it's being used correctly:
- Verify that
ERC20WrapperSaltis imported and used appropriately in files where ERC20 wrapper functionality is implemented.- Check if any existing documentation needs to be updated to reflect this new salt.
Let's search for potential usages of this new salt:
✅ Verification successful
ERC20WrapperSalt Usage Verified
The
ERC20WrapperSaltis correctly defined and utilized within the codebase:
- x/evm/keeper/genesis.go: Used in the
EVMCreate2function to create ERC20 wrappers.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential usages of ERC20WrapperSalt rg --type go 'ERC20WrapperSalt' -g '!vendor/'Length of output: 223
x/evm/types/errors.go (1)
37-37: New error variable added: ErrFailedToGetERC20WrapperAddrThe addition of
ErrFailedToGetERC20WrapperAddris consistent with the existing error structure and naming conventions. The error message is clear and concise, following the style of other error messages in this file. The error code (28) is unique and follows the sequence of previously defined errors.This new error aligns well with the PR objectives of introducing an ERC20 wrapper for IBC transfers. It provides a specific error case for when the system fails to retrieve the ERC20 wrapper address, which is likely a critical step in the new functionality.
To ensure this error is used appropriately in the codebase, let's check for its usage:
✅ Verification successful
Usage of
ErrFailedToGetERC20WrapperAddrConfirmedThe new error
ErrFailedToGetERC20WrapperAddris properly utilized inx/evm/keeper/keeper.go, ensuring appropriate error handling when retrieving the ERC20 wrapper address.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the usage of the new error variable # Test: Search for the usage of ErrFailedToGetERC20WrapperAddr rg --type go "ErrFailedToGetERC20WrapperAddr"Length of output: 280
x/evm/keeper/erc721_test.go (1)
23-23: Verify the intention behind changing the contract address generation index.The contract address generation index has been changed from 1 to 2. While this change doesn't break the current test, it's important to understand the reasoning behind this modification.
Please confirm:
- Is this change intentional and aligned with updates in the contract address generation logic?
- Are there any other parts of the codebase that might be affected by this change in contract address?
To help verify the impact, you can run the following script:
This will help identify any hardcoded addresses or other uses of
CreateAddressthat might need to be updated.✅ Verification successful
Change to Contract Address Generation Index Verified
The contract address generation index has been changed from 1 to 2. Analysis shows that this modification is isolated to the
erc721_test.gofile and does not impact other parts of the codebase. No hardcoded contract addresses or other dependencies are affected by this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for hardcoded contract addresses that might be affected by this change # Search for potential hardcoded addresses in Go files echo "Searching for potential hardcoded addresses in Go files:" rg --type go '0x[a-fA-F0-9]{40}' # Search for the use of CreateAddress function in other test files echo "Searching for other uses of CreateAddress in test files:" rg --type go 'crypto\.CreateAddress\(types\.StdAddress,'Length of output: 127372
x/evm/keeper/query_server.go (1)
159-169: Summary: ERC20Wrapper method enhances queryServerImpl functionalityThe addition of the
ERC20Wrappermethod to thequeryServerImplstruct is a valuable enhancement that aligns with the PR objectives. It provides a way to query the ERC20 wrapper address, which is crucial for facilitating IBC transfers between MiniEVM and other virtual machine chains.The implementation follows the established patterns in the file, ensuring consistency and maintainability. With the suggested minor improvements, this addition will contribute effectively to the overall functionality of the EVM module.
x/evm/keeper/keeper.go (3)
63-63: LGTM: New ERC20WrapperAddr field added correctly.The addition of the
ERC20WrapperAddrfield to the Keeper struct is consistent with the existing pattern and appropriately typed for storing a single address.
133-133: LGTM: ERC20WrapperAddr field initialized correctly.The initialization of the
ERC20WrapperAddrfield in theNewKeeperfunction is consistent with other similar fields and uses the correct key and value type.
Line range hint
63-265: Summary: ERC20 wrapper functionality successfully integrated.The changes in this file successfully integrate the ERC20 wrapper functionality into the Keeper. The additions include:
- A new
ERC20WrapperAddrfield in the Keeper struct.- Proper initialization of the new field in the
NewKeeperfunction.- A new
GetERC20WrapperAddrmethod to retrieve the wrapper address.These changes align well with the PR objective of introducing an ERC20 wrapper for IBC transfers. They provide the necessary infrastructure to manage and access the ERC20 wrapper address within the EVM module.
x/evm/contracts/util/Strings.sol (2)
7-25:toStringfunction is efficiently implementedThe
toStringfunction effectively converts auint256value to its decimal string representation. Utilizing thelog10function to determine the string length and inline assembly for character insertion optimizes performance. The use ofuncheckedblocks is appropriate here to save gas without sacrificing safety in this context.
27-59:log10function accurately computes digit countThe
log10function correctly calculates the number of digits in theuint256value by iteratively dividing the value by powers of 10. This approach efficiently reduces the number of computations needed, which enhances performance for large numbers.x/evm/keeper/genesis.go (4)
15-18: Well-documented initialization stepsThe addition of step 3 in the initialization process enhances clarity by explicitly stating the deployment of the wrapper ERC20 factory contract for IBC transfers. This improves the readability and maintainability of the code.
80-82: Proper initialization of the ERC20 wrapper addressSetting the
ERC20WrapperAddrfrom the genesis state ensures that the wrapper contract address is correctly initialized during genesis, which is essential for the contract's proper functioning.
165-169: Including the ERC20 wrapper address in the exported genesis stateBy retrieving and including the
ERC20Wrapperaddress in the exported genesis state, you ensure that the wrapper contract's state is preserved across network restarts or upgrades, maintaining consistency in the state.Also applies to: 176-176
52-62:⚠️ Potential issuePotential issue: Deploying the incorrect contract code for the wrapper contract
In line 53, the
codevariable used to deploy the wrapper contract is the same as the one used for the ERC20 factory contract (erc20_factory.Erc20FactoryBin). If the wrapper ERC20 factory contract is intended to be different from the ERC20 factory contract, ensure that you're using the appropriate bytecode for the wrapper. Deploying the same contract code may not provide the intended functionality for IBC transfers.Run the following script to verify that the correct contract code is being used for the wrapper contract:
x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (2)
45-45: Verify thatmintfunction exists and operates correctlyEnsure that the
ERC20implementation used supports themintfunction and that it operates as expected when minting wrapped tokens.
77-77: ConfirmburnFromfunction availability and access controlEnsure that the
ERC20contract used includes theburnFromfunction and that appropriate approvals are in place formsg.senderto burn tokens.x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (1)
1-717: No issues found in the auto-generated codeThe
ERC20Wrapper.gofile is auto-generated, and any manual changes will be lost. The generated code appears correct and adheres to standard conventions for Go Ethereum bindings. All methods and structures are appropriately defined for interacting with theErc20Wrappercontract.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
x/evm/contracts/util/Strings.sol (4)
1-11: LGTM! Consider adding NatSpec comments for better documentation.The library declaration, constants, and error definition look good. The use of a recent Solidity version (^0.8.24) is commendable for security and feature benefits.
Consider adding NatSpec comments for the library and error definition to improve documentation. For example:
/// @title Strings Library /// @notice A library for string manipulation and conversion operations library Strings { // ... existing code ... /// @notice Error thrown when the hex string length is insufficient to represent the value /// @param value The value being converted /// @param length The specified length for the hex string error StringsInsufficientHexLength(uint256 value, uint256 length); }
13-33: LGTM! Consider a small optimization for gas efficiency.The
toHexStringfunctions are well-implemented and handle both uint256 and address inputs correctly. The error handling for insufficient length is appropriate.Consider a small optimization in the uint256 version to potentially save gas:
function toHexString( uint256 value, uint256 length ) internal pure returns (string memory) { uint256 localValue = value; bytes memory buffer = new bytes(2 * length + 2); buffer[0] = "0"; buffer[1] = "x"; - for (uint256 i = 2 * length + 1; i > 1; --i) { + for (uint256 i = 2 * length + 1; i > 1; ) { buffer[i] = HEX_DIGITS[localValue & 0xf]; localValue >>= 4; + unchecked { --i; } } if (localValue != 0) { revert StringsInsufficientHexLength(value, length); } return string(buffer); }This change uses an unchecked decrement in the loop, which can save gas since overflow is not a concern in this case.
35-53: LGTM! Consider adding a check for zero value.The
toStringfunction is well-implemented, using efficient techniques like unchecked math and inline assembly. The algorithm for converting the number to a string is correct and optimized.Consider adding a check for zero value at the beginning of the function for better handling of edge cases:
function toString(uint256 value) internal pure returns (string memory) { + if (value == 0) { + return "0"; + } unchecked { uint256 length = log10(value) + 1; string memory buffer = new string(length); // ... rest of the function } }This addition will handle the zero case more efficiently and make the function more robust.
1-88: Excellent implementation of the Strings library. Consider future enhancements.The Strings library is well-implemented, providing essential string manipulation functions for Solidity contracts. The code is efficient, gas-optimized, and based on the reputable OpenZeppelin implementation.
For future improvements, consider:
- Adding more string utility functions, such as
concat,slice, orreplace.- Implementing a comprehensive test suite to ensure the robustness of the library.
- Adding more detailed NatSpec comments for better documentation.
- Considering the addition of Unicode support for more advanced string operations, if relevant to your use case.
These enhancements would further increase the utility and maintainability of the library.
x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (1)
38-44: Consider removing deprecated variables if they are no longer neededThe variables
Erc20WrapperABIandErc20WrapperBinare marked as deprecated. If they're no longer used elsewhere in the codebase, consider removing them to clean up the code.However, if they are kept for backward compatibility or used in external packages, it's acceptable to retain them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (1 hunks)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (1 hunks)
- x/evm/contracts/util/Strings.go (1 hunks)
- x/evm/contracts/util/Strings.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol
🧰 Additional context used
🔇 Additional comments (3)
x/evm/contracts/util/Strings.sol (1)
55-87: LGTM! Efficient implementation of log10.The
log10function is well-implemented, using an efficient algorithm to calculate the base-10 logarithm of a uint256 value. The use of unchecked math operations is appropriate in this context and helps optimize gas usage.The series of comparisons and divisions effectively computes the result without resorting to floating-point operations, which is ideal for Solidity. This implementation correctly handles all possible uint256 values.
x/evm/contracts/util/Strings.go (1)
1-3: Auto-generated code: Do not editThis file is auto-generated and any manual changes will be lost. The generated code appears to be correct.
x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (1)
1-759: LGTM: The generated code is well-structured and accurateThe auto-generated Go bindings for the
Erc20Wrappercontract are correctly implemented and adhere to Go conventions. The methods and structures properly map to the contract's functionalities.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (4)
x/evm/keeper/query_server_test.go (1)
68-79: LGTM with minor suggestions for improvementThe new test function
Test_Query_ERC20Wrapperis well-structured and adequately tests the functionality of querying the ERC20 wrapper address. It follows the established patterns in the file and covers the essential aspects of the feature.To further enhance the test, consider the following suggestions:
Add more specific error assertions. For example:
require.NoError(t, err, "Failed to get ERC20 wrapper address")Consider renaming the test to be more specific, e.g.,
TestQueryServer_ERC20Wrapper.Add comments to explain the purpose of each step in the test, improving readability for future developers.
app/ibc-hooks/README.md (3)
Line range hint
80-139: LGTM: Comprehensive explanation of ICS20 packet structure with minor suggestionThe ICS20 packet structure section clearly defines the JSON structure and includes the new
valuefield in thememo.evm.messageobject. The criteria for a correctly formatted ICS20 packet have been updated to include thevaluefield, which is consistent with the changes in the MsgCall struct.However, there's a minor improvement that could be made to enhance clarity:
Consider updating the formatting criteria explanation to explicitly mention the
valuefield:- `memo["evm"]["message"]` has exactly 3 entries, `"contract_addr"`, `"input"`, `"value"` + `memo["evm"]["message"]` has exactly 3 entries: `"contract_addr"`, `"input"`, and `"value"`This change makes it clearer that
valueis now a required field in the message structure.🧰 Tools
🪛 LanguageTool
[style] ~122-~122: Consider removing “of” to be more concise
Context: ...packet as directed towards evmhooks iff all of the following hold: -memois not blank ...(ALL_OF_THE)
Line range hint
140-170: LGTM: Clear explanation of Execution flow and Async Callback with minor formatting suggestionThe Execution flow and Async Callback sections provide clear and valuable information. The explanation of how contracts can implement callback functions for IBC transfers is particularly helpful.
Consider improving the formatting of the new lines explaining the
idandcontract_addrfields in the async callback data:- - `memo['evm']['async_callback']['id']`: the async callback id is assigned from the contract. so later it will be passed as argument of `ibc_ack` and `ibc_timeout`. - - `memo['evm']['async_callback']['contract_addr']`: The address of contract which defines the callback function. + - `memo['evm']['async_callback']['id']`: The async callback ID assigned by the contract. This ID will be passed as an argument to `ibc_ack` and `ibc_timeout`. + - `memo['evm']['async_callback']['contract_addr']`: The address of the contract that defines the callback function.This change improves readability and consistency with the rest of the document.
🧰 Tools
🪛 LanguageTool
[grammar] ~166-~166: Possibly, a comma is missing after an introductory clause, the verb inflection is not correct, or a question mark is missing. Did you mean “contract, make” or “contract makes”?
Context: ...llback_id) external; } ``` Also when a contract make IBC transfer request, it should provide...(IF_DT_NN_VBZ)
[uncategorized] ~168-~168: Loose punctuation mark.
Context: ... -memo['evm']['async_callback']['id']: the async callback id is assigned from ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~169-~169: Loose punctuation mark.
Context: ...vm']['async_callback']['contract_addr']`: The address of contract which defines t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~173-~173: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: use the ERC20Wrapper contract to wrap a...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: unwrapped the wrapped token by execute ...(UNLIKELY_OPENING_PUNCTUATION)
171-202: LGTM: Comprehensive explanation of IBC Transfer using ERC20Wrapper with minor typo correctionThe new section on IBC Transfer using ERC20Wrapper is a valuable addition that aligns well with the PR objectives. It provides clear explanations and examples for transfers in both directions, along with a comprehensive JSON example of the data structure.
There's a minor typo in the explanation of the dst -> src transfer:
- `dst -> src`: unwrapped the wrapped token by execute hook + `dst -> src`: unwrap the wrapped token by executing hookThis change corrects the grammar and makes the explanation more clear.
The JSON example provided is comprehensive and correctly includes the new
valuefield, which is consistent with the changes made earlier in the document.🧰 Tools
🪛 LanguageTool
[uncategorized] ~173-~173: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: use the ERC20Wrapper contract to wrap a...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: unwrapped the wrapped token by execute ...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/evm/types/query.pb.gois excluded by!**/*.pb.gox/evm/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (6)
- app/ibc-hooks/README.md (7 hunks)
- proto/minievm/evm/v1/query.proto (2 hunks)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (1 hunks)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (1 hunks)
- x/evm/keeper/query_server.go (1 hunks)
- x/evm/keeper/query_server_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/keeper/query_server.go
🧰 Additional context used
🪛 LanguageTool
app/ibc-hooks/README.md
[uncategorized] ~168-~168: Loose punctuation mark.
Context: ... -memo['evm']['async_callback']['id']: the async callback id is assigned from ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~169-~169: Loose punctuation mark.
Context: ...vm']['async_callback']['contract_addr']`: The address of contract which defines t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~173-~173: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: use the ERC20Wrapper contract to wrap a...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: unwrapped the wrapped token by execute ...(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (5)
proto/minievm/evm/v1/query.proto (2)
102-104: LGTM: QueryERC20WrapperRequest message definitionThe
QueryERC20WrapperRequestmessage definition is correct and consistent with other similar request messages in the file. An empty request is appropriate for querying a singleton contract address.
106-112: LGTM: QueryERC20WrapperResponse message definitionThe
QueryERC20WrapperResponsemessage definition is correct and consistent with other similar response messages in the file. Theaddressfield is properly documented, and the use ofgogoproto.equalis appropriate.app/ibc-hooks/README.md (3)
Line range hint
1-14: LGTM: Clear introduction and explanation of EVM HooksThe introduction and EVM Hooks section provide a clear explanation of the module's purpose and mechanism. The renaming from "Move Hooks" to "EVM Hooks" is consistent with the focus on EVM contract execution.
Line range hint
15-79: LGTM: Clear explanation of EVM Contract Execution Format with newValuefieldThe EVM Contract Execution Format section clearly defines the structure of HookData and MsgCall types. The addition of the
Valuefield to the MsgCall struct is a significant change that aligns with the PR objectives of enhancing IBC transfers. The explanation of how each field is populated, including the newValuefield, is comprehensive and helpful.
Line range hint
1-202: LGTM: Comprehensive update aligning with PR objectivesThis README.md file has been significantly and effectively updated to document the new ERC20 wrapper functionality and its integration with IBC transfers. The changes include:
- Renaming "Move Hooks" to "EVM Hooks" throughout the document.
- Adding the new
Valuefield to the MsgCall struct and related JSON structures.- Updating the criteria for correctly formatted ICS20 packets.
- Adding a new section on IBC Transfer using ERC20Wrapper with clear examples and explanations.
These updates align perfectly with the PR objectives of introducing an ERC20 wrapper for IBC transfers between MiniEVM and other virtual machine chains. The document maintains a consistent structure and style, providing clear and comprehensive guidance for developers working with this functionality.
🧰 Tools
🪛 LanguageTool
[grammar] ~166-~166: Possibly, a comma is missing after an introductory clause, the verb inflection is not correct, or a question mark is missing. Did you mean “contract, make” or “contract makes”?
Context: ...llback_id) external; } ``` Also when a contract make IBC transfer request, it should provide...(IF_DT_NN_VBZ)
[uncategorized] ~168-~168: Loose punctuation mark.
Context: ... -memo['evm']['async_callback']['id']: the async callback id is assigned from ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~169-~169: Loose punctuation mark.
Context: ...vm']['async_callback']['contract_addr']`: The address of contract which defines t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~173-~173: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: use the ERC20Wrapper contract to wrap a...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: unwrapped the wrapped token by execute ...(UNLIKELY_OPENING_PUNCTUATION)
djm07073
force-pushed
the
feat/wrapper-erc20
branch
from
662ef0c to
3e397bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (12)
x/evm/keeper/genesis_test.go (1)
19-19: LGTM! Consider adding a comment for clarity.The addition of the
Erc20Wrapperfield to thegenStateis correct and aligns with the changes in theGenesisStatemessage. This update ensures that the test case covers the new field.Consider adding a brief comment explaining the purpose of this field for better clarity:
+ // Set ERC20 wrapper address for testing genState.Erc20Wrapper = []byte{5, 6, 7, 8}x/evm/keeper/genesis.go (4)
15-18: LGTM! Consider adding a note about potential future compatibility.The updated comments clearly explain the new step for deploying the wrapper ERC20 factory contract. The note about incompatibility due to 18 decimals is crucial information.
Consider adding a brief note about potential plans for future compatibility or if there are any ongoing efforts to address this limitation.
25-25: LGTM! Consider enhancing error handling.The implementation of the wrapper ERC20 factory contract deployment is consistent with the updated comments and follows the same pattern as the ERC20 factory deployment.
Consider wrapping the errors returned from
EVMCreate2andERC20WrapperAddr.Setwith additional context to aid in debugging. For example:if err != nil { return fmt.Errorf("failed to deploy wrapper ERC20 factory: %w", err) }Also applies to: 52-62
232-235: LGTM! Consider consistent error handling.The addition of retrieving and including the ERC20 wrapper address in the exported genesis state is correct and consistent with how other components are handled.
For consistency with the error handling of
ERC20FactoryAddr.Get, consider usingpanic(err)directly instead of the if-statement for theERC20WrapperAddr.Geterror:wrapperAddr, err := k.ERC20WrapperAddr.Get(ctx) if err != nil { panic(err) }Also applies to: 245-245
Line range hint
1-248: Overall LGTM! Changes align well with PR objectives.The modifications to
genesis.gosuccessfully implement the ERC20 wrapper functionality for IBC transfers. The changes are well-integrated with the existing code and follow consistent patterns for initialization, genesis import/export, and state management.Key points:
- Proper initialization of the ERC20 wrapper contract
- Consistent handling in genesis import and export
- Clear comments explaining the new functionality and limitations
These changes effectively support the PR's objective of introducing an ERC20 wrapper for compatible IBC transfers from MiniEVM to other virtual machine chains.
As the project evolves, consider the following architectural improvements:
- Implement a strategy for handling different decimal configurations across chains to address the current 18-decimal limitation.
- Explore options for making the wrapper contract upgradeable to facilitate future improvements or bug fixes.
app/ibc-hooks/README.md (4)
Line range hint
15-79: LGTM: Addition ofValuefield enhances EVM Contract Execution FormatThe addition of the
Valuefield to theMsgCallstruct and its inclusion in the EVM call message construction is a valuable enhancement. This allows for specifying the amount of fee-denominated tokens to transfer to the contract, which is crucial for the ERC20 wrapper functionality.However, to improve clarity, consider adding a brief explanation of the purpose and usage of the
Valuefield in the context of ERC20 wrapping and IBC transfers.Would you like me to suggest an additional explanatory sentence for the
Valuefield?
Line range hint
81-119: LGTM: Updated ICS20 packet structure withValuefieldThe inclusion of the
Valuefield in thememo["evm"]["message"]structure and the updated validation criteria are consistent with the changes in the EVM Contract Execution Format. This ensures that the ICS20 packet structure properly supports the ERC20 wrapper functionality.To improve clarity, consider adding a brief comment in the JSON example explaining the purpose of the
Valuefield, similar to the comments for other fields.Would you like me to suggest an additional comment for the
Valuefield in the JSON example?🧰 Tools
🪛 LanguageTool
[style] ~122-~122: Consider removing “of” to be more concise
Context: ...packet as directed towards evmhooks iff all of the following hold: -memois not blank ...(ALL_OF_THE)
Line range hint
121-170: LGTM: Execution flow and Async Callback sectionsThe Execution flow and Async Callback sections provide valuable information for developers implementing contracts that interact with IBC hooks. The explanation of the callback functions and the required interface is particularly helpful.
To enhance the documentation further, consider adding a brief example of how a contract might use the async callback feature in the context of the ERC20 wrapper. This could help developers better understand the practical application of this feature.
Would you like me to suggest a brief example of using async callbacks with the ERC20 wrapper?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~165-~165: Possible missing comma found.
Context: ...ut(uint64 callback_id) external; } ``` Also when a contract make IBC transfer reque...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~166-~166: Possibly, a comma is missing after an introductory clause, the verb inflection is not correct, or a question mark is missing. Did you mean “contract, make” or “contract makes”?
Context: ...llback_id) external; } ``` Also when a contract make IBC transfer request, it should provide...(IF_DT_NN_VBZ)
[uncategorized] ~168-~168: Loose punctuation mark.
Context: ... -memo['evm']['async_callback']['id']: the async callback id is assigned from ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~169-~169: Loose punctuation mark.
Context: ...vm']['async_callback']['contract_addr']`: The address of contract which defines t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~173-~173: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: use the ERC20Wrapper contract to wrap a...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: unwrapped the wrapped token by execute ...(UNLIKELY_OPENING_PUNCTUATION)
171-202: LGTM: New section on IBC Transfer using ERC20WrapperThe addition of the "IBC Transfer using ERC20Wrapper" section is excellent and directly addresses the PR objectives. It provides valuable information on how to use the ERC20Wrapper for IBC transfers in both directions.
To further improve this section:
- Consider adding a brief explanation of why the ERC20Wrapper is necessary and its benefits.
- The
src -> dstexample could be expanded with a JSON example similar to thedst -> srccase.- In the
dst -> srcJSON example, add comments explaining the purpose of thecontract_addrandinputfields, similar to other comments in the example.- Consider adding a note on how to query the ERC20 wrapper contract address, as mentioned in the comment for the
contract_addrfield.Would you like me to suggest expanded explanations or examples for any of these points?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~173-~173: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: use the ERC20Wrapper contract to wrap a...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: unwrapped the wrapped token by execute ...(UNLIKELY_OPENING_PUNCTUATION)
x/evm/contracts/util/Strings.sol (2)
35-53: Consider adding comments to explain inline assembly intoStringThe
toStringfunction uses inline assembly to optimize performance when converting auint256to a string. Adding comments to explain the assembly code will improve readability and maintainability.
5-88: Consider adding unit tests for theStringslibrary functionsAdding unit tests for each function in the
Stringslibrary will help ensure correctness and catch potential edge cases, such as handling zero values.x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (1)
465-466: Clarify comment wording for error variableIn the struct definitions for
Erc20WrapperERC20CreatedIteratorandErc20WrapperOwnershipTransferredIterator, the comment for thefailfield reads "Occurred error to stop iteration". For clarity and grammatical correctness, consider rephrasing it to "Error occurred to stop iteration".Apply this diff to improve the comments:
type Erc20WrapperERC20CreatedIterator struct { Event *Erc20WrapperERC20Created // Event containing the contract specifics and raw log contract *bind.BoundContract // Generic contract to use for unpacking event data event string // Event name to use for unpacking event data logs chan types.Log // Log channel receiving the found contract events sub ethereum.Subscription // Subscription for errors, completion and termination done bool // Whether the subscription completed delivering logs - fail error // Occurred error to stop iteration + fail error // Error occurred to stop iteration } type Erc20WrapperOwnershipTransferredIterator struct { Event *Erc20WrapperOwnershipTransferred // Event containing the contract specifics and raw log contract *bind.BoundContract // Generic contract to use for unpacking event data event string // Event name to use for unpacking event data logs chan types.Log // Log channel receiving the found contract events sub ethereum.Subscription // Subscription for errors, completion and termination done bool // Whether the subscription completed delivering logs - fail error // Occurred error to stop iteration + fail error // Error occurred to stop iteration }Also applies to: 618-619
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
x/evm/types/genesis.pb.gois excluded by!**/*.pb.gox/evm/types/query.pb.gois excluded by!**/*.pb.gox/evm/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (16)
- app/ibc-hooks/README.md (7 hunks)
- proto/minievm/evm/v1/genesis.proto (1 hunks)
- proto/minievm/evm/v1/query.proto (2 hunks)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (1 hunks)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (1 hunks)
- x/evm/contracts/util/Strings.go (1 hunks)
- x/evm/contracts/util/Strings.sol (1 hunks)
- x/evm/keeper/erc721_test.go (1 hunks)
- x/evm/keeper/genesis.go (5 hunks)
- x/evm/keeper/genesis_test.go (1 hunks)
- x/evm/keeper/keeper.go (3 hunks)
- x/evm/keeper/query_server.go (1 hunks)
- x/evm/keeper/query_server_test.go (1 hunks)
- x/evm/types/address.go (1 hunks)
- x/evm/types/errors.go (1 hunks)
- x/evm/types/keys.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/evm/contracts/util/Strings.go
🚧 Files skipped from review as they are similar to previous changes (7)
- proto/minievm/evm/v1/genesis.proto
- proto/minievm/evm/v1/query.proto
- x/evm/keeper/query_server.go
- x/evm/keeper/query_server_test.go
- x/evm/types/address.go
- x/evm/types/errors.go
- x/evm/types/keys.go
🧰 Additional context used
🪛 LanguageTool
app/ibc-hooks/README.md
[uncategorized] ~168-~168: Loose punctuation mark.
Context: ... -memo['evm']['async_callback']['id']: the async callback id is assigned from ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~169-~169: Loose punctuation mark.
Context: ...vm']['async_callback']['contract_addr']`: The address of contract which defines t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~173-~173: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: use the ERC20Wrapper contract to wrap a...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~175-~175: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: unwrapped the wrapped token by execute ...(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (10)
x/evm/keeper/genesis_test.go (1)
Line range hint
1-85: Test updated correctly for new ERC20 wrapper fieldThe
Test_Genesisfunction has been appropriately updated to include the newErc20Wrapperfield in the genesis state. This change ensures that the test coverage remains comprehensive after the addition of the ERC20 wrapper functionality to the system.The test still verifies the full cycle of initializing the genesis state and then exporting it, implicitly testing the new field. This approach maintains the integrity of the genesis state handling in the EVM keeper.
x/evm/keeper/erc721_test.go (1)
23-23: LGTM. Consider adding a comment to explain the change.The change from index 1 to 2 for generating the contract address seems intentional and the test still passes. However, it would be helpful to add a comment explaining why index 2 is now used. This could provide context for future maintainers.
Consider adding a comment like:
// Using index 2 to account for the ERC20Wrapper contract address contractAddr := crypto.CreateAddress(types.StdAddress, 2)Let's verify if this change is consistent with other parts of the codebase:
✅ Verification successful
Verified: Consistent usage of
crypto.CreateAddressacross the codebase.The change from index 1 to 2 in
erc721_test.goaligns with the usage in other tests. Consider adding a comment to explain the reason for using index 2 for clarity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of CreateAddress to ensure consistency rg "crypto\.CreateAddress\(.*\)" --type goLength of output: 407
x/evm/keeper/genesis.go (1)
80-82: LGTM! Consistent initialization of the ERC20 wrapper address.The addition of setting the ERC20WrapperAddr from the genesis state is consistent with the initialization of other components and ensures proper state management.
app/ibc-hooks/README.md (1)
Line range hint
5-14: LGTM: Improved introduction and EVM Hooks explanationThe updated introduction and EVM Hooks section provide a clear and concise explanation of the purpose and functionality of EVM hooks in the context of IBC transfers. This aligns well with the PR objectives of introducing an ERC20 wrapper for compatible IBC transfers.
x/evm/keeper/keeper.go (3)
63-63: LGTM: New field for ERC20 wrapper address.The addition of the
ERC20WrapperAddrfield to theKeeperstruct is appropriate and aligns with the PR objectives. It's correctly typed ascollections.Item[[]byte]for storing an address and is logically placed among other ERC20-related fields.
133-133: LGTM: Proper initialization of ERC20WrapperAddr.The
ERC20WrapperAddrfield is correctly initialized in theNewKeeperfunction usingcollections.NewItem. The key (types.ERC20WrapperAddrKey) and value type (collections.BytesValue) are appropriate for storing an address. The initialization is logically placed among other ERC20-related initializations.
258-265: LGTM: Well-implemented GetERC20WrapperAddr method.The
GetERC20WrapperAddrmethod is correctly implemented:
- It has the appropriate signature, returning
(common.Address, error).- Error handling is consistent with other methods, using a specific error type
types.ErrFailedToGetERC20WrapperAddr.- The byte slice is correctly converted to a
common.Addressbefore returning.The implementation aligns well with the PR objectives and maintains consistency with other getter methods in the file.
x/evm/contracts/util/Strings.sol (3)
13-29:toHexString(uint256, uint256)function implementation looks goodThe function correctly converts a
uint256value to its hexadecimal string representation with the specified length, handling insufficient length cases appropriately.
31-33:toHexString(address)function implementation looks goodThe function correctly converts an
addressto its hexadecimal string representation using thetoHexStringhelper.
55-87:log10function implementation is efficientThe
log10function efficiently calculates the base-10 logarithm of auint256value using integer arithmetic.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
x/evm/keeper/query_server.go (1)
159-169: LGTM! Consider a minor improvement for consistency.The implementation of the
ERC20Wrappermethod looks good. It correctly retrieves the ERC20 wrapper address and handles potential errors. For consistency with other methods in this file, consider using a named return value for the error.Here's a suggested minor improvement:
-func (qs *queryServerImpl) ERC20Wrapper(ctx context.Context, req *types.QueryERC20WrapperRequest) (*types.QueryERC20WrapperResponse, error) { +func (qs *queryServerImpl) ERC20Wrapper(ctx context.Context, req *types.QueryERC20WrapperRequest) (res *types.QueryERC20WrapperResponse, err error) { wrapper, err := qs.Keeper.GetERC20WrapperAddr(ctx) if err != nil { return nil, err } return &types.QueryERC20WrapperResponse{ Address: wrapper.Hex(), }, nil }This change aligns the method signature with others in the file, such as the
Callmethod, which uses named return values.x/evm/keeper/genesis.go (3)
15-18: LGTM with a minor suggestion.The updated comments accurately reflect the new functionality for deploying the wrapper ERC20 factory contract. However, to improve clarity, consider elaborating on the incompatibility issue mentioned in step 3.
Suggestion: Expand on the incompatibility issue, e.g., "Deploy and store the wrapper ERC20 factory contract for IBC transfers. Note: This may not be compatible with all destination chains due to the fixed 18 decimal configuration."
52-62: LGTM with a suggestion for improved error handling.The implementation for deploying and storing the wrapper ERC20 factory contract is consistent with the existing pattern and uses appropriate constants and collections.
Consider combining the error checks to reduce nesting and improve readability:
wrapperAddr, _, _, err := k.EVMCreate2(ctx, types.StdAddress, code, nil, types.ERC20WrapperSalt, nil) if err != nil { return err } return k.ERC20WrapperAddr.Set(ctx, wrapperAddr.Bytes())This approach eliminates the need for multiple error checks and simplifies the code structure.
232-236: LGTM with a suggestion for consistent error handling.The addition of exporting the ERC20 wrapper address is implemented correctly and consistently with the existing pattern.
For consistency with the error handling for
factoryAddr, consider combining the error check and panic forwrapperAddr:wrapperAddr, err := k.ERC20WrapperAddr.Get(ctx) if err != nil { panic(err) }This approach maintains consistency with the error handling pattern used for
factoryAddr.Also applies to: 245-245
x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (1)
20-32: Consider changingibcCallBackmapping visibilityThe
ibcCallBackmapping is currently private, which might limit the ability to verify callback data externally. Consider changing it to public for better transparency and easier debugging, unless there are specific security concerns for keeping it private.app/ibc-hooks/README.md (4)
Line range hint
15-77: LGTM: Updated MsgCall struct with Value fieldThe addition of the 'Value' field to the MsgCall struct is a good improvement, allowing for the specification of token amounts in contract calls. This is crucial for the ERC20 wrapper functionality.
Consider adding a brief explanation of how the 'Value' field interacts with the ICS20 transfer amount, if there's any specific relationship or constraints between them.
Line range hint
82-118: LGTM: Updated ICS20 packet structure and validation criteriaThe JSON structure for the ICS20 packet has been correctly updated to include the 'value' field in the 'evm' message. The validation criteria have also been appropriately adjusted to require the 'value' field.
Consider adding a brief explanation of the expected format or constraints for the 'value' field (e.g., numeric string, decimal places, etc.) to provide more clarity for implementers.
🧰 Tools
🪛 LanguageTool
[style] ~121-~121: Consider removing “of” to be more concise
Context: ...packet as directed towards evmhooks iff all of the following hold: -memois not blank ...(ALL_OF_THE)
167-169: LGTM: Well-documented Async Callback functionalityThe Async Callback section provides a clear explanation of the purpose and implementation of callbacks for IBC transfers. The new fields in the memo structure for async callbacks are correctly documented.
Consider adding a brief example of how these fields would be used in a typical scenario to further illustrate their purpose and usage.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~167-~167: Loose punctuation mark.
Context: ... -memo['evm']['async_callback']['id']: the async callback id is assigned from ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~168-~168: Loose punctuation mark.
Context: ...vm']['async_callback']['contract_addr']`: The address of contract which defines t...(UNLIKELY_OPENING_PUNCTUATION)
170-198: LGTM: New section on IBC Transfer using ERC20WrapperThe addition of this section directly addresses the PR objective of introducing an ERC20 wrapper for IBC transfers. The JSON example provides a clear template for implementers.
Consider the following improvements:
- Expand on the explanation of the transfer directions (src -> dst and dst -> src) to provide more context on how these transfers work in practice.
- Add a brief description of how to query the ERC20 wrapper contract address, as mentioned in the comment on line 190.
- Provide more details on the
packfunction used in theinputfield (line 191), including its parameters and expected format.These additions would make the documentation more comprehensive and easier for developers to implement correctly.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~172-~172: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: Execute the ERC20Wrapper contract to wr...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~174-~174: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: ibc-trasfer and execute the ERC20Wrappe...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- app/ibc-hooks/README.md (7 hunks)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (1 hunks)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (1 hunks)
- x/evm/keeper/genesis.go (5 hunks)
- x/evm/keeper/query_server.go (1 hunks)
🧰 Additional context used
🪛 LanguageTool
app/ibc-hooks/README.md
[uncategorized] ~167-~167: Loose punctuation mark.
Context: ... -memo['evm']['async_callback']['id']: the async callback id is assigned from ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~168-~168: Loose punctuation mark.
Context: ...vm']['async_callback']['contract_addr']`: The address of contract which defines t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~172-~172: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: Execute the ERC20Wrapper contract to wr...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~174-~174: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: ibc-trasfer and execute the ERC20Wrappe...(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (12)
x/evm/keeper/genesis.go (2)
80-82: LGTM: Consistent implementation for ERC20 wrapper initialization.The addition of setting the
ERC20WrapperAddrfrom thegenStateis consistent with the existing pattern and ensures proper initialization of the wrapper address when the state root is not empty.
52-56: Addressing past review comment: ERC20 wrapper code deployment.The implementation now deploys the ERC20 wrapper contract, addressing the concern raised in the past review comment. However, it's using the same code as the ERC20 factory contract.
Consider whether the ERC20 wrapper contract requires different functionality from the ERC20 factory contract. If so, it might be beneficial to use a separate bytecode for the wrapper contract.
To verify the contract deployment, you can run the following script:
#!/bin/bash # Description: Verify that the ERC20 wrapper contract is deployed with the correct bytecode # Test: Check if there's a separate bytecode for the ERC20 wrapper contract rg -i "erc20wrapper" -A 10 -g "*.go" # Test: Verify that the ERC20WrapperSalt is different from the ERC20FactorySalt rg "ERC20WrapperSalt" -A 1 -g "*.go" rg "ERC20FactorySalt" -A 1 -g "*.go"This script will help identify if there's a separate bytecode for the ERC20 wrapper contract and confirm that different salts are used for the factory and wrapper contracts.
x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (1)
1-19: LGTM: Imports and contract declaration look good.The contract imports necessary dependencies and inherits from relevant base contracts. The inheritance structure seems appropriate for the intended functionality.
app/ibc-hooks/README.md (2)
Line range hint
5-14: LGTM: Clear introduction to EVM HooksThe updated introduction effectively explains the purpose and mechanism of EVM Hooks in the context of IBC transfers. This aligns well with the PR objectives and sets a good foundation for the technical details that follow.
Line range hint
1-198: LGTM: Comprehensive documentation of ERC20 wrapper for IBC transfersThis updated README provides a thorough and well-structured explanation of the ERC20 wrapper functionality for IBC transfers. The changes align perfectly with the PR objectives and cover all necessary aspects, including:
- Introduction to EVM Hooks
- EVM Contract Execution Format
- ICS20 packet structure
- Async Callback functionality
- IBC Transfer using ERC20Wrapper
The documentation now offers a clear guide for developers implementing or working with this feature. The minor suggestions provided in the previous comments would further enhance the clarity and completeness of the documentation.
Great job on improving the documentation to support this new feature!
🧰 Tools
🪛 LanguageTool
[uncategorized] ~164-~164: Possible missing comma found.
Context: ...ut(uint64 callback_id) external; } ``` Also when a contract make IBC transfer reque...(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~165-~165: Possibly, a comma is missing after an introductory clause, the verb inflection is not correct, or a question mark is missing. Did you mean “contract, make” or “contract makes”?
Context: ...llback_id) external; } ``` Also when a contract make IBC transfer request, it should provide...(IF_DT_NN_VBZ)
[uncategorized] ~167-~167: Loose punctuation mark.
Context: ... -memo['evm']['async_callback']['id']: the async callback id is assigned from ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~168-~168: Loose punctuation mark.
Context: ...vm']['async_callback']['contract_addr']`: The address of contract which defines t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~172-~172: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: Execute the ERC20Wrapper contract to wr...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~174-~174: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: ibc-trasfer and execute the ERC20Wrappe...(UNLIKELY_OPENING_PUNCTUATION)
x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (7)
1-3: File Header is Appropriate and CorrectThe autogenerated file header correctly indicates that this code is generated and manual changes will be lost.
6-17: Imports are Well-Organized and NecessaryAll imported packages are necessary for the contract binding and are correctly referenced.
46-61: Correct Implementation ofDeployErc20WrapperFunctionThe
DeployErc20Wrapperfunction properly handles the deployment of the contract, including error handling for ABI parsing and deployment issues.
123-156: Factory Functions are Properly DefinedThe constructor functions for creating instances of
Erc20Wrapper,Erc20WrapperCaller,Erc20WrapperTransactor, andErc20WrapperFiltererare correctly implemented with appropriate error handling.
205-327: Getter Methods Function CorrectlyThe getter methods (
OriginTokens,Owner,SupportsInterface,WrappedTokens) correctly implement data retrieval with proper handling of call options and returned values.
329-453: Transaction Methods are Accurately ImplementedThe transaction methods (
CreateERC20,IbcAck,IbcTimeout,TransferOwnership,Unwrap,Wrap) are appropriately defined to interact with the contract's payable functions.
455-759: Event Handling is Correct and ComprehensiveThe event iterators and parsers for
ERC20CreatedandOwnershipTransferredevents are correctly implemented, enabling efficient event filtering and subscription.
fix: add comment to understand feat: wrapper erc20 factory fix: unwrap and wrap the tokens feat: add erc20 wrapper when initialize keeper fix: async callback to handle failure of ibc transfer feat: add query for erc20 wrapper fix query path
djm07073
force-pushed
the
feat/wrapper-erc20
branch
from
aec06b8 to
5e5409f
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
x/evm/keeper/genesis_test.go (1)
85-105: LGTM: NewTest_Initializefunction added with minor suggestions.The new test function effectively verifies the initialization of the ERC20 wrapper by checking if the factory address is correctly set. It includes proper error handling and assertions.
Suggestions for improvement:
- Consider using a non-zero address for the caller to better simulate real-world scenarios.
- Add a test case to verify the actual content of the
Erc20Wrapperfield in the genesis state.Here's a suggestion for improvement:
func Test_Initialize(t *testing.T) { ctx, input := createTestInput(t, false, true) + + // Verify the Erc20Wrapper field in the genesis state + genState := input.EVMKeeper.ExportGenesis(ctx) + require.Equal(t, []byte{5, 6, 7, 8}, genState.Erc20Wrapper) + wrapperAddr, err := input.EVMKeeper.GetERC20WrapperAddr(ctx) require.NoError(t, err) - caller := common.HexToAddress("0x0") + caller := common.HexToAddress("0x1234567890123456789012345678901234567890") abi, err := erc20_wrapper.Erc20WrapperMetaData.GetAbi() require.NoError(t, err) viewArg, err := abi.Pack("factory") require.NoError(t, err) factoryAddrBytes, err := input.EVMKeeper.EVMStaticCall(ctx, caller, wrapperAddr, viewArg, nil) require.NoError(t, err) factoryAddr := common.BytesToAddress(factoryAddrBytes) expectedFactoryAddr, err := input.EVMKeeper.GetERC20FactoryAddr(ctx) require.NoError(t, err) require.Equal(t, expectedFactoryAddr, factoryAddr) }x/evm/contracts/util/Strings.sol (1)
35-53: LGTM with suggestion: Add comment for assembly block intoStringThe
toStringfunction is efficiently implemented using unchecked arithmetic and assembly for performance. While the implementation is correct, consider adding a comment explaining the purpose and functionality of the assembly block to improve code readability and maintainability.x/evm/keeper/genesis.go (1)
53-72: LGTM: Implementation of wrapper ERC20 factory contract deployment.The implementation for deploying and storing the wrapper ERC20 factory contract looks good. It follows the existing patterns and includes proper error handling.
A minor suggestion for improved readability:
Consider extracting the wrapper deployment logic into a separate method, similar to how
CreateERC20is used for fee denom ERC20 coins. This would make theInitializeWithDecimalsmethod more concise and easier to read.func (k Keeper) deployWrapperERC20Factory(ctx context.Context, factoryAddr []byte) error { // Move the wrapper deployment logic here }Then call it in
InitializeWithDecimals:if err := k.deployWrapperERC20Factory(ctx, factoryAddr); err != nil { return err }x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (3)
35-37: Consider adding input validation in the constructor.While the constructor correctly initializes the
factoryvariable, it's advisable to add a check to ensure the providederc20Factoryaddress is not the zero address. This can prevent potential issues if the contract is deployed with an invalid factory address.Consider adding the following check:
constructor(address erc20Factory) { + require(erc20Factory != address(0), "Invalid factory address"); factory = ERC20Factory(erc20Factory); }
124-131: Clean up callback data in_handleFailedIbcTransfer.After handling a failed IBC transfer, the corresponding entry in the
ibcCallBackmapping remains stored. This can lead to unnecessary storage consumption. Consider deleting the mapping entry after it has been processed.Apply this change:
function _handleFailedIbcTransfer(uint64 callback_id) internal { IbcCallBack memory callback = ibcCallBack[callback_id]; unwrap( callback.wrappedTokenDenom, callback.sender, callback.wrappedAmt ); + delete ibcCallBack[callback_id]; }
133-143: Validate created wrapped token address in_ensureWrappedTokenExists.The function doesn't verify if the created wrapped token address is valid. Add a check to ensure the address returned by
factory.createERC20is not the zero address.Consider adding this check:
function _ensureWrappedTokenExists(address token) internal { if (wrappedTokens[token] == address(0)) { address wrappedToken = factory.createERC20( string.concat(NAME_PREFIX, IERC20(token).name()), string.concat(SYMBOL_PREFIX, IERC20(token).symbol()), WRAPPED_DECIMAL ); + require(wrappedToken != address(0), "Failed to create wrapped token"); wrappedTokens[token] = wrappedToken; originTokens[wrappedToken] = token; } }app/ibc-hooks/README.md (4)
51-54: LGTM! Consider clarifying theAccessListexplanation.The addition of
ValueandAccessListfields to theMsgCallstruct enhances the EVM contract execution capabilities. The explanation of field sources is clear and consistent.Consider adding a brief explanation of the purpose and benefits of using the
AccessListfield, as it might not be immediately clear to all readers why this optimization exists.Also applies to: 78-81
171-173: LGTM! Consider improving formatting.The explanation for the async callback fields is clear and consistent with the previously described functionality.
Consider improving the formatting of the bullet points for better readability:
- `memo['evm']['async_callback']['id']`: The async callback id is assigned from the contract. It will be passed as an argument to `ibc_ack` and `ibc_timeout`. - `memo['evm']['async_callback']['contract_addr']`: The address of the contract which defines the callback function.🧰 Tools
🪛 LanguageTool
[uncategorized] ~171-~171: Loose punctuation mark.
Context: ... -memo['evm']['async_callback']['id']: the async callback id is assigned from ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~172-~172: Loose punctuation mark.
Context: ...vm']['async_callback']['contract_addr']`: The address of contract which defines t...(UNLIKELY_OPENING_PUNCTUATION)
174-206: LGTM! Consider expanding the explanation.The addition of the ERC20Wrapper section provides valuable context for the IBC transfer process. The JSON example is comprehensive and illustrates the structure well.
Consider expanding the explanation of the transfer process for both directions to provide more context. For example:
### IBC Transfer using ERC20Wrapper `src -> dst`: Execute the ERC20Wrapper contract to wrap the native token and initiate an IBC transfer to the destination chain. `dst -> src`: Initiate an IBC transfer from the destination chain and execute the ERC20Wrapper contract via IBC hook on the source chain to unwrap the token. These processes enable seamless token transfers between chains while maintaining the ERC20 standard compatibility on the EVM chain. - Data example:Also, consider adding a brief explanation of the
access_listfield in the JSON example, as it's a new concept introduced in this PR.🧰 Tools
🪛 LanguageTool
[uncategorized] ~176-~176: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: Execute the ERC20Wrapper contract to wr...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~178-~178: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: ibc-trasfer and execute the ERC20Wrappe...(UNLIKELY_OPENING_PUNCTUATION)
Line range hint
1-206: Great improvements! Consider adding a table of contents.The updates to this README significantly enhance its clarity and completeness. The new sections on EVM hooks and ERC20Wrapper are well-integrated and provide valuable information for developers.
To further improve the document's navigability, consider adding a table of contents at the beginning. This would help readers quickly find specific sections, especially as the document has grown in complexity. For example:
## Table of Contents 1. [Introduction](#introduction) 2. [EVM Hooks](#evm-hooks) 2.1 [EVM Contract Execution Format](#evm-contract-execution-format) 2.2 [ICS20 packet structure](#ics20-packet-structure) 2.3 [Execution flow](#execution-flow) 3. [Async Callback](#async-callback) 4. [IBC Transfer using ERC20Wrapper](#ibc-transfer-using-erc20wrapper)🧰 Tools
🪛 LanguageTool
[grammar] ~169-~169: Possibly, a comma is missing after an introductory clause, the verb inflection is not correct, or a question mark is missing. Did you mean “contract, make” or “contract makes”?
Context: ...llback_id) external; } ``` Also when a contract make IBC transfer request, it should provide...(IF_DT_NN_VBZ)
[uncategorized] ~171-~171: Loose punctuation mark.
Context: ... -memo['evm']['async_callback']['id']: the async callback id is assigned from ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~172-~172: Loose punctuation mark.
Context: ...vm']['async_callback']['contract_addr']`: The address of contract which defines t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~176-~176: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: Execute the ERC20Wrapper contract to wr...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~178-~178: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: ibc-trasfer and execute the ERC20Wrappe...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
x/evm/types/genesis.pb.gois excluded by!**/*.pb.gox/evm/types/query.pb.gois excluded by!**/*.pb.gox/evm/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (16)
- app/ibc-hooks/README.md (7 hunks)
- proto/minievm/evm/v1/genesis.proto (1 hunks)
- proto/minievm/evm/v1/query.proto (2 hunks)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (1 hunks)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (1 hunks)
- x/evm/contracts/util/Strings.go (1 hunks)
- x/evm/contracts/util/Strings.sol (1 hunks)
- x/evm/keeper/erc721_test.go (1 hunks)
- x/evm/keeper/genesis.go (5 hunks)
- x/evm/keeper/genesis_test.go (3 hunks)
- x/evm/keeper/keeper.go (3 hunks)
- x/evm/keeper/query_server.go (1 hunks)
- x/evm/keeper/query_server_test.go (1 hunks)
- x/evm/types/address.go (1 hunks)
- x/evm/types/errors.go (1 hunks)
- x/evm/types/keys.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- proto/minievm/evm/v1/genesis.proto
- proto/minievm/evm/v1/query.proto
- x/evm/keeper/erc721_test.go
- x/evm/keeper/keeper.go
- x/evm/keeper/query_server.go
- x/evm/keeper/query_server_test.go
- x/evm/types/address.go
- x/evm/types/errors.go
- x/evm/types/keys.go
🧰 Additional context used
🪛 LanguageTool
app/ibc-hooks/README.md
[uncategorized] ~171-~171: Loose punctuation mark.
Context: ... -memo['evm']['async_callback']['id']: the async callback id is assigned from ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~172-~172: Loose punctuation mark.
Context: ...vm']['async_callback']['contract_addr']`: The address of contract which defines t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~176-~176: Loose punctuation mark.
Context: ...ransfer using ERC20Wrappersrc -> dst: Execute the ERC20Wrapper contract to wr...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~178-~178: Loose punctuation mark.
Context: ...o wrap and do ibc-transferdst -> src: ibc-trasfer and execute the ERC20Wrappe...(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (19)
x/evm/keeper/genesis_test.go (2)
6-7: LGTM: New imports are relevant and necessary.The added imports for
go-ethereum/commonanderc20_wrapperare appropriate for the new test functionTest_Initialize.
21-21: LGTM: NewErc20Wrapperfield added togenState.The addition of the
Erc20Wrapperfield to thegenStateis consistent with the changes in theGenesisStatestruct. This ensures that the genesis test covers the new ERC20 wrapper functionality.x/evm/contracts/util/Strings.sol (4)
13-29: LGTM: Efficient implementation oftoHexStringThe
toHexStringfunction is well-implemented with efficient use of bitwise operations and a single loop. The use of a custom error for length validation is a good practice for gas optimization.
31-33: LGTM: Correct implementation oftoHexStringfor addressesThis overload correctly converts an address to a hexadecimal string by leveraging the main
toHexStringfunction. The use of theADDRESS_LENGTHconstant ensures proper padding.
55-87: LGTM: Efficient implementation oflog10The
log10function is implemented efficiently using a series of comparisons and divisions. The use of unchecked arithmetic is appropriate and helps with gas optimization. The algorithm correctly calculates the base-10 logarithm of the input value.
1-4:⚠️ Potential issueInclude OpenZeppelin's full MIT License and Copyright Notice
While the SPDX identifier is present and there's a reference to OpenZeppelin's implementation, the full MIT License and Copyright Notice from OpenZeppelin are missing. To comply with their license terms, please include the complete license text and copyright notice at the beginning of this file.
x/evm/keeper/genesis.go (4)
12-12: LGTM: New import for ERC20 wrapper contract.The addition of the ERC20 wrapper contract import is appropriate for the new functionality being implemented.
16-19: LGTM: Updated Initialize method comment.The comment accurately reflects the new functionality for deploying the wrapper ERC20 factory contract. However, could you please clarify the statement about incompatibility due to 18 decimals? It might be helpful to explain why this is an issue and if there are any plans to address it in the future.
91-93: LGTM: Initialization of ERC20WrapperAddr in InitGenesis.The addition of setting the ERC20WrapperAddr from the genesis state is consistent with the existing pattern and includes proper error handling.
243-246: LGTM: Inclusion of ERC20WrapperAddr in ExportGenesis.The addition of retrieving and including the wrapperAddr in the exported genesis state is consistent with the existing pattern and includes proper error handling.
Also applies to: 256-256
x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol (3)
1-19: LGTM: Contract declaration and imports look good.The contract uses a recent Solidity version (0.8.24) and inherits from appropriate base contracts. The import statements are clear and organized.
20-33: LGTM: State variables and constants are well-defined.The contract uses appropriate data structures for storing callback information and tracking wrapped/origin tokens. The use of constants for naming conventions and decimals is a good practice for consistency and maintainability.
145-160: LGTM:_convertDecimalfunction looks good.The
_convertDecimalfunction correctly handles different decimal conversions and includes a check to ensure the converted amount is not zero. No changes are necessary for this function.x/evm/contracts/util/Strings.go (1)
1-203: Auto-generated bindings are correctly implementedThe auto-generated Go bindings for the
Utilcontract appear to be correctly implemented with appropriate error handling and standard struct definitions. No issues found.x/evm/contracts/erc20_wrapper/ERC20Wrapper.go (5)
47-61: LGTM: Correct implementation ofDeployErc20WrapperfunctionThe
DeployErc20Wrapperfunction properly initializes the contract's ABI, handles errors effectively, and returns the expected values, ensuring seamless deployment of theErc20Wrappercontract.
122-129: LGTM: Accurate construction inNewErc20WrapperfunctionThe
NewErc20Wrapperfunction correctly binds the deployed contract to the Go structures, facilitating interactions with theErc20Wrappercontract.
360-379: LGTM:CreateERC20transaction methods are well-definedThe
CreateERC20methods for both the transactor and session are correctly implemented, allowing for the creation of new ERC20 tokens with the specified parameters.
381-400: LGTM: Proper handling ofIbcAcktransactionsThe
IbcAckfunctions are properly set up to handle IBC acknowledgments, ensuring correct interaction with inter-blockchain communication protocols.
465-484: LGTM: Implementation ofWrapfunctionThe
Wrapmethods correctly facilitate wrapping tokens for IBC transfers, accepting all necessary parameters and ensuring compatibility with the contract.
Feat
Summary by CodeRabbit
Release Notes
New Features
ERC20Wrappercontract for wrapping and unwrapping ERC20 tokens, enhancing interoperability with Cosmos via IBC.Valuefield in the message structure and examples for IBC transfers.Bug Fixes
Documentation
README.mdfor theapp/ibc-hooksmodule to reflect changes in EVM contract execution and new field requirements.Chores