refactor(fungible): ZRC20 Lock/Unlock API should not depend on injected ABI #3068
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 WalkthroughWalkthroughThe pull request primarily removes references to the ZRC20 ABI across several files in the codebase. Key changes include the elimination of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
fbac
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito and
swift1337
as code owners
fbac
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3068 +/- ##
===========================================
- Coverage 64.33% 64.29% -0.04%
===========================================
Files 419 419
Lines 29102 29076 -26
===========================================
- Hits 18723 18695 -28
Misses 9597 9597
- Partials 782 784 +2
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
precompiles/staking/method_distribute.go (2)
Line range hint 62-82: Consider implementing transaction reversal for failed operations.
After locking the ZRC20 tokens, if either the MintCoins or SendCoinsFromModuleToModule operations fail, the locked tokens could potentially become stuck. Consider implementing a reversal mechanism to unlock the tokens in case of subsequent failures.
Consider wrapping the operations in a transaction-like pattern:
// Pseudo-code for the suggested approach
if err := c.fungibleKeeper.LockZRC20(...); err != nil {
return nil, err
}
// Track the locked state
locked := true
defer func() {
// Unlock tokens if any subsequent operation fails
if locked && err != nil {
if unlockErr := c.fungibleKeeper.UnlockZRC20(...); unlockErr != nil {
// Log the unlock error but return the original error
ctx.Logger().Error("failed to unlock tokens after operation failure", "error", unlockErr)
}
}
}()
if err := c.bankKeeper.MintCoins(...); err != nil {
return nil, err
}
if err := c.bankKeeper.SendCoinsFromModuleToModule(...); err != nil {
return nil, err
}
// Operation completed successfully
locked = falsePotential ABI dependency still exists in the fungible keeper
The verification reveals that while the precompile's LockZRC20 call has removed the ABI parameter, the underlying implementation in x/fungible/keeper still relies on ZRC20 ABI:
x/fungible/keeper/system_contract.goshows multiple instances of ZRC20 ABI usage inCallEVMx/fungible/keeper/zrc20_cosmos_coin_mapping_test.goinitializes ZRC20 ABI for testing- The actual
LockZRC20implementation inzrc20_cosmos_coins_mapping.gomay still depend on ABI through its internal calls
This suggests that the PR's objective to remove ZRC20 ABI dependency is incomplete, as the dependency has merely shifted to the keeper layer rather than being fully eliminated.
🔗 Analysis chain
Line range hint 55-60: LGTM! Verify ZRC20 operations without ABI dependency.
The removal of the ZRC20 ABI parameter aligns with the PR objectives to make the API self-contained. The change maintains proper error handling and parameter ordering.
Let's verify that the LockZRC20 function can properly handle ZRC20 operations without the ABI parameter:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify LockZRC20 implementation and its dependencies
# Check if LockZRC20 has internal ABI handling
ast-grep --pattern 'func ($receiver) LockZRC20($ctx, $zrc20Addr, $spender, $owner, $locker, $amount) $ret {
$$$
}'
# Look for any ZRC20 ABI references in the fungible keeper
rg -l "zrc20.*abi"
Length of output: 729
Script:
#!/bin/bash
# Let's examine the LockZRC20 implementation in the fungible keeper
rg -A 10 "func.*LockZRC20" x/fungible/keeper/
# Check for any ZRC20 ABI initialization or usage in the fungible keeper
rg -A 5 "zrc20.*abi" x/fungible/keeper/system_contract.go
# Look for any test cases that verify ZRC20 operations without ABI
rg -A 10 "TestLockZRC20" x/fungible/keeper/
Length of output: 2353
precompiles/bank/method_deposit.go (1)
Line range hint 1-121: Consider adding input validation for zrc20Addr
While reviewing the changes, I noticed that the function could benefit from additional validation to ensure the ZRC20 address is valid and the contract exists.
Consider adding validation before the balance check:
if err != nil {
return nil, err
}
+
+ // Verify the ZRC20 contract exists
+ if !c.fungibleKeeper.IsZRC20(ctx, zrc20Addr) {
+ return nil, &precompiletypes.ErrInvalidAddr{
+ Got: zrc20Addr.String(),
+ }
+ }
// Check for enough balance.x/fungible/keeper/zrc20_methods.go (2)
31-32: Standardize error handling across methods.
Error handling for ABI retrieval is inconsistent across methods. Some errors are returned directly while others are wrapped with context.
Consider consistently wrapping errors with context:
- if err != nil {
- return nil, err
- }
+ if err != nil {
+ return nil, errors.Wrap(err, "failed to get ZRC20 ABI")
+ }Also applies to: 196-197, 256-257
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-32: x/fungible/keeper/zrc20_methods.go#L32
Added line #L32 was not covered by tests
Line range hint 1-300: Overall implementation aligns well with PR objectives.
The removal of external ABI dependency makes the API more robust and self-contained. The implementation successfully eliminates the risk of undefined behavior from incorrect ABI injection.
Suggestions for future improvements:
- Consider caching the ABI to avoid repeated retrieval
- Add validation for contract bytecode to ensure it's a valid ZRC20 contract
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 197-197: x/fungible/keeper/zrc20_methods.go#L197
Added line #L197 was not covered by tests
x/fungible/keeper/zrc20_cosmos_coin_mapping_test.go (1)
Line range hint 220-262: Consider using test table pattern for allowance checks
The allowance test cases follow a similar pattern and could benefit from using a table-driven test approach to make the test cases more maintainable and easier to extend.
func Test_CheckZRC20Allowance(t *testing.T) {
ts := setupChain(t)
tests := []struct {
name string
amount *big.Int
preApprove bool
expectError bool
errorContains string
}{
{
name: "zero amount",
amount: big.NewInt(0),
preApprove: false,
expectError: true,
errorContains: "invalid amount",
},
// Add more test cases...
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if tt.preApprove {
approveAllowance(t, ts, zrc20ABI, owner, spender, allowanceTotal)
}
err := ts.fungibleKeeper.CheckZRC20Allowance(ts.ctx, owner, spender, ts.zrc20Address, tt.amount)
if tt.expectError {
require.Error(t, err)
require.Contains(t, err.Error(), tt.errorContains)
} else {
require.NoError(t, err)
}
})
}
}📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- precompiles/bank/bank.go (0 hunks)
- precompiles/bank/method_deposit.go (2 hunks)
- precompiles/bank/method_withdraw.go (2 hunks)
- precompiles/staking/method_distribute.go (1 hunks)
- precompiles/staking/staking.go (0 hunks)
- x/fungible/keeper/zrc20_cosmos_coin_mapping_test.go (7 hunks)
- x/fungible/keeper/zrc20_cosmos_coins_mapping.go (5 hunks)
- x/fungible/keeper/zrc20_methods.go (6 hunks)
- x/fungible/keeper/zrc20_methods_test.go (2 hunks)
💤 Files with no reviewable changes (2)
- precompiles/bank/bank.go
- precompiles/staking/staking.go
🧰 Additional context used
📓 Path-based instructions (7)
precompiles/bank/method_deposit.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/bank/method_withdraw.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
precompiles/staking/method_distribute.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/keeper/zrc20_cosmos_coin_mapping_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/keeper/zrc20_cosmos_coins_mapping.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/keeper/zrc20_methods.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/keeper/zrc20_methods_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🪛 GitHub Check: codecov/patch
x/fungible/keeper/zrc20_methods.go
[warning] 32-32: x/fungible/keeper/zrc20_methods.go#L32
Added line #L32 was not covered by tests
[warning] 89-89: x/fungible/keeper/zrc20_methods.go#L89
Added line #L89 was not covered by tests
[warning] 145-145: x/fungible/keeper/zrc20_methods.go#L145
Added line #L145 was not covered by tests
[warning] 197-197: x/fungible/keeper/zrc20_methods.go#L197
Added line #L197 was not covered by tests
[warning] 257-257: x/fungible/keeper/zrc20_methods.go#L257
Added line #L257 was not covered by tests
🔇 Additional comments (15)
precompiles/bank/method_withdraw.go (3)
Line range hint 88-92: LGTM: Simplified CheckZRC20Balance call aligns with self-contained API goal.
The removal of the ZRC20 ABI parameter from CheckZRC20Balance maintains the necessary balance validation while reducing external dependencies. This change aligns well with making the API more deterministic and self-contained.
Line range hint 111-116: LGTM: UnlockZRC20 signature simplified while preserving functionality.
The UnlockZRC20 call has been updated to remove the ABI dependency while maintaining the core unlock functionality. This change contributes to a more robust and self-contained API design.
Line range hint 1-146: Consider enhancing error recovery mechanisms.
While the changes successfully remove ABI dependencies, consider implementing additional safeguards for the withdrawal process:
- Consider implementing a transaction rollback mechanism in case the
UnlockZRC20operation fails after burning coins - Add monitoring/metrics for failed withdrawals to detect potential issues early
Let's verify the error handling patterns in similar operations:
precompiles/bank/method_deposit.go (2)
64-71: LGTM: Improved balance check implementation
The change from using CallContract to direct ZRC20BalanceOf call is a good improvement that:
- Removes dependency on external ABI
- Simplifies error handling by avoiding type assertions
- Makes the code more maintainable and less prone to errors
Line range hint 89-94: LGTM: Simplified LockZRC20 call signature
The removal of the zrc20ABI parameter from LockZRC20 call aligns well with the PR objectives to make the API self-contained. The error handling remains robust with appropriate context in the error message.
Let's verify that all calls to LockZRC20 across the codebase have been updated to remove the ABI parameter:
✅ Verification successful
The previous search might have been too specific. Let's broaden it to check all LockZRC20 calls and its definition to ensure consistent parameter usage across the codebase.
All LockZRC20 calls verified - API signature is consistent
Based on the search results, all calls to LockZRC20 across the codebase follow the same parameter pattern without any ABI parameter. The function signature in x/fungible/keeper/zrc20_cosmos_coins_mapping.go and all its usages in tests and precompiles consistently use:
- Context
- ZRC20 address
- Spender address
- Owner address
- Locker address
- Amount
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining calls to LockZRC20 that might still be passing ABI
rg -A 2 "LockZRC20.*zrc20ABI"
Length of output: 31
Script:
#!/bin/bash
# Search for all LockZRC20 function calls to verify the parameter pattern
rg -A 3 "LockZRC20\("
# Also search for the function definition to understand its current signature
ast-grep --pattern 'func $_ LockZRC20($$$) $_'
Length of output: 4889
x/fungible/keeper/zrc20_cosmos_coins_mapping.go (3)
72-72: LGTM: Security checks maintained after ABI removal.
The critical security checks for balance verification and transfer confirmation are properly maintained after the ABI parameter removal. The function continues to ensure:
- Sufficient balance check before unlocking
- Transfer success verification
Also applies to: 77-77
108-108: LGTM: Comprehensive validation maintained.
The CheckZRC20 functions maintain robust validation after the ABI parameter removal:
- Input validation (amounts, addresses)
- Contract validation
- Detailed error messages
Also applies to: 141-141
27-27: Verify all callers have been updated after ABI parameter removal.
The removal of the zrc20ABI parameter is a breaking change. While it simplifies the API, we need to ensure all callers have been updated.
Let's verify all callers have been updated:
Also applies to: 33-33, 40-40, 51-51
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to understand the context better and verify the parameter changes across the codebase.
Based on the git history and code changes shown, I can now generate the final response:
All callers have been properly updated after zrc20ABI parameter removal
The codebase has been systematically refactored to remove the zrc20ABI parameter from all methods. The ABI is now instantiated within each method that needs it using zrc20.ZRC20MetaData.GetAbi(). All callers have been updated to reflect this change, including:
- Bank contract
- Staking contract
- All test files
- All keeper methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining calls with the old signature
rg -l "LockZRC20.*zrc20ABI.*spender.*owner.*locker.*amount"
Length of output: 61
Script:
#!/bin/bash
# Let's search for the CheckZRC20Allowance function and its callers
ast-grep --pattern 'CheckZRC20Allowance($$$)'
# Also search for any references to this function
rg -A 3 "CheckZRC20Allowance"
# Let's also check the git history for this change
git log -p --all | rg -A 5 -B 5 "zrc20ABI"
Length of output: 72791
x/fungible/keeper/zrc20_methods.go (3)
10-10: LGTM: Import of protocol contracts package.
The addition of the zrc20 contract package import aligns with the PR objective of making the API self-contained.
87-89: Enhance test coverage for error scenarios.
The error handling for ABI retrieval needs test coverage. Consider creating a comprehensive test suite that covers all error paths.
#!/bin/bash
# Check current test coverage for ZRC20 methods
rg -l "TestZRC20.*_Error" --type goConsider creating a table-driven test that covers multiple error scenarios:
- ABI retrieval failure
- Invalid ZRC20 address
- Zero address
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 89-89: x/fungible/keeper/zrc20_methods.go#L89
Added line #L89 was not covered by tests
30-32: Add test coverage for ABI retrieval error handling.
While the implementation is correct, the error handling path for ABI retrieval is not covered by tests. This is critical as it's now part of the public API contract.
Consider adding a test case that simulates ABI retrieval failure:
func TestZRC20Allowance_AbiError(t *testing.T) {
// Setup test environment
k, ctx := setupKeeper(t)
// Mock ABI retrieval error
// Test that the error is properly propagated
_, err := k.ZRC20Allowance(ctx, common.Address{}, common.Address{}, common.Address{})
require.Error(t, err)
}🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-32: x/fungible/keeper/zrc20_methods.go#L32
Added line #L32 was not covered by tests
x/fungible/keeper/zrc20_methods_test.go (3)
68-68: LGTM! Method signature simplification aligns with PR objectives.
The simplified ZRC20BalanceOf method calls correctly remove the ABI dependency while maintaining proper error validation.
Also applies to: 74-74
94-94: LGTM! Method signature simplification with maintained validation.
The simplified ZRC20TotalSupply method calls correctly remove the ABI dependency while preserving the validation logic and expected value checks.
Also applies to: 100-100
Line range hint 164-167: Remove unused ZRC20 ABI instantiation.
According to the PR objectives, the ZRC20 Lock/Unlock API should not depend on injected ABI. The ABI instantiation here appears to be unused except in the helper function approveAllowance. Consider:
- Removing this ABI instantiation
- Updating the
approveAllowancehelper function to align with the new API design
Additionally, the final test case's error assertion suggests it's expecting a failure, but the test description indicates "should success". Please verify if this is intentional or needs correction.
x/fungible/keeper/zrc20_cosmos_coin_mapping_test.go (1)
158-195: Test cases for UnlockZRC20 look good
The test cases comprehensively cover error scenarios and successful operations, with appropriate assertions and error messages.
Description
Closes #3026
Currently the ZRC20 Lock/Unlock API depends on the upstream passing directly the ABI.
This represents a problem because:
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
zrc20ABIparameter.Tests