feat(test): Add support for custom decimals to test suite. #3006
Conversation
WalkthroughThis pull request introduces comprehensive modifications across multiple files in the Evmos testing and integration framework. The changes primarily focus on enhancing test configurations, improving balance and fee handling, and adding more flexibility for chain ID and decimal configurations. Key modifications include updating test suites to support dynamic chain IDs, refactoring balance retrieval methods, and introducing more modular approaches to handling initial amounts, fees, and network configurations. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1)CHANGELOG.md (1)
The changelog entry properly documents the test suite improvements for custom decimals support and ante test fixes, which aligns with the PR objectives. 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 (
|
add support for custom decimals + fix tests
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
testutil/integration/evmos/network/setup.go (1)
458-461: Add comment explaining the min deposit calculationThe min deposit calculation logic would benefit from a comment explaining the conversion factor usage.
+ // Calculate min deposit by dividing 1e18 by the EVM coin decimals conversion factor + // to maintain consistent deposit requirements across different decimal configurations minDepositAmt := sdkmath.NewInt(1e18).Quo(evmtypes.GetEVMCoinDecimals().ConversionFactor()) updatedParams.MinDeposit = sdktypes.NewCoins(sdktypes.NewCoin(overwriteParams.denom, minDepositAmt)) updatedParams.ExpeditedMinDeposit = sdktypes.NewCoins(sdktypes.NewCoin(overwriteParams.denom, minDepositAmt))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testutil/integration/evmos/network/setup.go(5 hunks)
🔇 Additional comments (1)
testutil/integration/evmos/network/setup.go (1)
11-11: LGTM! Good choice using the experimental maps package
The addition of golang.org/x/exp/maps is appropriate for standardized map key operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
testutil/integration/evmos/network/amounts.go (1)
15-18: Add documentation for the InitialAmounts struct.Consider adding field-level documentation to clarify the purpose of each field and their relationship.
type InitialAmounts struct { + // Base represents the initial amount for the base coin Base math.Int + // Evm represents the initial amount for the EVM coin Evm math.Int }testutil/integration/evmos/network/config.go (1)
44-46: Improve documentation for otherCoinDenoms field.The current documentation could be more specific about the usage and constraints of these denominations.
- // otherCoinDenoms represents the other possible coin denominations that can be passed during - // test suite intialization to provide other coins initial balances. + // otherCoinDenoms represents additional coin denominations that can be initialized with balances + // during test suite setup. These denominations will use the base coin's decimal configuration. otherCoinDenoms []stringx/inflation/v1/keeper/inflation_test.go (2)
142-151: Consider extracting chain configuration logic.The chain configuration logic is repeated in multiple test cases. Consider extracting it into a helper function.
+func getChainConfig(isTestnet bool, foundationAcc []sdk.AccAddress) (string, math.Int) { + if isTestnet { + return utils.TestnetChainID + "-1", + network.PrefundedAccountInitialBalance.MulRaw(int64(len(foundationAcc))) + } + return utils.MainnetChainID + "-1", math.ZeroInt() +} + func TestGetCirculatingSupplyAndInflationRate(t *testing.T) { // ... - switch isTestnet { - case true: - chainID = utils.TestnetChainID + "-1" - teamAllocation = network.PrefundedAccountInitialBalance.MulRaw(int64(len(foundationAcc))) - default: - chainID = utils.MainnetChainID + "-1" - teamAllocation = math.ZeroInt() - } + chainID, teamAllocation = getChainConfig(isTestnet, foundationAcc)
153-162: Add documentation for supply calculations.The calculation logic for bonded amounts and supply would benefit from explanatory comments.
+ // Get base coin information from the chain ID baseCoinInfo := evmtypes.ChainsCoinInfo[strings.Split(chainID, "-")[0]] + // Calculate total bonded amount: validator bonds + account bonds valBondedAmt := network.GetInitialBondedAmount(baseCoinInfo.Decimals).MulRaw(nVals) accsBondAmount := math.OneInt().MulRaw(nVals) bondedAmount := valBondedAmt.Add(accsBondAmount) + // Calculate free amount: initial balance * number of accounts - bonded amount + team allocation accsFreeAmount := network.PrefundedAccountInitialBalance. MulRaw(nAccs). Sub(accsBondAmount). Add(teamAllocation)CHANGELOG.md (1)
1049-1051: Fix indentation in changelog entryThe list items under the ERC20 bug fixes section have inconsistent indentation. Consider adjusting the indentation to match the surrounding text.
- - Check if supply exists for a token before when submitting a `RegisterCoinProposal`, allowing users to create an ERC20 representation of an invalid Cosmos Coin. - - Sanitize the ERC20 token name when creating coin metadata on ER `RegisterERC20Proposal`. - - Fix coin metadata validation error when registering an ERC20 with 0 denom units. + - Check if supply exists for a token before when submitting a `RegisterCoinProposal`, allowing users to create an ERC20 representation of an invalid Cosmos Coin. + - Sanitize the ERC20 token name when creating coin metadata on ER `RegisterERC20Proposal`. + - Fix coin metadata validation error when registering an ERC20 with 0 denom units.🧰 Tools
🪛 Markdownlint (0.37.0)
1049-1049: Expected: 120; Actual: 165
Line length(MD013, line-length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(2 hunks)app/ante/evm/09_gas_consume_test.go(5 hunks)testutil/integration/evmos/network/amounts.go(1 hunks)testutil/integration/evmos/network/config.go(5 hunks)testutil/integration/evmos/network/network.go(4 hunks)testutil/integration/evmos/utils/gov.go(1 hunks)x/inflation/v1/keeper/grpc_query_test.go(4 hunks)x/inflation/v1/keeper/inflation_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/ante/evm/09_gas_consume_test.go
- testutil/integration/evmos/utils/gov.go
🧰 Additional context used
🪛 Markdownlint (0.37.0)
CHANGELOG.md
1049-1049: Expected: 120; Actual: 165
Line length
(MD013, line-length)
🔇 Additional comments (6)
testutil/integration/evmos/network/amounts.go (3)
35-47: 🛠️ Refactor suggestion
Improve error handling in GetInitialAmount.
The function currently panics on validation errors. Consider returning an error instead to allow better error handling by callers.
-func GetInitialAmount(decimals evmtypes.Decimals) math.Int {
+func GetInitialAmount(decimals evmtypes.Decimals) (math.Int, error) {
if err := decimals.Validate(); err != nil {
- panic("unsupported decimals")
+ return math.Int{}, fmt.Errorf("unsupported decimals: %w", err)
}
initialBalance, _ := math.NewIntFromString("100_000_000_000_000_000_000_000")
- return initialBalance.Quo(decimals.ConversionFactor())
+ return initialBalance.Quo(decimals.ConversionFactor()), nil
}Likely invalid or redundant comment.
49-60:
Critical: Global SDK configuration modification and error handling.
Two issues need to be addressed:
- Modifying
sdk.DefaultPowerReductionglobally could have unintended side effects in concurrent tests. - The function uses panic for error handling.
Consider this safer implementation:
-func GetInitialBondedAmount(decimals evmtypes.Decimals) math.Int {
+func GetInitialBondedAmount(decimals evmtypes.Decimals) (math.Int, error) {
if err := decimals.Validate(); err != nil {
- panic("unsupported decimals")
+ return math.Int{}, fmt.Errorf("unsupported decimals: %w", err)
}
- sdk.DefaultPowerReduction = math.NewIntFromBigInt(new(big.Int).Exp(big.NewInt(10), big.NewInt(int64(decimals)), nil))
- initialBondedAmount := sdk.TokensFromConsensusPower(1, types.PowerReduction)
+ powerReduction := math.NewIntFromBigInt(new(big.Int).Exp(big.NewInt(10), big.NewInt(int64(decimals)), nil))
+ initialBondedAmount := sdk.TokensFromConsensusPower(1, powerReduction)
- return initialBondedAmount.Quo(decimals.ConversionFactor())
+ return initialBondedAmount.Quo(decimals.ConversionFactor()), nil
}Likely invalid or redundant comment.
62-75: 🛠️ Refactor suggestion
Improve error handling and maintainability of GetInitialBaseFeeAmount.
The function could benefit from better error handling and a more maintainable structure.
-func GetInitialBaseFeeAmount(decimals evmtypes.Decimals) math.LegacyDec {
+func GetInitialBaseFeeAmount(decimals evmtypes.Decimals) (math.LegacyDec, error) {
if err := decimals.Validate(); err != nil {
- panic("unsupported decimals")
+ return math.LegacyDec{}, fmt.Errorf("unsupported decimals: %w", err)
}
- switch decimals {
- case evmtypes.EighteenDecimals:
- return math.LegacyNewDec(1_000_000_000)
- case evmtypes.SixDecimals:
- return math.LegacyNewDecWithPrec(1, 3)
- default:
- panic("base fee not specified")
+ baseFees := map[evmtypes.Decimals]math.LegacyDec{
+ evmtypes.EighteenDecimals: math.LegacyNewDec(1_000_000_000),
+ evmtypes.SixDecimals: math.LegacyNewDecWithPrec(1, 3),
}
+
+ if fee, ok := baseFees[decimals]; ok {
+ return fee, nil
+ }
+ return math.LegacyDec{}, fmt.Errorf("base fee not specified for decimals: %d", decimals)
}Likely invalid or redundant comment.
testutil/integration/evmos/network/config.go (1)
191-194: Verify usage of renamed WithOtherDenoms function.
The function has been renamed from WithOtherDenom to WithOtherDenoms. Let's verify all callers have been updated.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information:
All usages of the renamed function are up to date
The search results show:
- No instances of the old function name
WithOtherDenomwere found - The new function name
WithOtherDenomsis being used correctly in all locations:- x/ibc/transfer/keeper/keeper_test.go
- precompiles/erc20/integration_test.go
- precompiles/bank/setup_test.go
- precompiles/bank/integration_test.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of old function name
rg "WithOtherDenom[^s]" --type go
# Show current usage of new function name
rg "WithOtherDenoms" -A 2 -B 2 --type go
Length of output: 2172
x/inflation/v1/keeper/grpc_query_test.go (1)
226-228: LGTM: Test updates for custom decimal support
The changes correctly update the test code to use chain-specific decimal precision when calculating bonded amounts, aligning with the PR's objective of supporting custom decimals in the test suite.
Also applies to: 113-113
testutil/integration/evmos/network/network.go (1)
157-160: Verify base fee calculation with different decimal configurations
The base fee is now adapted to the configured base decimals. Please ensure this scales correctly across different decimal configurations.
✅ Verification successful
Let me try a different approach to verify the base fee calculation implementation.
Let me check the Decimals type definition and validation to complete the verification.
Based on the verification results, I can now provide a final response:
Base fee calculation correctly handles supported decimal configurations
The implementation in GetInitialBaseFeeAmount properly handles different decimal configurations with explicit validation:
- For 18 decimals: returns 1,000,000,000 (1 Gwei)
- For 6 decimals: returns 0.001
- Other decimal values are rejected through validation
The function includes proper validation through the Decimals.Validate() method which ensures only supported decimal configurations (6 and 18) are accepted, preventing any scaling issues with unsupported configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify base fee calculation for different decimal configurations
# Test: Check if GetInitialBaseFeeAmount returns expected values for different decimals
# Get base fee for 18 decimals
echo "Testing 18 decimals base fee:"
ast-grep --pattern 'GetInitialBaseFeeAmount(18)'
# Get base fee for 6 decimals
echo "Testing 6 decimals base fee:"
ast-grep --pattern 'GetInitialBaseFeeAmount(6)'
Length of output: 236
Script:
#!/bin/bash
# Let's first find the GetInitialBaseFeeAmount function implementation
ast-grep --pattern 'func GetInitialBaseFeeAmount($$$)'
# Also search for any tests related to base fee calculation
rg -l "GetInitialBaseFeeAmount" --type go
# Look for actual usage of this function
rg "GetInitialBaseFeeAmount\(" --type go -A 2
Length of output: 1794
Script:
#!/bin/bash
# Find the Decimals type definition and validation
rg "type Decimals" --type go -A 5
rg "func.*Validate\(\)" --type go -A 5
# Also check for any test cases
rg "TestGetInitialBaseFeeAmount" --type go -A 10
Length of output: 14396
Description
The PR update the testsuite to support custom decimals representations for the EVM token in tests and update ante handler tests to run with different decimal representations.
Changes breakdown
network.PrefundedAccountInitialBalanceis nownetwork.GetInitialAmount(evmtypes.EighteenDecimals).Since this PR is targeting Evmos repo and not the evmOS one, TODO have been added to inform where changes in the os repo should be done.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation