Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(evm)!: Refactor out dead code from the evm.Params #2065

Merged
merged 3 commits into from
Oct 7, 2024

Conversation

Unique-Divine
Copy link
Member

@Unique-Divine Unique-Divine commented Oct 7, 2024

Purpose / Abstract

Summary by CodeRabbit

Release Notes

  • New Features

    • Added a structured "Unreleased" section in the changelog for better organization of pending changes.
    • Introduced various Ethereum-related functionalities in the Nibiru EVM, including event management and transaction handling.
  • Improvements

    • Simplified transaction fee verification and gas refund processes by directly referencing the new EVMBankDenom.
    • Enhanced transaction validation logic to ensure all Ethereum transactions are protected.
    • Streamlined the parameter structure and validation logic for EVM configuration.
  • Bug Fixes

    • Addressed issues in transaction handling and keyring import functions.
  • Documentation

    • Updated the changelog format for improved readability and navigation.
  • Tests

    • Expanded test coverage for Ethereum transactions, ensuring robust validation against various scenarios.

@Unique-Divine Unique-Divine requested a review from a team as a code owner October 7, 2024 08:06
Copy link
Contributor

coderabbitai bot commented Oct 7, 2024

Caution

Review failed

The pull request is closed.

Walkthrough

The changes introduced in this pull request encompass a comprehensive update to the NibiruChain project, focusing on the CHANGELOG.md format and content, along with several modifications in various source files. Key updates include the addition of an "Unreleased" section in the changelog, the refactoring of Ethereum transaction handling methods to streamline the usage of the Ethereum denomination, and the deprecation of certain parameters in the protocol definitions. The overall structure and functionality of the affected components remain intact, with an emphasis on improving clarity and consistency across the codebase.

Changes

File Path Change Summary
CHANGELOG.md Updated to include an "Unreleased" section, organized changes into categories, added features and improvements, reflected refactoring and bug fixes, and updated dependency information.
app/evmante/evmante_gas_consume.go Removed retrieval of Ethereum denomination from evmKeeper, directly used evm.EVMBankDenom in VerifyFee method call, and reformatted the method call for readability.
app/evmante/evmante_mempool_fees.go Updated minimum gas price retrieval to use evm.EVMBankDenom instead of evmKeeper parameters.
app/evmante/evmante_sigverify.go Removed retrieval of Ethereum parameters, hardcoded allowUnprotectedTxs to false, tightening transaction validation logic.
app/evmante/evmante_validate_basic.go Removed retrieval of EVM parameters, replaced evmDenom with evm.EVMBankDenom, and maintained existing validation checks.
eth/eip712/eip712_test.go Updated denom field in EIP712TestSuite to use evm.EVMBankDenom.
eth/rpc/backend/call_tx.go Replaced query for EVM parameters with direct reference to evm.EVMBankDenom in SendRawTransaction method.
eth/rpc/backend/node_info.go Updated RPCMinGasPrice method to directly use evm.EVMBankDenom instead of querying parameters.
eth/rpc/rpcapi/eth_api_test.go Adjusted assignment of pendingEthTxEventHash and pendingEthTxEventIndex for readability in Test_SimpleTransferTransaction.
proto/eth/evm/v1/evm.proto Deprecated fields in Params and TraceConfig messages, replacing them with reserved tags.
x/evm/genesis.go Minor change in error message for duplicate genesis account detection.
x/evm/genesis_test.go Restructured TestValidateGenesis function, added wantErr field for expected error messages, and updated test case names for clarity.
x/evm/keeper/gas_fees_test.go Renamed variable feeDenom from evm.DefaultEVMDenom to evm.EVMBankDenom.
x/evm/keeper/grpc_query_test.go Updated token denomination from evm.DefaultEVMDenom to evm.EVMBankDenom in multiple test cases.
x/evm/keeper/keeper.go Simplified GetEvmGasBalance method by using evm.EVMBankDenom directly.
x/evm/keeper/msg_server.go Updated gas refund logic in ApplyEvmTx and fee calculation in FeeForCreateFunToken to use evm.EVMBankDenom.
x/evm/keeper/statedb.go Replaced parameterized EvmDenom with evm.EVMBankDenom in SetAccBalance method.
x/evm/keeper/statedb_test.go Changed references from evm.DefaultEVMDenom to evm.EVMBankDenom in TestStateDBBalance.
x/evm/msg_test.go Updated transaction fee denomination to evm.EVMBankDenom and expanded test cases for ValidateBasic method.
x/evm/params.go Removed DefaultEVMDenom, added EVMBankDenom, and updated validation logic in DefaultParams and Validate methods.

