feat: pre-compilation get ERC20 token #949
Conversation
WalkthroughThis pull request introduces a comprehensive enhancement to the crosschain ERC20 token functionality across multiple files. The changes primarily focus on adding a new method Changes
Sequence DiagramsequenceDiagram
participant Client
participant CrosschainContract
participant Keeper
participant Erc20Keeper
Client->>CrosschainContract: getERC20Token(denom)
CrosschainContract->>Keeper: GetERC20Token(context, args)
Keeper->>Erc20Keeper: GetERC20Token(baseDenom)
Erc20Keeper-->>Keeper: Return Token Details
Keeper-->>CrosschainContract: Return Token Address & Enable Status
CrosschainContract-->>Client: Token Information
Possibly related PRs
Poem
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
solidity/contracts/interfaces/ICrosschain.sol (1)
31-34: Consider documenting the_enablereturn value.
Though the newgetERC20Tokenfunction extends the interface effectively, it might be helpful to include comments or NatSpec annotations clarifying what_enablesignifies (e.g., whether the token is active, tradable, etc.).precompiles/crosschain/keeper.go (1)
19-21: Add clarity on usage oferc20Keeper.
Introducingerc20KeeperinKeeperis beneficial for integrating ERC20 logic. Consider adding a docstring or comment explaining the purpose of this field and how interactions differ from the existingbankKeeper.precompiles/crosschain/contract.go (2)
28-28: Document theerc20Keeperparameter.
When extendingNewPrecompiledContractto accepterc20Keeper, consider updating the function’s docstring or inline comment to clarify how this keeper is used for ERC20 integration.
32-34: Maintain order or alignment for consistent readability.
TheKeeperfields are arranged in a logical order. While not critical, you might keep all keeper references (e.g.,bankKeeperand thenerc20Keeper) grouped together consistently throughout the codebase.precompiles/crosschain/get_erc20_token_test.go (3)
16-20: Rename the test function.
The function nameTestCrosschainGetERC20Tokwnseems to contain a minor spelling mistake in the word "Tokwn." Consider renaming it toTestCrosschainGetERC20Tokenfor clarity and consistency.-func TestCrosschainGetERC20Tokwn(t *testing.T) { +func TestCrosschainGetERC20Token(t *testing.T) {
28-44: Add a test scenario for Enabled = true.
The current test covers settingEnabledto false. To fully validate functionality, consider adding a test that setsEnabledto true and confirms its behavior.
56-67: Consider asserting error messages for negative scenarios.
While the test confirms whether an error occurred, you might also want to assert the exact error message or error code to ensure correctness and maintainability.testutil/helpers/crosschain_precompile_suite.go (1)
61-65: Return the error for flexible handling.
This method enforces error handling internally viarequireError. For scenarios that need nuanced error handling, consider returning the error to the caller to allow finer control over test assertions.-func (s CrosschainPrecompileSuite) GetERC20Token(ctx context.Context, args contract.GetERC20TokenArgs) (common.Address, bool) { +func (s CrosschainPrecompileSuite) GetERC20Token(ctx context.Context, args contract.GetERC20TokenArgs) (common.Address, bool, error) { token, enable, err := s.CrosschainPrecompileKeeper.GetERC20Token(ctx, args) s.requireError(err) return token, enable }solidity/contracts/test/CrosschainTest.sol (1)
124-128: Add NatSpec documentation to enhance maintainabilityWhile this function is straightforward, adding NatSpec comments can help other developers understand its purpose and expected behavior.
function getERC20Token( bytes32 _denom ) external view returns (address _token, bool _enable) { + /** + * @notice Retrieves the ERC20 token address and whether it's enabled for the specified denom. + * @dev Calls into the crosschain precompile at CROSS_CHAIN_ADDRESS + * @param _denom Bytes32 representation of the token denomination + * @return _token Address of the ERC20 token + * @return _enable Boolean indicating whether the token is enabled + */ return ICrosschain(CROSS_CHAIN_ADDRESS).getERC20Token(_denom); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/keepers/keepers.go(1 hunks)contract/crosschain.go(1 hunks)contract/crosschain_precompile.go(1 hunks)contract/icrosschain.sol.go(2 hunks)precompiles/crosschain/contract.go(1 hunks)precompiles/crosschain/get_erc20_token.go(1 hunks)precompiles/crosschain/get_erc20_token_test.go(1 hunks)precompiles/crosschain/keeper.go(1 hunks)precompiles/types/expected_keepers.go(1 hunks)solidity/contracts/interfaces/ICrosschain.sol(1 hunks)solidity/contracts/test/CrosschainTest.sol(1 hunks)tests/contract/crosschain_test.sol.go(2 hunks)testutil/helpers/crosschain_precompile_suite.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (13)
precompiles/crosschain/get_erc20_token.go (1)
1-76: Thoroughly review input validation and potential error handling paths.
- The method
Run(lines 36-48) gracefully returns an error ifUnpackInputfails orGetERC20Tokenfails, which is good. Consider adding (or clarifying) logs or error details where beneficial, especially for debugging potential input data issues.- The gas requirement is fixed at 1,000 (lines 28-30). Ensure this static cost is sufficient for typical usage and that it remains stable over time.
PackOutput(lines 74-76) returns the packed data directly. Consider including checks or logs if the token address is zero or if any unexpected state arises.precompiles/crosschain/contract.go (1)
42-42: Great addition ofNewGetERC20TokenMethod(keeper).
Adding this method to the contract expands functionality effectively. Validate test coverage to confirm all success and failure scenarios are handled.precompiles/crosschain/get_erc20_token_test.go (2)
22-27: Good structure for test cases array.
Defining multiple scenarios using a slice of structures is a clean, scalable way to handle test variants.
45-53: Well-handled error scenario.
This test case properly verifies the error path when the token is not found.contract/crosschain.go (1)
128-133: Validation approach looks good.
TheValidatemethod correctly checks for an emptyDenom. This aligns with the rest of the argument-validation pattern.precompiles/types/expected_keepers.go (1)
95-95: Ensure interface naming consistency and clarityThis new interface method aligns well with existing patterns, but verify that other code references consistently use
GetERC20Tokenand handle the returned error.app/keepers/keepers.go (1)
373-373: Verify the non-nil initialization of Erc20KeeperYou're passing
appKeepers.Erc20Keeperto the precompiled contract. Ensure it is properly initialized and never nil to avoid runtime panics.✅ Verification successful
Erc20Keeper initialization verified as safe and properly handled
The Erc20Keeper is properly initialized during app setup with all required dependencies and used safely throughout the codebase with proper error handling. The initialization happens before its usage in precompiled contracts and is well-tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify all references to appKeepers.Erc20Keeper are guarded or properly initialized rg --context 5 'Erc20Keeper'Length of output: 59055
tests/contract/crosschain_test.sol.go (2)
34-35: Auto-generated ABI and bytecode fieldsThese lines reflect auto-generated contract metadata. No issues identified.
267-311: Confirm correct usage of ABIGen-style bindingsThis auto-generated getter for
getERC20Tokenappears valid and consistent with the contract. Make sure to cover this in your tests to validate the correct data is retrieved.contract/icrosschain.sol.go (4)
34-34: Ensure consistency in ABI versioning.Double-check that the updated ABI is in sync with any relevant contract updates. If the Solidity contract changes again, remember to regenerate these bindings to maintain accuracy.
214-237: Validate error handling for_ICrosschain.contract.Call.While this code appears auto-generated and conventional for Go-Ethereum bindings, ensure there's adequate error handling and logging around contract calls in higher-level logic. If an error is returned here, confirm upstream callers handle it gracefully (e.g., logging, fallback flows).
239-248: Confirm usage of session-based calls.This session-based
GetERC20Tokenmethod leverages preset call options (e.g.,Context,BlockNumber). Verify that these session parameters are correctly set before usage to avoid unexpected behaviors in production.
249-258: Check consistency in caller session logic.As with the other session-based function, ensure that any user-specified
CallOptsfor the session remain consistent across your integration’s environment. This includes confirming that proper addresses, contexts, or override blocks are set in tests and production.
Summary by CodeRabbit
New Features
Improvements