imp(all): Move latest changes from Evmos main (incl. SDK v50) #48

MalteHerrmann · 2024-09-19T07:24:15Z

This PR moves the latest changes from the https://github.com/evmos/evmos main branch into the OS repository. This includes a bunch of dependency bumps and other refactors, most notably bumping the Cosmos SDK version to v0.50.9.

Summary by CodeRabbit

New Features
- Introduced a benchmarking suite for performance testing of various transaction types in the Evmos blockchain.
- Added an integration test suite for validating Cosmos transaction execution and reward management scenarios.
Improvements
- Streamlined transaction creation and signing processes, enhancing clarity and maintainability.
- Enhanced account management capabilities within the test suite.
Bug Fixes
- Improved accuracy in gas wanted calculations across multiple transaction types.
Documentation
- Updated testing framework to ensure comprehensive coverage for transaction handling and performance analysis.

coderabbitai

Actionable comments posted: 52

Outside diff range and nitpick comments (45)

ante/evm/setup_test.go (1)
16-28: Great improvement in test suite setup and coverage.

The refactoring of the TestAnteTestSuite function is excellent. It simplifies the setup process while ensuring comprehensive test coverage for both current and legacy EIP-712 implementations. Explicitly enabling the London hard fork is a good practice for testing the latest EVM features.

Consider adding a brief comment explaining the purpose of running the suite twice, for improved clarity:
 	})
 