Assessment against linked issues

Objective Addressed Explanation
EvmDenom should be a constant.
AllowUnprotectedTxs is always false; it should be removed.
EVMChannels is unused; it should be documented. Documentation for EVMChannels was not provided.

Possibly related PRs

Poem

🐇 In the code where rabbits hop,
Changes made, we can't stop!
With EVMBankDenom in sight,
Our transactions now feel right.
Changelog clear, a joyful cheer,
Nibiru's path is bright this year! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (12)
app/evmante/evmante_sigverify.go (1)

Line range hint 51-57: Consider removing the redundant check for unprotected transactions

Since allowUnprotectedTxs is now hardcoded to false, the conditional check for unprotected transactions has become redundant. To further simplify the code, consider removing this check entirely.

You can replace the current code with:

-		allowUnprotectedTxs := false
-		ethTx := msgEthTx.AsTransaction()
-		if !allowUnprotectedTxs && !ethTx.Protected() {
+		ethTx := msgEthTx.AsTransaction()
+		if !ethTx.Protected() {
 			return ctx, errors.Wrapf(
 				sdkerrors.ErrNotSupported,
 				"rejected unprotected Ethereum transaction. "+
 					"Please EIP155 sign your transaction to protect it against replay-attacks",
 			)
 		}

This change maintains the same behavior while reducing code complexity.

x/evm/params.go (4)

19-23: Approved: EVMBankDenom constant addition

The addition of the EVMBankDenom constant aligns well with the PR objective to define the EVM denomination as a constant. This change enhances code clarity and prevents potential governance attacks by fixing the denomination.

Consider adding a brief note about why this constant is important for security, e.g.:

 // EVMBankDenom specifies the bank denomination for the asset used to run EVM
 // state transitions as the analog to "ether". 1 ether in solidity means 1
 // NIBI on Nibru EVM, implying that the EVMBankDenom is "unibi", the coin
-// base of the NIBI token.
+// base of the NIBI token. Defining this as a constant enhances security by
+// preventing potential governance attacks that could alter the denomination.

30-33: Approved: DefaultParams function update with suggestion

The changes in the DefaultParams function align well with the PR objectives. The removal of the EvmDenom assignment is consistent with defining it as a constant elsewhere. The added comment about EVMChannels being unused but intended for future IBC functionality addresses the documentation objective from the linked issue.

Consider addressing the CreateFuntokenFee parameter:

  1. If it's still needed, add a comment explaining its purpose and why it's retained.
  2. If it's no longer necessary, remove it to fully align with the PR's goal of refactoring out dead code.

Example:

return Params{
    ExtraEIPs:   []int64{},
    EVMChannels: []string{},
    // CreateFuntokenFee: Explain why this is retained or remove if unnecessary
    CreateFuntokenFee: math.NewIntWithDecimal(10_000, 6), // 10_000 NIBI
}

Line range hint 61-66: Approved: Simplified Validate function

The simplification of the Validate function aligns well with the PR objectives. Removing the validation for EvmDenom and the boolean parameter (likely AllowUnprotectedTxs) is consistent with the goals of refactoring out dead code and removing unused parameters.

Consider updating the function comment to reflect the current validation scope:

// Validate performs basic validation on evm parameters,
// checking ExtraEIPs and EVMChannels.
func (p Params) Validate() error {
    // ... (rest of the function remains the same)
}

Line range hint 1-105: Overall: Successful refactoring with minor suggestions

The changes in this file successfully address the main objectives of the PR:

  1. EvmDenom is now defined as a constant (EVMBankDenom), enhancing security.
  2. The likely AllowUnprotectedTxs parameter has been removed from the Validate function.
  3. EVMChannels is now documented as unused but intended for future IBC functionality.

These changes effectively refactor out dead code from evm.Params and improve the overall structure and security of the module.

To further improve the code:

  1. Consider addressing the CreateFuntokenFee parameter in DefaultParams() as mentioned earlier.
  2. Ensure that all uses of the former EvmDenom parameter throughout the codebase are updated to use the new EVMBankDenom constant.
  3. If there are any other files that depend on the removed parameters or changed structures, make sure they are updated accordingly to maintain consistency across the codebase.
x/evm/keeper/statedb_test.go (1)

73-73: LGTM: Consistent denomination changes.

The changes from evm.DefaultEVMDenom to evm.EVMBankDenom at lines 73 and 79 are consistent with the earlier change and align with the PR objectives. This ensures that the entire test function uses the correct EVM denomination.

Consider defining a constant for evm.EVMBankDenom at the package level to improve maintainability. This would allow for easier updates if the denomination changes in the future. For example:

const testEVMDenom = evm.EVMBankDenom

// Then use testEVMDenom throughout the test file

Also applies to: 79-79

eth/rpc/backend/node_info.go (1)

Line range hint 90-96: Approve changes with a suggestion for improved error handling

The refactoring of RPCMinGasPrice aligns well with the PR objectives. The simplification improves code clarity and potentially performance by removing an unnecessary query. The core functionality of determining the minimum gas price is preserved, which is excellent.

However, to further improve robustness, consider adding a check to ensure evm.EVMBankDenom is not empty before using it.

Consider adding a check for evm.EVMBankDenom:

 func (b *Backend) RPCMinGasPrice() int64 {
 	minGasPrice := b.cfg.GetMinGasPrices()
+	if evm.EVMBankDenom == "" {
+		// Log an error or return a default value
+		return eth.DefaultGasPrice
+	}
 	amt := minGasPrice.AmountOf(evm.EVMBankDenom).TruncateInt64()
 	if amt == 0 {
 		return eth.DefaultGasPrice
 	}
 	return amt
 }

This addition ensures that the function behaves predictably even if evm.EVMBankDenom is not set correctly.

x/evm/keeper/gas_fees_test.go (1)

99-99: LGTM! Consider adding a clarifying comment.

The change from evm.DefaultEVMDenom to evm.EVMBankDenom aligns with the PR's objective of refactoring and standardizing EVM-related parameters. This update likely reflects changes in the main codebase where the denomination for EVM transactions has been standardized.

Consider adding a brief comment explaining the significance of EVMBankDenom to improve code readability. For example:

// EVMBankDenom is the standard denomination used for EVM transactions
feeDenom := evm.EVMBankDenom
proto/eth/evm/v1/evm.proto (1)

Line range hint 1-231: Overall assessment of changes in evm.proto

The changes in this file align well with the PR objectives, particularly in simplifying the Params structure and improving naming conventions in TraceConfig. These modifications represent good practices in maintaining protobuf definitions.

However, to ensure a smooth transition:

  1. Verify that the EvmDenom is indeed defined as a constant elsewhere in the codebase.
  2. Confirm that all code using the deprecated fields has been updated accordingly.
  3. Ensure that these changes do not introduce any breaking changes in the API or existing implementations.

Consider updating the documentation for EVMChannels as mentioned in the PR objectives. While the field is present in the Params message, there's no comment explaining its purpose or current usage status.

eth/rpc/backend/call_tx.go (1)

Line range hint 1-424: Consider documenting reasons for keeping other parameters dynamic

While the change to use evm.EVMBankDenom is excellent, it's worth noting that other functions like SetTxDefaults, EstimateGas, and DoCall still use dynamic querying for various parameters. This is expected as per the PR objectives, but it might be beneficial to add comments explaining why these parameters remain dynamic. This documentation would help future developers understand the design decisions and prevent accidental changes to these dynamic queries.

Would you like me to suggest specific locations where such documentation could be added?

eth/eip712/eip712_test.go (1)

Line range hint 1-1024: Consider enhancing test coverage and readability

While the current test suite is comprehensive, consider the following improvements:

  1. Add more edge cases to TestEIP712TestSuite, especially around the new EVMBankDenom usage.
  2. Consider using table-driven tests for TestTypToEth and TestUnpackAny to improve readability and ease of adding new test cases.
  3. Add comments explaining the purpose of each test case in TestEIP712 for better maintainability.

These suggestions aim to further strengthen the robustness and clarity of the test suite.

x/evm/keeper/msg_server.go (1)

506-506: Approved: Consistent use of constant denomination

This change replaces evmParams.EvmDenom with evm.EVMBankDenom for fee calculation, which is consistent with the previous modification and aligns with the PR objectives.

For improved consistency, consider updating the variable name evmParams to reflect that it no longer contains the EvmDenom. For example:

-evmParams := k.GetParams(ctx)
+params := k.GetParams(ctx)
 return sdk.NewCoins(sdk.NewCoin(evm.EVMBankDenom, evmParams.CreateFuntokenFee))
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 37913c4 and 29b40bf.

⛔ Files ignored due to path filters (1)
  • x/evm/evm.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (20)
  • CHANGELOG.md (1 hunks)
  • app/evmante/evmante_gas_consume.go (1 hunks)
  • app/evmante/evmante_mempool_fees.go (1 hunks)
  • app/evmante/evmante_sigverify.go (1 hunks)
  • app/evmante/evmante_validate_basic.go (1 hunks)
  • eth/eip712/eip712_test.go (1 hunks)
  • eth/rpc/backend/call_tx.go (1 hunks)
  • eth/rpc/backend/node_info.go (1 hunks)
  • eth/rpc/rpcapi/eth_api_test.go (1 hunks)
  • proto/eth/evm/v1/evm.proto (2 hunks)
  • x/evm/genesis.go (1 hunks)
  • x/evm/genesis_test.go (5 hunks)
  • x/evm/keeper/gas_fees_test.go (1 hunks)
  • x/evm/keeper/grpc_query_test.go (5 hunks)
  • x/evm/keeper/keeper.go (1 hunks)
  • x/evm/keeper/msg_server.go (2 hunks)
  • x/evm/keeper/statedb.go (2 hunks)
  • x/evm/keeper/statedb_test.go (3 hunks)
  • x/evm/msg_test.go (2 hunks)
  • x/evm/params.go (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • eth/rpc/rpcapi/eth_api_test.go
  • x/evm/genesis.go
🧰 Additional context used
🔇 Additional comments (26)
app/evmante/evmante_sigverify.go (1)

50-50: Approved: Improved security by disallowing unprotected transactions

The change to hardcode allowUnprotectedTxs to false aligns with the PR objective and enhances security by ensuring that all Ethereum transactions must be protected. This modification:

  1. Simplifies the code by removing the dependency on evm.Params.
  2. Eliminates a potential attack vector by preventing unprotected transactions.
  3. May slightly improve performance by removing the GetParams call.

These changes contribute to a more robust and secure implementation.

app/evmante/evmante_mempool_fees.go (1)

41-41: Approve change with verification request.

The modification aligns with the PR objective to define EvmDenom as a constant, simplifying the code and potentially improving performance. This change enhances code clarity and reduces the risk of governance attacks on the economic stability of Nibiru's EVM.

Please run the following script to verify the evm.EVMBankDenom constant:

Ensure that:

  1. EVMBankDenom is defined as a constant.
  2. Its value is set to "NIBI".
  3. It is not reassigned elsewhere in the codebase.
x/evm/keeper/statedb_test.go (2)

Line range hint 1-94: Overall assessment: Changes are consistent and improve code clarity.

The modifications in this file consistently update the EVM denomination from DefaultEVMDenom to EVMBankDenom. These changes align well with the PR objectives and improve the clarity of the test code. The test logic remains intact, ensuring that the functionality is still properly verified.


32-32: LGTM: Denomination change is consistent with PR objectives.

The change from evm.DefaultEVMDenom to evm.EVMBankDenom aligns with the PR's goal to refactor EVM parameters. This modification ensures that the test uses the correct denomination for EVM transactions.

To ensure consistency, let's verify this change across the codebase:

✅ Verification successful

Verification Successful: No remaining instances of DefaultEVMBankDenom found.

All usages of EVMBankDenom are consistent with the PR’s objectives, and no instances of DefaultEVMBankDenom remain in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of DefaultEVMDenom and compare with EVMBankDenom usage

echo "Instances of DefaultEVMDenom:"
rg --type go "DefaultEVMDenom"

echo "\nInstances of EVMBankDenom:"
rg --type go "EVMBankDenom"

Length of output: 2647

app/evmante/evmante_validate_basic.go (1)

127-129: LGTM: Change aligns with PR objectives

The replacement of evmDenom with evm.EVMBankDenom is a positive change that aligns with the PR objective of defining the EVM denomination as a constant. This modification enhances code consistency and potentially improves performance by eliminating the need to fetch the denomination from EVM parameters.

x/evm/keeper/keeper.go (1)

92-92: Excellent simplification aligning with PR objectives

The changes to GetEvmGasBalance method effectively simplify the implementation while aligning with the PR objectives. By directly using the evm.EVMBankDenom constant, we achieve the following:

  1. Simplify the code by removing unnecessary parameter retrieval and checks.
  2. Align with the objective of defining EvmDenom as a constant, potentially preventing governance attacks that could alter the denomination.
  3. Maintain the existing functionality while improving code readability.

This change strikes a good balance between simplification and security.

app/evmante/evmante_gas_consume.go (1)

100-105: LGTM! Simplified fee verification process.

The changes align with the PR objectives by removing the dynamic retrieval of evmDenom and using evm.EVMBankDenom directly. This simplification improves code clarity and potentially prevents governance attacks by using a constant denomination.

Please run the following script to verify the evm.EVMBankDenom constant:

This script will help confirm that evm.EVMBankDenom is indeed a constant set to "NIBI" and is not reassigned elsewhere in the codebase.

x/evm/keeper/statedb.go (1)

73-73: Excellent refactoring of EvmDenom to use the constant evm.EVMBankDenom

This change aligns perfectly with the PR objective of defining EvmDenom as a constant. By replacing the parameterized EvmDenom with evm.EVMBankDenom, you've enhanced the security of the system by preventing potential governance attacks that could alter the denomination. The modification is consistent across all occurrences within the SetAccBalance method, maintaining its original functionality while improving its robustness.

Also applies to: 79-79, 88-88

proto/eth/evm/v1/evm.proto (2)

46-48: Approve removal of allow_unprotected_txs field

This change aligns with the PR objective to remove the AllowUnprotectedTxs parameter, which was always set to false. This simplification of the Params structure is a positive step.

To ensure this change doesn't have unintended consequences, please run the following script to check for any remaining references to AllowUnprotectedTxs in the codebase:

#!/bin/bash
# Search for any remaining references to AllowUnprotectedTxs
rg --type go 'AllowUnprotectedTxs'

Line range hint 180-182: Approve renaming of memory and return data fields

The deprecation of DisableMemory and DisableReturnData in favor of EnableMemory and EnableReturnData improves code clarity by using positive naming conventions. The reservation of field numbers and names is a good practice in protobuf definitions.

To ensure this change is fully implemented, please run the following script to check for any remaining uses of the old field names and to verify the presence of the new field names:

#!/bin/bash
# Search for any remaining uses of the old field names
rg --type go 'DisableMemory|DisableReturnData'

# Verify the presence of the new field names
rg --type go 'EnableMemory|EnableReturnData'
🧰 Tools
🪛 GitHub Check: break-check

[failure] 30-30:
Previously present field "1" with name "evm_denom" on message "Params" was deleted.


[failure] 30-30:
Previously present field "6" with name "allow_unprotected_txs" on message "Params" was deleted.

eth/rpc/backend/call_tx.go (1)

52-52: Excellent refactoring to use constant EVMBankDenom

This change aligns perfectly with the PR objectives. By using evm.EVMBankDenom directly:

  1. It eliminates the need for querying EVM parameters, simplifying the code.
  2. It potentially improves performance by removing an extra database call.
  3. It enhances security by preventing potential changes to the EVM denomination through governance.
  4. It makes the code more maintainable and less prone to errors.

Great job on this refactoring!

eth/eip712/eip712_test.go (1)

83-83: LGTM! Verify consistency across codebase.

The change from evm.DefaultEVMDenom to evm.EVMBankDenom aligns with the PR objectives of refactoring the evm.Params structure. This update ensures that the test suite uses the correct denomination.

To ensure consistency, let's check if this change has been applied uniformly across the codebase:

✅ Verification successful

Verified: DefaultEVMDenom is no longer used in the codebase.

The change from DefaultEVMDenom to EVMBankDenom has been successfully applied and is consistent across all relevant files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining instances of DefaultEVMDenom
rg "DefaultEVMDenom" --type go

# Search for usage of EVMBankDenom to verify it's used consistently
rg "EVMBankDenom" --type go

Length of output: 2511

x/evm/keeper/msg_server.go (2)

114-114: Approved: Improved security by using a constant denomination

This change replaces evmConfig.Params.EvmDenom with the constant evm.EVMBankDenom for gas refunds. This aligns with the PR objective to refactor out dead code from evm.Params and enhances security by preventing potential governance attacks that could alter the EVM denomination.


Line range hint 1-824: Summary: Successful refactoring of EvmDenom usage

The changes in this file successfully address the PR objectives by replacing the mutable EvmDenom parameter with the constant evm.EVMBankDenom. This refactoring improves security by preventing potential governance attacks that could alter the EVM denomination. The modifications are consistent throughout the file and do not introduce any apparent issues.

x/evm/keeper/grpc_query_test.go (5)

79-81: LGTM: Updated EVM denomination

The change from evm.DefaultEVMDenom to evm.EVMBankDenom is consistent with the PR objectives to refactor the evm.Params structure. This update ensures that the test cases use the correct denomination for EVM-related operations.


104-106: LGTM: Consistent denomination update

The change from evm.DefaultEVMDenom to evm.EVMBankDenom is consistent with the previous modification and aligns with the PR objectives to refactor the evm.Params structure. This ensures consistency across test cases.


563-565: LGTM: Consistent denomination update

The change from evm.DefaultEVMDenom to evm.EVMBankDenom is consistent with the previous modifications and aligns with the PR objectives to refactor the evm.Params structure. This ensures consistency across different operations in the test cases.


680-682: LGTM: Consistent denomination update

The change from evm.DefaultEVMDenom to evm.EVMBankDenom is consistent with all previous modifications and aligns with the PR objectives to refactor the evm.Params structure. This ensures consistency across all test cases and operations.


Line range hint 460-466: Verify relevance of EVMChannels test case

The update from EvmDenom to EVMChannels aligns with the PR objectives. However, given that the PR objectives mention EVMChannels as unused, we should verify if this test case is still relevant or if it needs further adjustment.

CHANGELOG.md (5)

Line range hint 19-131: LGTM! Well-structured "Unreleased" section.

The "Unreleased" section is well-organized and provides a clear overview of upcoming changes. It follows good changelog practices by categorizing changes into "State Machine Breaking", "Non-breaking/Compatible Improvements", and "Dependencies". This structure helps users and developers quickly understand the nature and impact of recent changes.


Line range hint 133-220: LGTM! Well-structured v1.0.0 release section with room for minor improvements.

The v1.0.0 release section effectively communicates the significance of this major release for the mainnet network. The categorization of changes into "Features", "State Machine Breaking", and "Improvements" is helpful for users.

Suggestions for improvement:

  1. Consider adding brief explanations for some of the more technical changes to help users understand their impact.
  2. It might be beneficial to include a high-level summary of the most important changes at the beginning of the section.

Line range hint 222-1130: LGTM! Comprehensive release history with room for consistency improvements.

The release sections from v0.21.11 to v0.19.4 provide a detailed historical record of changes across multiple versions. The categorization of changes and inclusion of pull request numbers is very helpful for tracking the project's evolution.

Suggestions for improvement:

  1. Strive for more consistent formatting across all release sections. Some sections are more detailed than others.
  2. Consider using a standard set of categories across all releases for easier comparison (e.g., "Features", "Bug Fixes", "State Machine Breaking", "API Breaking", "Improvements").
  3. For larger changes or features, a brief explanation of their impact or purpose could be beneficial.

Line range hint 1132-1556: LGTM! Valuable historical record of earlier releases.

The inclusion of older release information (v0.16.3 and earlier) provides a comprehensive history of the project's development. The consistent use of categorized changes and pull request numbers throughout the project's history is commendable.

Suggestions for improvement:

  1. For significant changes in these earlier versions, consider adding brief explanations of their impact or purpose, especially if they laid the groundwork for future features.
  2. If possible, retroactively add links to specific commits or pull requests for easier reference.
  3. Consider summarizing major milestones or themes for each significant version change to provide a higher-level overview of the project's evolution.

Line range hint 1-1556: LGTM! Well-maintained and comprehensive CHANGELOG.md.

This CHANGELOG.md file is an excellent resource, providing a detailed history of the project's development from its early stages to the latest unreleased changes. The consistent use of categorized changes and pull request references throughout is commendable and greatly aids in understanding the project's evolution.

Suggestions for further improvement:

  1. Consider adding a brief introduction at the top of the file explaining the changelog's purpose and how to interpret the different sections.
  2. For major releases or significant changes, consider adding a high-level summary of the most important updates to help users quickly grasp the key points.
  3. Strive for more consistent levels of detail across all versions, particularly in explaining the impact or purpose of significant changes.
  4. Consider using a tool or script to help maintain consistent formatting and ensure all pull request numbers are correctly linked.

Overall, this CHANGELOG.md is a valuable asset to the project and demonstrates a strong commitment to keeping users and developers informed about the project's progress and changes.

x/evm/genesis_test.go (1)

126-130: Verify the use of zero address in the test case "happy genesis with account"

In the test case named "happy genesis with account", the zero address (gethcommon.Address{}.String()) is used as the account address. Please ensure that using the zero address is acceptable in this context, as it may be considered invalid or reserved in Ethereum standards.

Run the following script to check how the zero address is handled in the codebase:

✅ Verification successful

The use of the zero address in the "happy genesis with account" test case is appropriate and does not introduce issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for validation logic related to the zero address.

# Test: Find any occurrence of address validation that may reject the zero address.
rg --type go 'IsZero\(\)|common.Address\{\}' -A 5

Length of output: 12160

x/evm/msg_test.go (1)

Line range hint 115-124: Updates to use evm.EVMBankDenom are appropriate

The changes correctly update the transaction building and fee assertions to use evm.EVMBankDenom instead of the previous denomination constant. This aligns with the PR objectives to define EvmDenom as a constant, enhancing code clarity and preventing potential governance attacks related to denomination changes.

Comment on lines +32 to +33
// DEPRECATED: evm_denom
reserved 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

EvmDenom constant not found

The EvmDenom constant definition was not found in the codebase. Please ensure that EvmDenom is defined as a constant to align with the PR objectives.

🔗 Analysis chain

Approve deprecation of evm_denom field

This change aligns with the PR objective to define EvmDenom as a constant, which helps prevent potential governance attacks by removing the ability to alter the denomination dynamically.

To ensure this change is fully implemented, please run the following script to verify that EvmDenom is defined as a constant elsewhere in the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for EvmDenom constant definition
rg --type go 'const\s+EvmDenom\s*='

Length of output: 1420

Comment on lines 91 to 92
wantErr string
expPass bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove unused expPass field from test case struct

The expPass field in the test case struct is no longer used in the test logic and can be safely removed to clean up the code.

Apply this diff to remove the unused field:

 type testCase struct {
     name     string
     genState *evm.GenesisState
     wantErr  string
-    expPass  bool
 }

Committable suggestion was skipped due to low confidence.

s.Run(tc.name, func() {
err := tc.genState.Validate()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Capture loop variable tc when running subtests

When iterating over testCases and running subtests with s.Run, the loop variable tc should be captured to prevent variable capture issues, ensuring each subtest receives the correct tc value.

Apply this diff to capture the loop variable:

 for _, tc := range testCases {
+    tc := tc
     s.Run(tc.name, func() {
         // Test code
     })
 }

Committable suggestion was skipped due to low confidence.

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 95.23810% with 1 line in your changes missing coverage. Please review.

Project coverage is 65.04%. Comparing base (37913c4) to head (25e4313).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
x/evm/keeper/msg_server.go 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2065      +/-   ##
==========================================
+ Coverage   65.03%   65.04%   +0.01%     
==========================================
  Files         270      270              
  Lines       17078    17050      -28     
==========================================
- Hits        11106    11091      -15     
+ Misses       5028     5021       -7     
+ Partials      944      938       -6     
Files with missing lines Coverage Δ
app/evmante/evmante_gas_consume.go 69.56% <100.00%> (+1.38%) ⬆️
app/evmante/evmante_mempool_fees.go 88.23% <100.00%> (ø)
app/evmante/evmante_sigverify.go 100.00% <100.00%> (ø)
app/evmante/evmante_validate_basic.go 86.36% <100.00%> (-0.31%) ⬇️
eth/rpc/backend/call_tx.go 58.88% <100.00%> (+0.73%) ⬆️
eth/rpc/backend/node_info.go 71.05% <100.00%> (+2.75%) ⬆️
x/evm/genesis.go 100.00% <100.00%> (ø)
x/evm/keeper/keeper.go 81.81% <100.00%> (+3.43%) ⬆️
x/evm/keeper/statedb.go 54.73% <100.00%> (-0.48%) ⬇️
x/evm/params.go 36.84% <100.00%> (-7.61%) ⬇️
... and 1 more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[evm] Refactor out dead code from the evm.Params
1 participant