imp(all): Bump to latest state on Evmos main (0402a01c) #69
Conversation
…o 2.0.0-beta.7 (#3023)
MalteHerrmann
requested review from
0xstepit and
GAtom22
and removed request for
a team
WalkthroughThis pull request introduces a comprehensive set of changes across multiple packages and files in the Evmos OS repository. The modifications primarily focus on updating dependencies, enhancing precompile functionality, and refining test configurations. Key updates include bumping Cosmos SDK and related dependencies to newer versions, introducing an evidence precompile, adding governance parameter querying capabilities, and updating various test utilities and configurations. Changes
Sequence DiagramsequenceDiagram
participant Client
participant PrecompileContract
participant EvidenceKeeper
participant BlockchainState
Client->>PrecompileContract: Submit Evidence
PrecompileContract->>EvidenceKeeper: Validate Evidence
EvidenceKeeper-->>PrecompileContract: Validation Result
alt Validation Successful
PrecompileContract->>BlockchainState: Record Evidence
BlockchainState-->>PrecompileContract: Confirmation
PrecompileContract-->>Client: Evidence Submitted Successfully
else Validation Failed
PrecompileContract-->>Client: Evidence Submission Error
end
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (26)
precompiles/evidence/tx.go (2)
28-36: Confirm Consistency of Evidence Submissions and Event Emissions
The code submits evidence via themsgServerand then emits an event. Both steps are crucial for proof-of-submission. Consider verifying that the event emission does not silently fail in surrounding logic. If the event emission is essential to off-chain listeners or indexing services, ensure that these events are properly handled and tested in integration or E2E scenarios.
34-36: Consider Serializing More Detailed Results Instead of a Boolean
Currently, the transaction completion returns a packedtrue. For improved transparency, consider returning a more informative response (e.g., the evidence hash) if the ABI and contract logic allow it. This can help downstream contracts or off-chain services verify submission details without relying on additional queries.Also applies to: 38-39
precompiles/evidence/evidence.go (4)
31-35: LeverageevidenceKeeperInjection for Testing
Storing the keeper as a struct field is correct for modular design. Consider providing utility constructors in tests that pass a mocked or specializedevidenceKeeperfor robust integration tests, ensuring your precompile handles edge cases gracefully.
43-69: Gas Settings & Address Assignment
Assigning a default expiration duration and usingSetAddressis aligned with typical Evmos precompiles. Good job. You may want to expose some of these configs (e.g.,ApprovalExpiration) through parameters if different modules or chains need dynamic configuration in the future.
88-127: Graceful Out-of-Gas Handling
The defer-based error handling is a helpful approach to avoid panics. The final usage ofcontract.UseGas(cost)ensures no partial states remain if the gas is insufficient. This pattern fosters resilience in EVM execution. You might consider logging more details (method name, cost used) to assist debugging if frequent out-of-gas errors surface.
129-140: Switch Approach vs. Method Lookup Table
The switch statement withinIsTransactionandRunis sufficient for a small set of methods. If the precompile grows in the future, you could utilize a method-lookup map to avoid multiple switches. For now, this is straightforward and easy to read.precompiles/evidence/events.go (1)
31-34: Enhance error context for topic creation failure.The error from
cmn.MakeTopicis returned without additional context, making it harder to debug issues.- topics[1], err = cmn.MakeTopic(origin) - if err != nil { - return err - } + topics[1], err = cmn.MakeTopic(origin) + if err != nil { + return fmt.Errorf("failed to create topic for origin address: %w", err) + }precompiles/evidence/tx_test.go (2)
47-47: Use deterministic time in tests.Using
time.Now()in tests can lead to flaky behavior. Consider using a fixed timestamp for reproducibility.- Time: uint64(time.Now().UTC().Unix()), //nolint:gosec // G115 + Time: uint64(time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC).Unix()),
17-59: Add more test cases for edge scenarios.The current test suite only covers basic success and empty input cases. Consider adding tests for:
- Invalid evidence data (e.g., future timestamp, zero power)
- Malformed consensus address
- Duplicate evidence submission
precompiles/evidence/IEvidence.sol (2)
13-22: Consider adding validation constraints to the Equivocation struct.The struct fields could benefit from additional validation:
heightshould be positivetimeshould be within reasonable boundspowershould be non-negativeconsensusAddressshould be non-empty and validstruct Equivocation { // height is the equivocation height - int64 height; + int64 height; // @dev must be > 0 // time is the equivocation time - uint64 time; + uint64 time; // @dev must be > 0 and <= current block time // power is the validator's power at the time of the equivocation - int64 power; + int64 power; // @dev must be >= 0 // consensusAddress is the validator's consensus address - string consensusAddress; + string consensusAddress; // @dev must be a valid consensus address }
33-36: Consider adding return value documentation for submitEvidence.The function's success return value could benefit from more detailed documentation explaining what constitutes a successful submission.
/// @dev Submit evidence of misbehavior (equivocation) /// @param evidence The evidence of misbehavior - /// @return success True if the evidence was submitted successfully + /// @return success True if the evidence was validated and submitted successfully to the evidence store function submitEvidence(Equivocation calldata evidence) external returns (bool success);precompiles/evidence/query.go (2)
35-41: Consider caching the query server instance.Creating a new query server for each request might be inefficient. Consider caching the query server instance in the Precompile struct.
+// Add to Precompile struct +type Precompile struct { + evidenceKeeper evidencekeeper.Keeper + queryServer evidencetypes.QueryServer +} + +// Update NewPrecompile constructor +func NewPrecompile(...) *Precompile { + return &Precompile{ + evidenceKeeper: evidenceKeeper, + queryServer: evidencekeeper.NewQuerier(&evidenceKeeper), + } +} + +// Update Evidence method - queryServer := evidencekeeper.NewQuerier(&p.evidenceKeeper) - res, err := queryServer.Evidence(ctx, &evidencetypes.QueryEvidenceRequest{ + res, err := p.queryServer.Evidence(ctx, &evidencetypes.QueryEvidenceRequest{
83-96: Optimize memory allocation in GetAllEvidence.Pre-allocating the slice with a capacity might improve performance for large result sets.
- evidenceList := make([]EquivocationData, len(res.Evidence)) + evidenceList := make([]EquivocationData, 0, len(res.Evidence)) for i, evidence := range res.Evidence { equivocation, ok := evidence.GetCachedValue().(*evidencetypes.Equivocation) if !ok { return nil, fmt.Errorf("invalid evidence type at index %d: expected Equivocation", i) } - evidenceList[i] = EquivocationData{ + evidenceList = append(evidenceList, EquivocationData{ Height: equivocation.Height, Time: uint64(equivocation.Time.Unix()), //nolint:gosec // G115 Power: equivocation.Power, ConsensusAddress: equivocation.ConsensusAddress, - } + }) }example_chain/precompiles.go (1)
120-120: Consider grouping related precompiles together.For better organization, consider grouping the evidence precompile with other related precompiles (e.g., slashing, governance).
precompiles[bankPrecompile.Address()] = bankPrecompile precompiles[govPrecompile.Address()] = govPrecompile precompiles[slashingPrecompile.Address()] = slashingPrecompile + // Evidence and validation related precompiles precompiles[evidencePrecompile.Address()] = evidencePrecompileprecompiles/gov/utils_test.go (2)
35-42: Improve function documentation.The function documentation should include:
- Parameter descriptions
- Return value descriptions
- Example usage
43-51: Add default case handling in switch statement.The switch statement should handle unexpected call types to prevent silent failures.
switch callType { case directCall: txArgs.To = &cd.precompileAddr callArgs.ContractABI = cd.precompileABI case contractCall: txArgs.To = &cd.precompileCallerAddr callArgs.ContractABI = cd.precompileCallerABI +default: + panic(fmt.Sprintf("unsupported call type: %v", callType)) }example_chain/config.go (1)
52-52: Enhance error message for unknown chain ID.The error message could be more descriptive to help users understand the expected format and available options.
- return fmt.Errorf("unknown chain id: %s", id) + return fmt.Errorf("unknown chain id: %s. Available chain IDs: %v", id, getAvailableChainIDs(ChainsCoinInfo))Add this helper function:
func getAvailableChainIDs(chainInfo map[string]evmtypes.EvmCoinInfo) []string { ids := make([]string, 0, len(chainInfo)) for id := range chainInfo { ids = append(ids, id) } return ids }testutil/integration/os/network/amounts.go (2)
41-41: Consider using a named constant for the initial balance.The hardcoded string literal could be moved to a package-level constant for better maintainability and reusability.
+const ( + // DefaultInitialBalance represents 100,000 tokens in 18 decimal places (1e23) + DefaultInitialBalance = "100_000_000_000_000_000_000_000" +) func GetInitialAmount(decimals evmtypes.Decimals) math.Int { // ... - initialBalance, _ := math.NewIntFromString("100_000_000_000_000_000_000_000") + initialBalance, _ := math.NewIntFromString(DefaultInitialBalance) // ... }
62-75: Enhance error handling and use constants for magic numbers.The function could benefit from:
- More descriptive panic messages
- Constants for the fee amounts
- Consider returning an error instead of panicking
+const ( + EighteenDecimalBaseFee = 1_000_000_000 + SixDecimalBaseFee = 0.001 +) func GetInitialBaseFeeAmount(decimals evmtypes.Decimals) math.LegacyDec { if err := decimals.Validate(); err != nil { - panic("unsupported decimals") + panic(fmt.Sprintf("unsupported decimals: %v", decimals)) } switch decimals { case evmtypes.EighteenDecimals: - return math.LegacyNewDec(1_000_000_000) + return math.LegacyNewDec(EighteenDecimalBaseFee) case evmtypes.SixDecimals: - return math.LegacyNewDecWithPrec(1, 3) + return math.LegacyNewDecWithPrec(SixDecimalBaseFee * 1000, 3) default: - panic("base fee not specified") + panic(fmt.Sprintf("base fee not specified for decimals: %v", decimals)) } }ethereum/eip712/eip712_legacy_test.go (1)
15-93: Enhance test coverage with additional test cases.The test is well-structured but could benefit from additional edge cases:
- Test with empty messages
- Test with invalid fee amounts
- Test with malformed addresses
Also, consider extracting test constants:
+const ( + testChainID = uint64(1) + testGasAmount = "200000" + testStakeAmount = "1000" + testMemo = "Test Memo" +) func TestLegacyWrapTxToTypedData(t *testing.T) { // Setup cdc := codec.NewProtoCodec(types.NewInterfaceRegistry()) - chainID := uint64(1) + chainID := testChainID // ... rest of the test }ante/evm/07_can_transfer_test.go (1)
49-54: Improve error handling for balance retrieval.The balance retrieval and parsing could be more robust:
- balanceResp, err := grpcHandler.GetBalanceFromEVM(senderKey.AccAddr) - suite.Require().NoError(err) - - balance, ok := math.NewIntFromString(balanceResp.Balance) - suite.Require().True(ok) + balanceResp, err := grpcHandler.GetBalanceFromEVM(senderKey.AccAddr) + suite.Require().NoError(err, "failed to get balance from EVM") + + balance, ok := math.NewIntFromString(balanceResp.Balance) + suite.Require().True(ok, "failed to parse balance string: %s", balanceResp.Balance)ante/evm/10_gas_wanted_test.go (1)
29-29: Define gas limit as a named constant.Move the common gas limit to a package-level constant for better maintainability.
+const ( + // DefaultTestGasLimit represents the default gas limit used in tests + DefaultTestGasLimit = uint64(100_000) +) - commonGasLimit := uint64(100_000) + commonGasLimit := DefaultTestGasLimittestutil/integration/os/network/network.go (1)
93-96: Consider using underscores for better readability.The large number could be more readable with underscores between groups of zeros.
-var PrefundedAccountInitialBalance, _ = sdkmath.NewIntFromString("100000000000000000000000") +var PrefundedAccountInitialBalance, _ = sdkmath.NewIntFromString("100_000_000_000_000_000_000_000")precompiles/gov/query_test.go (1)
648-688: LGTM! Well-structured test cases with proper error handling.The TestGetParams function follows good testing practices:
- Clear test case structure with descriptive names
- Proper error message validation
- Consistent use of test suite utilities
Consider adding test cases for:
- Different parameter types
- Edge cases with invalid parameter values
precompiles/gov/types.go (1)
614-623: Consider adding parameter type validation.While the function correctly validates the number of arguments, consider adding validation for the
ParamsTypefield to ensure it matches the expected governance parameter types.func BuildQueryParamsRequest(args []interface{}) (*govv1.QueryParamsRequest, error) { if len(args) != 0 { return nil, fmt.Errorf(cmn.ErrInvalidNumberOfArgs, 0, len(args)) } + // List of valid parameter types + validParamTypes := []string{"", "voting", "tallying", "deposit"} + paramType := "" + if len(args) > 0 { + var ok bool + paramType, ok = args[0].(string) + if !ok { + return nil, fmt.Errorf("invalid parameter type: expected string") + } + if paramType != "" && !slices.Contains(validParamTypes, paramType) { + return nil, fmt.Errorf("invalid parameter type: %s", paramType) + } + } return &govv1.QueryParamsRequest{ - ParamsType: "", + ParamsType: paramType, }, nil }testutil/integration/os/network/setup.go (1)
454-457: Consider adding validation for baseFee.The
FeeMarketCustomGenesisStatestruct should include validation to ensure thebaseFeeis non-negative.type FeeMarketCustomGenesisState struct { baseFee sdkmath.LegacyDec } + +func (f FeeMarketCustomGenesisState) Validate() error { + if f.baseFee.IsNegative() { + return fmt.Errorf("base fee cannot be negative: %s", f.baseFee) + } + return nil +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
example_chain/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumx/evm/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (66)
.github/workflows/bsr-push.yml(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/proto.yml(2 hunks)CHANGELOG.md(1 hunks)ante/evm/06_account_verification_test.go(3 hunks)ante/evm/07_can_transfer_test.go(3 hunks)ante/evm/08_gas_consume_test.go(7 hunks)ante/evm/09_increment_sequence_test.go(1 hunks)ante/evm/10_gas_wanted_test.go(4 hunks)ante/evm/suite_test.go(2 hunks)ethereum/eip712/eip712_legacy_test.go(1 hunks)ethereum/eip712/preprocess_test.go(1 hunks)example_chain/activators.go(1 hunks)example_chain/app.go(3 hunks)example_chain/config.go(3 hunks)example_chain/config_testing.go(3 hunks)example_chain/eips/testdata/Counter.json(1 hunks)example_chain/eips/testdata/CounterFactory.json(1 hunks)example_chain/go.mod(8 hunks)example_chain/osd/cmd/root.go(3 hunks)example_chain/precompiles.go(5 hunks)example_chain/test_helpers.go(2 hunks)example_chain/testutil/eth_setup.go(1 hunks)go.mod(8 hunks)precompiles/bank/testdata/BankCaller.json(1 hunks)precompiles/distribution/distribution.go(1 hunks)precompiles/evidence/IEvidence.sol(1 hunks)precompiles/evidence/abi.json(1 hunks)precompiles/evidence/errors.go(1 hunks)precompiles/evidence/events.go(1 hunks)precompiles/evidence/evidence.go(1 hunks)precompiles/evidence/query.go(1 hunks)precompiles/evidence/query_test.go(1 hunks)precompiles/evidence/setup_test.go(1 hunks)precompiles/evidence/tx.go(1 hunks)precompiles/evidence/tx_test.go(1 hunks)precompiles/evidence/types.go(1 hunks)precompiles/gov/IGov.sol(6 hunks)precompiles/gov/abi.json(1 hunks)precompiles/gov/gov.go(2 hunks)precompiles/gov/integration_test.go(6 hunks)precompiles/gov/query.go(2 hunks)precompiles/gov/query_test.go(5 hunks)precompiles/gov/testdata/GovCaller.json(1 hunks)precompiles/gov/testdata/GovCaller.sol(1 hunks)precompiles/gov/testdata/gov.go(1 hunks)precompiles/gov/types.go(1 hunks)precompiles/gov/utils_test.go(1 hunks)precompiles/staking/testdata/StakingCallerTwo.json(1 hunks)precompiles/testutil/contracts/FlashLoan.json(1 hunks)precompiles/testutil/contracts/StakingReverter.json(1 hunks)precompiles/werc20/integration_test.go(0 hunks)rpc/backend/chain_info_test.go(1 hunks)rpc/backend/evm_query_client_test.go(1 hunks)testutil/constants/constants.go(2 hunks)testutil/integration/os/network/amounts.go(1 hunks)testutil/integration/os/network/coins.go(2 hunks)testutil/integration/os/network/config.go(5 hunks)testutil/integration/os/network/config_test.go(3 hunks)testutil/integration/os/network/network.go(5 hunks)testutil/integration/os/network/setup.go(6 hunks)testutil/integration/os/utils/bank_test.go(2 hunks)testutil/integration/os/utils/gov.go(1 hunks)testutil/network/network.go(2 hunks)x/evm/types/msg_test.go(1 hunks)x/evm/types/precompiles.go(2 hunks)
💤 Files with no reviewable changes (1)
- precompiles/werc20/integration_test.go
✅ Files skipped from review due to trivial changes (9)
- precompiles/bank/testdata/BankCaller.json
- .github/workflows/bsr-push.yml
- example_chain/eips/testdata/Counter.json
- precompiles/testutil/contracts/FlashLoan.json
- precompiles/testutil/contracts/StakingReverter.json
- example_chain/eips/testdata/CounterFactory.json
- precompiles/staking/testdata/StakingCallerTwo.json
- precompiles/distribution/distribution.go
- .github/workflows/proto.yml
🧰 Additional context used
🪛 golangci-lint (1.62.2)
example_chain/activators.go
12-12: var evmosActivators is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (79)
precompiles/evidence/tx.go (2)
1-3: License Header Looks Good
The license is properly included and references the correct file.
14-22: Validate Additional Checks onNewMsgSubmitEvidenceUsage
The call toNewMsgSubmitEvidence(origin, args)is concise, but it's not immediately clear how thorough the validation is on the providedargs. If the message creation function doesn’t already perform all necessary validations (e.g., checking for nil or type mismatches), consider including them to prevent malformed evidence from proceeding.Also applies to: 23-26
precompiles/evidence/evidence.go (4)
1-3: License Header Matches Project Conventions
All references align with the Evmos licensing model.
37-41: Loading the ABI is Well-Handled
TheLoadABIfunction correctly handles file embedding and error reporting.
71-86: Preventing Panics inRequiredGas
The defensive check to avoid panics when input length < 4 is a good guard. The fallback to 0 gas usage is suitable when the method is unknown; however, consider ensuring invalid input scenarios quickly fail in theRunmethod to avoid unexpected calls draining zero gas.
142-145: Logger Method Follows Best Practices
Providing a dedicated logger scoping is beneficial for debugging precompiles.precompiles/evidence/errors.go (2)
1-3: License Declaration is Correct
All references comply with Evmos licensing guidelines.
6-11: Consider Using the Cosmos SDK Error Registry
These string constants provide clarity, but for cross-module error handling, you may want to integrate them with Cosmos SDK’ssdkerrorsor a localtypes/errors.gowith error codes. This approach can produce structured error messages and improve error matching logic across the codebase.Use this script to confirm if the constants are consistently referenced in the new features:
x/evm/types/precompiles.go (1)
19-19: LGTM! Evidence precompile address follows established patterns.The new
EvidencePrecompileAddressconstant follows the sequential pattern of other precompile addresses and is properly integrated into theAvailableStaticPrecompileslist.Also applies to: 36-36
precompiles/evidence/setup_test.go (1)
74-76: Document rationale for height restriction.The requirement for odd-numbered evidence heights seems arbitrary. If this is an intentional test restriction, please document the reasoning.
precompiles/evidence/IEvidence.sol (1)
7-7: Verify uniqueness of the precompile address.The precompile address
0x0000000000000000000000000000000000000807needs to be verified for uniqueness to avoid conflicts with other precompiles.✅ Verification successful
Precompile address 0x807 is unique and correctly allocated.
The address follows the sequential pattern of chain precompiles (0x800-0x807) and is consistently defined across the codebase with no conflicts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other usage of this address in the codebase rg -i "0x0000000000000000000000000000000000000807" --type go --type solLength of output: 99
Script:
#!/bin/bash # Search for hex address variations across all files rg -i "0x[0]*807\b" # Also search for the full address to catch any other format rg -i "0x0000000000000000000000000000000000000807" # Search for potential decimal representation rg -i "2055"Length of output: 104168
Script:
#!/bin/bash # Search for precompile addresses in the 0x800 range rg -i "0x0{16}80[0-9]" # Also check the Go precompiles file specifically cat x/evm/types/precompiles.goLength of output: 1518
example_chain/precompiles.go (1)
104-107: LGTM! Error handling follows the established pattern.The initialization and error handling for the evidence precompile follows the same pattern as other precompiles in the file.
precompiles/evidence/query_test.go (2)
1-14: LGTM! Well-organized imports.The imports are properly structured and include all necessary dependencies for testing the evidence precompile functionality.
16-120: Consider documenting gas limit rationale and adding edge cases.While the test coverage is comprehensive, consider the following improvements:
- Document why 200000 gas was chosen as the limit
- Add edge cases for:
- Maximum values for height and power
- Special characters in consensus address
- Different time zones
Verify gas consumption across different scenarios.
Let's analyze the gas usage patterns to ensure the limit is appropriate:
precompiles/evidence/abi.json (3)
1-24: LGTM! Well-structured event definition.The SubmitEvidence event is properly defined with:
- Indexed submitter address for efficient filtering
- Non-indexed hash field for evidence identification
192-197: LGTM! Correct interface contract structure.The empty bytecode and lack of link references is appropriate for an interface contract.
25-191: Consider adding input validation requirements in the Solidity interface.While the ABI correctly defines the function signatures, consider adding:
- @dev comments in the Solidity interface to document input validation requirements
- Value ranges for numeric fields (height, power)
- Format requirements for consensusAddress
Verify the interface implementation across the codebase.
Let's check for consistent usage:
example_chain/config_testing.go (5)
10-12: Imports look good.
No issues noted with these new import statements.
35-39: Clear separation of responsibilities.
Defining a dedicated function type (EvmosOptionsFn) helps ensure extensibility for future configuration needs.
40-44: Useful default implementation.
The no-op function is a convenient placeholder when no additional configuration is needed, keeping the codebase clean and reusable.
Line range hint
46-92: Consider safer chain ID parsing and concurrency implications.
- Calling
strings.Split(chainID, "-")[0]might throw off parsing if no dash is present inchainID. Consider a fallback mechanism or validation.- Globally resetting and reconfiguring might not be thread-safe if this function gets called in parallel. Confirm this code remains test-specific or ensure proper synchronization for production usage.
96-99: Documentation clarity.
This comment clarifies howsdk.RegisterDenomoverwrites the base denom units. It’s beneficial to highlight that partial reconfiguration may occur if multiple denominators are registered.precompiles/gov/testdata/GovCaller.sol (1)
7-11: EnsureGOV_CONTRACTis valid.
The function returns parameters by delegating the call toGOV_CONTRACT. Confirm thatGOV_CONTRACTis properly initialized and points to a valid contract address at runtime.precompiles/gov/testdata/gov.go (1)
11-13: Validate JSON artifact availability.
Loading the contract from a JSON file is straightforward but relies on the file being present. Ensure that “GovCaller.json” is consistently packaged or generated in the correct location.ante/evm/suite_test.go (1)
23-32: Well-structured test enhancement!The refactoring improves test coverage by:
- Testing all transaction types with multiple chain IDs
- Using predefined chain ID constants
- Maintaining a clean nested loop structure
testutil/integration/os/utils/bank_test.go (1)
31-38: Great improvement in balance testing!The changes enhance the test robustness by:
- Using dynamic initial amounts based on decimal precision
- Leveraging standardized decimal constants from evmtypes
- Supporting both 18 and 6 decimal configurations
ante/evm/09_increment_sequence_test.go (1)
17-17: LGTM! Chain ID configuration properly added.The addition of
WithChainID(suite.chainID)aligns with the standardization of chain ID configuration across test files, ensuring consistent test network initialization.testutil/constants/constants.go (1)
29-29: Verify chain ID and prefix changes across the codebase.The changes to
ExampleEIP155ChainID(9000 → 9001) andExampleChainIDPrefixformat ("evmos_%d" → "os_%d") represent significant updates that could impact various parts of the codebase.Let's verify that all references have been updated consistently:
Also applies to: 39-39
testutil/integration/os/network/coins.go (2)
43-48: LGTM! Well-structured utility function added.The new
getCoinInfofunction improves code reusability and follows the single responsibility principle by encapsulating the coin info conversion logic.
31-35: LGTM! Improved coin info handling.The refactored
DefaultChainCoinsfunction now uses the newgetCoinInfoutility, making the code more consistent and maintainable.example_chain/config.go (2)
18-27: LGTM! Well-designed configuration function type.The new
EvmosOptionsFntype andNoOpEvmosOptionsfunction provide a clean and flexible way to handle app-specific configurations.
87-89: LGTM! Improved documentation clarity.The updated comments better explain the behavior of
sdk.RegisterDenomregarding base denom overwriting.ante/evm/10_gas_wanted_test.go (1)
84-84: LGTM! Good error message improvement.The error message for fee market params update is now more descriptive, which helps in debugging test failures.
precompiles/gov/gov.go (1)
Line range hint
105-122: LGTM! The new query methods enhance the governance precompile functionality.The additions follow the established pattern for query methods and maintain proper separation between transactions and queries.
example_chain/test_helpers.go (2)
57-57: LGTM! Added EvmosAppOptions to NewExampleApp initialization.The change consistently applies the new configuration requirement.
133-133: LGTM! Consistent addition of EvmosAppOptions in SetupTestingApp.The change maintains consistency with the setup function implementation.
ante/evm/06_account_verification_test.go (3)
24-24: LGTM! Added chain ID to network configuration.Explicit chain ID configuration improves test clarity.
57-62: LGTM! Improved balance retrieval and validation.The changes enhance safety by:
- Using EVM-specific balance retrieval
- Adding explicit validation of the parsed balance
104-104: LGTM! Enhanced test case naming with chain ID.Including the chain ID in test names improves test output clarity.
testutil/integration/os/network/config_test.go (2)
22-27: LGTM! Improved test case structure with explicit fee and amount fields.The restructuring provides better separation of concerns between base fees and cosmos amounts.
66-102: LGTM! Enhanced balance and base fee validation.The changes improve test robustness by:
- Adding explicit sections for different validation types
- Including comprehensive base fee checks
- Maintaining proper decimal handling for both EVM and bank balances
precompiles/gov/query.go (2)
28-29: LGTM! Consistent method naming.The new constant follows the established naming pattern for precompile methods.
188-208: LGTM! Well-structured parameter query implementation.The implementation follows best practices:
- Proper error handling
- Consistent with other query methods
- Clean separation of concerns using the query server
testutil/integration/os/utils/gov.go (1)
22-22: LGTM! Improved precision handling for proposal submission.The adjustment of proposal submission amount using
evmtypes.GetEVMCoinDecimals().ConversionFactor()makes the test more robust by accounting for different EVM decimal configurations.Also applies to: 29-32
ethereum/eip712/preprocess_test.go (1)
79-79: LGTM! Chain ID update aligns with global changes.The change to TypedDataChainID from 9000 to 9001 maintains consistency with other chain ID updates across the codebase.
ante/evm/08_gas_consume_test.go (4)
23-23: LGTM! Explicit chain ID configuration in test setup.Adding explicit chain ID configuration to the network setup improves test clarity and maintainability.
Also applies to: 96-96
105-105: LGTM! Improved fee amount handling.Refactoring to use
sdkmath.Intfor fee amounts provides better precision control and type safety.Also applies to: 109-110, 117-118, 127-127
157-159: LGTM! Proper EVM decimals handling.The conversion of fee amounts using EVM decimals ensures accurate fee calculations across different EVM configurations.
152-156: LGTM! Enhanced balance verification.The balance checking logic has been improved with:
- Proper error handling for balance parsing
- Clear balance comparison messages
- Accurate balance calculations considering EVM decimals
Also applies to: 180-187
testutil/integration/os/network/config.go (3)
36-39: LGTM! Good addition of initialBondedAmount field.The addition of
initialBondedAmountfield of typemath.Intenhances the configuration capabilities for bonded amounts in the test network setup.
40-41: LGTM! Good addition of initialAmounts field.The addition of
initialAmountsfield improves the configuration structure by providing a dedicated type for managing initial amounts and fees.
85-89: LGTM! Good refactor of denomination decimals handling.The refactoring to use a loop for setting denomination decimals removes dependency on the experimental
mapspackage and provides a more straightforward implementation.example_chain/testutil/eth_setup.go (1)
84-84: LGTM! Good addition of EvmosAppOptions.The addition of
exampleapp.EvmosAppOptionsenhances the application's initialization by providing custom configuration options.precompiles/gov/IGov.sol (2)
73-91: LGTM! Well-structured Params struct for governance parameters.The new
Paramsstruct provides a comprehensive set of governance parameters including:
- Voting periods and thresholds
- Deposit requirements and periods
- Quorum and veto settings
- Burn configurations
225-227: LGTM! Good addition of getParams function.The new
getParamsfunction provides a clean interface to retrieve the current governance parameters.rpc/backend/chain_info_test.go (1)
157-157: LGTM! Chain ID updated to 9001.The expected chain ID has been updated from 9000 to 9001 to align with the new configuration.
testutil/integration/os/network/network.go (3)
101-108: LGTM! Good use of comments and dynamic calculation.The code correctly calculates the bonded amount based on the base decimals, improving flexibility. The comments provide good context for the configuration process.
143-146: LGTM! Good practice using dynamic base fee calculation.The code properly initializes the fee market parameters using the chain's base decimals, ensuring consistency with the chain's configuration.
289-289: Verify the impact of the field name change.The change from
otherCoinDenomtootherCoinDenomssuggests a switch from single to multiple denominations.✅ Verification successful
Field name is consistently used as plural throughout the codebase
The field
otherCoinDenomsis correctly defined as[]stringand used consistently in its plural form across the codebase. No instances of the singular form were found, suggesting this was either always plural or was completely refactored.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any other references to the old field name rg "otherCoinDenom[^s]" --type goLength of output: 35
Script:
#!/bin/bash # Search for both singular and plural forms echo "=== Searching for both forms ===" rg "otherCoinDenom" --type go -A 2 -B 2 # Search for struct definitions containing this field echo -e "\n=== Searching for struct definitions ===" ast-grep --pattern 'type $_ struct { $$$ otherCoinDenom$_ $$$ }'Length of output: 2011
example_chain/osd/cmd/root.go (1)
68-68: LGTM! Consistent application of EvmosAppOptions.The changes consistently add EvmosAppOptions to all NewExampleApp calls, improving the application's configuration capabilities. The modification is applied uniformly across different initialization paths.
Also applies to: 370-370, 405-405, 411-411
precompiles/gov/types.go (2)
573-591: LGTM! Well-structured governance parameters struct.The
ParamsOutputstruct is well-organized with appropriate types and consistent naming conventions for all governance parameters.
593-612: LGTM! Comprehensive parameter mapping.The
FromResponsemethod correctly maps all governance parameters from the response to the struct fields, maintaining type safety.testutil/integration/os/network/setup.go (1)
446-449: LGTM! Proper scaling of deposit amounts.The calculation correctly uses the EVM coin decimals conversion factor to scale the minimum deposit amount.
testutil/network/network.go (2)
110-110: LGTM! Consistent app options usage.The
EvmosAppOptionsparameter is correctly added to theNewExampleAppcall, maintaining consistency with the app's configuration requirements.
143-143: LGTM! Consistent app constructor configuration.The
EvmosAppOptionsparameter is properly added to the app constructor, aligning with the app's initialization requirements.x/evm/types/msg_test.go (1)
57-57: LGTM! Updated to use new configuration method.The change correctly updates the app configuration call to use
EvmosAppOptions, maintaining the same functionality while aligning with the new configuration pattern.precompiles/gov/integration_test.go (3)
11-11: LGTM: Import alias change improves clarity.The rename from
v1togovv1makes the code more explicit about the governance types being used.
498-499: LGTM: Consistent usage of renamed import.The status checks have been correctly updated to use the new
govv1package reference.Also applies to: 559-559, 588-589
663-748: Well-structured test implementation for params query.The new test context for params query is comprehensive and follows good testing practices:
- Proper setup with contract deployment
- Thorough parameter validation
- Both direct and contract-based call testing
example_chain/app.go (3)
234-236: LGTM: Enhanced app configuration flexibility.The addition of
evmosAppOptionsparameter allows for more flexible application configuration during initialization.
578-578: LGTM: Evidence keeper integration with EVM precompiles.The addition of EvidenceKeeper to static precompiles extends the EVM's capabilities to handle evidence-related operations.
284-286: Verify error handling in initialization.The error from evmosAppOptions is properly handled by panicking, which is appropriate for initialization-time errors.
✅ Verification successful
Error handling pattern is consistent and appropriate
The use of
panic()for initialization-time errors is consistently applied throughout the codebase and follows standard Go practices for handling unrecoverable startup errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other initialization error handling patterns rg -A 2 'panic\(' example_chain/Length of output: 4670
.github/workflows/lint.yml (1)
29-29: LGTM: Dependency version bump.Update of golangci-lint-action to v6.2.0 keeps the workflow up to date with the latest improvements.
CHANGELOG.md (1)
11-11: LGTM: Clear and well-formatted changelog entry.The new entry follows the established format and clearly indicates the scope and nature of the change.
go.mod (2)
228-228: Verify impact of major OpenTelemetry update.OpenTelemetry packages have been updated from v1.24.0 to v1.31.0, which is a significant version jump. Please ensure:
- All OpenTelemetry instrumentation is compatible with the new version
- No breaking changes in the metrics or tracing APIs
- Migration guide has been followed if required
Also applies to: 229-229, 230-230
10-10: Verify compatibility with updated Cosmos SDK dependencies.The following core dependencies have been updated:
cosmossdk.io/mathfrom v1.4.0 to v1.5.0cosmossdk.io/x/txfrom v0.13.5 to v0.13.7github.com/cosmos/cosmos-dbfrom v1.0.2 to v1.1.1github.com/cosmos/cosmos-sdkfrom v0.50.10 to v0.50.11Please ensure there are no breaking changes in these updates that could affect the codebase.
Also applies to: 15-15, 19-19, 21-21
✅ Verification successful
Dependencies update appears safe to proceed
The changes are well-contained:
- Cosmos SDK v0.50.11 includes security fixes (ABS-0043/ABS-0044) with no breaking changes
- cosmos-db v1.1.1 contains minor refactoring
- Other updates follow semantic versioning with minor/patch bumps, indicating backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for breaking changes in the updated dependencies # Function to fetch changelog or release notes fetch_changes() { local repo=$1 local old_ver=$2 local new_ver=$3 echo "Checking changes for $repo from $old_ver to $new_ver:" gh api repos/$repo/releases --jq ".[] | select(.tag_name == \"$new_ver\") | .body" } # Check Cosmos SDK changes fetch_changes "cosmos/cosmos-sdk" "v0.50.10" "v0.50.11" # Check cosmos-db changes fetch_changes "cosmos/cosmos-db" "v1.0.2" "v1.1.1"Length of output: 2088
Script:
#!/bin/bash # Check release notes for math and tx modules echo "Checking cosmossdk.io/math changes:" gh api repos/cosmos/cosmos-sdk/releases --jq ".[] | select(.tag_name == \"math/v1.5.0\") | .body" echo -e "\nChecking cosmossdk.io/x/tx changes:" gh api repos/cosmos/cosmos-sdk/releases --jq ".[] | select(.tag_name == \"tx/v0.13.7\") | .body" echo -e "\nChecking cosmos-db commits:" git clone --quiet https://github.com/cosmos/cosmos-db.git cd cosmos-db git log --oneline v1.0.2..v1.1.1Length of output: 570
Script:
#!/bin/bash # Check math module repository echo "Checking math module changes:" gh api repos/cosmos/math/releases --jq ".[] | select(.tag_name == \"v1.5.0\") | .body" echo -e "\nChecking tx module repository:" gh api repos/cosmos/x-tx/releases --jq ".[] | select(.tag_name == \"v0.13.7\") | .body" echo -e "\nChecking cosmos-db changes (alternative approach):" gh api repos/cosmos/cosmos-db/commits --jq ".[].commit.message" | head -n 10Length of output: 1157
Script:
#!/bin/bash echo "Checking Cosmos SDK repository for math and tx module changes:" git clone --quiet https://github.com/cosmos/cosmos-sdk.git cd cosmos-sdk echo -e "\nChecking math module changes:" git log --oneline -- math/ | head -n 10 echo -e "\nChecking x/tx module changes:" git log --oneline -- x/tx/ | head -n 10Length of output: 423
example_chain/go.mod (1)
26-26: Verify test compatibility with updated test frameworks.The test framework dependencies have been updated:
github.com/onsi/ginkgo/v2from v2.22.0 to v2.22.2github.com/onsi/gomegafrom v1.35.1 to v1.36.2Please ensure all tests pass with the new versions and check for any deprecated test assertions or syntax.
Also applies to: 27-27
✅ Verification successful
Test framework updates are safe to apply
The updates to Ginkgo (v2.22.0 → v2.22.2) and Gomega (v1.35.1 → v1.36.2) are maintenance releases containing only dependency updates and security patches. No breaking changes or deprecated features were introduced that would affect existing tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for Ginkgo/Gomega usage and potential deprecated patterns # Search for test files using Ginkgo/Gomega echo "Searching for test files using Ginkgo/Gomega:" rg --type go "github.com/onsi/(ginkgo|gomega)" # Search for potential deprecated patterns echo "Checking for deprecated patterns:" rg --type go "Eventually|Consistently|BeTrue|BeFalse|HaveOccurred|Not"Length of output: 65798
Script:
#!/bin/bash # Get changelog/release notes for the updated versions echo "Checking Ginkgo v2.22.2 release:" gh release view v2.22.2 --repo onsi/ginkgo echo -e "\nChecking Gomega v1.36.2 release:" gh release view v1.36.2 --repo onsi/gomegaLength of output: 1570
precompiles/gov/testdata/GovCaller.json (1)
1-130: LGTM! Well-structured governance parameter contract.The contract artifact is well-defined with:
- Proper view function modifier for parameter queries
- Comprehensive governance parameter structure
- Correct type definitions for all parameters
precompiles/gov/abi.json (1)
218-336: LGTM! Consistent ABI definition for governance parameters.The getParams function ABI is well-defined and consistent with the contract implementation:
- Proper view function modifier
- Comprehensive parameter structure matching the contract
- Correct type definitions for all parameters
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (30)
precompiles/gov/utils_test.go (2)
25-33: Consider adding field documentation.While the struct itself is documented, individual fields lack documentation. Consider adding comments for each field to improve code maintainability.
type CallsData struct { + // precompileAddr is the address of the precompile contract precompileAddr common.Address + // precompileABI is the ABI of the precompile contract precompileABI abi.ABI + // precompileCallerAddr is the address of the contract calling the precompile precompileCallerAddr common.Address + // precompileCallerABI is the ABI of the contract calling the precompile precompileCallerABI abi.ABI }
35-58: Fix function name inconsistency and document argument modification.
- The function name in the comment (getTxCallArgs) doesn't match the actual function name (getTxAndCallArgs).
- The function modifies input arguments which should be documented.
-// getTxCallArgs is a helper function to return the correct call arguments and -// transaction data for a given call type. +// getTxAndCallArgs is a helper function that modifies and returns the provided call arguments +// and transaction data based on the given call type. Note: This function modifies the input +// arguments. func (cd CallsData) getTxAndCallArgs(precompiles/gov/query.go (1)
188-208: Document input validation requirements.While the implementation is correct, it would be helpful to document the expected format and validation rules for the input arguments that BuildQueryParamsRequest handles.
-// GetParams implements the query logic for getting governance parameters +// GetParams implements the query logic for getting governance parameters. +// The function expects no arguments as it returns all governance parameters. +// Returns an error if the BuildQueryParamsRequest fails or if the query server +// encounters an error. func (p *Precompile) GetParams(precompiles/gov/testdata/GovCaller.json (1)
6-124: Consider adding input validation for numeric parameters.The governance parameters include several numeric values (
votingPeriod,maxDepositPeriod, etc.) and ratios stored as strings. While the types are appropriate, consider:
- Adding validation for the numeric parameters to ensure they're within acceptable ranges
- Documenting the expected format and constraints for ratio strings (e.g., decimal places, valid ranges)
precompiles/gov/abi.json (1)
218-336: Add NatSpec documentation for the getParams function.The
getParamsfunction is well-structured and correctly integrated into the interface. However, consider adding NatSpec documentation to:
- Describe the purpose and usage of each parameter in the return tuple
- Specify any constraints or valid ranges for numeric values
- Document the format of ratio strings
Example NatSpec documentation:
/// @notice Retrieves the current governance parameters /// @return params A tuple containing: /// - votingPeriod: Duration of the voting period in seconds /// - minDeposit: Array of coins required as minimum deposit /// - maxDepositPeriod: Maximum duration for deposit collection in seconds /// - quorum: Required participation ratio (format: decimal string, e.g., "0.334") /// [continue with other parameters...] function getParams() external view returns (Params memory params);example_chain/config_testing.go (1)
96-99: Ensure ignoring denom registration errors is intentional.
InsetBaseDenom, you deferSetBaseDenom(ci.Denom)even whenRegisterDenomfails due to it being already registered. Confirm that ignoring the error in these scenarios is indeed desired.precompiles/evidence/query.go (1)
70-82: Potential large result sets could strain memory usage.
For extremely large evidence data, returning all in one go might be inefficient. Consider streaming or chunked reads if you anticipate massive blocks of evidence.precompiles/evidence/evidence.go (2)
43-59: Initialize default expiration as a config item.
You introduced a fallback forApprovalExpiration. Consider allowing injection of a custom duration if you foresee different approval policies across environments.
129-140: Transaction vs. query detection is concise.
Switching onSubmitEvidenceMethodalone is clear. If more transactions are introduced, consider an expanded approach or automated mapping.precompiles/evidence/tx.go (2)
15-22: Consider adding input validation for args.The function accepts
args []interface{}without explicit validation. Consider adding validation before passing toNewMsgSubmitEvidence.
28-32: Consider adding context timeout handling.The context passed to
msgServer.SubmitEvidenceshould be checked for cancellation or timeout to ensure proper resource cleanup.Example:
+ if ctx.Err() != nil { + return nil, ctx.Err() + } res, err := msgServer.SubmitEvidence(ctx, msg)precompiles/evidence/events.go (1)
21-51: Consider adding event documentation and parameter validation.The event emission implementation is solid, but could benefit from additional documentation and validation:
- Consider adding documentation for the event structure in the ABI
- Add validation for the evidenceHash parameter to ensure it's not empty or malformed
// EmitSubmitEvidenceEvent creates a new event emitted on a SubmitEvidence transaction. +// The event has the following structure in the ABI: +// event SubmitEvidence(address indexed sender, bytes evidenceHash) func (p Precompile) EmitSubmitEvidenceEvent(ctx sdk.Context, stateDB vm.StateDB, origin common.Address, evidenceHash []byte) error { + // Validate input parameters + if len(evidenceHash) == 0 { + return errors.New("evidence hash cannot be empty") + } + // Prepare the event topics event := p.ABI.Events[EventTypeSubmitEvidence]precompiles/evidence/tx_test.go (1)
29-29: Document gas estimation basis.The gas value of 200000 appears arbitrary. Consider adding a comment explaining how this value was determined.
Also applies to: 55-55
precompiles/evidence/setup_test.go (2)
74-76: Document the odd height requirement.The requirement for evidence height to be odd seems arbitrary. Please add documentation explaining the reasoning behind this validation rule.
56-61: Improve error handling in SetupTest.Using panic for initialization errors in tests makes debugging harder. Consider using
s.Require()instead.- if s.precompile, err = evidence.NewPrecompile( - s.network.App.EvidenceKeeper, - s.network.App.AuthzKeeper, - ); err != nil { - panic(err) - } + var err error + s.precompile, err = evidence.NewPrecompile( + s.network.App.EvidenceKeeper, + s.network.App.AuthzKeeper, + ) + s.Require().NoError(err)precompiles/evidence/IEvidence.sol (2)
15-15: Consider using uint64 for consistency.Using
int64forheightandpowermight lead to unexpected behavior with negative values. Consider usinguint64if negative values are not meaningful in this context.Also applies to: 19-19
47-50: Consider adding events for pagination queries.While there's an event for evidence submission, adding events for pagination queries could help with off-chain tracking and indexing.
testutil/integration/os/network/amounts.go (2)
35-47: Consider parameterizing the initial balance.The function uses a hardcoded initial balance of 100 quintillion (in 18 decimals). Consider making this value configurable to support different use cases.
-func GetInitialAmount(decimals evmtypes.Decimals) math.Int { +func GetInitialAmount(decimals evmtypes.Decimals, initialBalance math.Int) math.Int { if err := decimals.Validate(); err != nil { panic("unsupported decimals") } - - // initialBalance defines the initial balance represented in 18 decimals. - initialBalance, _ := math.NewIntFromString("100_000_000_000_000_000_000_000") // 18 decimals is the most precise representation we can have, for this // reason we have to divide the initial balance by the decimals value to // have the specific representation. return initialBalance.Quo(decimals.ConversionFactor()) }
62-75: Consider a more flexible approach for base fee calculation.The function only supports 18 and 6 decimals, which could limit future use cases. Consider implementing a more flexible calculation based on the number of decimals.
func GetInitialBaseFeeAmount(decimals evmtypes.Decimals) math.LegacyDec { if err := decimals.Validate(); err != nil { panic("unsupported decimals") } - 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") - } + // Calculate base fee based on decimals + if decimals >= evmtypes.EighteenDecimals { + return math.LegacyNewDec(1_000_000_000) + } + // For lower decimals, scale down proportionally + scale := evmtypes.EighteenDecimals - decimals + return math.LegacyNewDecWithPrec(1, uint64(3*scale/6)) }precompiles/evidence/types.go (4)
17-25: Consider adding method descriptions to constants.While the constant names are self-explanatory, adding detailed method descriptions would improve documentation.
const ( - // SubmitEvidenceMethod defines the ABI method name for the evidence SubmitEvidence - // transaction. + // SubmitEvidenceMethod defines the ABI method name for submitting evidence to the chain. + // This method allows validators to submit evidence of malicious behavior. SubmitEvidenceMethod = "submitEvidence" - // EvidenceMethod defines the ABI method name for the Evidence query. + // EvidenceMethod defines the ABI method name for querying specific evidence by hash. + // This method returns detailed information about a specific evidence submission. EvidenceMethod = "evidence" - // GetAllEvidenceMethod defines the ABI method name for the GetAllEvidence query. + // GetAllEvidenceMethod defines the ABI method name for querying all submitted evidence. + // This method returns a paginated list of all evidence submissions. GetAllEvidenceMethod = "getAllEvidence" )
46-50: Consider adding validation tags for struct fields.The
EquivocationDatastruct could benefit from validation tags to ensure data integrity.type EquivocationData struct { - Height int64 `abi:"height"` - Time uint64 `abi:"time"` - Power int64 `abi:"power"` - ConsensusAddress string `abi:"consensusAddress"` + Height int64 `abi:"height" validate:"required,min=1"` + Time uint64 `abi:"time" validate:"required"` + Power int64 `abi:"power" validate:"required"` + ConsensusAddress string `abi:"consensusAddress" validate:"required,bech32"` }
56-57: Add comment explaining time conversion.The time conversion logic could benefit from a comment explaining the UTC conversion and the gosec linter directive.
- Time: time.Unix(int64(e.Time), 0).UTC(), //nolint:gosec // G115 + // Convert Unix timestamp to UTC time. G115 linter warning is suppressed as the timestamp + // is validated before reaching this point. + Time: time.Unix(int64(e.Time), 0).UTC(), //nolint:gosec // G115: Potential time.Time injection
62-86: Consider enhancing error messages and input validation.The
NewMsgSubmitEvidencefunction could benefit from more descriptive error messages and additional input validation.func NewMsgSubmitEvidence(origin common.Address, args []interface{}) (*evidencetypes.MsgSubmitEvidence, error) { if len(args) != 1 { - return nil, fmt.Errorf(cmn.ErrInvalidNumberOfArgs, 1, len(args)) + return nil, fmt.Errorf("%w: expected 1 argument for evidence submission, got %d", cmn.ErrInvalidNumberOfArgs, len(args)) } equivocation, ok := args[0].(EquivocationData) if !ok { - return nil, fmt.Errorf("invalid equivocation evidence") + return nil, fmt.Errorf("invalid evidence type: expected EquivocationData, got %T", args[0]) + } + + // Add basic validation for equivocation data + if equivocation.Height <= 0 { + return nil, fmt.Errorf("invalid height: must be positive") + } + if equivocation.Power <= 0 { + return nil, fmt.Errorf("invalid power: must be positive") + } + if equivocation.ConsensusAddress == "" { + return nil, fmt.Errorf("invalid consensus address: must not be empty") } // Convert the EquivocationData to a types.Equivocation evidence := equivocation.ToEquivocation()ante/evm/07_can_transfer_test.go (1)
49-54: Consider adding error message assertions.The balance retrieval logic is correct, but error handling could be more descriptive.
balanceResp, err := grpcHandler.GetBalanceFromEVM(senderKey.AccAddr) - suite.Require().NoError(err) + suite.Require().NoError(err, "failed to retrieve balance from EVM") balance, ok := math.NewIntFromString(balanceResp.Balance) - suite.Require().True(ok) + suite.Require().True(ok, "failed to parse balance string to Int")ante/evm/10_gas_wanted_test.go (1)
76-84: Consider extracting fee market params update to a helper function.The fee market params update logic could be moved to a helper function to improve test readability.
+func (suite *EvmAnteTestSuite) updateFeeMarketParams(txFactory *factory.Factory, network *network.UnitTestNetwork, privKey *ethsecp256k1.PrivKey, noBaseFee bool) error { + feeMarketParams, err := grpcHandler.GetFeeMarketParams() + if err != nil { + return fmt.Errorf("failed to get fee market params: %w", err) + } + + feeMarketParams.Params.NoBaseFee = noBaseFee + return integrationutils.UpdateFeeMarketParams(integrationutils.UpdateParamsInput{ + Tf: txFactory, + Network: network, + Pk: privKey, + Params: feeMarketParams.Params, + }) +}Then use it in the test:
- feeMarketParams, err := grpcHandler.GetFeeMarketParams() - suite.Require().NoError(err) - - feeMarketParams.Params.NoBaseFee = true - err = integrationutils.UpdateFeeMarketParams(integrationutils.UpdateParamsInput{ - Tf: txFactory, - Network: unitNetwork, - Pk: keyring.GetPrivKey(0), - Params: feeMarketParams.Params, - }) - suite.Require().NoError(err, "expected no error when updating fee market params") + err := suite.updateFeeMarketParams(txFactory, unitNetwork, keyring.GetPrivKey(0), true) + suite.Require().NoError(err, "failed to update fee market params")example_chain/precompiles.go (1)
50-50: Consider adding evidence precompile documentation.The evidence precompile initialization is correct, but could benefit from documentation explaining its purpose and configuration.
+ // Initialize the evidence precompile which allows for submitting and querying + // validator misbehavior evidence. This precompile requires both the evidence + // keeper for managing evidence and the authz keeper for authorization checks. evidencePrecompile, err := evidenceprecompile.NewPrecompile(evidenceKeeper, authzKeeper) if err != nil { panic(fmt.Errorf("failed to instantiate evidence precompile: %w", err)) }Also applies to: 104-107, 120-120
precompiles/evidence/query_test.go (1)
72-80: Consider adding more edge cases for evidence data.While the current test covers basic functionality, consider adding test cases for:
- Evidence with maximum allowed values
- Evidence with zero values
- Evidence with malformed consensus addresses
testutil/integration/os/network/config.go (1)
22-24: Consider adding documentation for the default chain ID value.While the variable name is descriptive, it would be helpful to document why this specific chain ID is chosen as the default.
testutil/integration/os/network/setup.go (1)
11-12: Consider using a more stable alternative to golang.org/x/exp/maps.While
golang.org/x/exp/mapsprovides useful map operations, it's an experimental package. Consider implementing the map keys functionality directly or waiting for these features to be included in the standard library.-import ( - "golang.org/x/exp/maps" -) +// Alternative implementation: +func getMapKeys[K comparable, V any](m map[K]V) []K { + keys := make([]K, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + return keys +}precompiles/evidence/abi.json (1)
25-191: Add NatSpec documentation for better developer experience.While the function definitions are correct, adding NatSpec documentation would improve the contract's usability.
Add documentation in the Solidity interface file:
/// @notice Retrieves evidence details for a given hash /// @param evidenceHash The hash of the evidence to query /// @return evidence The equivocation details function evidence(bytes calldata evidenceHash) external view returns (Equivocation memory evidence); /// @notice Retrieves all evidence with pagination /// @param pageRequest The pagination parameters /// @return evidence Array of equivocation details /// @return pageResponse Pagination result details function getAllEvidence(PageRequest calldata pageRequest) external view returns (Equivocation[] memory evidence, PageResponse memory pageResponse); /// @notice Submits new evidence of equivocation /// @param evidence The equivocation details to submit /// @return success True if the submission was successful function submitEvidence(Equivocation calldata evidence) external returns (bool success);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
example_chain/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumx/evm/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (66)
.github/workflows/bsr-push.yml(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/proto.yml(2 hunks)CHANGELOG.md(1 hunks)ante/evm/06_account_verification_test.go(3 hunks)ante/evm/07_can_transfer_test.go(3 hunks)ante/evm/08_gas_consume_test.go(7 hunks)ante/evm/09_increment_sequence_test.go(1 hunks)ante/evm/10_gas_wanted_test.go(4 hunks)ante/evm/suite_test.go(2 hunks)ethereum/eip712/eip712_legacy_test.go(1 hunks)ethereum/eip712/preprocess_test.go(1 hunks)example_chain/activators.go(1 hunks)example_chain/app.go(3 hunks)example_chain/config.go(3 hunks)example_chain/config_testing.go(3 hunks)example_chain/eips/testdata/Counter.json(1 hunks)example_chain/eips/testdata/CounterFactory.json(1 hunks)example_chain/go.mod(8 hunks)example_chain/osd/cmd/root.go(3 hunks)example_chain/precompiles.go(5 hunks)example_chain/test_helpers.go(2 hunks)example_chain/testutil/eth_setup.go(1 hunks)go.mod(8 hunks)precompiles/bank/testdata/BankCaller.json(1 hunks)precompiles/distribution/distribution.go(1 hunks)precompiles/evidence/IEvidence.sol(1 hunks)precompiles/evidence/abi.json(1 hunks)precompiles/evidence/errors.go(1 hunks)precompiles/evidence/events.go(1 hunks)precompiles/evidence/evidence.go(1 hunks)precompiles/evidence/query.go(1 hunks)precompiles/evidence/query_test.go(1 hunks)precompiles/evidence/setup_test.go(1 hunks)precompiles/evidence/tx.go(1 hunks)precompiles/evidence/tx_test.go(1 hunks)precompiles/evidence/types.go(1 hunks)precompiles/gov/IGov.sol(6 hunks)precompiles/gov/abi.json(1 hunks)precompiles/gov/gov.go(2 hunks)precompiles/gov/integration_test.go(6 hunks)precompiles/gov/query.go(2 hunks)precompiles/gov/query_test.go(5 hunks)precompiles/gov/testdata/GovCaller.json(1 hunks)precompiles/gov/testdata/GovCaller.sol(1 hunks)precompiles/gov/testdata/gov.go(1 hunks)precompiles/gov/types.go(1 hunks)precompiles/gov/utils_test.go(1 hunks)precompiles/staking/testdata/StakingCallerTwo.json(1 hunks)precompiles/testutil/contracts/FlashLoan.json(1 hunks)precompiles/testutil/contracts/StakingReverter.json(1 hunks)precompiles/werc20/integration_test.go(0 hunks)rpc/backend/chain_info_test.go(1 hunks)rpc/backend/evm_query_client_test.go(1 hunks)testutil/constants/constants.go(2 hunks)testutil/integration/os/network/amounts.go(1 hunks)testutil/integration/os/network/coins.go(2 hunks)testutil/integration/os/network/config.go(5 hunks)testutil/integration/os/network/config_test.go(3 hunks)testutil/integration/os/network/network.go(5 hunks)testutil/integration/os/network/setup.go(6 hunks)testutil/integration/os/utils/bank_test.go(2 hunks)testutil/integration/os/utils/gov.go(1 hunks)testutil/network/network.go(2 hunks)x/evm/types/msg_test.go(1 hunks)x/evm/types/precompiles.go(2 hunks)
💤 Files with no reviewable changes (1)
- precompiles/werc20/integration_test.go
✅ Files skipped from review due to trivial changes (7)
- precompiles/bank/testdata/BankCaller.json
- precompiles/testutil/contracts/StakingReverter.json
- .github/workflows/bsr-push.yml
- precompiles/distribution/distribution.go
- .github/workflows/proto.yml
- precompiles/staking/testdata/StakingCallerTwo.json
- precompiles/testutil/contracts/FlashLoan.json
🧰 Additional context used
🪛 golangci-lint (1.62.2)
example_chain/activators.go
12-12: var evmosActivators is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (103)
precompiles/gov/testdata/gov.go (3)
1-2: LGTM! License header is properly formatted.The copyright notice and SPDX license identifier are correctly specified.
4-9: LGTM! Package and imports are well-structured.The package name follows Go conventions for test data, and imports are properly organized with clear aliases.
11-13: Add documentation and verify contract file existence.While the implementation is straightforward, consider the following improvements:
- Add godoc comments describing the function's purpose and return values
- Verify that "GovCaller.json" exists in the expected location
Let's verify the contract file existence and its references:
example_chain/eips/testdata/Counter.json (1)
34-35: Verify bytecode compatibility with existing deployments.The contract bytecode has been updated while maintaining the same interface. The new bytecode includes proper arithmetic checks and is compiled with Solidity 0.8.20.
Run the following script to check for any existing deployments that might be affected:
✅ Verification successful
Bytecode changes are safe - test-only contract confirmed
The Counter contract is a test utility located in
example_chain/eips/testdata/with no production deployments. The updated bytecode is compatible with the contract's version constraints (>=0.7.0 <0.9.0).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for Counter contract deployments and version references # Look for deployment scripts or configuration files referencing the Counter contract rg -l "Counter\.(json|sol)" --type yaml --type javascript --type typescript # Look for hardhat/truffle artifacts that might have cached the old bytecode fd -g "*artifact*json" -x grep -l "Counter"Length of output: 156
Script:
#!/bin/bash # Search for Counter contract references with explicit file extensions rg -l "Counter" --type=ts --type=js --type=json --type=yaml # Find all Solidity files referencing Counter fd -e sol -x grep -l "Counter" {} # Look for deployment addresses or configurations rg -l "deploy.*Counter" --type=ts --type=js # Search for version constraints in package.json and hardhat config fd -e json -e js -e ts -g "*config*" -x grep -l "solidity.*0\.8\."Length of output: 949
Script:
#!/bin/bash # Check the Counter.sol source in example_chain/eips/testdata cat example_chain/eips/testdata/Counter.sol # Look for any test files using this specific Counter implementation rg -l "example_chain/eips/testdata/Counter" --type=ts --type=jsLength of output: 362
example_chain/eips/testdata/CounterFactory.json (1)
52-53: Verify CounterFactory deployment and Counter contract interaction.The CounterFactory bytecode has been updated and includes:
- Embedded Counter contract bytecode matching the new version
- Proper error handling for contract creation and method calls
Run the following script to verify contract dependencies and deployment order:
✅ Verification successful
Test contracts are properly coordinated
The Counter and CounterFactory contracts are test artifacts in the EIPs test suite, where both contracts are updated together ensuring bytecode compatibility. Being test contracts, they don't require strict deployment ordering as they're deployed fresh for each test.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify contract dependencies and deployment configurations # Check deployment scripts for proper deployment order echo "Checking deployment scripts..." rg -A 5 -B 5 "deploy.*Counter(Factory)?" --type javascript --type typescript # Verify that Counter.json and CounterFactory.json are always updated together echo "Checking git history for coordinated updates..." git log --oneline -p -- "**/Counter.json" "**/CounterFactory.json"Length of output: 289
Script:
#!/bin/bash # Find deployment scripts and contract artifacts echo "Searching for deployment files..." fd -e js -e ts deploy echo "Checking deployment content..." rg -l "Counter" $(fd -e js -e ts deploy) echo "Verifying contract artifacts..." fd -e json CounterLength of output: 1708
precompiles/gov/testdata/GovCaller.sol (3)
1-3: LGTM! License and pragma version are appropriate.The LGPL-3.0-only license is correctly specified, and the Solidity version pragma is appropriately set to >=0.8.18, which includes important safety features.
5-5: LGTM! Import statement is correct.The import statement correctly references the IGov interface from the parent directory.
7-11: LGTM! Contract implementation follows best practices.The contract is well-structured with:
- Appropriate use of
external viewmodifier for gas efficiency- Correct return type using
memoryfor the Params struct- Clean delegation to the GOV_CONTRACT interface
precompiles/gov/utils_test.go (1)
16-23: LGTM! Type and constant declarations are clear.The callType enum and its constants are well-defined and documented.
precompiles/gov/gov.go (1)
105-105: LGTM! GetParams method integration is correct.The GetParams method is properly integrated into the Run method's switch statement, following the established pattern for query methods.
Also applies to: 121-122
precompiles/gov/query.go (1)
28-29: LGTM! Constant follows naming convention.The GetParamsMethod constant is properly defined and follows the established naming pattern.
precompiles/gov/IGov.sol (2)
73-91: LGTM! Well-structured governance parameters.The
Paramsstruct is well-organized with appropriate types for each parameter and clear field names. The structure comprehensively covers all governance parameters including voting periods, deposits, thresholds, and burn settings.
225-227: LGTM! Well-documented query function.The
getParams()function is well-defined with clear NatSpec documentation and appropriate return type.precompiles/gov/query_test.go (2)
36-43: LGTM! Improved test case structure.The test cases for GetVotes have been enhanced with better error handling and clearer expectations. The addition of
errContainsfield improves test readability and maintainability.
648-688: LGTM! Comprehensive test coverage for GetParams.The test suite for GetParams is well-structured with:
- Clear test case organization
- Both positive and negative test scenarios
- Proper error message validation
precompiles/gov/types.go (3)
573-591: LGTM! Well-structured output type.The
ParamsOutputstruct is well-defined with:
- Appropriate types for each field
- Clear ABI tags for serialization
- Comprehensive coverage of all governance parameters
593-612: LGTM! Thorough response handling.The
FromResponsemethod correctly handles the conversion of all fields, including proper handling of time durations to nanoseconds.
614-623: LGTM! Clear and focused request builder.The
BuildQueryParamsRequestfunction is well-implemented with proper validation of input arguments.precompiles/gov/integration_test.go (1)
663-748: LGTM! Comprehensive integration tests for params query.The test suite is well-structured with:
- Proper setup of test environment
- Tests for both direct precompile calls and contract calls
- Thorough validation of all governance parameters
- Clear error handling and expectations
precompiles/gov/testdata/GovCaller.json (2)
1-5: LGTM! Contract metadata is well-structured.The contract metadata follows the Hardhat artifact format and correctly specifies the contract name and source path.
126-127: Verify the bytecode matches the source code.The bytecode appears to be valid EVM bytecode. However, it's important to verify that it matches the source code compilation.
Run the following script to verify the bytecode:
✅ Verification successful
Bytecode complexity aligns with contract functionality
The bytecode is valid for this contract as it contains the necessary logic to:
- Handle the complex
Paramsstruct with multiple fields- Make calls to the governance precompile at
0x805- Encode/decode ABI for struct types including dynamic arrays and strings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the bytecode matches the source code compilation. # Search for the source file fd -t f "GovCaller.sol" --exec cat {} \; # Search for any related test files that verify the bytecode fd -t f "GovCaller.*test.*" --exec cat {} \;Length of output: 329
Script:
#!/bin/bash # Find IGov interface and related files fd -t f "IGov.sol" --exec cat {} \; # Find potential constant definitions rg -l "GOV_CONTRACT" # Find compilation config files fd -e json -e toml "^hardhat\.config|^foundry\.toml" --exec cat {} \;Length of output: 7524
precompiles/gov/abi.json (1)
218-336: Verify integration with existing governance methods.The new
getParamsfunction complements the existing governance functionality. Ensure it's properly integrated with related methods likegetProposalandgetVotes.Run the following script to analyze the integration:
✅ Verification successful
The
getParamsfunction is correctly implemented as a standalone query methodThe function is properly integrated into the governance module and comprehensively tested. Its separation from
getProposalis by design since they serve different purposes - global parameters vs proposal-specific data.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration with existing governance methods. # Search for references to governance parameters in the codebase rg -A 5 "votingPeriod|minDeposit|maxDepositPeriod|quorum|threshold|vetoThreshold" # Search for files that might need updates to use the new getParams function ast-grep --pattern 'function $func($$$) { $$$ getProposal($$$) $$$ }'Length of output: 13139
example_chain/config_testing.go (4)
10-11: Imports look clean and purposeful.
No concerns with adding"fmt"and"strings"as they are directly used for error reporting and string manipulation.
35-39: Good introduction of a dedicated function type.
DefiningEvmosOptionsFnclearly separates concerns for chain-specific configurations. This improves overall modularity.
40-45: NoOp function is helpful for test stubs.
Providing a no-op function avoids nil checks and simplifies testing.
Line range hint
46-69: Validate chain-splitting logic for chain IDs without a dash.
If a chain ID does not follow the<ID>-<revision>format, splitting on"-"might still return the entire string in index zero, which is safe. However, confirm thatChainsCoinInfois keyed properly for such chain IDs or handle missing entries accordingly.precompiles/evidence/query.go (4)
20-29: Argument count validation is clear.
The function checks that exactly one argument is provided; this is clean and explicit.
30-48: Verify uppercase requirement for hex-encoded evidence hash.
Usingstrings.ToUpper(hex.EncodeToString(evidenceHash))aligns with typical evidence module queries. Ensure the upstream keeper explicitly expects uppercase or is case-insensitive.
60-69: Page request usage for pagination is correct.
The code properly unpacks*query.PageRequest. This is a good approach for handling paginated queries.
83-99: Robust check for evidence type.
The code ensures each entry is anEquivocation. This is a safe type check that prevents unexpected data from polluting results.precompiles/evidence/evidence.go (5)
31-35: Struct extension is well-structured.
Embeddingcmn.Precompileand addingevidenceKeeperis clean. This extends functionality while keeping the base precompile logic reusable.
37-42: Loading ABI from embedded file is concise.
The approach is straightforward and self-contained, minimizing external dependencies for ABI loading.
71-86: Graceful avoidance of panics is commendable.
if len(input) < 4 { return 0 }helps avoid panics when method ID is incomplete. This is a neat fail-safe for malformed inputs.
88-127: Comprehensive out-of-gas handling.
The deferred call toHandleGasErroris a robust approach, ensuring you gracefully recover from OOG situations. The cost check and state journaling at the end are well-structured.
142-145: Context-specific logger is helpful.
Attaching"evm extension", "evidence"clarifies logs, improving debuggability.precompiles/evidence/errors.go (1)
6-11: LGTM! Error constants are well-defined.The error messages are clear, descriptive, and follow Go naming conventions.
example_chain/activators.go (1)
10-16: Verify the intended usage ofevmosActivators.The
evmosActivatorsmap is currently unused according to static analysis. Please verify if this is intentional or if integration with the EVM configuration is pending.Run the following script to check for potential usage:
🧰 Tools
🪛 golangci-lint (1.62.2)
12-12: var
evmosActivatorsis unused(unused)
ante/evm/suite_test.go (1)
23-32: LGTM! Test coverage improvements.The changes enhance test coverage by systematically testing all combinations of transaction types and chain IDs. The nested loop structure is clean and efficient.
x/evm/types/precompiles.go (1)
19-19: LGTM! Well-structured precompile address addition.The new
EvidencePrecompileAddressfollows the established sequential pattern and is properly integrated into both the constants andAvailableStaticPrecompilesslice.Also applies to: 36-36
precompiles/evidence/events.go (1)
47-47: Verify block number conversion safety.The block height conversion to uint64 is marked with a nolint directive. Consider adding a check to ensure the block height doesn't exceed uint64 max value.
- BlockNumber: uint64(ctx.BlockHeight()), //nolint:gosec // G115 + BlockNumber: uint64(ctx.BlockHeight()), // Safe conversion: Cosmos SDK block heights are always positive and within uint64 rangetestutil/integration/os/utils/bank_test.go (1)
12-12: LGTM! Enhanced test flexibility with dynamic initial amounts.The change from hardcoded values to dynamic initial amounts based on decimal places improves test maintainability and supports different token configurations.
Also applies to: 31-31, 37-37
ante/evm/09_increment_sequence_test.go (1)
17-17: LGTM! Improved test configuration with explicit chain ID.Adding the chain ID configuration ensures the test environment matches the expected chain setup, improving test reliability.
testutil/constants/constants.go (1)
39-39: Verify chain prefix rebranding impact.The chain prefix format has been updated from "evmos_%d" to "os_%d", which appears to be part of a rebranding effort.
Let's verify the rebranding is consistently applied:
✅ Verification successful
Chain prefix rebranding is consistently applied ✅
The change from "evmos_%d" to "os_%d" is consistently implemented across the codebase. While there are some occurrences of the old prefix in test files and example implementations (likely for testing backward compatibility), all main definitions and new code use the updated "os_%d" format.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old prefix format rg '"evmos_\d+"' --type go # Search for other potential inconsistencies in prefix naming rg 'fmt\.Sprintf\("evmos_%d"' --type goLength of output: 1387
Script:
#!/bin/bash # Search for the new prefix format rg '"os_\d+"' --type go rg 'fmt\.Sprintf\("os_%d"' --type go # Search for chain prefix related configuration rg -i 'chain.*prefix' --type go -A 2 # Search for other potential prefix patterns in sprintf rg 'fmt\.Sprintf\(".*_%d"' --type goLength of output: 5390
precompiles/evidence/IEvidence.sol (1)
1-51: LGTM! Well-documented interface.The interface is thoroughly documented with NatSpec comments, making it clear and maintainable.
testutil/integration/os/network/coins.go (2)
43-48: LGTM!The new helper function
getCoinInfoimproves code reusability and standardizes the creation ofCoinInfoinstances.
31-36: Verify the impact of usingdefaultChain.The change from using
testconstants.ExampleChainIDtodefaultChainmight affect test cases that rely on specific chain IDs. Please ensure that all test cases are updated accordingly.Run the following script to verify the usage of chain IDs in test files:
✅ Verification successful
Change from ExampleChainID to defaultChain is safe ✅
The change is safe as
defaultChainis directly assignedtestconstants.ExampleChainIDin the network package. This is merely a local alias that maintains the same chain ID value, ensuring all test cases continue to use the same identifier.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for test files that might be affected by the chain ID change. # Test: Search for references to ExampleChainID in test files echo "Searching for ExampleChainID references in test files..." rg -l "ExampleChainID" --type go --type-add "test:*.{test.go,testing.go}" # Test: Search for defaultChain references in test files echo "Searching for defaultChain references in test files..." rg -l "defaultChain" --type go --type-add "test:*.{test.go,testing.go}"Length of output: 1091
Script:
#!/bin/bash # Check the definition and usage of defaultChain echo "Checking defaultChain definition..." rg "defaultChain :?=" --type go -A 2 echo "\nChecking ExampleChainID definition..." rg "ExampleChainID :?=" --type go -A 2Length of output: 697
example_chain/config.go (4)
18-21: LGTM!The new type
EvmosOptionsFnis well-documented and follows Go's idiomatic function type definition.
23-27: LGTM!The
NoOpEvmosOptionsfunction provides a clean no-op implementation for cases where no specific configuration is needed.
87-88: LGTM!The updated comments provide clear and accurate information about the behavior of
sdk.RegisterDenom.
42-52: Verify the impact of stricter error handling.The function now returns an error for unknown chain IDs instead of using default values. This change could break existing code that relied on the default behavior.
Run the following script to verify the usage of this function:
✅ Verification successful
The stricter error handling is appropriate and safe ✅
The change to return an error for unknown chain IDs is well-handled:
- It's used during app initialization where early failure is appropriate
- Test code has
NoOpEvmosOptionsalternative available- Preventing silent defaults for unknown chains improves reliability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for calls to EvmosAppOptions to identify potential impact. # Test: Search for direct function calls echo "Searching for direct function calls..." ast-grep --pattern 'EvmosAppOptions($$$)' # Test: Search for error handling patterns echo "Searching for error handling patterns..." ast-grep --pattern 'err := EvmosAppOptions($$$)' ast-grep --pattern 'if err := EvmosAppOptions($$$); err != nil {'Length of output: 340
Script:
#!/bin/bash # Broader search for any mentions of EvmosAppOptions echo "Searching for any mentions of EvmosAppOptions..." rg "EvmosAppOptions" -l # Search for test files containing the function name echo "Searching in test files..." rg "EvmosAppOptions" -l --glob "*_test.go" # Get the current file's package name to search for imports echo "Getting package name..." head -n 1 example_chain/config.go # Search for potential imports of this package echo "Searching for imports..." rg "\".*example_chain\"" --type goLength of output: 3094
Script:
#!/bin/bash # Check implementation in main application setup echo "Checking main application setup..." rg -A 5 -B 5 "EvmosAppOptions" example_chain/osd/cmd/root.go # Check network setup implementation echo "Checking network setup..." rg -A 5 -B 5 "EvmosAppOptions" testutil/network/network.go # Check test helper implementation echo "Checking test implementation..." rg -A 5 -B 5 "EvmosAppOptions" example_chain/config_testing.goLength of output: 2826
testutil/integration/os/network/amounts.go (3)
15-18: LGTM!The
InitialAmountsstruct is well-defined with appropriate field types.
20-27: LGTM!The function provides a clean way to get default initial amounts for both base and EVM tokens.
29-33: LGTM!The function provides a clean way to get default initial bonded amount.
precompiles/evidence/types.go (1)
1-15: LGTM! Imports are well-organized and appropriate.The imports cover all necessary dependencies for evidence handling, including Cosmos SDK types and Ethereum common types.
ante/evm/07_can_transfer_test.go (2)
23-23: LGTM! Chain ID configuration is properly set.The network setup correctly includes the chain ID configuration.
68-68: LGTM! Test case naming is clear and includes all relevant information.The test case naming convention properly includes transaction type, chain ID, and test name.
ante/evm/10_gas_wanted_test.go (3)
6-7: LGTM! Import organization is clean and necessary.The added imports for
fmtandevmtypesare properly organized and required for the changes.Also applies to: 18-18
29-29: LGTM! Improved number readability with underscores.The use of underscores in numeric literals (
100_000,10_000) improves code readability.Also applies to: 86-86
95-95: LGTM! Test case naming follows the established convention.The test case naming properly includes transaction type, chain ID, and test name.
example_chain/precompiles.go (1)
10-10: LGTM! Evidence-related imports are properly organized.The imports for evidence keeper and precompile are correctly placed and organized.
Also applies to: 22-22
example_chain/test_helpers.go (2)
133-133: LGTM! Consistent parameter update.The change maintains consistency with the earlier modification to include
EvmosAppOptions.
57-57: Verify backward compatibility with existing test suites.The addition of
EvmosAppOptionsparameter toNewExampleAppconstructor could impact existing tests. Ensure all test suites are updated accordingly.✅ Verification successful
All test suites have been updated with the EvmosAppOptions parameter
Verified that all calls to
NewExampleAppacross the codebase have been properly updated to include theEvmosAppOptionsparameter with consistent usage and correct ordering.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all NewExampleApp constructor calls to verify they've been updated rg "NewExampleApp\(" --type goLength of output: 1134
Script:
#!/bin/bash # Search for NewExampleApp calls with surrounding context rg "NewExampleApp\(" --type go -B 2 -A 2Length of output: 3770
ante/evm/06_account_verification_test.go (3)
24-24: LGTM! Enhanced test configuration.Adding chainID to network configuration improves test environment specificity.
57-62: Improved balance retrieval and validation.The changes enhance robustness by:
- Using EVM-specific balance retrieval
- Adding explicit validation of balance parsing
104-104: LGTM! Better test case identification.Including chainID in test names improves test case identification and debugging.
testutil/integration/os/network/config_test.go (2)
22-27: LGTM! Improved test structure with explicit expectations.The new structure clearly separates different aspects of testing:
- Chain configuration
- Decimal handling
- Base fee expectations
- Amount expectations
66-102: Well-structured balance and fee validation.The changes introduce comprehensive validation:
- Clear separation of EVM and bank balance checks
- Explicit base fee validation
- Detailed comments explaining the checks
testutil/integration/os/utils/gov.go (1)
29-32: LGTM! Improved token amount handling in proposals.The changes correctly adjust proposal submission amounts to account for EVM coin decimals, preventing potential precision issues.
ethereum/eip712/preprocess_test.go (1)
79-79: Verify chain ID consistency across the codebase.The chain ID update from 9000 to 9001 appears to be part of a broader effort to standardize chain IDs.
Let's verify the chain ID consistency:
✅ Verification successful
Chain ID update is consistent with codebase standards
The change to chain ID 9001 aligns with the defined example chain ID constant (ExampleEIP155ChainID) in the codebase. Remaining references to 9000 are in test fixtures and documentation examples that don't affect production code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to the old chain ID (9000) # to ensure all instances have been updated. echo "Searching for '9000' chain ID references..." rg -l "9000" --type go echo "Searching for '9001' chain ID references to verify the update..." rg -l "9001" --type goLength of output: 772
Script:
#!/bin/bash # Check the context of chain ID usage echo "Checking context of '9000' usage..." rg "9000" --type go -C 2 echo -e "\nChecking context of '9001' usage..." rg "9001" --type go -C 2Length of output: 7462
precompiles/evidence/query_test.go (2)
16-120: Well-structured test cases with comprehensive coverage.The test suite effectively covers various scenarios including:
- Empty input validation
- Invalid evidence hash handling
- Successful evidence submission
- Proper error propagation
122-227: Well-implemented pagination tests with proper validation.The pagination tests effectively cover:
- Empty input validation
- Empty evidence list handling
- Successful evidence retrieval with pagination
ante/evm/08_gas_consume_test.go (3)
105-105: Improved fee handling with precise amount tracking.The change from
feestofeesAmtwithsdkmath.Intprovides more precise control over fee amounts and better type safety.Also applies to: 109-110, 117-118, 127-127
157-159: Good practice: Using conversion factor for EVM decimals.The code correctly handles EVM decimal conversion when calculating fees, ensuring accurate fee representation across different decimal places.
180-187: Thorough balance validation with precise error checking.The balance checks are well-implemented with:
- Proper error handling
- Precise balance comparison
- Clear expected vs actual balance validation
testutil/integration/os/network/config.go (2)
36-39: Good addition: Initial bonded amount configuration.The addition of
initialBondedAmountprovides better control over validator setup in tests.
85-90: Improved denomination handling with proper decimal mapping.The code now correctly handles other coin denominations by mapping them to base decimals, making the configuration more flexible.
example_chain/testutil/eth_setup.go (1)
84-84: Verify test coverage with the new app option.The addition of
exampleapp.EvmosAppOptionsenhances the app configuration capabilities. Please ensure that test cases cover scenarios with different app options to validate the configuration behavior.✅ Verification successful
Test coverage for EvmosAppOptions is sufficient
The new app option is well-covered through:
- Direct testing in
x/evm/types/msg_test.go- Integration tests using network setup
- Test helpers ensuring consistent usage across test suites
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files that use EthSetupWithDB to ensure they handle the new parameter rg -l "EthSetupWithDB" --type go | grep "_test.go"Length of output: 48
Script:
#!/bin/bash # Find definition and usage of EvmosAppOptions rg "EvmosAppOptions" -A 3 --type go # Find test files in example_chain directory that might test app options fd -e go -e _test.go . example_chain/ # Search for any app options related test patterns rg "AppOptions|SetupTest" --type go -l | grep "_test"Length of output: 7927
rpc/backend/chain_info_test.go (1)
157-157: Verify chain ID consistency across the codebase.The chain ID has been updated from 9000 to 9001. This change should be consistently reflected across all files.
testutil/integration/os/network/network.go (4)
93-96: Well-documented variable with clear purpose.The documentation clearly explains the purpose and representation of the
PrefundedAccountInitialBalancevariable.
101-108: Improved configuration handling with dynamic bond amount.The bond amount calculation now properly reflects the base denom decimals, making it more flexible and maintainable.
143-145: Enhanced fee market configuration with base fee calculation.The addition of fee market parameters with a dynamically calculated base fee improves the configuration flexibility.
289-289: Updated return type for better clarity.The
GetOtherDenomsfunction now returnsotherCoinDenomsinstead ofotherCoinDenom, reflecting a more accurate plural representation.rpc/backend/evm_query_client_test.go (1)
38-38: Consistent chain ID updates across test methods.The chain ID has been updated from 9000 to 9001 across multiple test methods, maintaining consistency with the broader changes:
RegisterTraceTransactionWithPredecessorsRegisterTraceTransactionRegisterTraceTransactionErrorRegisterTraceBlockAlso applies to: 44-44, 49-49, 57-57
example_chain/osd/cmd/root.go (3)
68-68: LGTM!The addition of
EvmosAppOptionsparameter is consistent with the updatedNewExampleAppfunction signature.
370-371: LGTM!The addition of
EvmosAppOptionsparameter is correctly placed before the variadicbaseAppOptionsparameter.
405-411: LGTM!Both conditional branches are correctly updated to include the
EvmosAppOptionsparameter in theNewExampleAppcalls.testutil/integration/os/network/setup.go (4)
53-53: LGTM!The addition of the
feemarketfield todefaultGenesisParamsis consistent with the fee market module integration.
137-146: LGTM!The changes improve denomination handling by:
- Using a more descriptive parameter name
denomDecimals- Ensuring consistent ordering of denominations through sorting
454-457: LGTM!The
FeeMarketCustomGenesisStatetype is well-defined with a clear purpose of customizing the fee market genesis state.
459-465: LGTM!The
setDefaultFeeMarketGenesisStatefunction follows the established pattern for genesis state setup and properly initializes the fee market module.testutil/network/network.go (2)
110-110: LGTM!The addition of
EvmosAppOptionsparameter is consistent with the updatedNewExampleAppfunction signature.
143-143: LGTM!The addition of
EvmosAppOptionsparameter in theNewAppConstructorfunction is consistent with the updated signature.example_chain/app.go (3)
234-234: LGTM!The addition of
evmosAppOptionsparameter of typeEvmosOptionsFnprovides a clean way to customize application options.
284-284: LGTM!The initialization of evmOS application configuration now uses the provided options function, with proper error handling.
578-578: Verify the evidence keeper integration with EVM precompiles.The addition of
app.EvidenceKeeperto static precompiles setup is a significant change. Please ensure that:
- The evidence keeper's functionality is properly exposed through precompiles
- The integration is thoroughly tested
- Security implications of exposing evidence-related functionality via precompiles are considered
✅ Verification successful
Evidence keeper precompile integration is properly implemented ✅
The codebase shows a complete and well-tested evidence precompile implementation with:
- Full test coverage in
precompiles/evidence/- Proper keeper initialization and router configuration
- Complete implementation including queries, transactions, and events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for evidence keeper precompile tests and security measures # Look for evidence keeper precompile tests echo "Searching for evidence keeper precompile tests..." rg -A 5 "evidence.*precompile.*test" # Look for security-related comments or documentation echo "Searching for security documentation..." rg -A 5 "evidence.*precompile.*security"Length of output: 279
Script:
#!/bin/bash # Search for evidence keeper related code in precompiles directory echo "Searching for evidence keeper in precompiles..." rg -l "evidence" --type go | grep -i "precompile" # Look for evidence keeper imports in precompile files echo "Searching for evidence keeper imports in precompiles..." rg "\"evidence\"" --type go | grep -i "precompile" # Check the context where EvidenceKeeper is used echo "Checking EvidenceKeeper usage..." rg -A 5 "EvidenceKeeper" --type go # Look for recent changes in precompile configuration echo "Checking precompile configuration..." rg -A 10 "NewAvailableStaticPrecompiles" --type goLength of output: 5394
.github/workflows/lint.yml (1)
29-29: LGTM! Verify changelog for breaking changes.The update from
[email protected]tov6.2.0is a minor version bump.Run the following script to check the changelog for any breaking changes:
✅ Verification successful
Safe to upgrade: No breaking changes in v6.2.0
The update primarily adds Linux ARM64 support (preview) and includes internal improvements. All changes are backward compatible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the changelog for breaking changes between v6.1.1 and v6.2.0 # Fetch the changelog curl -s https://api.github.com/repos/golangci/golangci-lint-action/releases/tags/v6.2.0 | jq -r '.body'Length of output: 4673
CHANGELOG.md (1)
11-11: LGTM! Changelog entry follows the established format.The entry correctly documents the version bump with PR reference and commit hash.
precompiles/evidence/abi.json (1)
6-24: LGTM! Event parameters are properly indexed.The
SubmitEvidenceevent follows best practices by:
- Indexing the
submitteraddress for efficient filtering- Leaving the
hashunindexed due to its dynamic sizego.mod (2)
51-51: Verify security implications of crypto package updates.The updates to security-related packages should be reviewed:
golang.org/x/cryptofrom v0.29.0 to v0.32.0golang.org/x/netfrom v0.31.0 to v0.34.0Run the following script to check for security advisories:
Also applies to: 53-55
✅ Verification successful
Security review completed: Package updates improve security posture
The updates to both packages are security-positive:
golang.org/x/crypto v0.32.0includes fixes for all known vulnerabilities, including a critical SSH-related fix (v0.31.0)golang.org/x/net v0.34.0includes fixes for all known vulnerabilities, including a high-severity HTML parsing fix (v0.33.0)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories in updated packages # Check golang.org/x/crypto advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "golang.org/x/crypto") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' # Check golang.org/x/net advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "golang.org/x/net") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 3194
19-21: Verify compatibility with updated cosmos-sdk.The update of
github.com/cosmos/cosmos-dbto v1.1.1 andgithub.com/cosmos/cosmos-sdkto v0.50.11 should be tested for compatibility.Run the following script to check for breaking changes:
✅ Verification successful
Updates appear safe but testing is recommended
The changes in both dependencies are minor and don't introduce breaking changes:
- cosmos-db v1.1.1: Minor improvement for PebbleDB logging
- cosmos-sdk v0.50.11: Important security fixes (ABS-0043/ABS-0044) and optional features
While the changes appear safe, testing is recommended due to the extensive usage of these packages throughout the codebase, particularly in core functionality and x/ modules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for breaking changes in cosmos-sdk updates # Fetch cosmos-db changelog echo "Checking cosmos-db changelog..." curl -s https://api.github.com/repos/cosmos/cosmos-db/releases/tags/v1.1.1 | jq -r '.body' # Fetch cosmos-sdk changelog echo "Checking cosmos-sdk changelog..." curl -s https://api.github.com/repos/cosmos/cosmos-sdk/releases/tags/v0.50.11 | jq -r '.body'Length of output: 1711
Script:
#!/bin/bash # Check previous versions and usage # Get previous versions from go.mod history echo "Previous versions from go.mod:" git log -p go.mod | grep -A 3 "github.com/cosmos/cosmos-db\|github.com/cosmos/cosmos-sdk" # Check direct usage of these packages echo -e "\nUsage of cosmos-db:" rg "github.com/cosmos/cosmos-db" --type go -l echo -e "\nUsage of cosmos-sdk:" rg "github.com/cosmos/cosmos-sdk" --type go -l # Try to get cosmos-db changelog from raw GitHub content echo -e "\nTrying to fetch cosmos-db changelog:" curl -s https://raw.githubusercontent.com/cosmos/cosmos-db/v1.1.1/CHANGELOG.mdLength of output: 15223
example_chain/go.mod (1)
7-28: LGTM! Dependencies are consistently updated.The dependency updates in the example chain mirror those in the main module, maintaining consistency across the project.
Also applies to: 38-39, 42-42, 68-72, 80-80, 97-97, 149-149, 161-161, 216-218, 221-228, 230-230, 233-236
x/evm/types/msg_test.go (2)
Line range hint
1-824: LGTM! Comprehensive test coverage.The test suite provides thorough coverage of Ethereum transaction functionality including:
- Basic and advanced transaction validation
- Transaction signing with different signer types
- Transaction parameter validation
- Serialization/deserialization tests
57-58: Verify the chain ID parameter change across the codebase.The change from
InitializeAppConfiguration("evmos_9001-1")toEvmosAppOptions("os_9001-1")includes both a method name change and a chain ID parameter change. Let's verify this change is consistent across the codebase.✅ Verification successful
Chain ID and configuration method changes are consistent across the codebase
The migration from
InitializeAppConfiguration("evmos_9001-1")toEvmosAppOptions("os_9001-1")has been completely and consistently implemented throughout the codebase. No instances of the old chain ID or method remain.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining occurrences of the old chain ID and method name # Check for old chain ID usage echo "Checking for old chain ID usage..." rg "evmos_9001-1" # Check for old method name usage echo "Checking for old method name usage..." rg "InitializeAppConfiguration" # Check for new chain ID usage to ensure consistency echo "Checking for new chain ID usage..." rg "os_9001-1"Length of output: 379
Script:
#!/bin/bash # Search for similar chain ID patterns and configuration methods # Check for any chain IDs with pattern *_9001-1 echo "Checking for chain ID pattern..." rg "_9001-1" # Check for all usages of EvmosAppOptions echo "Checking EvmosAppOptions usage..." rg "EvmosAppOptions" # Look for any similar configuration methods echo "Checking for similar configuration methods..." rg "AppOptions|InitConfig|InitializeApp|SetupConfig"Length of output: 6013
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (20)
precompiles/evidence/tx.go (1)
15-22: Consider stricter typing forargs.
While accepting[]interface{}is typical for ABI-based methods, it can lead to runtime errors if the arguments aren't validated or cast properly. Consider introducing stricter type-checking or separate helper functions to reduce potential type mismatch issues.example_chain/config_testing.go (1)
Line range hint
60-96: Watch for repeatedRegisterDenomcalls and concurrency.
The sequence of deferringSetBaseDenomand registering multiple denominations is well-structured for single initialization. However, in more dynamic or concurrent scenarios, repeated calls could lead to conflicts or re-registrations. Confirm that this initialization pattern remains safe throughout the lifecycle of the chain.precompiles/evidence/query.go (2)
44-47: Consider supporting multiple evidence types.
Currently, onlyEquivocationis expected. If future updates introduce new evidence types, you may need a strategy to handle them within this function.
60-99: Implementation for retrieving all evidence is robust, but be mindful of performance.
For large amounts of evidence, iterating and packing everything into memory may be expensive. If you anticipate significant data volumes, consider pagination or streaming responses.precompiles/evidence/evidence.go (2)
31-35: Consider Interface-based Field Injection
StoringevidenceKeeper evidencekeeper.Keeperdirectly is fine, but you may consider injecting an interface to enhance testability and reduce coupling.
88-127: Refine Method Dispatch
The switch statement forSubmitEvidenceMethod,EvidenceMethod, etc., is clear. As the list grows, consider encapsulating each method in a separate function to improve readability and maintainability.example_chain/activators.go (1)
3-4: Consider renaming package to follow Go conventions.The package name
example_chainuses an underscore, which goes against Go naming conventions. Consider renaming it toexamplechainorexamples.🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 3-3: var-naming: don't use an underscore in package name
(revive)
precompiles/evidence/events.go (1)
21-51: Consider adding input validation and safer type conversion.Two suggestions for improvement:
- Add input validation for
evidenceHashto prevent potential issues with nil or empty slices.- The block height conversion on line 47 uses an unsafe type conversion. Consider adding bounds checking.
Here's a suggested implementation:
func (p Precompile) EmitSubmitEvidenceEvent(ctx sdk.Context, stateDB vm.StateDB, origin common.Address, evidenceHash []byte) error { + // Validate input + if len(evidenceHash) == 0 { + return fmt.Errorf("evidence hash cannot be empty") + } + // Prepare the event topics event := p.ABI.Events[EventTypeSubmitEvidence] topics := make([]common.Hash, 2) // The first topic is always the signature of the event. topics[0] = event.ID var err error topics[1], err = cmn.MakeTopic(origin) if err != nil { return err } // Pack the evidence hash arguments := abi.Arguments{event.Inputs[1]} packed, err := arguments.Pack(evidenceHash) if err != nil { return err } + // Safe conversion of block height + blockHeight := ctx.BlockHeight() + if blockHeight < 0 { + return fmt.Errorf("invalid block height: %d", blockHeight) + } + stateDB.AddLog(ðtypes.Log{ Address: p.Address(), Topics: topics, Data: packed, - BlockNumber: uint64(ctx.BlockHeight()), //nolint:gosec // G115 + BlockNumber: uint64(blockHeight), }) return nil }precompiles/gov/utils_test.go (2)
25-33: Consider adding validation for required fields.While the struct is well-documented, consider adding validation to ensure required fields are properly initialized.
type CallsData struct { precompileAddr common.Address precompileABI abi.ABI precompileCallerAddr common.Address precompileCallerABI abi.ABI + + // Add a constructor or validation method + func NewCallsData( + precompileAddr, callerAddr common.Address, + precompileABI, callerABI abi.ABI, + ) CallsData { + if (precompileAddr == common.Address{}) { + panic("precompile address cannot be empty") + } + return CallsData{ + precompileAddr: precompileAddr, + precompileABI: precompileABI, + precompileCallerAddr: callerAddr, + precompileCallerABI: callerABI, + } + } }
35-58: Consider adding error handling for invalid call types.The switch statement could benefit from handling invalid call types to make the tests more robust.
func (cd CallsData) getTxAndCallArgs( callArgs factory.CallArgs, txArgs evmtypes.EvmTxArgs, callType callType, args ...interface{}, ) (evmtypes.EvmTxArgs, factory.CallArgs) { switch callType { case directCall: txArgs.To = &cd.precompileAddr callArgs.ContractABI = cd.precompileABI case contractCall: txArgs.To = &cd.precompileCallerAddr callArgs.ContractABI = cd.precompileCallerABI + default: + panic(fmt.Sprintf("invalid call type: %d", callType)) }precompiles/evidence/IEvidence.sol (1)
33-36: Consider adding validation for evidence parameters.The
submitEvidencefunction should include input validation to ensure the evidence data is valid before submission.function submitEvidence(Equivocation calldata evidence) external returns (bool success); +function validateEvidence(Equivocation calldata evidence) external pure returns (bool valid);testutil/integration/os/network/coins.go (1)
43-48: Add validation in getCoinInfo helper.The helper function should validate the input parameters to ensure they meet the requirements (e.g., non-empty denom, valid decimals).
func getCoinInfo(coinInfo evmtypes.EvmCoinInfo) CoinInfo { + if coinInfo.Denom == "" { + panic("empty denom not allowed") + } + if coinInfo.Decimals < 0 || coinInfo.Decimals > 18 { + panic("decimals must be between 0 and 18") + } return CoinInfo{ Denom: coinInfo.Denom, Decimals: coinInfo.Decimals, } }example_chain/config.go (1)
Line range hint
42-63: Enhance error message for unknown chain ID.Consider including available chain IDs in the error message to help users identify valid options.
- return fmt.Errorf("unknown chain id: %s", id) + validChains := make([]string, 0, len(ChainsCoinInfo)) + for chainID := range ChainsCoinInfo { + validChains = append(validChains, chainID) + } + return fmt.Errorf("unknown chain id: %s. Available chain IDs: %v", id, validChains)ethereum/eip712/eip712_legacy_test.go (1)
15-93: Enhance test coverage with table-driven tests and negative cases.While the test is comprehensive, consider these improvements:
- Use table-driven tests to cover different scenarios
- Add negative test cases (e.g., invalid chain ID, malformed message)
- Move test values to constants
+const ( + testChainID = uint64(1) + testAccNum = "1" + testChainName = "cosmoshub-4" + testGas = "200000" + testStakeAmt = "1000" + testStakeDenom = "stake" +) + +func TestLegacyWrapTxToTypedData(t *testing.T) { + tests := []struct { + name string + chainID uint64 + msg sdk.Msg + txData map[string]interface{} + feeDelegation *eip712.FeeDelegationOptions + expectError bool + }{ + { + name: "valid transaction", + chainID: testChainID, + msg: banktypes.NewMsgSend( + sdk.AccAddress([]byte("from_address")), + sdk.AccAddress([]byte("to_address")), + sdk.Coins{sdk.NewInt64Coin(testStakeDenom, 1000)}, + ), + txData: map[string]interface{}{ + "account_number": testAccNum, + // ... rest of the test data + }, + feeDelegation: &eip712.FeeDelegationOptions{ + FeePayer: sdk.AccAddress([]byte("fee_payer")), + }, + expectError: false, + }, + // Add more test cases here + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // Test implementation + }) + } }precompiles/evidence/types.go (1)
62-86: Enhance error messages and add validation for equivocation data.Consider adding more detailed error messages and validation for the equivocation data fields.
func NewMsgSubmitEvidence(origin common.Address, args []interface{}) (*evidencetypes.MsgSubmitEvidence, error) { if len(args) != 1 { - return nil, fmt.Errorf(cmn.ErrInvalidNumberOfArgs, 1, len(args)) + return nil, fmt.Errorf("%w: expected 1 argument (EquivocationData), got %d", cmn.ErrInvalidNumberOfArgs, len(args)) } equivocation, ok := args[0].(EquivocationData) if !ok { - return nil, fmt.Errorf("invalid equivocation evidence") + return nil, fmt.Errorf("invalid argument type: expected EquivocationData, got %T", args[0]) + } + + // Validate equivocation data fields + if equivocation.Height <= 0 { + return nil, fmt.Errorf("invalid height: must be positive") + } + if equivocation.Power <= 0 { + return nil, fmt.Errorf("invalid power: must be positive") + } + if equivocation.ConsensusAddress == "" { + return nil, fmt.Errorf("invalid consensus address: must not be empty") } // Convert the EquivocationData to a types.Equivocation evidence := equivocation.ToEquivocation() + if evidence == nil { + return nil, fmt.Errorf("failed to convert equivocation data: invalid time") + } // Create the MsgSubmitEvidence using the SDK msg builder msg, err := evidencetypes.NewMsgSubmitEvidence( sdk.AccAddress(origin.Bytes()), evidence, ) if err != nil { return nil, fmt.Errorf("failed to create evidence message: %w", err) } return msg, nil }precompiles/gov/query.go (1)
188-208: LGTM! Well-structured params query implementation.The new
GetParamsmethod follows the established pattern of other query methods with proper:
- Error handling
- Query server initialization
- Response transformation
However, consider adding documentation to describe the expected arguments and return values.
// GetParams implements the query logic for getting governance parameters +// Args: none +// Returns: ParamsOutput containing the governance parameters func (p *Precompile) GetParams(precompiles/evidence/query_test.go (1)
122-227: Comprehensive pagination tests for evidence queries.The
TestGetAllEvidencefunction effectively tests pagination functionality with proper validation of the response structure.Consider adding test cases for:
- Invalid pagination parameters
- Multiple pages of evidence
- Different sorting orders
testutil/integration/os/network/network.go (1)
93-96: Consider using a constant or configuration value for the initial balance.The hardcoded value
100_000_000_000_000_000_000_000(100k tokens with 18 decimals) might be better defined as a constant or configuration value for better maintainability and reusability.-var PrefundedAccountInitialBalance, _ = sdkmath.NewIntFromString("100_000_000_000_000_000_000_000") +const DefaultPrefundedAmount = "100_000_000_000_000_000_000_000" +var PrefundedAccountInitialBalance, _ = sdkmath.NewIntFromString(DefaultPrefundedAmount)precompiles/gov/types.go (1)
614-623: Consider adding parameter validation.While the function correctly builds the request, consider adding validation for the
ParamsTypefield to ensure it matches the expected values from the governance module.func BuildQueryParamsRequest(args []interface{}) (*govv1.QueryParamsRequest, error) { if len(args) != 0 { return nil, fmt.Errorf(cmn.ErrInvalidNumberOfArgs, 0, len(args)) } + + // List of valid parameter types + validParamsTypes := []string{"voting", "tallying", "deposit"} + paramsType := "" + + // Validate params type if provided + if paramsType != "" && !slices.Contains(validParamsTypes, paramsType) { + return nil, fmt.Errorf("invalid params type: %s. Must be one of: %v", paramsType, validParamsTypes) + } return &govv1.QueryParamsRequest{ - ParamsType: "", + ParamsType: paramsType, }, nil }testutil/integration/os/network/setup.go (1)
137-146: Consider adding input validation for denomDecimals.While the function correctly creates balances using denomDecimals, it should validate the input map to ensure it's not empty.
func createBalances( accounts []sdktypes.AccAddress, denomDecimals map[string]evmtypes.Decimals, ) []banktypes.Balance { + if len(denomDecimals) == 0 { + panic("denomDecimals map cannot be empty") + } + numberOfAccounts := len(accounts) denoms := maps.Keys(denomDecimals) slices.Sort(denoms)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
example_chain/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumx/evm/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (66)
.github/workflows/bsr-push.yml(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/proto.yml(2 hunks)CHANGELOG.md(1 hunks)ante/evm/06_account_verification_test.go(3 hunks)ante/evm/07_can_transfer_test.go(3 hunks)ante/evm/08_gas_consume_test.go(7 hunks)ante/evm/09_increment_sequence_test.go(1 hunks)ante/evm/10_gas_wanted_test.go(4 hunks)ante/evm/suite_test.go(2 hunks)ethereum/eip712/eip712_legacy_test.go(1 hunks)ethereum/eip712/preprocess_test.go(1 hunks)example_chain/activators.go(1 hunks)example_chain/app.go(3 hunks)example_chain/config.go(3 hunks)example_chain/config_testing.go(3 hunks)example_chain/eips/testdata/Counter.json(1 hunks)example_chain/eips/testdata/CounterFactory.json(1 hunks)example_chain/go.mod(8 hunks)example_chain/osd/cmd/root.go(3 hunks)example_chain/precompiles.go(5 hunks)example_chain/test_helpers.go(2 hunks)example_chain/testutil/eth_setup.go(1 hunks)go.mod(8 hunks)precompiles/bank/testdata/BankCaller.json(1 hunks)precompiles/distribution/distribution.go(1 hunks)precompiles/evidence/IEvidence.sol(1 hunks)precompiles/evidence/abi.json(1 hunks)precompiles/evidence/errors.go(1 hunks)precompiles/evidence/events.go(1 hunks)precompiles/evidence/evidence.go(1 hunks)precompiles/evidence/query.go(1 hunks)precompiles/evidence/query_test.go(1 hunks)precompiles/evidence/setup_test.go(1 hunks)precompiles/evidence/tx.go(1 hunks)precompiles/evidence/tx_test.go(1 hunks)precompiles/evidence/types.go(1 hunks)precompiles/gov/IGov.sol(6 hunks)precompiles/gov/abi.json(1 hunks)precompiles/gov/gov.go(2 hunks)precompiles/gov/integration_test.go(6 hunks)precompiles/gov/query.go(2 hunks)precompiles/gov/query_test.go(5 hunks)precompiles/gov/testdata/GovCaller.json(1 hunks)precompiles/gov/testdata/GovCaller.sol(1 hunks)precompiles/gov/testdata/gov.go(1 hunks)precompiles/gov/types.go(1 hunks)precompiles/gov/utils_test.go(1 hunks)precompiles/staking/testdata/StakingCallerTwo.json(1 hunks)precompiles/testutil/contracts/FlashLoan.json(1 hunks)precompiles/testutil/contracts/StakingReverter.json(1 hunks)precompiles/werc20/integration_test.go(0 hunks)rpc/backend/chain_info_test.go(1 hunks)rpc/backend/evm_query_client_test.go(1 hunks)testutil/constants/constants.go(2 hunks)testutil/integration/os/network/amounts.go(1 hunks)testutil/integration/os/network/coins.go(2 hunks)testutil/integration/os/network/config.go(5 hunks)testutil/integration/os/network/config_test.go(3 hunks)testutil/integration/os/network/network.go(5 hunks)testutil/integration/os/network/setup.go(6 hunks)testutil/integration/os/utils/bank_test.go(2 hunks)testutil/integration/os/utils/gov.go(1 hunks)testutil/network/network.go(2 hunks)x/evm/types/msg_test.go(1 hunks)x/evm/types/precompiles.go(2 hunks)
💤 Files with no reviewable changes (1)
- precompiles/werc20/integration_test.go
✅ Files skipped from review due to trivial changes (9)
- example_chain/eips/testdata/Counter.json
- precompiles/bank/testdata/BankCaller.json
- CHANGELOG.md
- precompiles/testutil/contracts/FlashLoan.json
- precompiles/staking/testdata/StakingCallerTwo.json
- example_chain/eips/testdata/CounterFactory.json
- .github/workflows/lint.yml
- precompiles/distribution/distribution.go
- precompiles/testutil/contracts/StakingReverter.json
🧰 Additional context used
🪛 golangci-lint (1.62.2)
example_chain/activators.go
[warning] 3-3: var-naming: don't use an underscore in package name
(revive)
12-12: var evmosActivators is unused
(unused)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (88)
precompiles/evidence/tx.go (2)
1-2: License header looks valid.
All standard licensing information is present, and the file header is appropriate.
28-36: Good job verifyingSubmitEvidenceflow.
The message server integration and error handling are concise. The immediate returns handle potential failures appropriately.example_chain/config_testing.go (4)
10-12: Import statements appear consistent.
No issues with newly added imports.
35-39: Clear function type definition.
TheEvmosOptionsFnprovides a neat abstraction for custom configuration logic.
40-45: No-op configuration approach is clean.
NoOpEvmosOptionseffectively communicates no special configuration is needed for certain cases.
49-53: Validate chain ID parsing edge cases.
Splitting on-might fail if the chain ID has an unexpected format or contains multiple dashes. Consider validating the substring or handling special cases (e.g., empty or malformed chain IDs) to avoid runtime issues.precompiles/evidence/query.go (1)
20-28: Appropriate validation of argument length.
Rejecting unexpected numbers of arguments helps avoid panics and clarifies usage.precompiles/evidence/evidence.go (4)
1-2: License Header Looks Good
No issues found with the license or its placement.
43-69: Validate ABI Version or Schema
While loading the ABI, consider verifying that it matches the expected version or schema. It might help mitigate potential mismatches during integration with external contracts.
129-140: Transaction Identification Logic is Straightforward
TheIsTransactionmethod is concise and logically organized.
142-145: Logger Usage is Correct
Leveraging the context-specific logger ensures clarity in logs related to evidence precompile operations.precompiles/gov/testdata/GovCaller.sol (2)
1-2: LGPL License Declaration
Your use of LGPL-3.0-only is consistent with open-source standards.
7-11: Validate Contract Reference
EnsureGOV_CONTRACTis defined and assigned appropriately in the broader environment. If it’s uninitialized,getParams()calls may revert.precompiles/gov/testdata/gov.go (2)
7-9: Contract Utilities Import
Importingcontractutilsto load the contract JSON is well-structured and aligns with the existing pattern in the codebase.
11-13: Safeguard Against Missing Files
When callingLoadContractFromJSONFile("GovCaller.json"), handle the possibility that the file is missing or corrupted. Currently, the function does return an error, so ensure the caller checks it.precompiles/evidence/errors.go (1)
6-11: LGTM! Error constants are well-defined.The error constants are clear, descriptive, and follow Go naming conventions.
example_chain/activators.go (1)
12-16: Verify usage of evmosActivators map.The
evmosActivatorsmap is marked as unused by static analysis. Please verify if this map is intended to be used by other packages.Run the following script to check for usage:
🧰 Tools
🪛 golangci-lint (1.62.2)
12-12: var
evmosActivatorsis unused(unused)
x/evm/types/precompiles.go (1)
19-19: LGTM! Evidence precompile address properly configured.The new evidence precompile address follows the sequential pattern and is correctly added to the available precompiles list.
Also applies to: 36-36
precompiles/evidence/events.go (1)
16-19: LGTM! Event type constant is well-defined.The event type constant follows proper naming conventions and is clearly documented.
precompiles/gov/utils_test.go (1)
16-23: LGTM! Well-defined enum type for call differentiation.The
callTypeenum is clearly defined and documented, making the test code's intent explicit.testutil/integration/os/utils/bank_test.go (2)
12-12: LGTM! Clean import addition.The new import for
evmtypesis properly placed and aligned with the changes.
31-31: LGTM! Good transition to dynamic initial amounts.The change from hardcoded values to
GetInitialAmountwith appropriate decimal constants improves test maintainability.Also applies to: 37-37
ante/evm/09_increment_sequence_test.go (1)
17-17: LGTM! Proper chain ID configuration.Adding the chain ID configuration ensures test consistency with the broader changes in the codebase.
testutil/constants/constants.go (1)
39-39: LGTM! Chain ID prefix updated consistently.The prefix change from "evmos" to "os" aligns with the project's naming conventions.
precompiles/evidence/IEvidence.sol (2)
13-22: LGTM! Well-structured Equivocation data type.The struct uses appropriate types for each field and includes clear documentation.
7-7: Verify precompile address allocation.The precompile address
0x807needs to be verified to ensure it doesn't conflict with other precompile addresses in the system.✅ Verification successful
Precompile address allocation is correct ✅
The address
0x807follows the established sequential pattern for core module precompiles (0x800-0x807) and doesn't conflict with any other precompile addresses in the system.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other precompile address declarations to check for conflicts rg -i "0x[0-9a-f]*807" --type goLength of output: 298
Script:
#!/bin/bash # Search for all precompile address declarations to check for conflicts rg -i "0x[0-9a-f]{4}" x/evm/types/precompiles.goLength of output: 818
precompiles/evidence/setup_test.go (2)
37-62: LGTM! Well-structured test setup.The setup properly initializes all required dependencies and handles errors appropriately.
74-76: Verify the odd height requirement in equivocation handler.The handler only accepts evidence with odd heights. This seems like an unusual restriction that might not reflect real-world requirements.
testutil/integration/os/network/coins.go (1)
31-36: LGTM! Clean refactoring of chain coins initialization.The changes improve code organization by extracting the coin info creation logic into a separate helper function.
example_chain/config.go (2)
18-27: LGTM! Well-documented type definition and no-op function.The implementation follows Go best practices with clear documentation and purpose.
87-88: LGTM! Clear documentation of SDK behavior.The comment effectively explains the automatic overwrite behavior of
sdk.RegisterDenom.ante/evm/07_can_transfer_test.go (2)
49-54: LGTM! Improved balance handling with validation.The changes enhance the robustness of balance handling by:
- Using EVM-specific balance retrieval
- Adding validation for string to integer conversion
68-68: LGTM! Enhanced test case naming.Including the chain ID in test names improves clarity and debugging by providing more context.
ante/evm/10_gas_wanted_test.go (2)
29-29: LGTM! Improved number readability.Using underscores in large numbers (
100_000and10_000) enhances code readability without affecting functionality.Also applies to: 86-86
84-84: LGTM! Enhanced error message.Added descriptive error message improves debugging experience.
example_chain/precompiles.go (1)
104-107: LGTM! Well-structured evidence precompile integration.The evidence precompile is properly integrated with:
- Consistent error handling pattern
- Proper initialization using required keepers
- Correct registration in the precompiles map
Also applies to: 120-120
precompiles/gov/gov.go (1)
121-122: LGTM! Clean integration of governance parameters query.The GetParams method is properly integrated into the Run method, maintaining consistency with other query methods.
example_chain/test_helpers.go (1)
57-57: Verify the initialization of EvmosAppOptions.The
EvmosAppOptionsparameter has been added toNewExampleAppcalls, but its initialization is not visible in the provided code. This could lead to potential nil pointer dereferences.Also applies to: 133-134
ante/evm/06_account_verification_test.go (2)
57-62: LGTM! Balance retrieval refactored for consistency.The change from bank balance to EVM balance retrieval and the addition of proper balance parsing with error checking improves the robustness of the test.
104-104: LGTM! Enhanced test case identification.Adding the chain ID to the test case names improves test output clarity and helps in identifying test failures in multi-chain scenarios.
testutil/integration/os/network/config_test.go (2)
22-27: LGTM! Improved test case structure.The new fields
expBaseFeeandexpCosmosAmountprovide better clarity and separation of concerns in test expectations.
66-102: LGTM! Comprehensive balance and base fee validations.The added validations with clear section markers improve test readability and ensure thorough checking of:
- EVM balances in 18 decimals
- Bank balances in original decimals
- Base fee representation
precompiles/gov/query.go (1)
28-29: LGTM! Clear constant naming.The new constant follows the established naming pattern for method constants.
testutil/integration/os/utils/gov.go (1)
29-32: LGTM! Token amount adjustment properly handles EVM coin decimals.The change correctly adjusts the proposal submission amount by dividing it by the EVM coin decimals conversion factor, ensuring proper token amount calculation across different decimal configurations.
ethereum/eip712/preprocess_test.go (1)
79-79: Verify chain ID consistency across the codebase.The chain ID has been updated from 9000 to 9001. Let's ensure this change is consistent across all configurations.
✅ Verification successful
Chain ID variation in tests is intentional and beneficial
The change from 9000 to 9001 in the EIP-712 preprocessing test is specific to that component. Having different chain IDs across various test files helps ensure the system works correctly with different chain IDs and doesn't need to be synchronized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining occurrences of chain ID 9000 and verify consistency with 9001 echo "Checking for chain ID occurrences..." rg -l "9000|chainID.*9000" --type go echo "Checking for chain ID in configuration files..." rg -l "9000" --type json --type yaml --type tomlLength of output: 1087
Script:
#!/bin/bash # Check the context of chain ID occurrences in Go files echo "Checking context of potential chain ID occurrences..." rg "9000" --type go -B 2 -A 2 | grep -v "precompiles/.*json" | grep -v "testdata"Length of output: 3336
precompiles/evidence/query_test.go (1)
16-120: Well-structured test cases for evidence queries.The
TestEvidencefunction provides good coverage of edge cases and happy paths, including input validation and evidence data verification.ante/evm/08_gas_consume_test.go (2)
157-159: LGTM! Improved fee handling with proper decimal conversion.The changes correctly handle EVM decimals by multiplying the fee amount with the conversion factor before creating the coin.
180-187: Improved balance verification using EVM-specific queries.The change from generic balance queries to EVM-specific ones ensures more accurate testing of fee deductions.
✅ Verification successful
Correct usage of EVM-specific balance queries confirmed
The change to
GetBalanceFromEVMis appropriate as it's the dedicated method for querying EVM account balances, ensuring accurate fee deduction testing for EVM transactions. This usage is consistent with other EVM-related tests in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of GetBalanceFromEVM across test files echo "Checking for potential inconsistent balance query methods..." rg "GetAllBalances|bank.*balance" --type goLength of output: 5397
Script:
#!/bin/bash # Search for GetBalanceFromEVM implementations and usages echo "Searching for GetBalanceFromEVM..." rg "GetBalanceFromEVM" -A 3 --type go echo -e "\nChecking the context of the gas consume test..." cat ante/evm/08_gas_consume_test.goLength of output: 9487
testutil/integration/os/network/config.go (4)
22-23: LGTM! Good use of constants.Moving the chain ID to a constant improves maintainability and reduces magic values in the code.
36-39: LGTM! Well-structured field additions.The new
initialBondedAmountfield is properly typed withmath.Intand logically grouped with related validator fields.
40-45: Good field renaming for clarity.The rename from
otherCoinDenomtootherCoinDenomsbetter reflects that it's a slice of denominations. The comment clearly explains the field's purpose.
85-91: LGTM! Improved denomination handling.The code now correctly iterates over
otherCoinDenomsto set decimal values, making the denomination handling more robust.example_chain/testutil/eth_setup.go (1)
84-84: LGTM! Enhanced app configuration.The addition of
EvmosAppOptionsimproves the application's configurability by allowing custom initialization options.precompiles/gov/IGov.sol (2)
73-91: LGTM! Well-structured governance parameters.The
Paramsstruct provides a comprehensive and well-organized representation of governance parameters. Each field is appropriately typed for its purpose.
225-227: LGTM! Clean parameter access method.The
getParams()function provides a straightforward way to retrieve governance parameters.rpc/backend/chain_info_test.go (1)
157-157: LGTM! Consistent chain ID update.The chain ID update from 9000 to 9001 maintains consistency with related changes across the codebase.
testutil/integration/os/network/network.go (4)
101-105: LGTM! Clear documentation of configuration changes.The comment block clearly explains the purpose of the configuration changes.
143-145: LGTM! Proper initialization of fee market parameters.The fee market parameters are correctly initialized with a base fee derived from the chain's base decimals.
289-289: LGTM! Consistent naming convention.The variable name change from
otherCoinDenomtootherCoinDenomsbetter reflects its plural nature.
106-108: Verify the bonded amount calculation.The bonded amount calculation now depends on the base decimals. Ensure this change is compatible with all test scenarios.
✅ Verification successful
The bonded amount calculation is correctly implemented and safe ✅
The change is compatible with all scenarios because:
- The function validates decimal inputs
- It consistently uses the chain's base decimals configuration
- No direct test dependencies are affected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any test failures related to bonded amount calculations rg -A 5 "GetInitialBondedAmount" | grep -B 5 "Error"Length of output: 50
Script:
#!/bin/bash # Find implementation and usages of GetInitialBondedAmount echo "=== GetInitialBondedAmount Implementation ===" ast-grep --pattern 'func GetInitialBondedAmount($_) { $$$ }' echo -e "\n=== All usages of GetInitialBondedAmount ===" rg "GetInitialBondedAmount" -B 2 -A 2 echo -e "\n=== Test files containing GetInitialBondedAmount ===" fd -e test.go -x grep -l "GetInitialBondedAmount" {}Length of output: 1662
rpc/backend/evm_query_client_test.go (1)
38-38: Verify the chain ID update across the codebase.The chain ID has been updated from 9000 to 9001 across multiple test cases. Ensure this change is consistent with other parts of the codebase.
Also applies to: 44-44, 49-49, 57-57
✅ Verification successful
Chain ID update is consistent across the codebase
The chain ID value of 9001 is properly defined as constants and consistently used across the codebase. The only occurrence of 9000 is in an independent test case variable and does not represent the system-wide chain ID.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining occurrences of chain ID 9000 # and verify consistency with the new chain ID 9001 echo "Checking for old chain ID (9000)..." rg -l "9000" echo "Checking for new chain ID (9001)..." rg -l "9001" echo "Checking chain ID related constants..." rg -l "ExampleEIP155ChainID|ChainID"Length of output: 5962
Script:
#!/bin/bash # Check the actual context of chain ID usage in relevant files echo "Checking chain ID context in test files..." rg -C 3 "9000|9001" x/evm/types/msg_test.go testutil/constants/constants.go example_chain/constants.go echo "Looking for chain ID constant definitions..." rg "const.*[Cc]hain[Ii][Dd].*=.*9" testutil/constants/constants.go example_chain/constants.go echo "Checking specific test context in query client test..." rg -C 3 "9000|9001" rpc/backend/evm_query_client_test.goLength of output: 3956
example_chain/osd/cmd/root.go (1)
68-68: Verify the impact of adding EvmosAppOptions.The addition of
example_chain.EvmosAppOptionstoNewExampleAppcalls might affect application initialization. Ensure all callers ofNewExampleAppare updated accordingly.Also applies to: 370-370, 405-405, 411-411
✅ Verification successful
EvmosAppOptions usage is consistent and correct across the codebase
All calls to
NewExampleAppproperly include theEvmosAppOptionsparameter, which is essential for configuring chain-specific denominations and EVM settings. The implementation is properly handled in both production and test environments.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to NewExampleApp to ensure they include EvmosAppOptions echo "Checking NewExampleApp calls..." rg "NewExampleApp\(" --type go echo "Checking EvmosAppOptions usage..." rg "EvmosAppOptions" --type goLength of output: 2723
Script:
#!/bin/bash # Find implementation of EvmosAppOptions function ast-grep --pattern 'func EvmosAppOptions($_) $_ { $$$ }' # Also check the content of config.go and config_testing.go directly echo "\nChecking config.go content:" cat example_chain/config.go echo "\nChecking config_testing.go content:" cat example_chain/config_testing.goLength of output: 9366
precompiles/gov/query_test.go (4)
38-43: LGTM! Enhanced test case structure.The test case structure has been improved with better error handling and validation through the addition of
errContainsfield.
48-73: LGTM! Well-structured test data setup.The test data setup is comprehensive and includes proper error handling and validation.
80-93: LGTM! Thorough error case coverage.The test cases properly cover various error scenarios with specific error messages.
648-688: LGTM! Comprehensive params query test.The new
TestGetParamsfunction includes proper test cases for both success and failure scenarios.precompiles/gov/types.go (2)
573-591: LGTM! Well-structured governance parameters struct.The
ParamsOutputstruct is well-organized with appropriate field types and ABI tags for serialization. The fields comprehensively cover all governance parameters.
593-612: LGTM! Proper parameter mapping in FromResponse method.The method correctly maps all fields from the governance query response to the output struct, maintaining type consistency.
testutil/integration/os/network/setup.go (3)
11-12: LGTM! Appropriate use of maps package.The
mapspackage fromgolang.org/x/expis correctly imported for map operations.
454-465: LGTM! Well-structured fee market genesis state handling.The implementation correctly handles fee market genesis state with proper initialization and parameter setting.
446-446: LGTM! Correct minimum deposit calculation.The calculation of
minDepositAmtproperly accounts for EVM coin decimals using the conversion factor.testutil/network/network.go (2)
110-110: LGTM! Proper app initialization with options.The
NewExampleAppcall correctly includesEvmosAppOptionsfor consistent app configuration.
143-143: LGTM! Consistent app constructor configuration.The
NewAppConstructormaintains consistency with the DefaultConfig by using the sameEvmosAppOptions.x/evm/types/msg_test.go (1)
57-57: LGTM! Updated app configuration initialization.The change from
InitializeAppConfigurationtoEvmosAppOptionsaligns with the new configuration approach used throughout the codebase.precompiles/gov/integration_test.go (3)
498-498: LGTM! Package alias update for better clarity.The change from
v1togovv1for the status checks improves code readability by making it clear that these constants are from the governance module.Also applies to: 559-559, 588-589
598-598: LGTM! Consistent package alias usage.The vote option update maintains consistency with the
govv1package alias changes.
662-748: LGTM! Well-structured params query test.The new test context for params query is well-implemented with:
- Proper setup using
BeforeEach- Comprehensive parameter validation
- Support for both direct and contract calls
- Clear error handling
example_chain/app.go (2)
234-234: LGTM! Enhanced app configuration flexibility.The addition of
evmosAppOptionsparameter and its usage improves the application's configurability by allowing custom initialization options to be passed in.Also applies to: 284-284
578-578: LGTM! Evidence handling capability added to EVM.Adding
EvidenceKeeperto static precompiles enhances the EVM's functionality by enabling evidence-related operations through precompiled contracts..github/workflows/bsr-push.yml (1)
16-16: LGTM! Dependency version bump.Updating
bufbuild/buf-setup-actionto v1.50.0 keeps the workflow up-to-date with the latest improvements and bug fixes.Let's verify if this is the latest stable version:
✅ Verification successful
Confirmed: v1.50.0 is the latest stable version
The version update in the PR matches the latest release of bufbuild/buf-setup-action.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the latest version of bufbuild/buf-setup-action # Get latest release version curl -s https://api.github.com/repos/bufbuild/buf-setup-action/releases/latest | jq -r '.tag_name'Length of output: 106
ante/evm/suite_test.go (2)
7-7: LGTM! Enhanced test suite configuration.Adding the
chainIDfield and importing test constants improves the test suite's flexibility by allowing tests to run against different chain configurations.Also applies to: 19-19
23-32: LGTM! Improved test coverage with matrix approach.The nested loop implementation efficiently tests all combinations of:
- Transaction types (DynamicFee, Legacy, AccessList)
- Chain IDs (ExampleChainID, SixDecimalsChainID)
This ensures comprehensive coverage of the EVM ante handler functionality across different configurations.
.github/workflows/proto.yml (1)
35-35: LGTM! Consistent version updates for buf-setup-action.The version bump from
v1.47.2tov1.50.0forbufbuild/buf-setup-actionis applied consistently across both jobs.Also applies to: 44-44
precompiles/evidence/abi.json (1)
1-197: Well-structured ABI with comprehensive evidence handling capabilities.The ABI is well-designed with:
- Properly indexed event parameters for efficient filtering
- Clear function visibility modifiers
- Comprehensive pagination support for querying evidence
- Appropriate return types for operation status
go.mod (1)
10-10: Verify compatibility of updated dependencies.The dependency updates include security patches and feature enhancements. Please ensure:
- The updated cosmos-sdk version is compatible with your current implementation
- The security updates don't introduce breaking changes
Run the following script to check for any known compatibility issues:
Also applies to: 19-19, 21-21, 38-39, 51-55, 57-58
✅ Verification successful
Dependencies update is safe and recommended
The update to cosmos-sdk v0.50.11 is a security-focused patch release that:
- Fixes critical security vulnerabilities (ASA-2024-0012, ASA-2024-0013)
- Maintains backward compatibility
- Includes no breaking changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for compatibility issues and security advisories # Check for any known compatibility issues in cosmos-sdk gh api graphql -f query=' { repository(owner: "cosmos", name: "cosmos-sdk") { release(tagName: "v0.50.11") { description url } issues(first: 5, states: OPEN, labels: ["breaking-change"]) { nodes { title url labels(first: 5) { nodes { name } } } } } }' # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: GO, package: "github.com/cosmos/cosmos-sdk") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 4964
example_chain/go.mod (1)
7-7: LGTM! Consistent dependency updates with the main module.The dependency versions are aligned with those in the main go.mod file, maintaining consistency across the project.
Also applies to: 11-11, 16-16, 19-19, 20-20, 26-27, 28-28
precompiles/gov/testdata/GovCaller.json (1)
1-130: Well-structured ABI for governance parameter retrieval.The ABI is well-designed with:
- Comprehensive governance parameter structure
- Appropriate view function modifier
- Clear and descriptive parameter names
precompiles/gov/abi.json (1)
218-336: LGTM! Well-structured governance parameters interface.The new
getParamsfunction is well-designed with appropriate data types for each parameter category (time-based, monetary, thresholds, ratios).Consider adding documentation for parameters.
The interface would benefit from NatSpec documentation explaining:
- The expected format and valid ranges for ratio/threshold parameters
- The time unit (seconds/blocks) for period parameters
- The purpose and valid values for
proposalCancelDestVerify parameter validation in the implementation.
Ensure that the implementation includes validation for:
- Non-negative values for time periods
- Valid decimal strings for ratios and thresholds (between 0 and 1)
- Valid destination address format for
proposalCancelDest✅ Verification successful
Parameter validation is handled by Cosmos SDK governance module ✅
The precompile is correctly designed as a thin bridge to the Cosmos SDK governance module, which provides comprehensive parameter validation. This architectural approach ensures consistent validation across both EVM and Cosmos interfaces.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for parameter validation in the implementation ast-grep --pattern 'func $_(ctx $_, method $_, contract $_, args $_) { $$$ params := $$$ if $_ < 0 { $$$ } $$$ }'Length of output: 129
Script:
#!/bin/bash # Find relevant implementation files echo "=== Finding implementation files ===" fd -e go gov precompiles/ echo -e "\n=== Checking validation patterns ===" # Search for validation patterns in Go files rg -l "func.*getParams|func.*Params" $(fd -e go gov precompiles/) echo -e "\n=== Checking parameter validation ===" # Look for validation code rg "validate|check|verify" $(fd -e go gov precompiles/) -A 3Length of output: 1172
Script:
#!/bin/bash # Look for parameter handling and error patterns echo "=== Checking parameter handling ===" rg "func.*getParams|func.*Params" precompiles/gov/gov.go -A 10 echo -e "\n=== Checking error handling ===" rg "return.*err|errors\." precompiles/gov/gov.go -A 2 echo -e "\n=== Checking gov module integration ===" rg "gov\." precompiles/gov/gov.go -A 3Length of output: 793
This PR applies the changes from the recent developments on the github.com/evmos/evmos repository.
The applied diffs are from b72a32d76 to 0402a01c.
This tool was used to apply the changes with some manual merge conflicts resolutions required.
Summary by CodeRabbit
Based on the comprehensive summary of changes, here are the high-level release notes:
Release Notes
New Features
Improvements
Changes
9000to9001evmostoosRefactoring
Testing