+	// Run the suite again with legacy EIP-712 typed data to ensure backwards compatibility
 	// Re-run the tests with EIP-712 Legacy encodings to ensure backwards compatibility.
 	// LegacyEIP712Extension should not be run with current TypedData encodings, since they are not compatible.
 	suite.Run(t, &AnteTestSuite{
api/os/erc20/v1/msg.go (1)
13-24: LGTM with a suggestion: Consider simplifying the return type.

The GetSigners function is well-implemented with proper type assertion and error handling. However, the return type [][]byte seems unnecessarily complex for a single signer's address.

Consider simplifying the return type to []byte instead of [][]byte. This would make the function signature more straightforward and easier to use. Here's a suggested modification:
- func GetSigners(msg protov2.Message) ([][]byte, error) {
+ func GetSigners(msg protov2.Message) ([]byte, error) {
    // ... (rest of the function remains the same)
-   return [][]byte{sender.Bytes()}, nil
+   return sender.Bytes(), nil
}
This change would simplify the function's usage while maintaining its functionality. Make sure to update any calling code accordingly if you decide to implement this change.
.github/workflows/proto.yml (1)

Line range hint 1-48: Consider updating versions of other actions and approve overall structure.

The workflow structure looks well-organized and efficient. Here are some suggestions:

Consider checking for updates to other actions used in this workflow, such as:

actions/checkout

technote-space/get-diff-action

bufbuild/buf-lint-action

bufbuild/buf-breaking-action

The conditional execution of make proto-gen based on detected changes is a good practice for efficiency.

To check for the latest versions of these actions, you can visit their respective GitHub repositories or use the GitHub Marketplace.
api/os/evm/v1/legacy_tx.go (1)
19-34: LGTM with suggestion: AsEthereumData method correctly converts to Ethereum format.

The AsEthereumData method effectively converts the LegacyTx to an Ethereum-compatible ethtypes.TxData. All necessary fields are properly mapped.

Consider adding error handling for the stringToBigInt and stringToAddress functions. If these functions can return errors, it would be beneficial to propagate them up to the caller. For example:
func (tx *LegacyTx) AsEthereumData() (ethtypes.TxData, error) {
    v, r, s := tx.GetRawSignatureValues()
    gasPrice, err := stringToBigInt(tx.GetGasPrice())
    if err != nil {
        return nil, fmt.Errorf("invalid gas price: %w", err)
    }
    value, err := stringToBigInt(tx.GetValue())
    if err != nil {
        return nil, fmt.Errorf("invalid value: %w", err)
    }
    to, err := stringToAddress(tx.GetTo())
    if err != nil {
        return nil, fmt.Errorf("invalid to address: %w", err)
    }
    return &ethtypes.LegacyTx{
        Nonce:    tx.GetNonce(),
        GasPrice: gasPrice,
        Gas:      tx.GetGas(),
        To:       to,
        Value:    value,
        Data:     tx.GetData(),
        V:        v,
        R:        r,
        S:        s,
    }, nil
}
This change would make the method more robust and allow callers to handle potential conversion errors.
ante/evm/01_setup_ctx_test.go (3)
15-25: LGTM: Test function declaration and setup are well-structured.

The test function is clearly named and the setup is appropriate for testing the EthSetUpContextDecorator. The Ethereum transaction parameters are explicitly defined, which enhances test clarity.

Consider adding a brief comment explaining the purpose of ethContractCreationTxParams to improve code readability. For example:
// ethContractCreationTxParams simulates parameters for an Ethereum contract creation transaction
ethContractCreationTxParams := &evmtypes.EvmTxArgs{
    // ... (existing parameters)
}
26-37: LGTM: Test cases are well-defined and cover basic scenarios.

The test cases are structured clearly and cover both invalid and valid scenarios. This approach allows for easy addition of more test cases in the future.

Consider adding more test cases to increase coverage:

A case with a valid transaction but with extreme gas values to test boundary conditions.

A case with a valid transaction but with a mismatched ChainID to ensure proper error handling.

Example:
testCases := []struct {
    name    string
    tx      sdk.Tx
    expPass bool
}{
    // ... (existing test cases)
    {
        "extreme gas values",
        evmtypes.NewTx(&evmtypes.EvmTxArgs{
            ChainID:  suite.GetNetwork().App.EVMKeeper.ChainID(),
            GasLimit: math.MaxUint64,
            GasPrice: new(big.Int).SetUint64(math.MaxUint64),
            // ... (other necessary fields)
        }),
        true, // or false, depending on expected behavior
    },
    {
        "mismatched ChainID",
        evmtypes.NewTx(&evmtypes.EvmTxArgs{
            ChainID:  big.NewInt(9999), // Assuming this is not the correct ChainID
            // ... (other necessary fields)
        }),
        false,
    },
}
39-52: LGTM: Test execution loop is well-structured with appropriate assertions.

The use of subtests and the differentiation between expected pass and fail cases is well implemented. The additional assertions for passing cases provide good coverage.

To improve error diagnostics, consider adding more context to the error assertions. For example:
if tc.expPass {
    suite.Require().NoError(err, "Expected test case to pass, but got an error")
    suite.Equal(storetypes.GasConfig{}, ctx.KVGasConfig(), "Unexpected KVGasConfig")
    suite.Equal(storetypes.GasConfig{}, ctx.TransientKVGasConfig(), "Unexpected TransientKVGasConfig")
} else {
    suite.Require().Error(err, "Expected test case to fail, but it passed")
}
This will provide more informative output if a test fails unexpectedly.
ante/evm/sigs_test.go (4)
10-15: LGTM: Test setup is appropriate, but consider adding a comment.

The test function TestSignatures is well-named and the initial setup is appropriate. Disabling the fee market and resetting the setup ensures a clean test environment.

Consider adding a brief comment explaining why the fee market is disabled for this test. This would improve the test's readability and maintainability.
+	// Disable fee market for this test as it's not relevant to signature verification
 	suite.WithFeemarketEnabled(false)
 	suite.SetupTest() // reset
17-24: LGTM: Transaction arguments are well-structured.

The transaction arguments cover all necessary fields for an Ethereum transaction, and the values used seem appropriate for a test scenario.

For improved readability, consider adding comments to explain the purpose or significance of each argument, especially for non-obvious values like the gas limit and gas price.
 	txArgs := evmtypes.EvmTxArgs{
 		ChainID:  suite.GetNetwork().App.EVMKeeper.ChainID(),
 		Nonce:    0,
 		To:       &to,
 		Amount:   big.NewInt(10),
-		GasLimit: 100000,
-		GasPrice: big.NewInt(1),
+		GasLimit: 100000, // Arbitrary gas limit for test transaction
+		GasPrice: big.NewInt(1), // Minimal gas price for test
 	}
26-38: LGTM: Transaction creation and initial checks are correct.

The transaction is created correctly, and the checks for empty Cosmos signatures verify the expected behavior. The extraction and unpacking of the Ethereum transaction message are necessary steps for further verification.

Consider adding error checks after creating the transaction and casting the message to improve robustness:
 	tx := suite.CreateTxBuilder(privKey, txArgs).GetTx()
 	sigs, err := tx.GetSignaturesV2()
 	suite.Require().NoError(err)

 	// signatures of cosmos tx should be empty
 	suite.Require().Equal(len(sigs), 0)

 	msg := tx.GetMsgs()[0]
+	suite.Require().NotNil(msg, "Failed to get message from transaction")
 	msgEthTx, ok := msg.(*evmtypes.MsgEthereumTx)
 	suite.Require().True(ok)
 	txData, err := evmtypes.UnpackTxData(msgEthTx.Data)
 	suite.Require().NoError(err)
40-49: LGTM: Signature comparison logic is correct and thorough.

The test correctly extracts and compares all components of the signature (V, R, and S) from both the Ethereum transaction message and the corresponding Ethereum transaction. This ensures that the signature handling logic is functioning correctly within the Evmos framework.

To improve clarity, consider adding a brief comment explaining the significance of this comparison:
+	// Verify that the signatures in MsgEthereumTx match those in the Ethereum transaction
+	// This ensures that the Evmos framework correctly preserves Ethereum transaction signatures
 	suite.Require().Equal(msgV, ethV)
 	suite.Require().Equal(msgR, ethR)
 	suite.Require().Equal(msgS, ethS)
CHANGELOG.md (2)
11-14: LGTM! Consider adding dates to entries for better tracking.

The new changelog entries are well-formatted, consistent, and provide clear information about the recent changes. They align well with the PR objectives, particularly highlighting the integration of latest changes from the Evmos main branch and the SDK v50 update.

To enhance the changelog's usefulness, consider adding dates to each entry. This would help users and developers quickly identify when specific changes were made. For example:
- (all) [#48](https://github.com/evmos/os/pull/48) Move latest changes from evmOS main (f943af3b incl. SDK v50). (2023-09-15)
Line range hint 1-33: Well-structured changelog. Consider adding brief descriptions for non-obvious changes.

The changelog is well-organized and effectively communicates the project's recent improvements. The use of PR links and concise descriptions for each entry is commendable.

For future entries, consider adding brief explanations for changes that might not be immediately clear to all users. For example, the entry:
- (all) [#37](https://github.com/evmos/os/pull/37) Add EVM, feemarket and precompiles from evmOS.
Could be expanded to:
- (all) [#37](https://github.com/evmos/os/pull/37) Add EVM, feemarket and precompiles from evmOS to enhance compatibility and functionality with Ethereum-based systems.
This additional context can help users better understand the impact and purpose of each change.
ante/evm/11_emit_event.go (1)
32-37: Approve changes with a minor suggestion for error handling

The addition of the nil check for msgs is a good improvement in robustness. It prevents potential runtime errors that could occur if msgs were nil.

Consider using a more specific error type from the errortypes package for consistency with the rest of the codebase. For example:
-		return ctx, errorsmod.Wrap(errortypes.ErrUnknownRequest, "invalid transaction. Transaction without messages")
+		return ctx, errorsmod.Wrap(errortypes.ErrInvalidRequest, "invalid transaction: no messages")
This change uses ErrInvalidRequest which might be more appropriate for this case, and slightly adjusts the error message for conciseness.
ante/evm/eth_benchmark_test.go (3)
20-50: LGTM: Benchmark setup is comprehensive, but consider adding more test cases.

The benchmark function is well-structured and initializes all necessary components. However, to improve coverage and provide a more comprehensive performance evaluation, consider adding more test cases with different scenarios.

For example, you could add test cases for:

Transactions with insufficient funds

Transactions with different gas prices

Transactions with varying payload sizes

51-93: LGTM: Benchmark execution is well-structured, but consider enhancing error handling.

The benchmark loop and execution are correctly implemented, ensuring accurate measurement of the ConsumeFeesAndEmitEvent function's performance. The use of timer controls is appropriate.

Consider moving the error check inside the timed section to include it in the performance measurement, as error handling is part of the function's overall performance:
 b.StartTimer()

 err := ethante.ConsumeFeesAndEmitEvent(
   cacheCtx.WithIsCheckTx(true).WithGasMeter(storetypes.NewInfiniteGasMeter()),
   &keepers,
   fees,
   bechAddr,
 )
+s.Require().NoError(err)

 b.StopTimer()
-s.Require().NoError(err)
1-94: Enhance documentation and use constants for magic numbers.

The overall structure of the benchmark is good, but there are a few areas for improvement:

Consider adding a package-level comment to explain the purpose of this benchmark file.

Use constants for magic numbers to improve readability and maintainability.

Here's an example of how you could implement these suggestions:
// Package evm_test provides benchmarks for the Ethereum Virtual Machine (EVM) module.
// This file specifically benchmarks the gas consumption logic for Ethereum transactions.
package evm_test

import (
    // ... existing imports ...
)

const (
    benchmarkInitialBalance = 1e16
    benchmarkGasLimit       = 1_000_000
    benchmarkGasPrice       = 1_000_000
)

// ... rest of the file ...

func BenchmarkEthGasConsumeDecorator(b *testing.B) {
    // ... existing setup ...

    args := &evmtypes.EvmTxArgs{
        // ... other fields ...
        GasLimit: uint64(benchmarkGasLimit),
        GasPrice: big.NewInt(benchmarkGasPrice),
    }

    testCases := []struct {
        name    string
        balance sdkmath.Int
        rewards sdkmath.Int
    }{
        {
            "legacy tx - enough funds to pay for fees",
            sdkmath.NewInt(benchmarkInitialBalance),
            sdkmath.ZeroInt(),
        },
    }

    // ... rest of the function ...
}
ante/evm/05_signature_verification.go (1)
43-47: Approve changes with a minor suggestion for improvement

The addition of a nil check for messages is a good improvement to the robustness of the AnteHandle method. It ensures that only valid transactions with messages are processed.

Consider using a more specific error type instead of errortypes.ErrUnknownRequest. For example, you could define a new error type like ErrNoMessages in the appropriate error types file:
var ErrNoMessages = sdkerrors.Register(ModuleName, 1, "transaction contains no messages")
Then use it in this method:
if msgs == nil {
    return ctx, errorsmod.Wrap(ErrNoMessages, "invalid transaction")
}
This would provide more precise error handling and make it easier for clients to handle this specific error case.
ante/evm/10_gas_wanted_test.go (1)

73-78: LGTM: Fee market parameter update refactored and gas meter instantiation updated.

The changes improve code structure by using a utility function for updating fee market parameters. The gas meter instantiation is consistently updated.

Consider extracting the fee market parameter update into a separate helper function within the test suite to improve readability and reusability across test cases.

Also applies to: 81-81
ante/evm/04_validate.go (2)
60-65: Improved type safety in transaction validation.

The changes enhance type safety by introducing a type assertion for sdktypes.HasValidateBasic. This prevents potential runtime panics by ensuring that only transactions with the ValidateBasic method are validated.

Consider using a custom error type or adding more context to the error message. This could help in distinguishing between different validation failures:
 if t, ok := tx.(sdktypes.HasValidateBasic); ok {
 	err := t.ValidateBasic()
 	// ErrNoSignatures is fine with eth tx
 	if err != nil && !errors.Is(err, errortypes.ErrNoSignatures) {
-		return nil, errorsmod.Wrap(err, "tx basic validation failed")
+		return nil, errorsmod.Wrap(err, "ethereum tx basic validation failed")
 	}
 }
Line range hint 1-114: Summary: Changes align with PR objectives and enhance code robustness.

The modifications in this file contribute to the PR's goal of integrating latest updates and refactoring efforts. The changes improve type safety in transaction validation and update the fee comparison logic, which aligns with the objective of enhancing the functionality and performance of the repository.

As the codebase continues to evolve, consider the following architectural recommendations:

Implement comprehensive unit tests for the ValidateTx and CheckTxFee functions to ensure robustness across different transaction types and fee scenarios.

Document the rationale behind allowing ErrNoSignatures to pass without error wrapping in the ValidateTx function, as this might not be immediately clear to future maintainers.

Consider extracting the fee comparison logic into a separate, easily testable function to improve modularity and facilitate future updates to fee validation mechanisms.
ethereum/eip712/message.go (1)
102-104: Approve the null check with a suggestion for error logging.

The addition of the null check improves the robustness of the getPayloadMessages function by gracefully handling cases where rawMsgs is null. This change prevents potential errors when processing null values and aligns with good defensive programming practices.

Consider adding a debug log when a null rawMsgs is encountered. This can help with troubleshooting and monitoring the frequency of this edge case. Here's a suggested modification:
 if rawMsgs.Type == gjson.Null {
+    // TODO: Add debug logging here
+    // e.g., log.Debug("Encountered null rawMsgs in getPayloadMessages")
     return []gjson.Result{}, nil
 }
This logging can provide valuable insights into the occurrence of null payloads in your system.
example_chain/ante/handler_options_test.go (1)

16-175: Consider adding more edge cases and improving consistency

The test cases are well-structured and cover many scenarios. However, consider the following suggestions:

Consistency: Some fields (e.g., IBCKeeper) are sometimes set to nil explicitly, while others are omitted. Consider using a consistent approach for all fields.

Additional edge cases:

Test with invalid types for each field (e.g., wrong keeper types).

Test with zero values for numeric fields like MaxTxGasWanted.

Positive cases: Consider adding more positive cases with different valid configurations to ensure the validation logic isn't too strict.

Boundary testing: For MaxTxGasWanted, consider testing boundary values (e.g., 0, 1, MaxUint64).

These suggestions could further enhance the robustness of your test suite.
ante/cosmos/min_gas_price_test.go (1)
6-7: LGTM! Minor suggestion for import grouping.

The changes to imports and the denom assignment look good. They reflect a reorganization of the testutil package and improve code readability.

Consider grouping the imports more systematically. For example:
import (
	"fmt"

	"cosmossdk.io/math"

	sdk "github.com/cosmos/cosmos-sdk/types"
	banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"

	"github.com/evmos/os/ante/cosmos"
	"github.com/evmos/os/testutil"
	"github.com/evmos/os/testutil/constants"
	testutiltx "github.com/evmos/os/testutil/tx"
)
This grouping separates standard library imports, external dependencies, and internal packages, making the import section more organized.

Also applies to: 9-9, 27-27
ethereum/eip712/preprocess_test.go (1)

103-110: Improved fee payer address verification using Bech32Codec.

The introduction of Bech32Codec for address conversion is a positive change:

It ensures correct handling of bech32 addresses in the test.

The use of addrCodec.BytesToString provides a more robust way to convert and compare fee payer addresses.

Utilizing sdk.GetConfig().GetBech32AccountAddrPrefix() maintains consistency with the current SDK configuration.

These changes enhance the accuracy and reliability of the fee payer verification process.

Consider extracting the Bech32Codec initialization to a separate function or variable to improve readability and reusability, especially if it's used in multiple test cases.
ethereum/eip712/encoding_legacy.go (1)
231-235: Approve changes in legacyValidatePayloadMessages and suggest optimization.

The changes in this function are consistent with the new method of obtaining signers using protoCodec.GetMsgV1Signers(msg). The logic for validating payload messages remains intact.

Consider a minor optimization to avoid repeated array indexing:
-		if !msgSigner.Equals(sdk.AccAddress(signers[0])) {
+		signer := signers[0]
+		if !msgSigner.Equals(sdk.AccAddress(signer)) {
This change improves readability and prevents potential repeated array access.

Also applies to: 241-241, 249-249
Makefile (2)
81-90: LGTM: Improved linting structure

The new lint targets provide more granular control over linting different parts of the project. The use of gofumpt and golangci-lint for Go, pylint and flake8 for Python, and solhint for Solidity contracts is appropriate and follows good practices.

Consider adding a lint-all target that runs all linting tasks for convenience:
lint-all: lint-go lint-python lint-contracts
Line range hint 102-119: LGTM: Improved formatting structure

The new format targets provide more granular control over formatting different parts of the project. The exclusion of certain files in Go formatting, the use of both isort and black for Python, and the addition of shell script formatting using shfmt are all good practices. The update to the .PHONY declaration is also appropriate.

Consider adding a format-all target that runs all formatting tasks for convenience:
format-all: format-go format-python format-shell
ante/cosmos/eip712.go (1)

145-145: LGTM: Updated VerifySignature function call

The VerifySignature function call has been updated to remove the signModeHandler parameter, which is consistent with the changes made to the struct definition and constructor. This change maintains the coherence of the code.

Consider updating the function documentation for VerifySignature to reflect the removal of the signModeHandler parameter if it hasn't been done already.
.gitleaks.toml (2)
23-25: New stopwords added: Approved with suggestion

The addition of these three addresses (two hexadecimal and one Evmos) to the stopwords list is beneficial. This will prevent false positives for these specific addresses, which are likely public or test addresses used in the project.

However, for better maintainability and clarity, consider adding comments to explain the purpose or origin of these addresses. For example:
stopwords = [
    # ... (previous entries)
    # Public Ethereum address for project XYZ
    '''0x8FA78CEB7F04118Ec6d06AaC37Ca854691d8e963''',
    # Test Ethereum address
    '''0x205CF44075E77A3543abC690437F3b2819bc450a''',
    # Evmos test address
    '''evmos10d07y265gmmuvt4z0w9aw880jnsr700jcrztvm''',
]
This will help future maintainers understand why these specific addresses are in the stopwords list.

Line range hint 1-7: Consider documenting the process for custom rule additions

The file header indicates that this configuration is auto-generated. While this ensures consistency and comprehensive coverage of common secret patterns, it may pose challenges for adding project-specific custom rules or modifications.

To address this, consider:

Documenting the process for regenerating this file, including the command and any required inputs.

Creating a separate custom rules file (e.g., .gitleaks.custom.toml) that can be merged with the auto-generated file.

Updating the generation process to incorporate the custom rules file, ensuring that project-specific rules are not lost during regeneration.

This approach would maintain the benefits of the auto-generated rules while allowing for necessary customizations.
api/os/evm/v1/msg.go (1)
32-33: Adjust function comment to follow GoDoc conventions

The comment for the exported function GetSigners should start with the function name and describe its purpose, to align with GoDoc conventions. Consider rephrasing the comment as:
// GetSigners retrieves the signer's address from the Ethereum transaction signature.
encoding/config.go (1)

52-57: Ensure consistent encoding configuration across the application

While the returned sdktestutil.TestEncodingConfig includes the updated codecs and configurations, verify that all modules and transaction configurations throughout the application are using this updated encoding configuration to prevent any discrepancies.
ante/evm/signverify_test.go (2)
68-83: Ensure proper capture of loop variable in subtests

When running subtests within a loop, it's important to ensure that the loop variable tc is correctly captured within each subtest closure to prevent potential issues due to variable scoping. You can capture the variable by re-declaring it within the loop:
 for _, tc := range testCases {
+    tc := tc // capture loop variable
     suite.Run(tc.name, func() {
         // test code
     })
 }
This ensures that each subtest operates on the correct test case data.

84-84: Add comment to explain resetting EVM parameters

At the end of the test function, suite.WithEvmParamsOptions(nil) is called to reset the EVM parameters to their default values. Consider adding a comment to clarify this action, which will improve code readability and assist future maintainers.
example_chain/ante/evm_benchmark_test.go (1)
32-59: Rename table to benchmarks for improved clarity

The variable table holds benchmark configurations. Renaming it to benchmarks enhances readability and better conveys its purpose.

Apply this diff:
-var table = []struct {
+var benchmarks = []struct {
And update all references from table to benchmarks throughout the code.
ante/testutils/testutil.go (2)

64-72: Add comments to clarify EVM upgrade disablement

When disabling the London Hard Fork and subsequent EVM upgrades by setting their block numbers to math.MaxInt64, consider adding comments to explain this logic. This will enhance code readability and help future maintainers understand the purpose behind these configurations.

83-86: Consider documenting custom network initialization

Adding comments to the network initialization section can improve maintainability. Documenting the reasons for custom genesis states and any specific configurations will assist others in understanding the setup process.

example_chain/ante/integration_test.go (1)

57-57: Typographical error in comment

There's a typo in the comment on line 57. The word "minimun" should be "minimum".
ante/evm/utils_test.go (3)
128-128: Inconsistent denomination usage

In NewMsgCreateValidator, the coin denomination uses testconstants.ExampleAttoDenom, whereas other functions use suite.GetNetwork().GetDenom(). For consistency and to ensure the correct denomination is used, consider replacing testconstants.ExampleAttoDenom with suite.GetNetwork().GetDenom().

Apply this diff for consistency:
- sdk.NewCoin(testconstants.ExampleAttoDenom, sdkmath.NewInt(20)),
+ sdk.NewCoin(suite.GetNetwork().GetDenom(), sdkmath.NewInt(20)),
571-573: Formatting improvement for chained context modifications

The return statement is split in a way that might reduce readability. Consider adjusting the formatting for better clarity.

Consider reformatting the code as follows:
return ctx.
    WithBlockGasMeter(storetypes.NewGasMeter(1e19)).
    WithBlockHeight(ctx.BlockHeight() + 1)
50-55: Consider additional error handling

In the CreateTxBuilder function, after signing the Ethereum transaction, it's good practice to ensure that all potential errors are handled appropriately. While suite.Require().NoError(err) is used, consider if additional logging or error messages might be beneficial for debugging purposes.
ante/evm/ante_test.go (3)

1173-1181: Ensure EVM parameters are reset after each test

While the EVM parameters are modified using suite.WithEvmParamsOptions, ensure that they are properly reset after each test to prevent side effects on subsequent tests. Although defer suite.ResetEvmParamsOptions() is used, double-check that all parameter changes are correctly reverted to maintain test isolation.

170-177: Correct indentation for consistent code formatting

Lines within this test case are not consistently indented, which might affect readability. Ensure that the code follows the standard Go formatting conventions.

847-860: Clarify the purpose of modifying the message after signing

In the test case "Fails - Multi-Key with messages added after signing", the message is modified after signatures are applied. It might not be immediately clear why this is done. Adding comments to explain the intent can improve readability and understanding for future maintainers.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d22f510 and fbacf4a.

Files ignored due to path filters (10)

api/os/erc20/v1/query_grpc.pb.go is excluded by !**/*.pb.go
api/os/erc20/v1/tx_grpc.pb.go is excluded by !**/*.pb.go
api/os/evm/v1/query_grpc.pb.go is excluded by !**/*.pb.go
api/os/evm/v1/tx_grpc.pb.go is excluded by !**/*.pb.go
api/os/feemarket/v1/query_grpc.pb.go is excluded by !**/*.pb.go
api/os/feemarket/v1/tx_grpc.pb.go is excluded by !**/*.pb.go
example_chain/go.sum is excluded by !**/*.sum
go.sum is excluded by !**/*.sum
go.work is excluded by !**/*.work
go.work.sum is excluded by !**/*.sum

Files selected for processing (74)

.github/workflows/auto-format.yml (1 hunks)
.github/workflows/bsr-push.yml (1 hunks)
.github/workflows/proto.yml (2 hunks)
.github/workflows/super-linter.yml (1 hunks)
.gitignore (1 hunks)
.gitleaks.toml (1 hunks)
.pylintrc (3 hunks)
CHANGELOG.md (1 hunks)
Makefile (4 hunks)
ante/cosmos/authz.go (1 hunks)
ante/cosmos/authz_test.go (14 hunks)
ante/cosmos/eip712.go (4 hunks)
ante/cosmos/min_gas_price_test.go (12 hunks)
ante/cosmos/setup_test.go (1 hunks)
ante/cosmos/utils_test.go (4 hunks)
ante/evm/01_setup_ctx.go (2 hunks)
ante/evm/01_setup_ctx_test.go (1 hunks)
ante/evm/04_validate.go (2 hunks)
ante/evm/04_validate_test.go (10 hunks)
ante/evm/05_signature_verification.go (1 hunks)
ante/evm/07_can_transfer.go (1 hunks)
ante/evm/08_gas_consume_test.go (3 hunks)
ante/evm/09_increment_sequence.go (1 hunks)
ante/evm/09_increment_sequence_test.go (2 hunks)
ante/evm/10_gas_wanted_test.go (6 hunks)
ante/evm/11_emit_event.go (1 hunks)
ante/evm/ante_test.go (1 hunks)
ante/evm/eth_benchmark_test.go (1 hunks)
ante/evm/fee_checker_test.go (10 hunks)
ante/evm/fee_market_test.go (1 hunks)
ante/evm/setup_test.go (1 hunks)
ante/evm/signverify_test.go (1 hunks)
ante/evm/sigs_test.go (1 hunks)
ante/evm/utils_test.go (15 hunks)
ante/interfaces/cosmos.go (1 hunks)
ante/sigverify.go (1 hunks)
ante/sigverify_test.go (2 hunks)
ante/testutils/testutil.go (1 hunks)
api/os/crypto/v1/ethsecp256k1/keys.pulsar.go (1 hunks)
api/os/erc20/v1/genesis.pulsar.go (1 hunks)
api/os/erc20/v1/msg.go (1 hunks)
api/os/evm/v1/access_list_tx.go (1 hunks)
api/os/evm/v1/dynamic_fee_tx.go (1 hunks)
api/os/evm/v1/events.pulsar.go (1 hunks)
api/os/evm/v1/genesis.pulsar.go (1 hunks)
api/os/evm/v1/legacy_tx.go (1 hunks)
api/os/evm/v1/msg.go (1 hunks)
api/os/evm/v1/tx_data.go (1 hunks)
api/os/feemarket/v1/events.pulsar.go (1 hunks)
api/os/feemarket/v1/feemarket.pulsar.go (1 hunks)
api/os/feemarket/v1/genesis.pulsar.go (1 hunks)
api/os/feemarket/v1/tx.pulsar.go (1 hunks)
api/os/types/v1/dynamic_fee.pulsar.go (1 hunks)
api/os/types/v1/indexer.pulsar.go (1 hunks)
api/os/types/v1/web3.pulsar.go (1 hunks)
client/block/store.go (3 hunks)
client/keys/add.go (3 hunks)
client/keys/utils.go (5 hunks)
cmd/config/opendb.go (1 hunks)
contracts/hardhat.config.js (1 hunks)
encoding/config.go (1 hunks)
encoding/config_test.go (1 hunks)
ethereum/eip712/eip712_test.go (13 hunks)
ethereum/eip712/encoding.go (3 hunks)
ethereum/eip712/encoding_legacy.go (5 hunks)
ethereum/eip712/message.go (1 hunks)
ethereum/eip712/preprocess.go (2 hunks)
ethereum/eip712/preprocess_test.go (6 hunks)
example_chain/ante/cosmos_handler.go (1 hunks)
example_chain/ante/evm_benchmark_test.go (1 hunks)
example_chain/ante/evm_handler.go (1 hunks)
example_chain/ante/handler_options.go (2 hunks)
example_chain/ante/handler_options_test.go (1 hunks)
example_chain/ante/integration_test.go (1 hunks)

Files skipped from review due to trivial changes (2)

ante/sigverify.go
example_chain/ante/evm_handler.go

Additional context used

GitHub Check: CodeQL

api/os/crypto/v1/ethsecp256k1/keys.pulsar.go

[notice] 12-12: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

api/os/erc20/v1/genesis.pulsar.go

[notice] 13-13: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

api/os/evm/v1/events.pulsar.go

[notice] 11-11: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

api/os/evm/v1/genesis.pulsar.go

[notice] 13-13: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

api/os/feemarket/v1/events.pulsar.go

[notice] 11-11: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

api/os/feemarket/v1/feemarket.pulsar.go

[notice] 13-13: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

api/os/feemarket/v1/genesis.pulsar.go

[notice] 13-13: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

api/os/feemarket/v1/tx.pulsar.go

[notice] 15-15: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

api/os/types/v1/dynamic_fee.pulsar.go

[notice] 13-13: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

api/os/types/v1/indexer.pulsar.go

[notice] 12-12: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

api/os/types/v1/web3.pulsar.go

[notice] 12-12: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

Additional comments not posted (149)

contracts/hardhat.config.js (1)

3-3: Approved: Solidity compiler version update to 0.8.20

Updating the Solidity compiler version to 0.8.20 is a good practice as it likely includes bug fixes, security improvements, and potentially new features or optimizations.

To ensure a smooth transition, please:

Verify compatibility with all existing contracts in the project.

Review the Solidity 0.8.20 changelog for any new features or potential breaking changes that might affect your codebase.

Run all tests and check for any new compiler warnings or errors after the update.

You can use the following script to check for any compatibility issues:

This script will help identify any immediate issues with the compiler version update.

.gitignore (1)

31-33: LGTM! Good addition to ignore build files.

The new entry to ignore example_chain/build/ is a valuable addition to the .gitignore file. This change:

Prevents build artifacts from being accidentally committed, keeping the repository clean.

Aligns with best practices for version control in software development.

Improves consistency across different development environments.

The blank line separator also enhances the readability of the file.
.github/workflows/bsr-push.yml (1)
16-16: LGTM! Consider checking the changelog for notable updates.

The update of bufbuild/buf-setup-action from v1.39.0 to v1.41.0 is a good practice to keep the workflow up-to-date. This change aligns with the PR objective of incorporating the latest changes from Evmos main.

To ensure there are no breaking changes or important updates, you may want to check the changelog:

Verification successful

Verified: The update of bufbuild/buf-setup-action to v1.41.0 has been reviewed. The release notes do not indicate any breaking changes or important updates, ensuring the workflow remains stable.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Fetch the changelog or release notes for bufbuild/buf-setup-action
gh release view v1.41.0 --repo bufbuild/buf-setup-action
Length of output: 309
ante/evm/setup_test.go (3)

6-6: LGTM: Import of testutils package.

The addition of the testutils package import is a good practice. It suggests a move towards more modular and reusable test utilities.

11-13: Excellent refactoring of AnteTestSuite struct.

The embedding of *testutils.AnteTestSuite is a great improvement. It simplifies the struct, reduces code duplication, and promotes better code organization. Retaining the useLegacyEIP712TypedData field allows for maintaining backwards compatibility tests, which is crucial for ensuring robustness across different EIP-712 implementations.

1-28: Excellent refactoring and improvement of the test suite.

The changes made to this file represent a significant improvement in several areas:

Code organization: Moving common test utilities to a separate package enhances modularity and reusability.

Reduced duplication: Embedding the base AnteTestSuite eliminates the need for repetitive setup code.

Comprehensive testing: Running the suite with both current and legacy EIP-712 implementations ensures backwards compatibility and robust coverage.

Simplicity and maintainability: The overall simplification of the test setup makes the code more readable and easier to maintain.

These improvements align well with Go best practices for testing and will likely make future modifications and additions to the test suite easier to implement.

api/os/erc20/v1/msg.go (1)

1-11: LGTM: File structure and imports are well-organized.

The file structure follows Go best practices, and the imports are appropriate for the functionality provided. The use of aliasing for the protobuf import (protov2) is a good practice to avoid potential naming conflicts.

ante/evm/09_increment_sequence.go (1)

17-17: LGTM! Verify compatibility with existing account types.

The change from authtypes.AccountI to sdk.AccountI aligns with the Cosmos SDK v0.50.9 upgrade mentioned in the PR objectives. This modification likely broadens the compatibility of the IncrementNonce function with various account types.

To ensure this change doesn't introduce any compatibility issues, please run the following script to check for any custom account types that might be affected:
.github/workflows/auto-format.yml (8)
9-9: LGTM: Job name update accurately reflects expanded scope.

The change from format-go-code to format-code better represents the job's expanded functionality, which now includes formatting for both Go and Python.

30-31: LGTM: Go formatter (gofumpt) installation.

Moving the Go formatter installation after Go setup is logical. Using gofumpt, a stricter version of gofmt, is a good choice for maintaining high code quality standards.

47-47: LGTM: Updated commit message for auto-formatting.

The new commit message "run make format" is concise and accurately reflects the consolidated formatting approach.

Line range hint 1-47: Overall assessment: Improved and streamlined formatting workflow.

The changes to this workflow file significantly improve the code formatting process:

Expanded scope to include Go, Python, and shell script formatting.

Updated to latest versions of Go (1.22) and Python (3.10).

Consolidated formatting commands into a single make format command.

Improved job naming and commit message for clarity.

These changes should lead to more consistent code style across the project and a more maintainable formatting workflow.

21-22: LGTM: Addition of shell formatter (shfmt).

Including shell script formatting is a good practice for maintaining consistent code style across all file types.

To ensure shfmt is being utilized, please run the following script:

Verification successful

Verified: shfmt Installation and Usage Confirmed

shfmt is successfully installed and utilized within the Makefile at line 116 to format shell scripts. No issues found with the shell formatter setup.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of shfmt in the project

# Test: Check if there's a command to run shfmt in the Makefile
grep -n 'shfmt' Makefile

# Test: Look for shell scripts in the repository
fd -e sh -e bash
Length of output: 850

Script:
#!/bin/bash
# Description: Verify the usage of shfmt in the project

# Test: Check if there's a command to run shfmt in the Makefile
grep -n 'shfmt' Makefile

# Test: Look for shell scripts in the repository
fd -e sh -e bash
Length of output: 209
36-36: LGTM: Python version update to 3.10.

Updating to Python 3.10 is a good practice for leveraging newer language features and improvements.

Please ensure all dependencies and code are compatible with Python 3.10. Run the following script to check for any compatibility issues:

27-28: LGTM: Go version update to 1.22.

Updating to Go 1.22 and adding check-latest: true is a good practice for security and performance.

Please ensure all dependencies and code are compatible with Go 1.22. Run the following script to check for any compatibility issues:

41-42: LGTM: Combined format command using make format.

Consolidating formatting commands into a single make format command simplifies the workflow and makes it easier to maintain.

Please ensure that the Makefile has been updated to include formatting for all necessary file types. Run the following script to verify the format target in the Makefile:

Verification successful

Verified: Combined make format command correctly implements all formatting steps.

The format target in the Makefile includes all necessary formatting commands (gofumpt, black, isort, shfmt, and python3). No occurrences of thon were found in the codebase, indicating that the error thon: command not found is unrelated to the Makefile changes.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the format target in the Makefile

# Test: Check the format target in the Makefile
sed -n '/^format:/,/^[^\t]/ p' Makefile

# Test: Look for individual formatting commands in the Makefile
grep -n 'gofumpt\|black\|isort\|shfmt' Makefile
Length of output: 850

Script:
#!/bin/bash
# Description: Check for potential typos in formatting commands within the Makefile

# Search for the term 'thon' to identify possible typos
grep -n 'thon' Makefile

# Verify that all expected formatting tools are correctly referenced
grep -n -E 'gofumpt|black|isort|shfmt|python' Makefile
Length of output: 1696

Script:
#!/bin/bash
# Description: Search the entire codebase for occurrences of 'thon'

# Search for the term 'thon' in all files, excluding binary files
rg --ignore-dir=node_modules --ignore-dir=vendor 'thon' .
Length of output: 500

Script:
#!/bin/bash
# Description: Search the entire codebase for occurrences of 'thon' using fd and grep

# Use fd to find all files excluding 'node_modules' and 'vendor', then grep for 'thon'
fd --type f --exclude node_modules --exclude vendor . | xargs grep -n 'thon'
Length of output: 1195
encoding/config_test.go (2)
42-42: Update in commented-out code: tmtypes to cmttypes

While this line is commented out and doesn't affect the current functionality, the change from tmtypes.Tx(bz) to cmttypes.Tx(bz) suggests a potential update in the underlying libraries or types being used. This could be related to a migration from Tendermint to CometBFT.

Consider updating the comment above this line to reflect the current state and reason for the change:
- // FIXME: transaction hashing is hardcoded on Tendermint:
+ // FIXME: transaction hashing was hardcoded on Tendermint, now potentially on CometBFT:
Let's check if there are other occurrences of tmtypes that might need updating:
#!/bin/bash
# Description: Check for other occurrences of tmtypes in the codebase

echo "Checking for occurrences of tmtypes:"
rg --type go "tmtypes"

echo "\nChecking for occurrences of cmttypes:"
rg --type go "cmttypes"
34-34: Configuration setup has been simplified.

The change from app.ModuleBasics to encoding.MakeConfig() suggests a move towards a more modular and encapsulated configuration setup. This approach can improve maintainability and reduce dependencies between packages.

To ensure this change doesn't have unintended consequences, let's verify its usage across the codebase:

Verification successful

All references to ModuleBasics have been removed and replaced with encoding.MakeConfig(). Configuration changes are consistent across the codebase.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check for other occurrences of MakeConfig and ModuleBasics

echo "Checking for other occurrences of MakeConfig:"
rg --type go "MakeConfig"

echo "\nChecking for remaining occurrences of ModuleBasics:"
rg --type go "ModuleBasics"
Length of output: 11196

Script:
#!/bin/bash
# Description: Check for occurrences of MakeConfig and ModuleBasics with corrected syntax

echo -e "Checking for other occurrences of MakeConfig:"
rg --type go "MakeConfig"

echo -e "\nChecking for remaining occurrences of ModuleBasics:"
rg --type go "ModuleBasics"
Length of output: 953
client/keys/utils.go (4)
13-13: LGTM: Import statement added for keys package

The addition of the import for "github.com/cosmos/cosmos-sdk/client/keys" is consistent with the changes in type signatures from cryptokeyring.KeyOutput to keys.KeyOutput. This change appears to be part of a larger refactoring effort to use the keys package for KeyOutput related functionality.

Line range hint 1-62: Summary: Consistent refactoring to use keys.KeyOutput

The changes in this file are part of a larger refactoring effort to use the keys package instead of cryptokeyring for KeyOutput functionality. The modifications are consistent throughout the file, changing from cryptokeyring.KeyOutput to keys.KeyOutput in type signatures and function parameters.

Key points:

Import statement added for the keys package.

bechKeyOutFn type signature updated.

printTextRecords function signature updated.

The internal logic of the functions remains unchanged, suggesting that the structure of KeyOutput is likely the same between the two packages. This refactoring appears to be well-planned and consistently applied.

To ensure the changes have been applied correctly across the entire codebase, please run the verification scripts provided in the previous comments. These will help identify any remaining uses of cryptokeyring.KeyOutput and confirm that all relevant code is now using keys.KeyOutput.

51-51: LGTM: Updated printTextRecords function signature

The change from []cryptokeyring.KeyOutput to []keys.KeyOutput in the printTextRecords function signature is consistent with the previous changes in the file. The internal logic of the function remains unchanged, which is appropriate if the structure of KeyOutput remains the same between the two packages.

To ensure all callers of this function have been updated correctly, please run the following verification script:

This script will help identify all calls to printTextRecords and verify that they are passing the correct keys.KeyOutput type. If there are any discrepancies, they should be addressed to maintain consistency across the codebase.

Verification successful

Verified: printTextRecords function signature correctly updated

All instances of printTextRecords within the codebase are passing keys.KeyOutput as intended. No issues found with the function signature change.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check for all callers of printTextRecords and verify they're passing the correct type

# Test: Find all calls to printTextRecords
rg --type go 'printTextRecords\('

# Test: Verify that all calls are passing keys.KeyOutput
rg --type go 'printTextRecords\(.*keys\.KeyOutput'
Length of output: 438
23-23: LGTM: Updated bechKeyOutFn type signature

The change from cryptokeyring.KeyOutput to keys.KeyOutput in the bechKeyOutFn type signature is consistent with the import changes and the overall refactoring effort.

To ensure this change has been applied consistently throughout the codebase, please run the following verification script:

This script will help identify any places where the old type might still be in use and confirm that the new type is being used consistently.

Verification successful

Verified: bechKeyOutFn type signature updated correctly

The change from cryptokeyring.KeyOutput to keys.KeyOutput has been consistently applied throughout the codebase.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check for any remaining uses of cryptokeyring.KeyOutput

# Test: Search for any remaining uses of cryptokeyring.KeyOutput
rg --type go 'cryptokeyring\.KeyOutput'

# Test: Confirm all uses of KeyOutput are now from the keys package
rg --type go 'keys\.KeyOutput'
Length of output: 438
cmd/config/opendb.go (1)
12-12: LGTM! Verify new package compatibility.

The import change from github.com/cometbft/cometbft-db to github.com/cosmos/cosmos-db aligns with the PR objective of updating dependencies, particularly the Cosmos SDK upgrade. The unchanged dbm alias suggests API compatibility, but it's worth verifying this.

To ensure full compatibility, run the following script:

This script will help identify any potential compatibility issues or necessary adjustments in the codebase due to the package change.

Verification successful

Verified: Import Change Maintains Compatibility

The update from github.com/cometbft/cometbft-db to github.com/cosmos/cosmos-db preserves the dbm alias consistently across the codebase. The ast-grep and rg results confirm that all necessary types and functions remain intact, ensuring compatibility with existing implementations.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the compatibility of the new cosmos-db package

# Test 1: Check if the new package provides all necessary types and functions
ast-grep --lang go --pattern 'dbm.$_'

# Test 2: Verify if there are any breaking changes in the new package
rg --type go 'dbm\.' -A 3 -B 3
Length of output: 14389
.github/workflows/proto.yml (1)
35-35: Version update for bufbuild/buf-setup-action looks good.

The version of bufbuild/buf-setup-action has been updated from v1.39.0 to v1.41.0 in both the lint and break-check jobs. This update is consistent and likely brings improvements or bug fixes from the action's maintainers.

To ensure this update is comprehensive, let's check if there are any other instances of this action in the file or related files:

Also applies to: 44-44

Verification successful

All instances of bufbuild/buf-setup-action are updated to version v1.41.0 across the workflow files.

.github/workflows/proto.yml: Updated to v1.41.0 in both instances.

.github/workflows/bsr-push.yml: Already using v1.41.0.

This ensures consistency and leverages the latest improvements or bug fixes from the action's maintainers.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for any other instances of bufbuild/buf-setup-action in workflow files
rg --type yaml 'bufbuild/buf-setup-action@v' .github/workflows/
Length of output: 294
api/os/evm/v1/legacy_tx.go (5)

1-4: LGTM: Proper file header and package declaration.

The file header includes the necessary copyright notice and SPDX license identifier. The package declaration evmv1 is appropriate for the file's purpose and location.

6-11: LGTM: Appropriate imports for the file's functionality.

The import statements are well-organized and include necessary packages from both the standard library and external sources. The use of aliased imports for ethtypes and ethutils is a good practice to avoid naming conflicts and improve code readability.

13-17: LGTM: GetChainID method correctly derives the chain ID.

The GetChainID method effectively extracts the chain ID from the transaction's signature values using the ethutils.DeriveChainID function. This approach is consistent with Ethereum's EIP-155 for replay protection.

36-45: LGTM: GetRawSignatureValues and GetAccessList methods are correctly implemented.

Both methods are implemented correctly:

GetRawSignatureValues properly uses ethutils.RawSignatureValues to handle the conversion of signature values. The comment correctly warns that return values should not be modified.

GetAccessList correctly returns nil, which is appropriate for legacy transactions that don't use access lists.

These implementations align with the expected behavior for legacy Ethereum transactions.

1-45: Overall, the implementation of legacy_tx.go is well-structured and correctly handles legacy Ethereum transactions.

The file successfully implements the necessary methods for managing legacy Ethereum transactions within the Evmos framework. Key points:

Proper file structure with correct copyright and package declarations.

Well-organized imports using appropriate aliasing.

Correct implementation of GetChainID, AsEthereumData, GetRawSignatureValues, and GetAccessList methods.

Good use of utility functions and appropriate comments.

The only suggestion for improvement is to consider adding error handling in the AsEthereumData method for the string conversion functions.

This implementation provides a solid foundation for handling legacy transactions in the Evmos ecosystem.

ante/evm/01_setup_ctx_test.go (1)

1-13: LGTM: Package declaration and imports are appropriate.

The package name and imports are well-structured and relevant to the test suite's requirements. No issues found.

ante/evm/sigs_test.go (2)

1-8: LGTM: Package declaration and imports are appropriate.

The package name evm_test follows the Go convention for test files, and the imports are relevant to the test being performed.

1-49: Overall, the test implementation is well-structured and comprehensive.

This new test file, sigs_test.go, introduces a thorough test for verifying the signature handling of Ethereum transactions within the Evmos blockchain framework. The test covers all necessary steps from transaction creation to signature verification, ensuring that the Evmos framework correctly preserves and handles Ethereum transaction signatures.

The suggested improvements mainly focus on enhancing readability and robustness through additional comments and error checks. These minor changes will make the test more maintainable and easier to understand for future developers.

Great job on implementing this crucial test!
.pylintrc (2)
43-43: Consider the implications of increasing max-line-length to 120.

Increasing the maximum line length to 120 characters can improve readability for complex statements or long strings. However, it may also lead to overly long lines that are harder to read, especially when viewing code side-by-side or on smaller screens.

The PEP 8 style guide recommends a maximum of 79 characters for code and 72 for docstrings and comments. While many projects deviate from this, it's worth considering if 120 characters is the right balance for your project.

Consider adding a comment explaining the rationale for this change to help future maintainers understand the decision.

Let's check the current usage of long lines in the codebase:

Verification successful

Increase to max-line-length set to 120 characters is appropriate and does not currently introduce overly long lines.

The verification shows that no lines in the codebase exceed 120 characters. While 12 lines surpass the previous 79-character limit, they now comply with the updated configuration. It is recommended to add a comment in .pylintrc explaining the rationale behind this change to aid future maintainers.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check for lines exceeding 79 and 120 characters

echo "Lines exceeding 79 characters:"
rg --type python '(.{80,})' -g '!tests/' | wc -l

echo "Lines exceeding 120 characters:"
rg --type python '(.{121,})' -g '!tests/' | wc -l
Length of output: 1265

Script:
#!/bin/bash
# Description: Check for lines exceeding 79 and 120 characters in Python files, excluding the tests directory.

echo "Lines exceeding 79 characters:"
rg --type python '.{80,}' -g '!tests/' | wc -l

echo "Lines exceeding 120 characters:"
rg --type python '.{121,}' -g '!tests/' | wc -l
Length of output: 308

Script:
#!/bin/bash
# Description: Check for lines exceeding 79 and 120 characters in Python files, excluding the tests directory.

echo "Lines exceeding 79 characters:"
rg '.{80,}' -g '*.py' -g '!tests/' | wc -l

echo "Lines exceeding 120 characters:"
rg '.{121,}' -g '*.py' -g '!tests/' | wc -l
Length of output: 239
14-15: Consider the implications of disabling these pylint checks.

The following checks have been added to the disable list:

broad-exception-caught: This might lead to less specific exception handling, potentially masking important errors.

consider-using-f-string: F-strings are generally more readable and efficient. Disabling this check might lead to less optimal string formatting.

too-many-instance-attributes: This check helps maintain class simplicity. Disabling it might lead to overly complex classes.

While disabling these checks can reduce noise in the linter output, it may also lead to decreased code quality over time. Consider if these checks can be selectively enabled or if there are specific reasons for disabling them project-wide.

To understand the impact of these changes, let's check the current usage in the codebase:

Also applies to: 30-30
ante/evm/07_can_transfer.go (1)

46-46: Approve: Simplified header hash processing

This change removes the redundant .Bytes() call, as ctx.HeaderHash() likely already returns a byte slice. This simplification:

Improves code readability

Potentially offers a minor performance improvement by eliminating an unnecessary method call

Aligns with Go's idiomatic practices of favoring simplicity

The functionality remains unchanged, making this a safe and beneficial refactor.
ante/evm/01_setup_ctx.go (2)
8-8: LGTM: Import addition is correct and necessary.

The addition of the storetypes import is appropriate for the subsequent usage of storetypes.NewInfiniteGasMeter(). The import follows standard Go formatting conventions.

47-47: LGTM: Gas meter update aligns with SDK changes.

The change from sdktypes.NewInfiniteGasMeter() to storetypes.NewInfiniteGasMeter() is consistent with the PR objective of updating dependencies. This modification likely reflects changes in the structure of Cosmos SDK v0.50.9.

To ensure consistency across the codebase, please run the following script:

This will help ensure that all instances of NewInfiniteGasMeter() have been updated consistently.

Verification successful

Verified: Gas meter updates are consistent across the codebase.

All instances of sdktypes.NewInfiniteGasMeter() have been successfully replaced with storetypes.NewInfiniteGasMeter(), ensuring alignment with the updated Cosmos SDK v0.50.9.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the usage of NewInfiniteGasMeter across the codebase

# Test: Search for any remaining usage of sdktypes.NewInfiniteGasMeter(). Expect: No results.
rg --type go 'sdktypes\.NewInfiniteGasMeter'

# Test: Search for the new usage of storetypes.NewInfiniteGasMeter(). Expect: Results including this file.
rg --type go 'storetypes\.NewInfiniteGasMeter'
Length of output: 1473
example_chain/ante/cosmos_handler.go (2)
10-10: Approved: IBC module version update

The import statement has been correctly updated to use the latest version (v8) of the IBC module. This change aligns with the PR objective of moving to the latest changes from Evmos main.

To ensure consistency across the codebase, please run the following script to check for any remaining v7 imports of the IBC module:

If any v7 imports are found, they should be updated to v8 for consistency.

Verification successful

Verified: No remaining v7 IBC module imports found

All IBC module imports have been successfully updated to version v8. No instances of v7 imports were detected, ensuring consistency across the codebase.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check for any remaining v7 imports of the IBC module

# Search for v7 imports
echo "Checking for v7 imports:"
rg --type go 'github.com/cosmos/ibc-go/v7'

# Search for v8 imports (for context)
echo "Checking for v8 imports:"
rg --type go 'github.com/cosmos/ibc-go/v8'
Length of output: 65491

Script:
#!/bin/bash
# Description: Verify that there are no remaining v7 imports of the IBC module and confirm all are updated to v8.

# Search for v7 imports
echo "Checking for any remaining v7 imports of the IBC module:"
rg --type go 'github\.com/cosmos/ibc-go/v7'

# Search for v8 imports for context
echo "Verifying all IBC module imports are updated to v8:"
rg --type go 'github\.com/cosmos/ibc-go/v8'
Length of output: 9545
10-10: Verify compatibility with IBC v8 and update related code if necessary

The upgrade to IBC v8 may introduce changes in the API or behavior of IBC-related functions, such as ibcante.NewRedundantRelayDecorator. To ensure smooth integration:

Review the changelog or release notes for IBC v8 to understand any breaking changes or new features.

Verify that the usage of NewRedundantRelayDecorator and any other IBC-related functions in this file is still compatible with v8.

Check and update other parts of the codebase that interact with IBC functionality, if necessary.

Thoroughly test all IBC-related functionality to ensure it works as expected with the new version.

To help identify other areas that might need attention, run the following script:

Review the output to identify areas that might need updates or additional testing due to the IBC version change.
ante/evm/09_increment_sequence_test.go (2)

6-6: LGTM: Import change aligns with SDK update

The addition of sdktypes "github.com/cosmos/cosmos-sdk/types" is consistent with the PR objective of updating to the latest Cosmos SDK (v0.50.9). This change supports the transition to using sdktypes.AccountI in the test suite.

25-25: LGTM: Consistent update of AccountI type

The change from authtypes.AccountI to sdktypes.AccountI is applied consistently across all occurrences of the malleate function. This update aligns with the import change and the PR objectives of updating to the latest Cosmos SDK.

To ensure completeness of the test suite with the new sdktypes.AccountI, please verify if there are any new methods or behaviors introduced in this interface that should be considered in these tests. You can run the following command to check the interface definition:

This will help identify any new methods that might be relevant to add to the test cases.

Also applies to: 30-32, 37-39

ante/evm/11_emit_event.go (1)

43-43: Approve nolint directive for gosec warning

The addition of the nolint directive for the gosec G115 warning is appropriate. The comment clearly explains why it's safe to ignore this warning in this specific case.

This change improves code clarity by explicitly acknowledging and explaining why a potential security warning can be safely ignored.

ethereum/eip712/preprocess.go (2)

10-10: LGTM: New imports added correctly.

The new imports for the address package and sdk types are correctly added and align with the changes made in the PreprocessLedgerTx function.

Also applies to: 13-13

57-63: Improved address handling with Bech32 encoding.

The changes enhance the robustness of fee payer address handling:

A Bech32Codec is created using the SDK's configuration, ensuring consistency with the network's address format.

The fee payer's address is now properly converted from bytes to a Bech32-encoded string.

Error handling has been added for the address conversion process.

The FeePayer field in ExtensionOptionsWeb3Tx now uses the Bech32-encoded address string.

These modifications improve the reliability and consistency of address handling in the function, aligning it better with Cosmos SDK standards.

Also applies to: 67-67

ante/evm/eth_benchmark_test.go (1)

1-18: LGTM: Package declaration and imports are appropriate.

The package declaration and imports are well-structured and include all necessary dependencies for the benchmark test.
example_chain/ante/handler_options.go (3)
Line range hint 1-73: Verify broader impact of SDK updates.

The changes in this file are part of a larger update to align with SDK v50. While the modifications here are correct, it's crucial to ensure that these updates are consistently applied across the entire codebase.

To check for any inconsistencies or missed updates related to the SDK version change, run the following script:
#!/bin/bash
# Description: Verify consistency of SDK-related imports and usages

# Test 1: Check for any remaining v7 IBC imports
rg --type go 'github.com/cosmos/ibc-go/v7'

# Test 2: Check for any remaining authsigning imports
rg --type go 'github.com/cosmos/cosmos-sdk/x/auth/signing'

# Test 3: Check for new txsigning imports
rg --type go 'cosmossdk.io/x/tx/signing'

# Test 4: Look for potential missed updates in other ante handler files
rg --type go -g 'ante/*.go' 'authsigning|txsigning'
Please review the results of these checks to ensure all necessary updates have been made consistently across the project.

33-33: Verify usage of updated SignModeHandler type.

The change from authsigning.SignModeHandler to *txsigning.HandlerMap for the SignModeHandler field is consistent with the SDK v50 update. This modification may require updates in other parts of the codebase where SignModeHandler is used.

To ensure all usages of SignModeHandler are updated correctly, run the following script:
#!/bin/bash
# Description: Verify SignModeHandler usage across the codebase

# Test: Search for SignModeHandler usages. Expect: Only *txsigning.HandlerMap types.
rg --type go 'SignModeHandler.*authsigning\.SignModeHandler'
rg --type go 'SignModeHandler.*txsigning\.HandlerMap'
8-9: LGTM: Import changes align with SDK updates.

The new imports and the IBC keeper version upgrade are consistent with the PR objective of integrating the latest changes from Evmos main, including the SDK v50 update.

To ensure the IBC keeper version upgrade is consistent across the codebase, run the following script:

Also applies to: 15-15
ante/cosmos/authz.go (1)

34-34: Improved error handling with formatted messages

This change enhances the error reporting by using errorsmod.Wrapf instead of errorsmod.Wrap. The addition of the %s format specifier ensures that the original error message is preserved in its entirety within the new error context.

Benefits of this change:

More informative error messages, aiding in debugging and error tracing.

Maintains the original error context while adding the unauthorized error type.

Follows Go best practices for error handling and formatting.
ante/sigverify_test.go (5)
8-8: LGTM: Import addition is consistent with code changes.

The addition of the storetypes import aligns with the subsequent usage of storetypes.NewInfiniteGasMeter() in the test cases. This change appears to be part of a refactoring effort to use more specific types for gas meter operations.

62-62: LGTM: Consistent use of storetypes.NewInfiniteGasMeter().

The change from sdk.NewInfiniteGasMeter() to storetypes.NewInfiniteGasMeter() is consistent with the import changes and represents a move towards using more specific types for gas meter operations. This modification enhances code clarity and type safety.

68-68: LGTM: Consistent updates across test cases.

The changes to use storetypes.NewInfiniteGasMeter() in the test cases for "PubKeyEthsecp256k1", "PubKeySecp256k1", and "PubKeySecp256r1" are consistent with the previous modifications. This uniformity in gas meter initialization across all test cases enhances the overall consistency and maintainability of the test suite.

Also applies to: 74-74, 80-80

86-86: LGTM: Comprehensive and consistent updates to gas meter initialization.

The changes to use storetypes.NewInfiniteGasMeter() in the "Multisig" and "unknown key" test cases complete the consistent update across the entire test suite. These modifications:

Enhance type specificity and safety.

Maintain the original test logic and structure.

Align the test suite with the updated gas meter implementation.

The comprehensive nature of these changes suggests a deliberate refactoring effort to improve code quality and consistency throughout the module.

Also applies to: 92-92

27-27: Verify the impact of simplified encoding configuration.

The encoding.MakeConfig() call has been simplified by removing the app.ModuleBasics parameter. While this change streamlines the code, it's important to ensure that the new implementation still provides all necessary configurations for the test environment.

To confirm that this change doesn't affect the test setup, please run the following script:

This script will help us understand the new implementation of MakeConfig() and its usage across the codebase.

Verification successful

Verified: Encoding configuration remains intact without app.ModuleBasics parameter.

The encoding.MakeConfig() function has been successfully simplified by removing the app.ModuleBasics parameter. The internal implementation ensures that all necessary encoding configurations are properly initialized, and all test cases using this function operate as expected without any issues.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the implementation of encoding.MakeConfig()

# Test: Check the implementation of MakeConfig
ast-grep --lang go --pattern 'func MakeConfig() encoding.Config'

# Test: Verify usage of MakeConfig across the codebase
rg --type go 'encoding\.MakeConfig'
Length of output: 709

Script:
#!/bin/bash
# Description: Retrieve the implementation of encoding.MakeConfig() across the codebase

# Search for the implementation of MakeConfig in the encoding package
ast-grep --lang go --pattern 'func MakeConfig() encoding.Config' src/encoding/...

# Search for the implementation of MakeConfig in the evmosencoding package
ast-grep --lang go --pattern 'func MakeConfig() encoding.Config' src/evmosencoding/...
Length of output: 305

Script:
#!/bin/bash
# Description: Verify that MakeConfig initializes all necessary modules without external parameters

# Check for module initialization within encoding.MakeConfig in the encoding package
ast-grep --lang go --pattern 'MakeConfig() encoding.Config { $$$ config.InitModule($_, $_, $$$) $$$ }' src/encoding/...

# Check for module initialization within evmosencoding.MakeConfig in the evmosencoding package
ast-grep --lang go --pattern 'MakeConfig() encoding.Config { $$$ config.InitModule($_, $_, $$$) $$$ }' src/evmosencoding/...
Length of output: 1236

Script:
#!/bin/bash
# Description: Retrieve all implementations of MakeConfig() across the entire codebase

# Search for the definition of MakeConfig functions in all Go files
rg --type go 'func MakeConfig'

# Search for all usages of MakeConfig() in the codebase
rg --type go 'encoding\.MakeConfig'

# Additionally, search for any variations or namespaced versions of MakeConfig
rg --type go 'MakeConfig$'
Length of output: 1446

Script:
#!/bin/bash
# Description: Extract the implementation of encoding.MakeConfig() from encoding/config.go

# Retrieve the full MakeConfig function definition
rg --type go 'func MakeConfig\($ sdktestutil\.TestEncodingConfig \{' encoding/config.go -A 20
Length of output: 972
client/block/store.go (2)
48-48: LGTM! Consistent type update.

The update of the bss variable to type cmtstore.BlockStoreState is consistent with the method signature change and import updates. The rest of the method implementation remains unchanged, preserving the existing logic.

38-38: LGTM! Verify impact of method signature change.

The update to the state method signature, returning (*cmtstore.BlockStoreState, error), is consistent with the import changes.

To ensure this change doesn't break any existing code, run the following script:

Verification successful

LGTM! The state method signature change has been successfully verified with no remaining uses of tmstore.BlockStoreState.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Verify the impact of the state method signature change

# Test 1: Check for any remaining uses of tmstore.BlockStoreState
echo "Checking for any remaining uses of tmstore.BlockStoreState:"
rg --type go 'tmstore\.BlockStoreState'

# Test 2: Verify the usage of the new cmtstore.BlockStoreState
echo "Verifying the usage of the new cmtstore.BlockStoreState:"
rg --type go 'cmtstore\.BlockStoreState'

# Test 3: Check for any calls to the state method
echo "Checking for calls to the state method:"
rg --type go '\.state'
Length of output: 646
ante/evm/10_gas_wanted_test.go (3)
6-6: LGTM: Import statements updated correctly.

The new import statements for storetypes and integrationutils are correctly added and align with the changes made in the test cases.

Also applies to: 15-15

Line range hint 1-124: Overall assessment: Changes improve test suite maintainability and SDK compatibility.

The modifications in this file successfully update the test suite to use newer Cosmos SDK components, particularly in gas meter instantiation and fee market parameter updates. These changes align well with the PR objectives of integrating latest updates from the Evmos main branch.

Key improvements:

Consistent update of gas meter instantiation across all test cases.

Refactored fee market parameter update using a utility function, enhancing code readability.

Proper import updates to support these changes.

These changes contribute to better maintainability and alignment with the latest SDK version (v0.50.9) as mentioned in the PR objectives.

39-39: LGTM: Gas meter instantiation updated consistently.

The change from sdktypes.NewGasMeter to storetypes.NewGasMeter is correct and consistent with the new import. This update likely reflects changes in the Cosmos SDK structure.

Verification successful

Verified: Consistent replacement of sdktypes.NewGasMeter with storetypes.NewGasMeter.

The usage of storetypes.NewGasMeter is consistent across all test cases as shown by the search results.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Verify consistent usage of storetypes.NewGasMeter
rg --type go 'storetypes\.NewGasMeter' ante/evm/
Length of output: 508
example_chain/ante/handler_options_test.go (4)

3-12: LGTM: Import statements are appropriate and well-organized.

The import statements include all necessary packages for testing and internal dependencies. They are properly organized and relevant to the test file's purpose.

14-186: LGTM: Well-structured table-driven test for HandlerOptions.Validate()

The TestValidateHandlerOptions function is well-designed:

It uses a table-driven test approach, which is excellent for testing multiple scenarios efficiently.

The test cases cover various failure scenarios (empty or nil fields) and a success case.

The use of descriptive names for each test case enhances readability and maintainability.

The structure allows for easy addition of new test cases in the future if needed.

178-185: LGTM: Effective test execution logic

The test execution logic is well-implemented:

It iterates through all test cases efficiently.

It correctly uses require.NoError for expected passes and require.Error for expected failures.

The use of require instead of assert ensures that the test stops immediately if an unexpected result occurs, which is appropriate for these cases.

This approach ensures that each test case is properly validated against its expected outcome.

1-186: Overall, excellent test coverage for HandlerOptions validation

This test file is a valuable addition to the project:

It provides comprehensive coverage for the HandlerOptions.Validate() method.

The table-driven test approach allows for easy maintenance and extension.

It covers various failure scenarios, ensuring robustness in configuration validation.

The thoroughness of these tests will help prevent misconfiguration issues in the ante handler, which is critical for transaction processing in the Evmos chain. While there's room for some minor enhancements (as noted in previous comments), the current implementation significantly contributes to the project's overall quality and reliability.

ante/cosmos/min_gas_price_test.go (8)

33-34: LGTM! Good setup for network context.

The introduction of the network context (nw) and its corresponding context (ctx) is a good change. It aligns with the PR objectives and provides a more flexible testing environment.

55-57: LGTM! Consistent use of network context.

The changes correctly update the usage of FeeMarketKeeper to use the network context (nw.App) instead of the suite context. This is consistent with the overall refactoring approach.

70-72: LGTM! Consistent refactoring.

The changes in this test case are consistent with the previous one, correctly using the network context for FeeMarketKeeper operations.

85-87: LGTM! Consistent refactoring continues.

The changes in this test case maintain consistency with the previous ones, correctly using the network context for FeeMarketKeeper operations.

100-102: LGTM! Refactoring consistency maintained.

The changes in this test case continue to maintain consistency with the previous ones, correctly using the network context for FeeMarketKeeper operations.

115-117: LGTM! Consistent refactoring across test cases.

The changes in this test case maintain the consistency seen in previous cases, correctly using the network context for FeeMarketKeeper operations.

193-194: LGTM! Refactoring complete and consistent.

The changes in the test execution loop correctly use the new ctx and nw.App, consistent with the refactoring throughout the file. This completes the transition to using the network context instead of the suite context.

Overall, the refactoring in this file is thorough and consistent. It successfully updates all relevant parts to use the new network context, which should provide a more flexible and representative testing environment.

Line range hint 1-205: Overall assessment: Well-executed refactoring

This file has undergone a comprehensive refactoring to use a network context (nw) instead of the suite context. The changes are consistent throughout the file and align well with the PR objectives. Key points:

Import statements have been updated to reflect package reorganization.

All test cases have been updated to use nw.App.FeeMarketKeeper instead of suite.app.FeeMarketKeeper.

The test execution loop now uses the new context and app references.

These changes should provide a more flexible and representative testing environment. The refactoring has been executed cleanly and consistently, maintaining the original test logic while updating the context in which the tests run.

ante/evm/04_validate_test.go (7)

36-36: LGTM: Consistent usage of constants.ExampleAttoDenom.

The update to use constants.ExampleAttoDenom is consistent with the import change and maintains the same functionality.

50-50: LGTM: Consistent usage of constants.ExampleAttoDenom.

The update to use constants.ExampleAttoDenom is consistent with the previous changes and maintains the same functionality.

64-67: LGTM: Consistent usage of constants.ExampleAttoDenom in params initialization.

The update to use constants.ExampleAttoDenom in the params initialization is consistent with the previous changes and maintains the same test logic.

83-83: LGTM: Consistent usage of constants.ExampleAttoDenom.

The update to use constants.ExampleAttoDenom is consistent with the previous changes and maintains the same functionality in this test case.

97-98: LGTM: Consistent usage of constants.ExampleAttoDenom in params initialization.

The update to use constants.ExampleAttoDenom in the params initialization is consistent with the previous changes and maintains the same test logic for this case.

115-116: LGTM: Consistent usage of constants.ExampleAttoDenom throughout the file.

The update to use constants.ExampleAttoDenom in this params initialization, as well as all previous instances in the file, is consistent and maintains the same test logic throughout. This change improves code organization by centralizing the constant definition.

Line range hint 1-218: Summary: Successful refactoring to use constants package.

The changes in this file consistently replace testutil.ExampleAttoDenom with constants.ExampleAttoDenom across all test cases. This refactoring improves code organization by centralizing constant definitions. No functional changes were introduced, and all test logic remains intact.

ethereum/eip712/preprocess_test.go (6)

9-10: Improved import organization and dependency management.

The changes to the import statements are beneficial:

Centralizing constants in a separate package (testutil/constants) improves maintainability.

The switch from client/tx to codec/address suggests a more focused approach to address handling in tests.

These changes should lead to better code organization and potentially reduced coupling.

Also applies to: 13-13

30-30: Enhanced test consistency through centralized constants.

The updates to the testing constants are commendable:

Using constants.ExampleChainID for chainID ensures consistency across tests.

Updating ctx to use encoding.MakeConfig().TxConfig standardizes the configuration.

Employing constants.ExampleBech32Prefix in feePayerAddress maintains consistency in address prefixes.

These changes will likely improve test reliability and make future updates easier.

Also applies to: 33-33, 39-39

55-55: Consistent use of centralized constants for bech32 prefix.

The update to use constants.ExampleBech32Prefix for setting the bech32 prefix is a positive change. It maintains consistency with the earlier updates and ensures that the same prefix is used throughout the tests.

112-112: Consistent use of centralized constants for fee denomination.

The update to use constants.ExampleAttoDenom instead of a hardcoded value for fee verification is a positive change. It maintains consistency with the earlier updates to use centralized constants and makes it easier to update the denomination across all tests if needed in the future.

210-210: Consistent use of centralized constants in test case creation.

The updates in the createPopulatedTestCase function to use constants.ExampleAttoDenom instead of hardcoded values are commendable. These changes:

Maintain consistency with the earlier updates to use centralized constants.

Improve the overall consistency of the test suite.

Make it easier to update the denomination across all tests if needed in the future.

This approach reduces the risk of inconsistencies and makes the tests more maintainable.

Also applies to: 222-222

Line range hint 1-240: Overall improvement in test consistency and maintainability.

The changes made to this file significantly enhance its quality:

Consistent use of centralized constants (constants.ExampleChainID, constants.ExampleBech32Prefix, constants.ExampleAttoDenom) throughout the file improves maintainability and reduces the risk of inconsistencies.

Enhanced address handling with the introduction of Bech32Codec improves the robustness of address-related tests.

The overall structure and logic of the tests remain intact, preserving the original test coverage while improving code quality.

These changes will make the test suite more reliable and easier to maintain in the future. Great job on improving the code quality!
ante/evm/08_gas_consume_test.go (4)
125-125: LGTM: Consistent package update

This change is consistent with the previous modification, correctly using sdkmath.NewInt instead of sdktypes.NewInt. It maintains consistency within the test suite and aligns with the latest Cosmos SDK structure.

Line range hint 1-205: Summary: Successful SDK updates in EVM ante handler tests

The changes in this file successfully update the EVM ante handler tests to align with the latest Cosmos SDK version. Key modifications include:

Updating sdktypes.NewInt to sdkmath.NewInt, reflecting package reorganization in the SDK.

Changing the balance comparison method from IsEqual to Equal.

These changes maintain the functionality of the tests while ensuring compatibility with the updated SDK. The consistency of these changes suggests a thorough update process.

114-114: LGTM: Package update for NewInt function

The change from sdktypes.NewInt to sdkmath.NewInt is correct and aligns with recent updates in the Cosmos SDK package structure. This modification ensures compatibility with the latest SDK version.

To ensure consistency across the codebase, let's verify this change:
#!/bin/bash
# Verify the usage of sdkmath.NewInt across the codebase
rg --type go 'sdkmath\.NewInt'
# Check for any remaining usage of sdktypes.NewInt
rg --type go 'sdktypes\.NewInt'
186-186: LGTM: Updated comparison method for balances

The change from IsEqual to Equal for comparing balances is correct and aligns with updates in the Cosmos SDK. This modification ensures that the test remains compatible with the latest SDK version while maintaining its intended functionality.

To ensure consistency and catch any missed updates, let's verify this change across the codebase:
ante/evm/fee_checker_test.go (6)
7-7: LGTM: Updated logging package import.

The change from github.com/cometbft/cometbft/libs/log to cosmossdk.io/log aligns with the Cosmos SDK v0.50 update mentioned in the PR objectives.

38-38: LGTM: Updated constant reference in MockEVMKeeper.GetParams.

The change from testutil.ExampleAttoDenom to testconstants.ExampleAttoDenom is consistent with the new import for test constants.

101-101: LGTM: Updated constant references and package names in test cases.

The changes consistently replace testutil.ExampleAttoDenom with testconstants.ExampleAttoDenom and update the dynamic fee checker instantiation to use the new package name. These updates align with the earlier import and package changes.

Also applies to: 143-143, 159-159, 175-175, 195-195, 217-217, 235-235

Line range hint 1-244: Overall assessment: Changes improve test structure and consistency.

The modifications in this file align well with the PR objectives, particularly the upgrade to Cosmos SDK v0.50. The changes introduce a more standardized approach to test setup and constant management, which should improve maintainability. All updates appear to be consistent and well-implemented.

Key improvements:

Updated import statements to align with Cosmos SDK v0.50.

Introduced a dedicated package for test constants.

Implemented a new network package for integration testing.

Consistently updated references throughout the test cases.

Please ensure that the suggested verification tasks are completed to confirm the consistency of these changes across the codebase.

55-56: LGTM: Updated test setup to use new network instance.

The changes introduce a more standardized way of setting up the test environment using the new network package.

Please run the following script to ensure the network setup is consistent across all test files:
#!/bin/bash
# Description: Check for consistent usage of the new network setup

# Test: Search for the network setup in test files. Expect: Consistent usage.
rg --type go 'network\.New' 'test\.go$'
rg --type go 'nw\.GetEncodingConfig' 'test\.go$'
14-14: LGTM: Updated EVM ante package import path.

The import path change from github.com/evmos/os/ante/evmante to github.com/evmos/os/ante/evm reflects a project structure reorganization.

Please run the following script to ensure this import path change is consistent across the entire codebase:

Verification successful

Verified: EVM ante package import path change is consistent

All instances of the old import path github.com/evmos/os/ante/evmante have been removed, and the new import path github.com/evmos/os/ante/evm is used consistently across the codebase.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check for any remaining instances of the old import path

# Test: Search for the old import path. Expect: No results.
rg --type go 'github.com/evmos/os/ante/evmante'

# Test: Verify the new import path is used consistently. Expect: Consistent usage.
rg --type go 'github.com/evmos/os/ante/evm'
Length of output: 15836

Script:
#!/bin/bash
# Description: Verify EVM ante package import path changes across the codebase.

# Test 1: Ensure the old import path is completely removed.
echo "Checking for any instances of the old import path 'github.com/evmos/os/ante/evmante'..."
old_import_count=$(rg --type go 'github.com/evmos/os/ante/evmante' | wc -l)
if [ "$old_import_count" -eq 0 ]; then
    echo "✅ No instances of the old import path found."
else
    echo "❌ Found $old_import_count instances of the old import path:"
    rg --type go 'github.com/evmos/os/ante/evmante'
fi

# Test 2: Confirm the new import path is used consistently.
echo "Checking for instances of the new import path 'github.com/evmos/os/ante/evm'..."
new_import_count=$(rg --type go 'github.com/evmos/os/ante/evm' | wc -l)
if [ "$new_import_count" -gt 0 ]; then
    echo "✅ Found $new_import_count instances of the new import path."
else
    echo "❌ No instances of the new import path found."
fi
Length of output: 755
client/keys/add.go (2)
Line range hint 1-307: Changes align with PR objectives and SDK update.

The modifications in this file, specifically the transition from keyring.MkAccKeyOutput to keys.MkAccKeyOutput, are consistent with the PR objectives of integrating the latest updates from the Evmos main branch and upgrading to SDK v50.9. These changes appear to be part of a broader refactoring effort to enhance functionality and align with the latest developments.

263-263: LGTM! Verify usage of MkAccKeyOutput in other files.

The changes from keyring.MkAccKeyOutput to keys.MkAccKeyOutput are consistent and appear to be part of a larger refactoring effort. The implementation looks correct.

To ensure consistency across the codebase, please run the following script to check for any remaining usage of keyring.MkAccKeyOutput:

This will help identify any places where the change might need to be applied for consistency.

Also applies to: 276-276

Verification successful

Verification Complete!

All instances of keyring.MkAccKeyOutput have been successfully replaced with keys.MkAccKeyOutput, and no remaining usages or imports of the keys package were found.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Check for any remaining usage of keyring.MkAccKeyOutput

# Test: Search for keyring.MkAccKeyOutput usage
rg --type go 'keyring\.MkAccKeyOutput'

# Test: Verify keys.MkAccKeyOutput is imported and used correctly
rg --type go -A 5 'import $[^)]*\n\s*"github.com/cosmos/cosmos-sdk/client/keys"[^)]*$' | rg 'keys\.MkAccKeyOutput'
Length of output: 351

Script:
#!/bin/bash
# Description: Retry checking for any remaining usage of keyring.MkAccKeyOutput with multiline enabled

# Test: Search for keyring.MkAccKeyOutput usage
rg --type go 'keyring\.MkAccKeyOutput'

# Test: Verify keys.MkAccKeyOutput is imported and used correctly with multiline mode
rg --multiline --type go 'import $[^)]*\n\s*"github.com/cosmos/cosmos-sdk/client/keys"[^)]*$' | rg 'keys\.MkAccKeyOutput'
Length of output: 166

Script:
#!/bin/bash
# Description: Verify that all instances of MkAccKeyOutput are imported from the keys package and used correctly.

# Find all function calls to MkAccKeyOutput and check their package
# Using ast-grep to ensure accurate syntax parsing

# Check for MkAccKeyOutput calls not using the keys package
ast-grep --lang go --pattern '_.MkAccKeyOutput($_)' --type function_calls | grep -v '^keys\.' || echo "All MkAccKeyOutput calls use the keys package."

# Verify that the keys package is imported where MkAccKeyOutput is used
# Find files importing the keys package
export KEYS_IMPORTED_FILES=$(ast-grep --lang go --pattern 'import "github.com/cosmos/cosmos-sdk/client/keys"' --type imports)

# Check that all MkAccKeyOutput usages are within files that import the keys package
for file in $KEYS_IMPORTED_FILES; do
    ast-grep --file "$file" --lang go --pattern '_.MkAccKeyOutput($_)'
done
Length of output: 738

Script:
#!/bin/bash
# Description: Verify that all instances of MkAccKeyOutput are imported from the keys package and used correctly.

# Check for any remaining usage of keyring.MkAccKeyOutput
echo "Checking for any remaining usage of keyring.MkAccKeyOutput..."
rg --type go 'keyring\.MkAccKeyOutput' || echo "No remaining usage of keyring.MkAccKeyOutput found."

# Verify that keys.MkAccKeyOutput is imported and used correctly
echo "Verifying that keys.MkAccKeyOutput is imported and used correctly..."
ast-grep --lang go --pattern 'import "github.com/cosmos/cosmos-sdk/client/keys"' | while read -r file; do
    echo "Checking MkAccKeyOutput usage in $file..."
    ast-grep --file "$file" --lang go --pattern 'keys\.MkAccKeyOutput\(' || echo "MkAccKeyOutput not used in $file"
done
Length of output: 3796

Script:
#!/bin/bash
# Description: Verify that all instances of MkAccKeyOutput are imported from the keys package and used correctly.

echo "Checking for any remaining usage of keyring.MkAccKeyOutput..."
# Search for any usage of keyring.MkAccKeyOutput
remaining_usage=$(rg --type go 'keyring\.MkAccKeyOutput')

if [[ -n "$remaining_usage" ]]; then
    echo "Found remaining usages of keyring.MkAccKeyOutput:"
    echo "$remaining_usage"
else
    echo "No remaining usage of keyring.MkAccKeyOutput found."
fi

echo "Verifying that keys.MkAccKeyOutput is imported and used correctly..."

# Find all files that import the keys package
imported_files=$(rg --type go 'import\s+"github\.com/cosmos/cosmos-sdk/client/keys"' --files-with-matches)

if [[ -z "$imported_files" ]]; then
    echo "No files import the keys package."
    exit 0
fi

# Check that MkAccKeyOutput is used in the files that import the keys package
for file in $imported_files; do
    usage=$(rg 'keys\.MkAccKeyOutput\(' "$file")
    if [[ -n "$usage" ]]; then
        echo "In file $file:"
        echo "$usage"
    else
        echo "MkAccKeyOutput not used in $file despite importing the keys package."
    fi
done
Length of output: 677
ethereum/eip712/encoding_legacy.go (4)
171-171: Approve consistent changes in legacyDecodeProtobufSignDoc.

The changes in this function are consistent with those in legacyDecodeAminoSignDoc. The ParseChainID function is now called from ostypes, and the signer retrieval method has been updated to use protoCodec.GetMsgV1Signers(msg).

Also applies to: 181-185

Line range hint 1-274: Summary of changes and potential impact.

The changes in this file primarily involve:

Renaming evmostypes to ostypes in import statements and function calls.

Updating the method of obtaining signers to use protoCodec.GetMsgV1Signers(msg) instead of msg.GetSigners().

These changes appear to be part of a larger refactoring effort, possibly related to updating the SDK version. The consistency of changes across different functions suggests a thorough refactoring process.

Potential impacts:

Improved compatibility with the latest SDK version.

Possible changes in how signers are handled, which may affect transaction processing and validation.

It's crucial to ensure that these changes are consistently applied across the entire codebase and that all affected components are updated accordingly.

92-96: Approve changes and verify new signer retrieval method.

The changes in obtaining signers and parsing the chain ID are consistent with the import renaming and possibly a new SDK version. The new method of retrieving signers (protoCodec.GetMsgV1Signers(msg)) replaces the direct call to msg.GetSigners().

To ensure the new signer retrieval method is used consistently, let's check for any remaining instances of the old method:
#!/bin/bash
# Search for any remaining instances of 'GetSigners()'
rg --type go 'GetSigners'
Also applies to: 101-101

15-15: Verify consistency of import renaming across the codebase.

The import statement has been changed from evmostypes to ostypes. This change appears to be part of a larger refactoring effort.

To ensure consistency, let's check for any remaining instances of evmostypes in the codebase:

Verification successful

Import renaming from evmostypes to ostypes is consistent across the codebase.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Search for any remaining instances of 'evmostypes'
rg --type go 'evmostypes'
Length of output: 93

Script:
#!/bin/bash
# Search for all occurrences of 'GetSigners()' in Go files
rg --type go 'GetSigners'
Length of output: 93

Script:
#!/bin/bash
# Search for the definition of 'GetSigners()' in Go files
rg --type go '^func GetSigners\('
Length of output: 193

Script:
#!/bin/bash
# Search for any remaining instances of 'evmostypes' in the GetSigners() definitions
rg --type go 'evmostypes' api/os/evm/v1/msg.go api/os/erc20/v1/msg.go
Length of output: 69
Makefile (6)

14-14: LGTM: Good addition to .PHONY

Adding 'build' to the .PHONY declaration is a good practice. It ensures that the 'build' target will always run, even if a file named 'build' exists in the directory.

91-100: LGTM: Simplified lint-fix for contracts

The simplification of the lint-fix-contracts target to only use solhint is appropriate if that's the only tool needed for fixing Solidity contracts. The update to the .PHONY declaration to include the new lint targets is also good practice.

141-141: LGTM: New Protobuf generation script added

The addition of generate_protos_pulsar.sh to the proto-gen target is likely related to new functionality or changes in the project structure.

Could you provide more information about the purpose of generate_protos_pulsar.sh and how it differs from the existing generate_protos.sh? This will help ensure that the new script is properly integrated and doesn't conflict with existing Protobuf generation processes.

Line range hint 1-266: Overall assessment: Positive improvements to the Makefile

The changes to the Makefile generally improve the build, lint, and format processes of the project. The addition of more granular targets for different languages and the updates to tool versions enhance the project's development workflow. The suggestions for minor improvements (like adding lint-all and format-all targets) and the requests for verification of version updates should help further refine the build process.

These changes align well with the PR objectives of integrating the latest updates from the Evmos repository and upgrading dependencies. They contribute to enhancing the functionality and maintainability of the OS repository.

Line range hint 166-166: LGTM: Go cross-compilation version update

Updating the GOLANG_CROSS_VERSION to v1.22 is good as it allows the project to use the latest features and improvements in Go.

Please ensure that this update doesn't introduce any compatibility issues with the project's dependencies or build process. Run the following script to check for any potential problems:

124-124: LGTM: Protobuf version update

Updating the Protobuf version from 0.11.6 to 0.14.0 is good as it may include bug fixes and new features.

Please ensure that this update doesn't introduce any breaking changes to the project. Run the following script to check for any compatibility issues:

ante/cosmos/eip712.go (3)

43-43: LGTM: Simplified struct definition

The removal of the signModeHandler field from the LegacyEip712SigVerificationDecorator struct simplifies its definition. This change aligns well with the updates in the constructor and VerifySignature function.

51-51: LGTM: Updated constructor signature

The constructor NewLegacyEip712SigVerificationDecorator has been updated to remove the signModeHandler parameter, which is consistent with the changes made to the struct definition. The constructor now only initializes the ak field.

84-87: Improved error handling for GetSigners()

The addition of error handling for the GetSigners() call enhances the robustness of the code. This change ensures that any errors during the retrieval of signer addresses are properly caught and returned, aligning with Go best practices for error handling.

ante/cosmos/authz_test.go (8)

20-22: LGTM: Import changes enhance test consistency

The addition of testconstants, factory, and network imports indicates a move towards a more standardized test setup. This change should improve consistency across tests and make them easier to maintain.

29-30: LGTM: Standardized network setup

The introduction of a new network instance using network.New() and retrieving the transaction configuration from this instance standardizes the test setup. This change should lead to more consistent and reliable test results across different test cases.

60-60: LGTM: Consistent use of constants

The replacement of testutil.ExampleAttoDenom with testconstants.ExampleAttoDenom across multiple instances of banktypes.NewMsgSend calls improves consistency and maintainability. This change ensures that the same constant is used throughout the test cases, reducing the risk of inconsistencies.

Also applies to: 142-142, 176-176, 227-227, 245-245, 256-256

290-290: LGTM: Consistent network setup in TestRejectMsgsInAuthz

The use of suite.GetNetwork() to create a new network instance and retrieve the EIP155 chain ID and fee market keeper aligns this test with the standardized network approach. This change ensures consistency across different test cases and improves the overall reliability of the test suite.

Also applies to: 294-294, 296-297

287-287: LGTM: Gas limit variable improves readability

The introduction of the gasLimit variable with a value of 1000000 enhances code readability and makes it easier to adjust this value in the future if needed. This change contributes to better maintainability of the test suite.

447-453: LGTM: Updated transaction creation logic

The changes in the transaction creation logic, including the use of the new network instance, transaction factory, and context obtained from the network, align well with the standardized approach. This update improves consistency across tests and enhances maintainability. The use of suite.GetTxFactory() and suite.GetClientCtx() ensures that the test is using the correct configuration.

The updated block creation process using nw.App.FinalizeBlock() with detailed parameters is a good improvement, providing more control over the test environment.

Also applies to: 457-478

467-468: LGTM: Enhanced test assertions

The addition of assertions for both CheckTx and FinalizeBlock results significantly improves the robustness of the tests. By checking both the error and the response code for each stage, these tests now provide more comprehensive validation of the expected behavior. This change enhances the overall quality and reliability of the test suite.

Also applies to: 481-484

Line range hint 1-484: LGTM: Overall improvements to test suite

The changes in this file represent a significant improvement to the test suite:

Standardized network setup and usage across tests.

Consistent use of constants from a dedicated package.

Improved transaction creation logic using factory methods.

Enhanced assertions for more comprehensive result validation.

Better code readability with named variables and structured setup.

These changes collectively contribute to a more robust, consistent, and maintainable test suite. The standardization efforts should make it easier to write and maintain tests in the future, while the enhanced assertions provide stronger guarantees about the correct behavior of the system under test.

.gitleaks.toml (1)

18-18: Expanded allowlist for Go-related files: Approved

The modification to include go.work and go.work.sum in the allowlist is a good improvement. This change ensures that gitleaks will now consider these additional Go workspace-related files, which are part of Go's newer workspace feature. This expansion enhances the tool's coverage and reduces false positives for legitimate Go workspace configurations.
ante/cosmos/setup_test.go (3)
6-6: LGTM

The import statement correctly includes the necessary testing utilities from testutils.

15-19: Test suite refactoring enhances maintainability

The refactored TestAnteTestSuite function now initializes the base suite using testutils.AnteTestSuite, which simplifies the setup process. The configuration methods WithLondonHardForkEnabled(true) and WithFeemarketEnabled(true) correctly set up the test environment.

11-11: Verify that all required initializations are preserved

By embedding *testutils.AnteTestSuite into AnteTestSuite, the test suite now relies on the base suite's initializations. Please verify that any custom setup logic previously in AnteTestSuite or a SetupTest method is appropriately handled by the base suite to ensure all tests function correctly.

Run the following script to check for any previous SetupTest methods and review their contents for essential initializations:

Verification successful

All required initializations are preserved

No custom SetupTest method or additional setup logic was found in ante/cosmos/setup_test.go. All initializations are handled by the embedded *testutils.AnteTestSuite.
Scripts executed

The following scripts were executed for the analysis:

Script:
#!/bin/bash
# Description: Find definitions of `SetupTest` methods to identify custom initializations.

# Test: Search for `SetupTest` method definitions.
ast-grep --lang go --pattern 'func ($_ *AnteTestSuite) SetupTest() {$$$}'
Length of output: 4964

Script:
#!/bin/bash
# Description: Check for any `SetupTest` method definitions in ante/cosmos/setup_test.go

# Test: Search for `SetupTest` method definitions in ante/cosmos/setup_test.go
ast-grep --lang go --pattern 'func ($_ *AnteTestSuite) SetupTest() {$$$}' ante/cosmos/setup_test.go
Length of output: 101

Script:
#!/bin/bash
# Description: Check for any initialization code or custom setup logic in ante/cosmos/setup_test.go

# Test: Search for common initialization patterns and additional setup methods
grep -E 'func\s+\w+\s*\(' ante/cosmos/setup_test.go
grep -E 'var\s+\w+\s+' ante/cosmos/setup_test.go
grep -E ':=|=' ante/cosmos/setup_test.go
Length of output: 229
ante/interfaces/cosmos.go (5)
30-30: Addition of ValidatorAddressCodec method enhances address handling

The new ValidatorAddressCodec method in the StakingKeeper interface provides access to the addresscodec.Codec, which is beneficial for encoding and decoding validator addresses consistently across the codebase.

17-17: Update implementations of GetBalance to use context.Context

The GetBalance method now accepts context.Context, which aligns with Go's standard for context propagation. Ensure that all implementations of the BankKeeper interface have updated their method signatures and that all callers are passing the correct context.

Run the following script to find implementations or calls of GetBalance still using sdk.Context:
#!/bin/bash
# Description: Identify `GetBalance` methods or calls using `sdk.Context`.

# Find implementations of `GetBalance` with `sdk.Context`
rg 'func\s+($\s*\*?\w+\s*$)?\s*GetBalance\s*$.*sdk\.Context.*$' --type go

# Find calls to `GetBalance` passing `sdk.Context`
rg 'GetBalance$\s*[^,]*,\s*[^,]*,\s*[^,]*$' --type go
23-23: Verify updates to WithdrawDelegationRewards method signature

Changing WithdrawDelegationRewards to accept context.Context enhances context handling. Please ensure all implementations and invocations of this method are updated to reflect the new parameter type.

Use the following script to locate any implementations or calls still using sdk.Context:
#!/bin/bash
# Description: Find implementations and usages of `WithdrawDelegationRewards` with `sdk.Context`.

# Find implementations with `sdk.Context`
rg 'func\s+($\s*\*?\w+\s*$)?\s*WithdrawDelegationRewards\s*$.*sdk\.Context.*$' --type go

# Find calls to `WithdrawDelegationRewards` passing `sdk.Context`
rg 'WithdrawDelegationRewards$\s*[^,]*,\s*[^,]*,\s*[^,]*$' --type go
29-29: ⚠️ Potential issue

Handle the added error return in BondDenom method

The BondDenom method now returns (string, error) instead of just string, allowing for error handling. Ensure that:

All implementations of the StakingKeeper interface update the method to return an error.

All callers of BondDenom properly handle the potential error to prevent unintended behavior.

Run this script to find implementations and usages not handling the new error:
#!/bin/bash
# Description: Find `BondDenom` implementations and calls that need updating.

# Find implementations of `BondDenom` not returning error
rg 'func\s+($\s*\*?\w+\s*$)?\s*BondDenom\s*$[^)]*$\s+string\s*\{' --type go

# Find calls to `BondDenom` not handling the error
rg '(\w+\s*:=\s*)?\w+\.BondDenom$[^)]*$' --type go
31-31: ⚠️ Potential issue

Update IterateDelegations method to handle error return

The IterateDelegations method now returns an error and accepts context.Context. Ensure that:

All implementations of this method are updated to match the new signature and handle context appropriately.

All callers check and handle the returned error to maintain robustness.

Use this script to identify implementations and calls needing updates:
#!/bin/bash
# Description: Find `IterateDelegations` implementations and usages not handling the error.

# Find implementations not returning error
rg 'func\s+($\s*\*?\w+\s*$)?\s*IterateDelegations\s*$.*$\s*\{' --type go

# Find calls to `IterateDelegations` not handling the error
rg '(\w+\s*:=\s*)?\w+\.IterateDelegations$[^)]*$' --type go
api/os/evm/v1/access_list_tx.go (1)

64-65: Verify the correctness of signature extraction in GetRawSignatureValues.

Ensure that ethutils.RawSignatureValues(tx.V, tx.R, tx.S) correctly handles cases where tx.V, tx.R, or tx.S might be nil or improperly formatted. Adding validation checks can prevent errors during signature processing.

api/os/evm/v1/dynamic_fee_tx.go (1)

1-2: Verify SPDX license identifier

The SPDX license identifier "ENCL-1.0" used in the header may not be a standard SPDX identifier. Please verify that the license identifier is correct and recognized. If "ENCL-1.0" is a custom license, consider providing additional documentation or clarifying the license terms.

encoding/config.go (1)

26-42: Verify correct initialization of signing options and codecs

The initialization of signingOptions and the use of types.NewInterfaceRegistryWithOptions appear correct. However, ensure that the custom signer functions for evmtypes.MsgEthereumTxCustomGetSigner and erc20types.MsgConvertERC20CustomGetSigner are properly registered and compatible with the updated signing mechanisms.

Run the following script to verify the registration of custom signer functions:

ante/evm/signverify_test.go (1)

14-85: Well-structured test suite covering key scenarios

The TestEthSigVerificationDecorator function is well-structured, and the test cases effectively cover various scenarios of Ethereum signature verification, including valid and invalid transactions, as well as the handling of unprotected transactions based on configuration. This comprehensive approach enhances the reliability of the signature verification logic.

ante/cosmos/utils_test.go (2)

98-100: Confirm that setting Signature: nil is appropriate

In the SignatureV2 initialization, the Signature field is set to nil as a placeholder. Verify that this aligns with the expected behavior of your signing process and that it doesn't cause issues during transaction processing.

116-122: Verify parameters for tx.SignWithPrivKey

Ensure that the parameters passed to tx.SignWithPrivKey match the expected signature of the function. Specifically, check that the order and types of ctx, defaultSignMode, signerData, txBuilder, priv, txCfg, and sequence are correct according to the function's definition.

You can refer to the function's signature and adjust the parameters if necessary.

ante/evm/fee_market_test.go (1)

19-144: Well-structured test suite covers multiple transaction scenarios

The TestGasWantedDecorator function is comprehensive and effectively tests the GasWantedDecorator with various types of transactions, ensuring that the decorator behaves correctly under different conditions.

example_chain/ante/evm_benchmark_test.go (1)

155-156: Verify the implementation of SigGasConsumer

Ensure that ante.SigVerificationGasConsumer aligns with the latest Cosmos SDK best practices for gas consumption during signature verification. Updates in the SDK may require changes to this function to maintain accuracy and efficiency.

Run the following script to check for any updates or differences in the SigVerificationGasConsumer implementation:

Review the differences and update the local implementation if necessary.

ante/testutils/testutil.go (4)

1-2: License headers are correctly included

The file includes the appropriate copyright notice and SPDX license identifier.

28-42: Well-defined AnteTestSuite struct

The AnteTestSuite struct effectively encapsulates all necessary fields for testing ante handlers, allowing for flexible configuration and extension.

46-118: Comprehensive test setup in SetupTest method

The SetupTest method initializes the test environment effectively, including custom genesis states for the fee market and EVM configurations, as well as network and keyring setup. This provides a solid foundation for ante handler testing.

120-158: Accessor methods provide clear interfaces

The getter methods for keyring, transaction factory, network, client context, and ante handler are well-defined, providing easy access to these components for testing purposes.

ethereum/eip712/encoding.go (3)

11-11: Import addition is appropriate

The addition of "github.com/cosmos/cosmos-sdk/codec/types" is necessary for the use of types.InterfaceRegistry. This import is correct.

220-224: Validation of signers updated correctly

The update to use protoCodec.GetMsgV1Signers(m) enhances the signer retrieval by incorporating error handling. The checks for the number of signers and consistency across messages are appropriate.

Also applies to: 229-229, 233-233

28-30: Verify all usages of SetEncodingConfig are updated

The SetEncodingConfig function signature has changed to accept *codec.LegacyAmino and types.InterfaceRegistry. Please ensure that all calls to SetEncodingConfig in the codebase are updated to match the new signature to prevent potential runtime errors.

Run the following script to find all usages of SetEncodingConfig:

ethereum/eip712/eip712_test.go (1)

453-466: Proper nil check before iterating over messages

Adding the if ok check before iterating over messages prevents a potential nil pointer dereference when interfaceMessages is nil. This improves the robustness of the test by ensuring that the code only attempts to iterate over messages when they are present.
ante/evm/utils_test.go (1)
71-71: Ensure correct calculation of fees

The fees are calculated using txData.Fee(). Please verify that txData.Fee() returns the intended fee amount and that it aligns with the network's fee structure.

Run the following script to confirm the fee calculation:
#!/bin/bash
# Description: Verify that txData.Fee() returns the correct fee amount.

# Test: Check the usage of txData.Fee() in the codebase.
# Expect: The fee amount should be correctly calculated and used.
rg --type go 'txData\.Fee'
api/os/feemarket/v1/genesis.pulsar.go (1)

1-659: LGTM!

The generated code for the GenesisState struct and its associated methods appears correct and follows the expected protobuf conventions.

Tools

GitHub Check: CodeQL

[notice] 13-13: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

ante/evm/ante_test.go (1)

1064-1065: ⚠️ Potential issue

Handle error when setting balance in EVM Keeper

While setting the balance using SetBalance, if an error occurs, it could cause unintended test behaviors. Ensure that the error is properly checked and handled to prevent silent failures.

The code already handles the error with suite.Require().NoError(err); no action needed.

api/os/crypto/v1/ethsecp256k1/keys.pulsar.go (1)

1-1056: LGTM!

The generated code appears correct and adheres to protobuf conventions.

Tools

GitHub Check: CodeQL

[notice] 12-12: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

api/os/feemarket/v1/tx.pulsar.go (1)

1-1091: LGTM: Implementation of new message types is correct

The MsgUpdateParams and MsgUpdateParamsResponse message types are implemented correctly with their associated fast reflection methods. The code adheres to protobuf and Go best practices.

Tools

GitHub Check: CodeQL

[notice] 15-15: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

api/os/evm/v1/genesis.pulsar.go (1)

1-1448: Skip review of generated code

Since this file is entirely generated, a detailed manual code review may not be necessary. Ensure that the code generation process is well-documented and that the source .proto files are correctly defined and reviewed.

Tools

GitHub Check: CodeQL

[notice] 13-13: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

api/os/erc20/v1/genesis.pulsar.go (1)

13-13: Sensitive package import: Existing concern remains valid.

The previous comment about the import of the reflect package is still applicable.

Tools

GitHub Check: CodeQL

[notice] 13-13: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism

.github/workflows/super-linter.yml

.pylintrc

client/block/store.go

ante/evm/04_validate.go

ante/evm/04_validate_test.go

ante/evm/ante_test.go

api/os/crypto/v1/ethsecp256k1/keys.pulsar.go

api/os/evm/v1/genesis.pulsar.go

api/os/erc20/v1/genesis.pulsar.go

0xstepit · 2024-09-24T09:11:25Z

@MalteHerrmann we should add some branch protection rule because ATM I could squash and merge without any approval of the PR.

0xstepit

Great!

coderabbitai

Actionable comments posted: 0

Outside diff range and nitpick comments (3)

ante/evm/utils_test.go (3)

43-74: LGTM: CreateTxBuilder refactored for improved flexibility

The refactoring of CreateTxBuilder simplifies the function signature and improves flexibility. The use of suite.GetTxFactory() and suite.GetNetwork().GetDenom() enhances consistency across the test suite.

However, consider splitting the transaction generation and signing into separate methods to improve separation of concerns and make the code more modular.

Line range hint 110-309: LGTM: Message creation functions updated for consistency and expanded functionality

The changes in various message creation functions improve consistency by using suite.GetNetwork().GetDenom() and sdkmath. These modifications enhance flexibility and maintainability across the test suite. The new and modified functions also expand the test suite's capabilities.

Consider adding or updating function documentation to explain the purpose and usage of each message creation function, especially for the newly added ones.

563-574: LGTM: New prepareAccount function added

The new prepareAccount helper function is a valuable addition that simplifies account setup for delegation reward tests. It promotes code reuse by leveraging testutil.PrepareAccountsForDelegationRewards.

Consider adding a comment explaining why the context is modified with a new gas meter and incremented block height, as this might affect test behavior in ways that aren't immediately obvious.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between fbacf4a and 0d60901.

Files selected for processing (4)

ante/evm/fee_market_test.go (1 hunks)
ante/evm/utils_test.go (15 hunks)
example_chain/ante/evm_benchmark_test.go (1 hunks)
example_chain/ante/integration_test.go (1 hunks)

Files skipped from review as they are similar to previous changes (3)

ante/evm/fee_market_test.go
example_chain/ante/evm_benchmark_test.go
example_chain/ante/integration_test.go

Additional comments not posted (3)

ante/evm/utils_test.go (3)

9-12: LGTM: Import statements updated appropriately

The new import statements and updated import paths align with the changes made in the rest of the file. These updates suggest the integration of new functionality and are consistent with the overall modifications.

Also applies to: 31-32, 34-34, 38-38

Line range hint 96-104: LGTM: CreateTestCosmosTxBuilder updated with improved consistency

The changes in CreateTestCosmosTxBuilder improve consistency by using sdkmath.Int and constants from testutils for gas limit and fee calculations. These modifications enhance maintainability and align with the updated import statements.

Line range hint 314-341: Please clarify the changes in StdSignBytes function

The StdSignBytes function has undergone significant changes:

The fee legacytx.StdFee parameter has been removed from the function signature.

The implementation now uses fee.Bytes() instead of marshaling the fee directly.

Could you please provide more context on these changes? Specifically:

How are fees now being handled without the explicit fee parameter?

What prompted the change to use fee.Bytes(), and are there any implications for fee processing or serialization?

These clarifications will help ensure that the changes are correctly implemented and don't introduce any unintended side effects.

hanchon · 2024-09-24T15:53:06Z

@MalteHerrmann we should add some branch protection rule because ATM I could squash and merge without any approval of the PR.

I'll update the configuration tomorrow morning!

GAtom22

LGTM!! Great work @MalteHerrmann!

hanchon · 2024-09-26T16:37:34Z

LGTM! 🚀

MalteHerrmann added 30 commits September 17, 2024 11:41

bump buf-setup-action version to v1.41.0 (7a2294c7)

7273872

bump ibc-go to v7.8.0 (bfeba33a)

960b775

fix eip after protobuf params changes (5b933ddf)

35104db

add codehash on erc-20 precompiles and update mocks (88d2207a)

8a44a91

bump golang.org/x/crypto to v0.27.0 (9567d352)

f5806fb

bump golang.org/x/net to v0.29.0 (67f0b0cf)

042e3ea

additional methods for custom opcodes definitions (2ffe83e9)

c3c1d58

adjust overflow linter warnings (0822b8fc)

ad4f36e

fix typos in some proto files and regenerate protos (6d0cf345)

38c06a7

bump google grpc dep to v1.66.1 (0de07c73)

e367ba1

recap the highest gas limit with account's available balance (02c2414f)

44bf7bd

bump google grpc to v1.66.2 (bc9ca7b6)

587a12f

sdk50 - utils

618469b

sdk-v50 - server & indexer wip

9b3f5b7

sdk-v50 - encoding, testutil, eip712

16b3978

sdk-v50 - ibc, client

2ab0a76

sdk-v50 - protos

5b46e29

sdk-v50 - linter config

738dcde

sdk-v50 - rpc and some general renamings

94e5a31

sdk-v50 - feemarket

b6a80b1

sdk-v50 - evm

fa95f80

sdk-v50 - erc20

0464233

sdk-v50 - x/ibc/transfer

f08a586

sdk-v50 - precompiles - bank

2315b72

sdk-v50 - precompiles - bech32

be1e2f9

sdk-v50 - precompiles - common

192178a

sdk-v50 - precompiles - distribution

b5feef7

sdk-v50 - precompiles - erc20

9528822

sdk-v50 - precompiles - ics20

2955f79

sdk-v50 - precompiles - p256

ba4fdf2

MalteHerrmann requested a review from a team as a code owner September 24, 2024 08:18

MalteHerrmann requested review from Vvaradinov and 0xstepit and removed request for a team September 24, 2024 08:18

MalteHerrmann added 4 commits September 24, 2024 10:20

address markdown linter

ff88814

add changelog entry

ad39121

make script executable

6955288

add go work files to gitleaks ignore and run go mod tidy

fbacf4a

coderabbitai bot reviewed Sep 24, 2024

View reviewed changes

wip fix solidity

ce4684b

0xstepit approved these changes Sep 24, 2024

View reviewed changes

MalteHerrmann added 2 commits September 24, 2024 12:46

fix chain starting in solidity tests

b9c6cd6

wait some time for blocks in solidity tests

c4ea4b5

MalteHerrmann linked an issue Sep 24, 2024 that may be closed by this pull request

Move the latest state incl. Cosmos SDK v0.50 on Evmos main to the OS repository #46

Closed

address coderabbit comments

0d60901

coderabbitai bot reviewed Sep 24, 2024

View reviewed changes

MalteHerrmann and others added 3 commits September 24, 2024 18:30

remove local replace for Cosmos SDK

b2a4187

fix order in init-node.sh

a0c65ce

update example chain dep

1e43b90

GAtom22 approved these changes Sep 26, 2024

View reviewed changes

GAtom22 mentioned this pull request Sep 26, 2024

chore(api): use func to return txData evmos/evmos#2884

Merged

9 tasks

mergify bot mentioned this pull request Sep 26, 2024

chore(api): use func to return txData (backport #2884) evmos/evmos#2886

Merged

9 tasks

hanchon approved these changes Sep 26, 2024

View reviewed changes

fedekunze merged commit bb4b78d into main Sep 26, 2024
24 of 27 checks passed

fedekunze deleted the malte/bump-diff-from-evmos-main-20240917 branch September 26, 2024 17:30

coderabbitai bot mentioned this pull request Sep 30, 2024

imp(ante): Refactor ante handlers to be easier to use in partner chains (target main) #52

Merged

coderabbitai bot mentioned this pull request Oct 9, 2024

improvements(cli): Enable specifying default key type for adding keys #53

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imp(all): Move latest changes from Evmos main (incl. SDK v50) #48

imp(all): Move latest changes from Evmos main (incl. SDK v50) #48

MalteHerrmann commented Sep 19, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot left a comment

0xstepit commented Sep 24, 2024

0xstepit left a comment

coderabbitai bot left a comment

hanchon commented Sep 24, 2024

GAtom22 left a comment

hanchon commented Sep 26, 2024

imp(all): Move latest changes from Evmos main (incl. SDK v50) #48

imp(all): Move latest changes from Evmos main (incl. SDK v50) #48

Conversation

MalteHerrmann commented Sep 19, 2024 • edited by coderabbitai bot Loading

Summary by CodeRabbit

coderabbitai bot left a comment

Choose a reason for hiding this comment

0xstepit commented Sep 24, 2024

0xstepit left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

hanchon commented Sep 24, 2024

GAtom22 left a comment

Choose a reason for hiding this comment

hanchon commented Sep 26, 2024

MalteHerrmann commented Sep 19, 2024 •

edited by coderabbitai bot

Loading