feat: allow erc20 metadata onetime update #104
Conversation
|
Warning Rate limit exceeded@beer-1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request introduces several modifications across multiple files in the Ethereum smart contract ecosystem. Key changes include updates to metadata handling in ERC20 contracts, the addition of authority checks, and enhancements to the querying mechanisms for token metadata. Specific files reflect alterations to method signatures, the introduction of new functions, and changes to bytecode and ABI definitions. The overall structure and functionality of the contracts remain consistent, ensuring that existing operations are preserved while new features are integrated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ERC20Contract
participant CosmosPrecompile
User->>ERC20Contract: DeployErc20(name, symbol, decimals, metadataSealed)
ERC20Contract->>User: Return contract address
User->>ERC20Contract: UpdateMetadata(name, symbol, decimals)
ERC20Contract->>User: Confirm metadata updated
User->>CosmosPrecompile: CheckAuthority(address)
CosmosPrecompile->>User: Return authority status
Possibly related PRs
Suggested reviewers
Poem
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 #104 +/- ##
==========================================
- Coverage 28.31% 28.07% -0.25%
==========================================
Files 126 126
Lines 14049 14234 +185
==========================================
+ Hits 3978 3996 +18
- Misses 9508 9671 +163
- Partials 563 567 +4
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (14)
x/evm/contracts/erc20_factory/ERC20Factory.sol (1)
15-15: LGTM! Consider improving clarity and maintainability.The implementation correctly enforces metadata sealing based on the token creator's address, aligning with the PR objectives. However, there are a few suggestions to enhance the code:
Consider these improvements:
+ // Address constant for IBC/opbridge tokens + address constant CHAIN_BRIDGE_ADDRESS = address(0x1); function createERC20( string memory name, string memory symbol, uint8 decimals ) external returns (address) { - ERC20 erc20 = new ERC20(name, symbol, decimals, msg.sender != CHAIN_ADDRESS); + // Only tokens created by the bridge address have updateable metadata + bool sealMetadata = msg.sender != CHAIN_BRIDGE_ADDRESS; + ERC20 erc20 = new ERC20(name, symbol, decimals, sealMetadata);The suggested changes:
- Define a clear constant for the bridge address
- Extract the boolean expression to a named variable
- Add comments explaining the metadata sealing behavior
x/evm/contracts/erc20_acl/ERC20ACL.sol (1)
18-25: Consider adding natspec documentation for the modifier.While the implementation is correct, adding natspec documentation would improve code clarity and maintainability.
+ /// @notice Restricts function access to authorized addresses only + /// @dev Reverts if the sender is not recognized as an authority by COSMOS_CONTRACT modifier onlyAuthority() { require( COSMOS_CONTRACT.is_authority_address(msg.sender), "ERC20: caller is not the authority" ); _; }x/evm/precompiles/cosmos/types.go (1)
Line range hint
19-80: Consider additional security measures for authority checksGiven that this code is part of a system allowing one-time updates of ERC20 metadata, consider implementing:
- Events for authority checks to maintain an audit trail
- Rate limiting to prevent potential DoS attacks through repeated authority checks
- Clear documentation of the authority address management process
x/evm/contracts/i_cosmos/ICosmos.sol (1)
21-24: Consider adding natspec documentation.While the inline comment is helpful, consider adding formal natspec documentation to maintain consistency with Solidity best practices and improve developer experience.
+ /// @notice Checks if the given address has authority privileges + /// @param account The address to check for authority status + /// @return authority True if the address has authority privileges, false otherwise function is_authority_address( address account ) external view returns (bool authority);x/evm/contracts/erc20_acl/ERC20ACL.go (2)
Line range hint
27-27: Empty ABI suggests missing contract interface.The ABI is empty (
"[]"), which indicates that either:
- The contract has no public interface
- The ABI generation failed
- The contract is meant to be abstract
This seems inconsistent with a contract that should handle metadata updates.
Please verify that:
- The Solidity contract has the necessary public functions
- The ABI generation process is working correctly
- The contract is properly compiled with all interfaces
Line range hint
1-3: Consider improving the contract generation process.While this is an auto-generated file, consider:
- Adding a comment block describing the contract's purpose and features
- Including version information for traceability
- Adding references to the original Solidity source file
This would improve maintainability and make it easier to verify contract changes.
x/evm/precompiles/cosmos/contract.go (1)
186-207: LGTM: Authority check implementation with proper security measuresThe implementation includes:
- Appropriate gas metering
- Proper input validation
- Secure address comparison
- Consistent error handling
Consider adding a constant for the gas cost value to maintain consistency with other gas constants in the file.
Add the following constant at the file level:
+const IS_AUTHORITY_ADDRESS_GAS = uint64(1000) // Adjust value based on operation costx/evm/keeper/erc20.go (2)
Line range hint
164-179: Consider impact of metadata updates on denomination handlingThe special handling for 'u' and 'm' prefixes might become inconsistent with the one-time metadata update feature mentioned in the PR objectives. When metadata is updated for IBC/opbridge tokens, these prefix-based assumptions might no longer hold true.
Consider:
- Adding a check for IBC/opbridge tokens (created from address 0x1) before applying prefix-based rules
- Documenting how metadata updates should handle these special prefixes
- Adding validation to ensure metadata updates maintain consistent denomination structures
Line range hint
116-179: LGTM: Well-integrated metadata handling changesThe changes to metadata handling are well-integrated with the existing ERC20 keeper implementation. They maintain consistency with the codebase's patterns while adding the required flexibility for token metadata updates.
Consider adding test cases to verify:
- Metadata handling for tokens with matching denom and symbol
- Edge cases in denomination unit handling
- Interaction with the one-time metadata update feature
x/evm/keeper/erc20_test.go (2)
20-24: Consider using distinct values for name and symbol parameters.The function uses the same
symbolparameter for both the name and symbol arguments in thecreateERC20call. This might not be the best practice as name and symbol typically serve different purposes in ERC20 tokens (name is typically longer and more descriptive).-func deployERC20(t *testing.T, ctx sdk.Context, input TestKeepers, caller common.Address, symbol string) common.Address { +func deployERC20(t *testing.T, ctx sdk.Context, input TestKeepers, caller common.Address, name string, symbol string) common.Address { abi, err := erc20_factory.Erc20FactoryMetaData.GetAbi() require.NoError(t, err) - inputBz, err := abi.Pack("createERC20", symbol, symbol, uint8(6)) + inputBz, err := abi.Pack("createERC20", name, symbol, uint8(6))
521-560: Consider adding more test cases for comprehensive coverage.While the test covers the main scenarios, consider adding these additional test cases for better coverage:
- Attempt to update metadata from a non-authority address
- Validate initial metadata state before update
- Test with invalid metadata values (empty strings, zero decimals)
Example additional test case:
// Test update from non-authority address should fail _, _, nonAuthorityAddr := keyPubAddr() nonAuthorityEVMAddr := common.BytesToAddress(nonAuthorityAddr.Bytes()) err = updateMetadataERC20(t, ctx, input, nonAuthorityEVMAddr, fooDenom, "new name", "new symbol", 18) require.Error(t, err) // Validate initial metadata initialMetadata, err := erc20Keeper.GetMetadata(ctx, fooDenom) require.NoError(t, err) require.Equal(t, "foo", initialMetadata.Name)x/evm/precompiles/cosmos/contract_test.go (1)
69-69: Verify consistent authority address initialization.The authority address parameter is consistently initialized across all test functions using the governance module name. However, it would be beneficial to extract this common initialization to a constant or helper function to maintain DRY principles and ensure consistency.
Consider refactoring the repeated initialization into a helper function:
+func getAuthorityAddr() string { + return authtypes.NewModuleAddress(govtypes.ModuleName).String() +} func Test_CosmosPrecompile_IsBlockedAddress(t *testing.T) { ctx, cdc, ac, ak, bk := setup() - authorityAddr := authtypes.NewModuleAddress(govtypes.ModuleName).String() + authorityAddr := getAuthorityAddr()Also applies to: 114-114, 159-159, 201-201, 232-232, 263-263, 337-337, 444-444, 490-490, 531-531
x/evm/contracts/i_cosmos/ICosmos.go (1)
Line range hint
1-1: Note: This is an auto-generated file.This file is auto-generated and should not be manually edited. Any changes should be made to the contract source and then regenerated using the appropriate tools.
x/evm/contracts/counter/Counter.go (1)
Line range hint
278-338: Verify cross-chain execution safety measures.The
execute_cosmosand related methods involve cross-chain interactions. Ensure proper validation and error handling for these cross-chain calls to prevent potential security vulnerabilities.Consider implementing:
- Input validation for cross-chain messages
- Rate limiting for cross-chain calls
- Fallback mechanisms for failed cross-chain operations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
x/bank/keeper/grpc_query_test.go(1 hunks)x/evm/contracts/counter/Counter.go(1 hunks)x/evm/contracts/erc20/ERC20.go(5 hunks)x/evm/contracts/erc20/ERC20.sol(3 hunks)x/evm/contracts/erc20_acl/ERC20ACL.go(1 hunks)x/evm/contracts/erc20_acl/ERC20ACL.sol(1 hunks)x/evm/contracts/erc20_factory/ERC20Factory.go(1 hunks)x/evm/contracts/erc20_factory/ERC20Factory.sol(1 hunks)x/evm/contracts/erc20_wrapper/ERC20Wrapper.go(1 hunks)x/evm/contracts/i_cosmos/ICosmos.go(2 hunks)x/evm/contracts/i_cosmos/ICosmos.sol(1 hunks)x/evm/contracts/initia_erc20/InitiaERC20.go(1 hunks)x/evm/keeper/erc20.go(1 hunks)x/evm/keeper/erc20_test.go(3 hunks)x/evm/keeper/precompiles.go(1 hunks)x/evm/precompiles/cosmos/contract.go(4 hunks)x/evm/precompiles/cosmos/contract_test.go(13 hunks)x/evm/precompiles/cosmos/types.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.go
🔇 Additional comments (37)
x/evm/contracts/erc20_factory/ERC20Factory.sol (1)
15-15: Verify integration with ERC20 contract changes.
Let's verify that the ERC20 contract properly implements the metadata sealing functionality.
x/evm/contracts/erc20_acl/ERC20ACL.sol (1)
18-25: LGTM! The new authority modifier implementation looks secure.
The onlyAuthority modifier correctly implements the authority check using COSMOS_CONTRACT.is_authority_address and follows the established pattern of other modifiers in the contract.
Let's verify the interface compatibility:
✅ Verification successful
The onlyAuthority modifier implementation is verified and secure
The verification confirms:
is_authority_addressis properly defined in theICosmosinterface (x/evm/contracts/i_cosmos/ICosmos.sol)- The function is correctly implemented in the precompiled contract (
x/evm/precompiles/cosmos) - The ABI definition in
ICosmos.goshows the correct function signature:function is_authority_address(address account) view returns(bool authority) - The usage in
ERC20ACL.solmatches the interface specification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that ICosmos interface includes is_authority_address
# and check its usage across the codebase
# Check if the interface includes is_authority_address
echo "Checking ICosmos interface definition..."
rg -A 2 "interface ICosmos"
# Look for is_authority_address implementation
echo "Checking is_authority_address implementation..."
ast-grep --pattern 'function is_authority_address($$$) $$$'
# Check for any existing usage of is_authority_address
echo "Checking existing usage..."
rg "is_authority_address"
Length of output: 4766
x/evm/keeper/precompiles.go (2)
Line range hint 15-36: Verify precompile initialization remains intact.
The addition of the authority parameter only affects the cosmos precompile initialization. However, since this is a core precompile setup function, please ensure:
- All existing precompile functionality continues to work
- The authority checks don't interfere with other precompile operations
Let's verify the precompile initialization:
#!/bin/bash
# Description: Verify precompile initialization and usage
# Expected: Find all precompile initialization and usage patterns
# Search for precompile initialization patterns
rg -A 5 'NewCosmosPrecompile|NewERC20RegistryPrecompile' 'x/evm'
# Search for precompile usage patterns
rg -A 5 'CosmosPrecompileAddress|ERC20RegistryPrecompileAddress' 'x/evm'29-29: LGTM! Authority parameter addition aligns with PR objectives.
The addition of k.authority parameter to NewCosmosPrecompile enables authority checks, which is essential for controlling ERC20 metadata updates as specified in the PR objectives.
Let's verify the authority parameter usage in the cosmos precompile:
✅ Verification successful
Authority parameter is properly implemented and used for access control
The verification confirms that:
- The authority parameter is correctly stored in the CosmosPrecompile struct and initialized during construction
- The precompile implements an
is_authority_addressmethod to check if an address matches the authority - The authority is set to the governance module address in tests, indicating proper access control for administrative operations
- The implementation aligns with the PR's objective of enabling controlled ERC20 token metadata updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the authority parameter usage in cosmos precompile
# Expected: Find the authority check implementation in the cosmos precompile
# Search for authority-related code in cosmos precompile
ast-grep --pattern 'func $_(authority $_) $_'
rg -A 5 'authority' 'x/evm/precompiles/cosmos'
Length of output: 10728
Script:
#!/bin/bash
# Let's search for authority-related code in the cosmos precompile implementation
rg -A 10 'authority' x/evm/precompiles/cosmos/
Length of output: 19805
x/evm/precompiles/cosmos/types.go (3)
19-22: LGTM: Well-structured type definition
The new IsAuthorityAddressArguments type follows the established pattern and includes proper documentation.
78-80: Verify authority check implementation
The method name constant follows the established pattern. Let's verify its implementation in relation to the PR's objective of checking if the token creator is address 0x1 for IBC/opbridge tokens.
#!/bin/bash
# Description: Verify the authority check implementation
# Search for the method implementation
rg -g '*.{go,sol}' -A 10 "is_authority_address"
# Look for address constant definitions (0x1)
rg -g '*.{go,sol}' "0x1|authorityAddr"61-63: Verify gas cost for authority address check
The gas cost (200) matches other similar operations like IS_MODULE_ADDRESS_GAS. However, let's verify if this is appropriate for the authority check operation.
x/evm/contracts/i_cosmos/ICosmos.sol (2)
21-24: LGTM! The new authority check function is well-designed.
The is_authority_address function follows the established pattern of the interface's other address checking functions, maintaining consistency in the codebase. The view modifier is correctly used as this is a read-only operation.
Let's verify the integration with ERC20 metadata updates:
✅ Verification successful
The is_authority_address function is properly integrated and used correctly
The verification shows that:
- The
onlyAuthoritymodifier inERC20ACL.solusesis_authority_addressto restrict access to authorized addresses - The modifier is used in
ERC20.solto protect theupdateMetadatafunction, which allows one-time updates of ERC20 metadata (name, symbol, decimals) - The implementation includes a
metadataSealedflag to ensure updates can only happen once - The access control pattern follows the same structure as other security checks in the system (blocked addresses, module addresses)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of is_authority_address in ERC20 contracts
# Expected: Find calls to is_authority_address in ERC20-related contracts
# Search for ERC20 contracts using this authority check
rg -A 5 "is_authority_address" --type sol
# Search for potential metadata update functions in ERC20 contracts
ast-grep --pattern 'function $FUNC_NAME($$$) {
$$$
COSMOS_CONTRACT.is_authority_address($$$)
$$$
}'
Length of output: 234
Script:
#!/bin/bash
# Let's try a different approach to find usages and related code
# Search for any mentions of authority in Solidity files
rg -t solidity "is_authority_address|authority" -A 5
# Find all Solidity files that might be related to ERC20
fd -e sol . | grep -i "erc20\|token"
# Look for metadata update related functions
rg -t solidity "metadata|symbol|name|decimals" -A 5
Length of output: 559
Script:
#!/bin/bash
# Let's examine the ERC20 related files we found
# Check ERC20.sol for authority checks and metadata updates
cat x/evm/contracts/erc20/ERC20.sol
# Check ERC20ACL.sol as it might contain access control logic
cat x/evm/contracts/erc20_acl/ERC20ACL.sol
# Check InitiaERC20.sol as it might be the main implementation
cat x/evm/contracts/initia_erc20/InitiaERC20.sol
Length of output: 9593
21-24: Verify authority checks in the broader system.
Since this is a security-critical function used for metadata updates, ensure it's properly integrated with other system components.
Let's verify the broader security implications:
✅ Verification successful
Authority checks are properly integrated with appropriate access control
The verification shows that is_authority_address is well-integrated into the system's security architecture:
- It's used in the
onlyAuthoritymodifier in ERC20ACL.sol, which serves as a base contract for access control - The modifier properly validates the caller using
requirewith a clear error message - The pattern is consistent with other security modifiers (onlyChain, burnable, mintable, transferable)
- Multiple contracts import and can utilize this security infrastructure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential security issues in authority usage
# Expected: Find all authority-related checks and modifiers
# Look for other authority-related functions or modifiers
rg -A 5 "onlyAuthority|requireAuthority|authority" --type sol
# Check for potential privilege escalation vectors
ast-grep --pattern 'function $FUNC_NAME($$$) {
$$$
if ($$$) {
$$$
COSMOS_CONTRACT.is_authority_address($$$)
$$$
}
$$$
}'
Length of output: 234
Script:
#!/bin/bash
# Search for authority-related patterns in Solidity files
rg -t solidity -A 5 "onlyAuthority|requireAuthority|authority"
# Find all Solidity files that might use the ICosmos interface
fd -e sol -x rg -l "ICosmos|is_authority_address"
# Look for authority-related modifiers
rg -t solidity "modifier.*authority"
# Search for authority checks in function implementations
rg -t solidity "require.*authority|assert.*authority"
Length of output: 533
Script:
#!/bin/bash
# Check how is_authority_address is used in the identified files
rg -A 5 "is_authority_address" ./x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol ./x/evm/contracts/counter/Counter.sol ./x/evm/contracts/erc20_acl/ERC20ACL.sol
# Look for authority-related checks or modifiers without the solidity type flag
rg -A 5 "require.*authority|assert.*authority|modifier.*authority" ./x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol ./x/evm/contracts/counter/Counter.sol ./x/evm/contracts/erc20_acl/ERC20ACL.sol
# Check for any authority-related state variables or constructor parameters
rg "authority" ./x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol ./x/evm/contracts/counter/Counter.sol ./x/evm/contracts/erc20_acl/ERC20ACL.sol
Length of output: 1077
Script:
#!/bin/bash
# Check the full context of authority usage in ERC20ACL.sol
cat ./x/evm/contracts/erc20_acl/ERC20ACL.sol
# Check for any imports of ICosmos in these contracts
rg "import.*ICosmos" ./x/evm/contracts/erc20_wrapper/ERC20Wrapper.sol ./x/evm/contracts/counter/Counter.sol ./x/evm/contracts/erc20_acl/ERC20ACL.sol
# Look for functions that might be protected by authority checks
rg -B 5 "COSMOS_CONTRACT.is_authority_address" ./x/evm/contracts/erc20_acl/ERC20ACL.sol
Length of output: 1943
x/evm/contracts/erc20/ERC20.sol (2)
20-26: LGTM! State variable addition is well-structured.
The new metadataSealed boolean state variable is appropriately placed with other metadata variables and has correct visibility.
144-144: Verify authority control implementation.
The function uses the onlyAuthority modifier, but we need to verify its implementation in the ERC20ACL contract to ensure proper access control.
x/bank/keeper/grpc_query_test.go (1)
147-147: LGTM! Standardizing with ERC20 decimal places.
The change from 0 to 18 decimal places aligns with the standard ERC20 token precision in Ethereum, which is appropriate given the PR's focus on ERC20 metadata management.
Let's verify the consistency of this change with other token-related configurations:
x/evm/contracts/erc20_acl/ERC20ACL.go (1)
35-35: Verify the bytecode change and its implications.
The bytecode has been updated while maintaining the same Solidity version (0.8.25). While the change reflects the new contract logic for one-time metadata updates and authority checks, we should verify:
- That the bytecode was generated from the correct Solidity source
- That all necessary features are included in the bytecode
x/evm/precompiles/cosmos/contract.go (3)
51-52: LGTM: Authority address field addition
The addition of authorityAddr field to CosmosPrecompile struct is well-placed and correctly typed as sdk.AccAddress.
64-69: LGTM: Constructor changes with proper validation
The constructor changes appropriately handle the new authority parameter with proper validation and error handling.
Let's verify the authority address usage across the codebase:
#!/bin/bash
# Search for authority address initialization to ensure consistent usage
rg -l "NewCosmosPrecompile.*authority.*"
# Search for potential direct authority address comparisons
rg "authorityAddr.*Equals|Equals.*authorityAddr"Also applies to: 81-81
Line range hint 1-424: Verify complete implementation of one-time update feature
While this file correctly implements the authority checking mechanism, please ensure that the one-time update functionality is properly implemented in the ERC20 contract code.
Let's verify the complete implementation:
x/evm/keeper/erc20.go (2)
Line range hint 116-144: LGTM: Robust error handling
The error handling in the metadata retrieval process is thorough and well-structured. Each contract call is properly checked, and errors are appropriately propagated with context.
145-151: Verify edge cases in metadata handling
The changes to denomUnits handling introduce potential edge cases that need verification:
- When
denom == symbol, we remove the first unit, but this assumes the first unit always hasExponent: 0. This assumption might not hold if the order of units changes. - The removal of the first unit when
denom == symbolcould affect clients that expect at least one denomination unit.
Let's verify the token metadata handling:
✅ Verification successful
Based on the search results, I can now make a definitive assessment. The code shows consistent patterns in handling DenomUnits across the codebase:
- The base denom (with exponent 0) is always the first unit in the array, as seen in test files and bank keeper implementation.
- The removal of the first unit when
denom == symbolis safe because:- The base denom with exponent 0 is duplicated (once as denom, once as symbol)
- The codebase consistently maintains the base denom as the first unit
- Tests verify the metadata structure and denom units handling
The metadata handling changes are safe and correct
The implementation follows the established pattern where:
- Base denom (exponent 0) is always first
- When denom equals symbol, removing the duplicate unit prevents redundancy while maintaining the correct token representation
- The bank module's metadata handling supports this structure, as evidenced by the test implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential edge cases in token metadata handling
# Test 1: Search for other places where DenomUnit is constructed to verify consistency
rg -A 5 "DenomUnit"
# Test 2: Search for code that relies on the presence of specific denomination units
rg -A 5 "denomUnits\[0\]|denomUnits\[1:\]|len\(denomUnits\)"
# Test 3: Look for tests that verify metadata structure
rg -A 10 "TestGetMetadata|TestMetadata"
Length of output: 22231
x/evm/keeper/erc20_test.go (1)
133-145: LGTM! Well-structured helper function.
The implementation is clean, focused, and properly handles error propagation. The function follows the single responsibility principle and integrates well with the existing test infrastructure.
x/evm/precompiles/cosmos/contract_test.go (4)
28-28: LGTM: Required import for authority address.
The addition of govtypes import is necessary for deriving the authority address from the governance module name.
154-194: Test coverage for authority address verification looks comprehensive.
The new test function Test_CosmosPrecompile_IsAuthorityAddress thoroughly validates the authority address checking functionality:
- Tests non-authority address case (returns false)
- Tests actual authority address case (returns true)
- Includes gas consumption validation
- Follows the established test pattern in the file
172-174: LGTM: Proper gas consumption validation.
The test correctly validates the gas consumption requirements for the authority address check by testing the out-of-gas scenario.
183-193: LGTM: Thorough authority address validation.
The test properly validates the positive case by:
- Converting the governance module address to EVM format
- Testing the authority check with the correct address
- Verifying the expected true result
x/evm/contracts/i_cosmos/ICosmos.go (3)
189-204: LGTM: IsAuthorityAddress caller implementation.
The implementation follows the established pattern for read-only contract calls:
- Correct method binding to Solidity function
- Proper error handling
- Type conversion using abi.ConvertType
206-211: LGTM: Session wrapper implementations.
Both ICosmosSession and ICosmosCallerSession implementations:
- Correctly delegate to the underlying contract method
- Follow the same pattern as other session methods
- Maintain consistent error handling
Also applies to: 213-218
40-40: Verify ABI changes align with Solidity contract.
The ABI has been updated to include the new is_authority_address function. The function is correctly marked as view since it only reads contract state.
Let's verify the Solidity contract implementation matches this ABI:
✅ Verification successful
✓ ABI matches Solidity interface definition
The ABI in ICosmos.go correctly reflects the interface definition found in ICosmos.sol. The is_authority_address function is properly defined in the Solidity interface with the exact signature: function is_authority_address(address account) external view returns (bool authority). The function's visibility (external), mutability (view), parameter type (address), and return type (bool) all match the ABI specification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Solidity contract implementation matches the ABI
# Expected: Find the is_authority_address function declaration in the contract
# Search for the function declaration
ast-grep --pattern 'function is_authority_address(address account) view returns (bool authority)'
Length of output: 99
Script:
#!/bin/bash
# Let's try to find the Solidity contract file first
fd -e sol
# Then search for any function containing "authority" in case the signature is different
rg -l "function.*authority"
# Also search for interface or contract definitions related to ICosmos
rg "interface.*ICosmos|contract.*ICosmos" -l
Length of output: 1262
Script:
#!/bin/bash
# Let's examine the ICosmos.sol file to verify the function implementation
cat x/evm/contracts/i_cosmos/ICosmos.sol
# Also check if there are any related test files that might verify this functionality
rg -l "is_authority_address" --type go --type sol
Length of output: 3921
x/evm/contracts/counter/Counter.go (3)
Line range hint 642-828: LGTM: Event handling implementation is robust.
The event handling implementation follows best practices with proper:
- Error handling in event filtering and parsing
- Subscription management
- Resource cleanup
32-38: Review recursive function implementation for safety.
The contract includes a recursive function that emits events. This could potentially lead to stack overflow or excessive gas consumption if not properly bounded. Consider implementing a maximum recursion depth or using an iterative approach instead.
Also applies to: 606-610
35-35: Verify the bytecode changes against PR objectives.
The bytecode has been updated, but this is a Counter contract rather than an ERC20 contract. Given that the PR's objective is to enable one-time updates of ERC20 token metadata, please verify if these changes to the Counter contract are intentional and necessary.
✅ Verification successful
Bytecode update is correct and aligned with the codebase
The Counter contract is a test contract implementing IIBCAsyncCallback interface, used for testing IBC (Inter-Blockchain Communication) functionality. The bytecode changes reflect the latest implementation of the Counter contract which includes IBC callback handling, cosmos message execution, and recursive testing capabilities. This is not directly related to ERC20 metadata updates, but rather serves as a test contract for the IBC infrastructure that those ERC20 updates will utilize.
Key evidence from the codebase:
- Counter implements
IIBCAsyncCallbackinterface for handling IBC acknowledgments and timeouts - Contains test-specific functions like
recursiveandexecute_cosmos_with_options - Used in IBC hooks tests (referenced in
app/ibc-hooks/common_test.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if this Counter contract is related to or used by ERC20 contracts
# Look for imports or references to Counter contract in ERC20-related files
# Search for Counter contract usage in ERC20 files
rg -l "Counter" --glob "**/*ERC20*.{go,sol}"
# Search for potential dependencies between Counter and ERC20
ast-grep --pattern 'import { $_ } from "$_Counter$_"'
Length of output: 278
Script:
#!/bin/bash
# Let's try a different approach to find relationships between Counter and ERC20
# Search for Counter references in any file
rg -l "Counter"
# Search for files that contain both Counter and ERC20 references
rg -l "Counter.*ERC20|ERC20.*Counter"
# Look for imports or contract definitions in Solidity files
rg "^\s*(import|contract)\s+.*Counter" --type-add 'sol:*.sol' --type sol
# Check if Counter is used in any test files related to ERC20
rg -l "Counter" --glob "**/test/**/*ERC20*"
Length of output: 1034
Script:
#!/bin/bash
# Let's examine the Counter contract's purpose and its relationship with IBC
# Check the Counter.sol contract implementation
cat x/evm/contracts/counter/Counter.sol
# Look for IBC-related interfaces or implementations
rg "IIBCAsyncCallback" -A 5
# Look for any references to this contract in IBC-related code
rg -l "Counter.*IBC|IBC.*Counter"
Length of output: 5367
x/evm/contracts/erc20_factory/ERC20Factory.go (3)
35-36: Verify that the updated ABI and bytecode correspond to the latest contract changes.
The ABI and Bin fields in Erc20FactoryMetaData have been modified. Please ensure these updates accurately reflect the recent changes to the ERC20Factory contract, particularly the addition of the one-time update feature for ERC20 token metadata.
Line range hint 458-460: Confirm client compatibility with updated createERC20 function logic.
Although the createERC20 function signature remains unchanged, its internal logic now checks if msg.sender is not equal to CHAIN_ADDRESS. Please verify that any client code interacting with this function handles this new condition appropriately and that there are no unintended side effects.
Line range hint 547-549: Ensure event parsing aligns with updated ERC20Created event structure.
In the ParseERC20Created function, confirm that the parsing logic correctly corresponds to any changes made to the ERC20Created event in the contract's ABI, including parameter types and indexed fields.
x/evm/contracts/initia_erc20/InitiaERC20.go (1)
Line range hint 1-846: Go bindings regenerated correctly
The InitiaERC20.go file has been updated to reflect the latest changes in the Solidity contract. The regenerated Go bindings appear consistent with the updated ABI and bytecode.
x/evm/contracts/erc20/ERC20.go (4)
34-35: ABI and Binary updated to reflect contract changes
The ABI and Bin fields have been updated to include the new _metadataSealed parameter and related methods. This ensures the Go bindings are in sync with the updated Solidity contract.
298-328: Addition of MetadataSealed methods to Go bindings
The MetadataSealed methods have been added to the Erc20Caller, Erc20Session, and Erc20CallerSession types. This provides a way to check the sealed state of the metadata from the Go code, aligning with the new functionality in the Solidity contract.
694-714: Addition of UpdateMetadata methods to Go bindings
The UpdateMetadata methods have been introduced in the Erc20Transactor, Erc20Session, and Erc20TransactorSession types. This allows for updating the token's metadata via Go code. Ensure that appropriate access controls are enforced in the Solidity contract to prevent unauthorized metadata updates.
Run the following script to verify that the updateMetadata function is properly protected in the Solidity contract:
Consider adding unit tests to cover the UpdateMetadata functionality, ensuring it works as expected and handles edge cases.
✅ Verification successful
Let me check the actual implementation of the updateMetadata function in the Solidity contract to verify access controls.
Let me verify the onlyAuthority modifier to ensure it properly restricts access.
Let me try a broader search to find the authority-related code.
Let me check the contract's inheritance to understand the access control implementation.
Access controls are properly implemented for metadata updates
The updateMetadata function is protected by:
onlyAuthoritymodifier to restrict access to authorized addressesrequire(!metadataSealed)check to prevent updates once metadata is sealed
The contract inherits from Ownable and ERC20ACL which provide the access control infrastructure. The Go bindings correctly expose this protected functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the `updateMetadata` function has appropriate access control modifiers.
# Search the Solidity contract for the `updateMetadata` function definition.
rg 'function updateMetadata'
Length of output: 466
Script:
#!/bin/bash
# Get the implementation of updateMetadata function with surrounding context
rg -A 10 -B 2 'function updateMetadata' x/evm/contracts/erc20/ERC20.sol
Length of output: 365
Script:
#!/bin/bash
# Search for the onlyAuthority modifier definition
rg -B 5 -A 5 'modifier onlyAuthority' x/evm/contracts/erc20/ERC20.sol
Length of output: 71
Script:
#!/bin/bash
# Search for authority-related code and access control
rg -i "authority|owner" x/evm/contracts/erc20/ERC20.sol
Length of output: 261
Script:
#!/bin/bash
# Check contract inheritance and imports
rg -A 5 "contract ERC20" x/evm/contracts/erc20/ERC20.sol
Length of output: 310
Line range hint 47-56: Verify all calls to DeployErc20 include the new _metadataSealed parameter
The DeployErc20 function signature now includes the _metadataSealed parameter. This is a breaking change that may affect parts of the codebase where this function is called. Please verify that all invocations have been updated accordingly to prevent runtime errors.
Run the following script to identify all calls to DeployErc20 and ensure they include the new parameter:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
x/evm/contracts/erc20_factory/ERC20Factory.go (1)
Line range hint
279-284: Add documentation for metadata sealing behavior.The
CreateERC20method andERC20Createdevent should include documentation about the metadata sealing behavior, specifically mentioning:
- The one-time update capability for IBC/opbridge tokens
- The conditions under which metadata becomes sealed
Also applies to: 332-337
app/upgrade.go (3)
Line range hint
49-56: Use error wrapping or comparison instead of string matching for error handlingCurrently, error handling relies on string matching to ignore
vm.ErrContractAddressCollisionerrors when deploying contracts. This approach can be fragile and may not handle wrapped errors correctly.Consider using
errors.Isfor more robust error handling.Apply this diff to improve error handling:
import ( ... + "errors" ) ... // deploy and store erc20 factory contract address if err := app.EVMKeeper.DeployERC20Factory(ctx); err != nil { - if !strings.Contains(err.Error(), vm.ErrContractAddressCollision.Error()) { + if !errors.Is(err, vm.ErrContractAddressCollision) { return nil, err } } // deploy and store erc20 wrapper contract address if err := app.EVMKeeper.DeployERC20Wrapper(ctx); err != nil { - if !strings.Contains(err.Error(), vm.ErrContractAddressCollision.Error()) { + if !errors.Is(err, vm.ErrContractAddressCollision) { return nil, err } }
67-69: Clarify the use of magic constants for maintainabilityThe constant
"5ff3fe"used incommon.Hex2Bytes("5ff3fe")may not be immediately clear.Consider defining it with a descriptive name or adding comments to explain its purpose.
For example:
// runtime code -initCodeOP := common.Hex2Bytes("5ff3fe") +const selfDestructSequenceHex = "5ff3fe" // Sequence indicating the end of the contract creation code +initCodeOP := common.Hex2Bytes(selfDestructSequenceHex) initCodePos := bytes.Index(code, initCodeOP)
111-115: Simplifyuint64ToBytesusing standard library functionsThe
uint64ToBytesfunction can be simplified by usingbinary.BigEndian.AppendUint64, which appends the uint64 value directly to a byte slice.Apply this diff to streamline the function:
func uint64ToBytes(v uint64) []byte { - bz := make([]byte, 8) - binary.BigEndian.PutUint64(bz, v) - return bz + return binary.BigEndian.AppendUint64(nil, v) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app/upgrade.go(2 hunks)x/evm/contracts/erc20/ERC20.go(5 hunks)x/evm/contracts/erc20/ERC20.sol(3 hunks)x/evm/contracts/erc20_factory/ERC20Factory.go(1 hunks)x/evm/contracts/erc20_wrapper/ERC20Wrapper.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/evm/contracts/erc20_wrapper/ERC20Wrapper.go
🔇 Additional comments (10)
x/evm/contracts/erc20/ERC20.sol (3)
27-28: LGTM: State variable declaration is clear and well-documented.
The metadataSealed boolean state variable is appropriately declared with public visibility and clear documentation.
42-47:
Add creator address check for metadata sealing.
As per PR objectives, only tokens created via IBC/opbridge (from address 0x1) should allow unsealed metadata. This check is missing.
Apply this fix:
constructor(string memory _name, string memory _symbol, uint8 _decimals, bool _metadataSealed) {
name = _name;
symbol = _symbol;
decimals = _decimals;
- metadataSealed = _metadataSealed;
+ // Only tokens created from address(1) (IBC/opbridge) can have unsealed metadata
+ metadataSealed = msg.sender != address(1) ? true : _metadataSealed;
}137-154: 🛠️ Refactor suggestion
Add input validation and events for metadata updates.
The metadata update function needs additional safeguards and transparency:
- Add validation for reasonable metadata values
- Emit events for transparency
- Add NatSpec documentation
- Consider gas optimization
Apply these improvements:
+ /// @notice Updates token metadata once and seals it
+ /// @dev Only callable by authority when metadata is not sealed
+ /// @param _name New token name (must not be empty)
+ /// @param _symbol New token symbol (must not be empty)
+ /// @param _decimals New decimal places (must be <= 18)
+ event MetadataUpdated(string indexed name, string indexed symbol, uint8 decimals);
+
function updateMetadata(
string memory _name,
string memory _symbol,
uint8 _decimals
) external onlyAuthority {
require(!metadataSealed, "ERC20: metadata sealed");
+ require(bytes(_name).length > 0, "ERC20: empty name");
+ require(bytes(_symbol).length > 0, "ERC20: empty symbol");
+ require(_decimals <= 18, "ERC20: invalid decimals");
name = _name;
symbol = _symbol;
decimals = _decimals;
metadataSealed = true;
+ emit MetadataUpdated(_name, _symbol, _decimals);
}Likely invalid or redundant comment.
x/evm/contracts/erc20_factory/ERC20Factory.go (3)
Line range hint 89-102: LGTM: Contract deployment implementation is robust.
The contract deployment and binding functions follow best practices with proper error handling and null checks.
Line range hint 89-102: LGTM: Comprehensive error handling.
The implementation includes robust error handling throughout the contract interactions:
- Proper error checking during contract deployment
- Clean error propagation in method calls
- Appropriate error handling in event processing
Also applies to: 279-284
35-35: Verify contract bytecode changes.
The updated contract bytecode includes significant changes. Please ensure:
- The bytecode has been properly tested with the new metadata update functionality
- The changes maintain backward compatibility with existing ERC20 tokens
x/evm/contracts/erc20/ERC20.go (3)
298-327: LGTM: MetadataSealed getter methods are properly implemented
The new MetadataSealed getter methods follow the established patterns for ERC20 state variable access, with proper error handling and documentation.
Line range hint 47-56: LGTM: Contract deployment changes look good
The addition of the _metadataSealed parameter to DeployErc20 is well-implemented with proper error handling. This change supports the PR objective of enabling one-time metadata updates.
#!/bin/bash
# Verify the contract deployment usage across the codebase
# Look for any direct calls to DeployErc20 that might need updating
rg -A 5 "DeployErc20\(" 694-713: LGTM: UpdateMetadata methods are properly implemented
The new UpdateMetadata methods follow the established patterns for ERC20 contract mutations. Note that metadata sealing validation is handled at the contract level, not in these bindings.
✅ Verification successful
Let me search for the contract implementation to verify the metadata sealing validation.
Let me try a different approach to find the contract implementation.
Let me check the contract implementation since we found the file.
Verified: UpdateMetadata implementation is secure with proper validation
The Go bindings correctly reflect the Solidity contract's UpdateMetadata function. The contract implementation includes important security measures:
- Restricted to authority-only access via
onlyAuthoritymodifier - Validates metadata sealing with
require(!metadataSealed, "ERC20: metadata sealed") - Automatically seals metadata after update to prevent further modifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the UpdateMetadata method usage and potential validation
# Look for any direct calls to UpdateMetadata that might need additional checks
rg -A 5 "UpdateMetadata\("
Length of output: 1873
Script:
#!/bin/bash
# Look for the Solidity contract implementation
rg -l "updateMetadata.*function" --type solidity
# If found, show the implementation
ast-grep --pattern 'function updateMetadata($$$) $$$'
Length of output: 139
Script:
#!/bin/bash
# Look for the contract file and implementation
fd -e sol . | xargs rg -l "updateMetadata"
# Also check for any test files that might use this method
rg -g "**/*test*" "UpdateMetadata" -A 5
Length of output: 114
Script:
#!/bin/bash
# Show the contract implementation
cat x/evm/contracts/erc20/ERC20.sol
Length of output: 4591
app/upgrade.go (1)
86-87: Verify contract account updates for consistency
After updating the CodeHash of contractAcc, ensure that all related fields and state are consistent. This includes verifying if additional updates are needed for the contract account's storage or other metadata.
Description
Allow ERC20 metadata to be updated via gov if it is created from 0x1 (ibc or opbridge).
In the Solidity contract, we check if the ERC20 token creator is 0x1 to explicitly determine whether it is created for IBC or opbridge. If it is not created from 0x1, we seal the metadata.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
Release Notes
New Features
ICosmosinterface and theCosmosPrecompilestructure.Bug Fixes
Refactor
Tests