test: refactor the ERC20 token test suite #789
Conversation
WalkthroughThe changes in this pull request involve significant modifications to the handling of ERC20 tokens within the codebase. Key updates include the deprecation of the 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (10)
testutil/helpers/contract_base_suite.go (1)
9-13: Add documentation to the struct and its fields.
Consider adding documentation comments to improve code clarity and maintainability. This is particularly important for test utilities that other developers will use.
+// ContractBaseSuite provides base functionality for contract testing suites.
type ContractBaseSuite struct {
+ // require provides test assertions
require *require.Assertions
+ // signer represents the account interacting with the contract
signer *Signer
+ // contract holds the deployed contract's address
contract common.Address
}scripts/linter.sh (1)
Line range hint 4-13: Consider documenting pattern limits rationale.
The patternLimits array would benefit from comments explaining why each pattern has its specific limit. This would help maintainers understand the reasoning behind limit adjustments during refactoring.
patternLimits=(
+ # Allow up to 20 nolint directives for necessary suppressions
"nolint:20"
+ # Security-related suppressions, keep minimal
"#nosec:5"
+ # Core cross-chain functionality references
"CrossChain:3"
+ # Prefer "CrossChain" over "cross chain" for consistency
"cross chain:0"
+ # Proxy implementation references
"GetERC1967Proxy:4"
+ # Native token wrapper references
"GetWFX:9"
+ # FIP20 token interface calls, aligned with ERC20TokenKeeper refactoring
"GetFIP20:10"
)x/erc20/types/expected_keepers.go (1)
39-42: Consider documenting the architectural changes.
The interface changes suggest a shift towards more detailed transaction handling with the addition of MsgEthereumTxResponse. Consider:
- Documenting the rationale behind these changes in the package documentation
- Providing migration guides for implementations
- Updating relevant test utilities to leverage the new response information
contract/erc20_abi.go (1)
11-14: Enhance the deprecation notice with migration guidance.
While the deprecation notice correctly indicates the replacement struct, it would be helpful to provide more detailed migration guidance for users.
Consider expanding the comment:
-// Deprecated: please use ERC20TokenKeeper
+// Deprecated: This struct will be removed in a future release.
+// Please migrate to ERC20TokenKeeper which provides enhanced token management capabilities.
+// See contract/erc20_token.go for examples and documentation.x/evm/keeper/msg_server_test.go (2)
19-43: Consider parameterizing the gas price in ethereumTx helper.
The helper method is well-structured and handles all necessary aspects of transaction creation. However, the gas price is hardcoded to 500 Gwei (line 33). Consider making it a parameter to allow more flexible testing scenarios.
-func (s *KeeperTestSuite) ethereumTx(signer *helpers.Signer, to *common.Address, data []byte, value *big.Int, gasLimit uint64) (*types.MsgEthereumTxResponse, error) {
+func (s *KeeperTestSuite) ethereumTx(signer *helpers.Signer, to *common.Address, data []byte, value *big.Int, gasLimit uint64, gasPrice *big.Int) (*types.MsgEthereumTxResponse, error) {
// ... existing code ...
- big.NewInt(500*1e9),
+ gasPrice,
// ... rest of the code ...74-74: Maintain consistency in coin creation methods.
There's an inconsistency in how amounts are handled between the two test methods:
- Line 74 uses
helpers.NewStakingCoins - Line 102 uses
sdk.NewCoins(sdk.NewCoin())
Consider standardizing on one approach for better maintainability.
Also applies to: 102-102
x/evm/keeper/contract_code_test.go (1)
103-109: Consider adding deployment state assertions.
While the initialization sequence is well structured, consider adding assertions to verify:
- The deployed contract address is not zero
- The contract code is properly deployed
- The contract initialization parameters are correctly set
s.Require().NotEqual(common.Address{}, wfxAddress, "deployed address should not be zero")
s.Require().NotEmpty(s.App.EvmKeeper.GetCode(s.Ctx, wfxAddress), "contract code should be deployed")contract/precompile.go (1)
Line range hint 69-75: Check and handle the return value of the burn function
In the Burn method, the code calls the burn function but does not check or handle any return value. If the burn function returns a value indicating success or failure (common in ERC20 functions), consider unpacking and checking the return value to ensure the operation was successful, similar to the TransferFrom method.
Apply this diff to check the return value:
func (e *ERC20Call) Burn(account common.Address, amount *big.Int) error {
data, err := e.abi.Pack("burn", account, amount)
if err != nil {
return err
}
- _, err = e.call(data)
+ ret, err := e.call(data)
if err != nil {
return fmt.Errorf("call burn: %s", err.Error())
}
+ var unpackedRet struct{ Value bool }
+ if err = e.abi.UnpackIntoInterface(&unpackedRet, "burn", ret); err != nil {
+ return fmt.Errorf("unpack burn: %s", err.Error())
+ }
+ if !unpackedRet.Value {
+ return errors.New("burn failed")
+ }
return nil
}testutil/helpers/erc20_token_suite.go (2)
39-45: Consider renaming method Error to avoid confusion
The method Error(err error) may cause confusion with the standard error interface in Go. Consider renaming it to something more descriptive, such as CheckError or HandleError, to improve clarity.
18-23: Add documentation comments to exported struct and fields
The exported struct ERC20TokenSuite lacks a documentation comment. Adding comments to exported types and their fields improves code readability and complies with Go's best practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- contract/erc20_abi.go (1 hunks)
- contract/erc20_token.go (2 hunks)
- contract/precompile.go (5 hunks)
- scripts/linter.sh (1 hunks)
- testutil/helpers/bank.go (0 hunks)
- testutil/helpers/base_suite.go (1 hunks)
- testutil/helpers/contract_base_suite.go (1 hunks)
- testutil/helpers/erc20_token_suite.go (1 hunks)
- x/crosschain/keeper/bridge_call_in_test.go (2 hunks)
- x/erc20/keeper/convert.go (2 hunks)
- x/erc20/types/expected_keepers.go (2 hunks)
- x/evm/keeper/contract_code_test.go (3 hunks)
- x/evm/keeper/keeper_test.go (2 hunks)
- x/evm/keeper/msg_server_test.go (4 hunks)
- x/evm/testutil/erc20.go (0 hunks)
- x/evm/testutil/evm.go (0 hunks)
- x/staking/testutil/staking_precompile.go (4 hunks)
💤 Files with no reviewable changes (3)
- testutil/helpers/bank.go
- x/evm/testutil/erc20.go
- x/evm/testutil/evm.go
🔇 Additional comments (30)
testutil/helpers/contract_base_suite.go (2)
1-8: LGTM: Clean package structure and imports.
The package name and imports are well-organized and appropriate for a test helper package.
1-37: Verify integration with ERC20 token test suite.
Since this is a base suite for contract testing, let's verify its integration with the ERC20 token test suite mentioned in the PR description.
✅ Verification successful
Let's verify the integration by checking the content of erc20_token_suite.go to see how it uses ContractBaseSuite.
Let's check one of the test files to see how the ERC20TokenSuite is actually used in practice.
Integration with ERC20 token testing is properly implemented
The verification shows that:
ContractBaseSuiteis correctly embedded inERC20TokenSuiteERC20TokenSuiteproperly inherits and utilizes the base suite's functionality- The test files demonstrate proper usage of the suite for:
- Deploying ERC20 tokens
- Testing token operations (mint, transfer, etc.)
- Verifying token properties (name, symbol, decimals)
- Testing contract upgrades
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of ContractBaseSuite in ERC20 token tests
# Test 1: Find files that embed or use ContractBaseSuite
echo "Checking for ContractBaseSuite usage:"
rg -l "ContractBaseSuite"
# Test 2: Look for ERC20 test files that might use this suite
echo -e "\nChecking for ERC20 test files:"
fd -g "*erc20*_test.go"
Length of output: 291
Script:
#!/bin/bash
# Check how ContractBaseSuite is used in erc20_token_suite.go
echo "Content of erc20_token_suite.go:"
cat testutil/helpers/erc20_token_suite.go
# Look for actual test files that might use ERC20TokenSuite
echo -e "\nFiles using ERC20TokenSuite:"
rg -l "ERC20TokenSuite"
Length of output: 6379
Script:
#!/bin/bash
# Check the usage of ERC20TokenSuite in test files
echo "Content of x/evm/keeper/keeper_test.go where ERC20TokenSuite is used:"
rg -A 10 "ERC20TokenSuite" "x/evm/keeper/keeper_test.go"
echo -e "\nContent of x/evm/keeper/contract_code_test.go where ERC20TokenSuite is used:"
rg -A 10 "ERC20TokenSuite" "x/evm/keeper/contract_code_test.go"
Length of output: 16475
scripts/linter.sh (1)
12-12: Verify the GetFIP20 pattern count reduction.
The reduction from 12 to 10 instances aligns with the ERC20 refactoring, but let's verify the exact count.
✅ Verification successful
Pattern count matches the updated limit
The codebase contains exactly 10 instances of GetFIP20, which perfectly matches the new limit set in scripts/linter.sh. The reduction from 12 to 10 instances has been correctly reflected in the linter configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the exact count of GetFIP20 instances
# Expect: Exactly 10 instances after the refactoring
echo "Checking GetFIP20 instances (excluding generated files):"
rg --type go --glob '!*.pb.go' --glob '!*.pulsar.go' --glob '!*.sol.go' --glob '!legacy.go' "GetFIP20" ./
Length of output: 946
x/crosschain/keeper/bridge_call_in_test.go (1)
Line range hint 13-13: Address skipped test with TODO comment.
The test is currently skipped with a TODO comment. Since this PR is refactoring the ERC20 token test suite, we should either:
- Re-enable and fix the test as part of this PR
- Create a tracking issue for re-enabling it
Let's check if there's an existing issue for this:
Would you like me to help create a new issue to track re-enabling this test?
x/evm/keeper/keeper_test.go (2)
25-26: LGTM! Clean refactor of ERC20 suite initialization.
The change from testutil to helpers package aligns with the PR's objective to refactor the ERC20 token test suite.
49-53: LGTM! Improved error handling and parameter naming.
The changes enhance code clarity through:
- Better parameter naming (
senderinstead ofsinger) - More strict error handling with
s.Require().NoError(err)
x/erc20/types/expected_keepers.go (3)
12-12: LGTM: Import addition is appropriate.
The new import is necessary to support the updated return types in the ERC20TokenKeeper interface.
39-42: Clarify the removal of burn functionality.
The Burn method has been removed from the interface. Please clarify:
- Is this functionality being moved elsewhere?
- How should existing code handle token burning operations?
- Are there any migration steps needed for existing implementations?
Let's check for existing burn operation usage:
#!/bin/bash
# Search for existing burn operations
echo "Searching for Burn method usage..."
rg -A 3 "Burn\(.*context\.Context.*common\.Address.*big\.Int\)"40-41: Verify implementations of the updated interface methods.
The return type changes from error to (*evmtypes.MsgEthereumTxResponse, error) represent a breaking change that will require updates to all implementations of the ERC20TokenKeeper interface.
Let's verify all implementations of these methods:
x/erc20/keeper/convert.go (3)
73-73: LGTM: Updated error handling pattern for Mint operation.
The change correctly handles the new return signature (*evmtypes.MsgEthereumTxResponse, error) from the ERC20TokenKeeper interface, properly discarding the unused response value.
93-93: LGTM: Updated error handling pattern for Transfer operation.
The change correctly handles the new return signature (*evmtypes.MsgEthereumTxResponse, error) from the ERC20TokenKeeper interface, properly discarding the unused response value.
73-73: 🛠️ Refactor suggestion
Consider capturing transaction responses for testing.
Since this PR is focused on refactoring the test suite, it might be valuable to capture and verify the MsgEthereumTxResponse in test scenarios to ensure proper transaction execution.
Let's check if the test files are utilizing these response values:
Consider updating the tests to verify the transaction responses. Example pattern:
response, err := keeper.Mint(ctx, ...)
require.NoError(t, err)
require.NotNil(t, response)
// Add assertions for response fieldsAlso applies to: 93-93
contract/erc20_abi.go (1)
Line range hint 20-156: Verify the impact of deprecation on dependent code.
The file contains numerous methods that depend on the abi field. We should verify that all callers are aware of the deprecation and have a migration path.
Let's analyze the usage:
x/evm/keeper/msg_server_test.go (2)
9-9: LGTM: Import changes are appropriate.
The new imports are correctly added and necessary for the updated test implementations.
Also applies to: 11-11, 16-16
Line range hint 46-143: Consider adding test cases for edge scenarios.
While the current test coverage is good, consider adding tests for:
- Zero value transfers
- Transactions that fail due to insufficient gas
- Contract calls with invalid data
- Failed token minting scenarios
This would help ensure robust handling of error cases.
testutil/helpers/base_suite.go (2)
Line range hint 3-31: LGTM! Import cleanup aligns with ERC20 refactoring.
The removal of math/big and Ethereum's common package imports is consistent with the transition away from direct ERC20 token handling in BaseSuite.
68-73: LGTM! Clean signer creation method.
The new NewSigner method provides a focused way to create and set up signers without token minting, complementing the existing AddTestSigner method. This separation of concerns aligns well with the ERC20 test suite refactoring.
Let's verify the usage pattern of both signer methods:
✅ Verification successful
Usage patterns confirm distinct purposes of signer methods
The verification results show clear differentiation in usage:
NewSigner()is used in EVM tests for basic signer setup without token allocation (e.g., inx/evm/keeper/msg_server_test.go)AddTestSigner()is used when tests need initial token balances, particularly in governance and EVM contract tests (e.g.,x/gov/keeper/msg_server_test.go,x/evm/keeper/contract_code_test.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how NewSigner and AddTestSigner are used in tests
# Expect: Different usage patterns - NewSigner for basic setup, AddTestSigner for tests needing initial balance
rg -A 5 "NewSigner\(\)" --type go
rg -A 5 "AddTestSigner\(" --type go
Length of output: 5354
x/evm/keeper/contract_code_test.go (1)
43-44: LGTM! Improved naming clarity and test suite specificity.
The changes enhance readability by:
- Using
senderinstead ofsingerfor better clarity - Using more specific
NewERC20TokenSuite()instead of genericNewERC20Suite()
Also applies to: 52-55
contract/precompile.go (4)
31-31: Initialization of the abi field in ERC20Call
The addition of the abi field of type abi.ABI to the ERC20Call struct is appropriate for managing ABI interactions within contract calls.
81-93: Correct handling of the transferFrom return value
The TransferFrom method correctly packs the function call, executes it, and unpacks the returned boolean value to check for success. This ensures that any failure in the transfer is properly detected and handled.
100-112: Proper handling of the totalSupply return value
The TotalSupply method accurately packs the function call, performs a static call, and unpacks the returned *big.Int value. This implementation effectively retrieves the total supply from the contract.
44-44: Ensure GetWFX().ABI returns the correct ABI
In the NewERC20Call function, the abi field is initialized using GetWFX().ABI. Verify that GetWFX().ABI returns the correct ABI for the ERC20 contract and handle any potential errors if it does not.
Run the following script to confirm that GetWFX().ABI is correctly initialized:
✅ Verification successful
Let me gather more information about the ABI initialization.
Let me check the ABI initialization function and metadata.
Based on the gathered information, I can now provide the final response:
GetWFX().ABI correctly returns the WFX contract ABI
The implementation shows that GetWFX() returns a Contract struct where the ABI field is properly initialized using MustABIJson(WFXUpgradableMetaData.ABI). The MustABIJson function safely parses the ABI JSON string and panics on any error, ensuring the ABI is always valid when the code runs. The WFX contract's ABI is properly defined in the generated code and includes all standard ERC20 methods plus additional functionality for upgrades and initialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that GetWFX().ABI returns the expected ABI.
# Search for the definition of GetWFX and check the ABI initialization.
rg -A 5 'func GetWFX\(\)' contract/
Length of output: 262
Script:
#!/bin/bash
# Search for wfxInit declaration and initialization
rg -A 10 'wfxInit\s*:?=' contract/
# Also search for WFX-related struct definitions
ast-grep --pattern 'type WFX struct {
$$$
}'
Length of output: 14273
Script:
#!/bin/bash
# Check the MustABIJson function and WFXUpgradableMetaData
rg -A 5 'MustABIJson' contract/
rg -A 5 'WFXUpgradableMetaData' contract/
Length of output: 65328
contract/erc20_token.go (7)
25-31: LGTM!
The Owner and Allowance methods have been implemented correctly, adhering to the ERC20 token standard. The error handling and return values are appropriate.
Also applies to: 75-80
83-91: LGTM!
The unpackRet method is a helpful utility for handling the unpacking of responses and error cases consistently across the approve, transfer, and transferFrom methods. The error messages are descriptive and use appropriate error wrapping.
107-109: LGTM!
The Mint, Burn, TransferOwnership, Withdraw, WithdrawToSelf, and Deposit methods have been implemented correctly, following the ERC20 token standard and the contract's specific requirements. The method signatures and error handling are appropriate.
Also applies to: 111-113, 131-133, 135-137, 139-141, 143-144
35-35: Verify the context handling change in the codebase.
The QueryContract calls now use common.Address{} instead of the from field for the context. Ensure that this change is consistently applied throughout the codebase and that it does not introduce any unintended side effects or security vulnerabilities.
Run the following script to verify the QueryContract usages:
#!/bin/bash
# Description: Verify the usage of `QueryContract` in the codebase.
# Test: Search for `QueryContract` calls. Expect: `common.Address{}` as the second argument.
rg -A 5 $'QueryContract'Also applies to: 43-43, 51-51, 61-61, 69-69
94-100: Verify the error handling change in the codebase.
The Approve, Transfer, and TransferFrom methods now return nil for errors and use the unpackRet method to handle the response unpacking. Ensure that this change is consistently applied and does not introduce any unintended consequences or error-masking issues.
Run the following script to verify the error handling in these methods:
#!/bin/bash
# Description: Verify the error handling in `Approve`, `Transfer`, and `TransferFrom` methods.
# Test: Search for the method implementations. Expect: Consistent error handling using `unpackRet`.
rg -A 10 $'func \(k ERC20TokenKeeper\) (Approve|Transfer|TransferFrom)'Also applies to: 115-121, 123-129
102-105:
Ensure the PackMint method is not exposed in production code.
The comment indicates that the PackMint method is only used for testing purposes. Make sure that this method is not exposed or used in any production code paths to prevent potential security risks or unintended minting of tokens.
Run the following script to verify the PackMint usage:
#!/bin/bash
# Description: Verify that `PackMint` is only used in test files.
# Test: Search for `PackMint` usage. Expect: Only occurrences in test files.
rg -A 5 $'PackMint'15-15: Verify the ABI change in the codebase.
The ERC20TokenKeeper struct now uses GetWFX().ABI instead of GetFIP20().ABI to initialize the abi field. Ensure that this change is consistent with the modifications made in the contract/erc20_abi.go file and that it does not introduce any unintended consequences or compatibility issues.
Run the following script to verify the GetWFX and GetFIP20 usages:
Also applies to: 21-21
✅ Verification successful
Based on the search results, I can now provide a final response:
The ABI change in the codebase is consistent and verified.
The codebase shows that:
ERC20TokenKeeperis consistently initialized withGetWFX().ABIinNewERC20TokenKeeperGetWFX()is used across the codebase for token contract initialization- The change from
GetFIP20()toGetWFX()appears to be an intentional migration, with both contracts still available butGetWFX()being the preferred choice for new instances
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of `GetWFX` and `GetFIP20` in the codebase.
# Test 1: Search for `GetWFX` usage. Expect: Multiple occurrences.
rg -A 5 $'GetWFX'
# Test 2: Search for `GetFIP20` usage. Expect: No occurrences (except in comments or tests).
rg -A 5 $'GetFIP20'
Length of output: 5961
Script:
#!/bin/bash
# Let's verify the ABI changes and usage patterns
# Search for GetWFX and GetFIP20 function definitions and usages
rg -A 5 "func.*GetWFX|func.*GetFIP20|GetWFX\(\)|GetFIP20\(\)"
# Search for ERC20TokenKeeper initialization to see how abi is set
rg -A 5 "ERC20TokenKeeper.*struct|NewERC20TokenKeeper"
# Look for any ABI initialization in erc20_abi.go
rg -A 5 "abi.*=.*NewABI|abi.*:.*NewABI" contract/erc20_abi.go
Length of output: 53356
x/staking/testutil/staking_precompile.go (1)
20-20:
EthereumTx method returns nil, leading to potential nil pointer dereferences
The EthereumTx method now returns nil, effectively removing its transaction execution logic. Methods like Allowance (line 31), Delegation (line 44), and DelegationRewards (line 57) call EthereumTx and then access tx.Ret. This will result in a nil pointer dereference when attempting to unpack output from tx.Ret.
Please ensure that EthereumTx returns a valid *evmtypes.MsgEthereumTxResponse, or update the dependent methods to handle a nil return value appropriately to prevent runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
testutil/helpers/erc20_token_suite.go (1)
39-45: Ensure consistent error handling in the Error method.
The Error method currently checks if s.err is not nil and asserts the error accordingly. However, consider handling unexpected errors that do not match s.err to improve debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- testutil/helpers/erc20_token_suite.go (1 hunks)
🔇 Additional comments (3)
testutil/helpers/erc20_token_suite.go (3)
33-36: Good job fixing the variable naming.
The variable newErc20Suite is correctly named, improving code readability.
47-56: Validate initialization arguments in DeployERC20Token.
Ensure that the initializeArgs array correctly matches the expected constructor parameters of the ERC20 contract. Any mismatch may lead to deployment failures.
154-171: Explicitly handle errors in the OnTest method.
In the OnTest method, the calls to functions like Name, Symbol, and Decimals rely on error handling within those functions. If an error occurs, the test assertions may not properly reflect the failure. Consider explicitly checking for errors or ensuring that the assertions fail when an error has occurred to make debugging easier.
Summary by CodeRabbit
New Features
ERC20TokenKeeperfor enhanced ERC20 token operations, including minting, burning, and transferring tokens.ContractBaseSuiteandERC20TokenSuite.Bug Fixes
Documentation
ERC20ABIstruct, recommending users transition toERC20TokenKeeper.Chores