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

fix(gas-fees): use effective gas price in RefundGas #2076

Merged
merged 6 commits into from
Oct 13, 2024
Merged

Conversation

Unique-Divine
Copy link
Member

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

Purpose / Abstract

Use effective gas price in RefundGas and make sure that units are properly
reflected on all occurrences of "base fee" in the codebase.

Important Refactor on "baseFee"

All instances of variables referring to the EVM base fee now have units attached. Objects from geth expect everything in wei, whereas payments and gas management in the Ante Handler must be in micronibi ("unibi"). This change makes it much easier to notice discrepencies.

Justfile Changes

I've added several commands to the justfile based on operations I personally use quite often. These are:

  • log-localnet: This runs a local network while simultaneously writing all of its output to a file. This makes it easy to read into specifics as transactions are broadcasted or place useful print statements in the Go code as a debugging tool.
  • log-e2e: Similar to log-localnet, except it runs test-e2e.
  • test-e2e: Runs the EVM E2E tests

Summary by CodeRabbit

  • New Features

    • Enhanced base fee handling in Ethereum transactions, now supporting both wei and micronibi units.
    • Added EffectiveGasFeeCapWei method for calculating the maximum of the base fee and gas fee cap across transaction types.
    • Introduced new recipes in the justfile for improved logging and testing workflows.
  • Bug Fixes

    • Improved error handling and logging for transaction processing and base fee retrieval.
    • Fixed gas price handling in RefundGas to ensure proper unit reflection.
  • Documentation

    • Updated comments and documentation for clarity on base fee calculations and transaction handling, including references to Wallet Connect V2.
  • Refactor

    • Simplified transaction processing by directly using MsgEthereumTx in various methods.
    • Renamed variables and method signatures to enhance clarity regarding base fee representation.
    • Updated method names for effective gas pricing and costs to improve semantic clarity.
    • Enhanced the test suite to align with method renaming and improve clarity in legacy transaction tests.

@Unique-Divine Unique-Divine requested a review from a team as a code owner October 13, 2024 03:41
Copy link
Contributor

coderabbitai bot commented Oct 13, 2024

Walkthrough

This pull request introduces several modifications across multiple files, primarily focusing on enhancing the handling of base fees in Ethereum transactions and refining the structure of the .gitignore and CHANGELOG.md files. Key changes include the addition of log file entries in .gitignore, updates to method names and signatures related to gas pricing, and improvements in transaction validation and logging. The changes also involve the introduction of new test cases and enhancements to existing tests, ensuring comprehensive coverage of the updated functionality.

Changes

File Path Change Summary
.gitignore Added logs entry in "Main Gitignore" and "Node" sections.
CHANGELOG.md Comprehensive updates detailing changes across various versions, including features, improvements, and bug fixes.
app/evmante/evmante_can_transfer.go Updated base fee handling; replaced baseFee with baseFeeWeiPerGas and added conversion to Wei.
app/ante/gas.go Added no-op methods ConsumeGas and updated RefundGas to reflect no-op behavior.
x/evm/keeper/gas_fees.go Updated RefundGas method signature and logic to improve safety and clarity in gas refund calculations.
x/evm/keeper/keeper.go Renamed GetBaseFee to BaseFeeMicronibiPerGas and added BaseFeeWeiPerGas method.
x/evm/keeper/msg_server.go Updated transaction handling methods to reflect new parameter names and improve clarity.
x/evm/tx_data.go Renamed methods for clarity: EffectiveGasPriceWei to EffectiveGasPriceWeiPerGas, and EffectiveCost to EffectiveCostWei.
x/evm/tx_data_access_list.go Renamed methods for clarity, updating internal calls accordingly.
x/evm/tx_data_dynamic_fee.go Enhanced validation logic and renamed methods for clarity.
x/evm/tx_data_legacy.go Renamed methods for consistency and clarity.
x/evm/tx_data_dynamic_fee_test.go Updated test cases to reflect method renaming and ensure correct behavior.
x/evm/tx_data_legacy_test.go Adjusted test cases to align with the renamed methods.
e2e/evm/test/contract_send_nibi.test.ts Enhanced balance checks and debug output in transaction tests.
e2e/evm/test/native_transfer.test.ts Updated gas usage calculation to be dynamic based on transaction's gas limit.
justfile Added new recipes for logging and modified existing recipes to enhance functionality.

Assessment against linked issues

Objective Addressed Explanation
Ensure RefundGas does not allow negative gas refunds (#2059)
Update gethcore.Message to use effective gas price (#2059) The changes do not include updates to gethcore.Message.
Ensure RefundGas with 0 is a no-op (#2059) It is unclear if this specific case is handled.

🐰 "In the code where the rabbits hop,
New logs and fees, we’ll never stop!
With tests so bright, and changes galore,
Our EVM dances, forever to soar!
From base fees to gas, we’ve made it clear,
Hopping through code, we spread the cheer!" 🐇✨


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: 16

🧹 Outside diff range and nitpick comments (32)
e2e/evm/test/native_transfer.test.ts (2)

29-29: Approve the use of dynamic gas limit, but consider using actual gas used.

The change from a hardcoded gas value to transaction.gasLimit is an improvement in flexibility. However, for even more accuracy, consider using the actual gas used from the transaction receipt.

You could refactor this to use the actual gas used like this:

const gasUsed = (await txResponse.wait()).gasUsed

This will give you the precise amount of gas used in the transaction, which could be less than or equal to the gas limit.


38-38: Approve the addition of txResponse to debug log, but consider logging specific properties.

Including txResponse in the debug log is beneficial for providing more context. However, logging the entire object might produce excessive output.

Consider logging only specific properties of txResponse that are most relevant for debugging. For example:

txResponse: {
  hash: txResponse.hash,
  blockNumber: txResponse.blockNumber,
  gasUsed: txResponse.gasUsed,
  // Add other relevant properties as needed
},

This approach will provide the most important information while keeping the log output concise.

e2e/evm/test/contract_send_nibi.test.ts (4)

36-38: LGTM: Improved balance verification

The addition of balance checks for both the owner and recipient after the transaction enhances the test's thoroughness. This ensures that the transaction has the expected effect on both accounts.

Consider adding comments to explain the purpose of these balance checks for improved code documentation.


43-49: LGTM: Enhanced debug output

The addition of more fields to the debug output, including balances and transaction details, significantly improves the ability to troubleshoot and understand the test results. The inclusion of 'to' and 'from' fields from the receipt is particularly useful for transaction tracing.

Consider using a logging library or a custom debug function to manage these debug outputs more efficiently, especially if similar debug patterns are used across multiple test files.


51-53: LGTM: Improved balance assertion logic

The replacement of direct balance assertions with a more flexible comparison is a good improvement. This change allows for small discrepancies in gas costs, which can occur due to network conditions or minor implementation details. Using parseEther("0.001") as a threshold is a reasonable approach for handling potential rounding errors or minor gas cost variations.

Consider extracting the threshold value ("0.001") to a named constant at the top of the file for easier maintenance and adjustment if needed.


Line range hint 1-89: Overall assessment: Improved test accuracy and debuggability

The changes made to this test file enhance its effectiveness and maintainability. The additions of more precise balance checks, improved debug output, and flexible balance assertions contribute to a more robust testing approach. The modifications align well with the PR objective of addressing gas fee-related issues.

Consider implementing a helper function for balance comparisons and debug logging to promote code reuse across similar tests in the suite.

justfile (1)

76-76: Approve changes to the tidy recipe with a minor suggestion.

The modifications to the tidy recipe are good. Including proto generation and linting in the tidying process ensures a more comprehensive code cleanup.

Consider adding error handling to ensure the recipe stops if any step fails:

 tidy: 
   #!/usr/bin/env bash
-  go mod tidy
-  just proto-gen
-  just lint
-  just fmt
+  set -e
+  go mod tidy
+  just proto-gen
+  just lint
+  just fmt
+  echo "Tidy process completed successfully."

This change will cause the script to exit immediately if any command fails, preventing subsequent steps from running with potentially invalid state.

app/evmante/evmante_validate_basic.go (2)

Line range hint 118-122: LGTM. Consider enhancing the error message.

The updated condition and error handling look good and are consistent with the previous change. The error message is clear but could be more informative.

Consider updating the error message to provide more context:

- "dynamic fee tx not supported",
+ "dynamic fee transactions are not supported when base fee is nil",

This change would provide more information about why the transaction is not supported.


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

The changes in this file focus on updating the base fee retrieval method and improving error handling for dynamic fee transactions. These modifications enhance the clarity and specificity of the gas fee handling process.

Key points:

  1. The base fee is now retrieved using BaseFeeMicronibiPerGas instead of GetBaseFee, providing more precise unit information.
  2. Error handling for dynamic fee transactions has been updated to reflect the new base fee retrieval method.

Potential impact:

  • These changes may affect other parts of the codebase that rely on base fee calculations or dynamic fee transaction handling.
  • The more specific error message for unsupported dynamic fee transactions could improve debugging and user experience.

To ensure system-wide consistency and prevent potential issues:

  1. Review and update any other components that interact with base fee calculations.
  2. Consider adding unit tests to verify the behavior of the AnteHandle method with these new changes, especially for edge cases involving nil base fees and dynamic fee transactions.
  3. Update any relevant documentation to reflect these changes in gas fee handling.
x/evm/keeper/keeper.go (1)

Line range hint 1-141: Ensure consistency across the project

While the renaming of GetBaseFee to BaseFeeMicronibiPerGas is an improvement in this file, it's important to ensure this change is consistent with the project's overall design and documentation.

Consider the following:

  1. Update any project documentation that might reference the old function name.
  2. Review other similar functions in the project to see if they would benefit from similar renaming for consistency.
  3. If there are any interfaces or abstract classes that this Keeper implements, ensure they are updated to reflect this change.

These steps will help maintain consistency across the entire project and prevent potential confusion for other developers.

x/evm/const.go (1)

15-18: LGTM! Consider adding a brief comment for BASE_FEE_WEI.

The introduction of BASE_FEE_WEI and the grouping of related constants improve code organization and potentially performance. The use of NativeToWei ensures consistency in the conversion.

Consider adding a brief comment for BASE_FEE_WEI similar to the one for BASE_FEE_MICRONIBI:

var (
    // BASE_FEE_MICRONIBI is the global base fee value for the network in micronibi.
    BASE_FEE_MICRONIBI = big.NewInt(1)
    // BASE_FEE_WEI is the equivalent of BASE_FEE_MICRONIBI in wei units.
    BASE_FEE_WEI       = NativeToWei(BASE_FEE_MICRONIBI)
)
x/evm/tx_data_legacy.go (1)

193-196: Approve renaming and suggest comment improvement

The renaming of EffectiveGasPriceWei to EffectiveGasPriceWeiPerGas improves clarity by explicitly stating that the price is per gas unit. The implementation remains correct and consistent with its purpose.

Consider updating the comment to be more descriptive:

-// EffectiveGasPriceWeiPerGas is the same as GasPrice for LegacyTx
+// EffectiveGasPriceWeiPerGas returns the maximum of the transaction's gas price and the provided base fee, both in wei per gas unit
x/evm/tx_data_access_list.go (3)

295-296: LGTM: Consistent update with method renaming

The EffectiveFeeWei method has been correctly updated to use the renamed EffectiveGasPriceWeiPerGas method. The implementation remains logically correct.

Consider adding a comment explaining the calculation for improved readability:

// Calculate the effective fee by multiplying the effective gas price per gas with the gas limit
return priceTimesGas(tx.EffectiveGasPriceWeiPerGas(baseFeeWei), tx.GetGas())

300-303: LGTM: Consistent update with previous changes

The EffectiveCost method has been correctly updated to use the modified EffectiveFeeWei method. The implementation remains logically correct.

For consistency with the EffectiveFeeWei method, consider using a single-line return statement:

return cost(tx.EffectiveFeeWei(baseFeeWei), tx.GetValueWei())

This change would make the code more concise and consistent with the style used in other methods.


289-303: Summary: Effective implementation of gas price changes

The changes in this file successfully implement the use of effective gas price in the AccessListTx structure. The renaming of EffectiveGasPriceWei to EffectiveGasPriceWeiPerGas and the subsequent updates to EffectiveFeeWei and EffectiveCost methods improve code clarity and consistency.

These modifications align well with the PR objective of addressing gas fee issues and using the effective gas price in RefundGas. The implementation ensures that the maximum value between the transaction's gas price and the base fee is used, which is crucial for accurate gas fee calculations.

Consider adding unit tests to verify the behavior of these updated methods, especially with different combinations of transaction gas prices and base fees. This will ensure the correctness of the gas fee calculations across various scenarios.

x/evm/tx_data_dynamic_fee.go (1)

Line range hint 309-311: LGTM! Consider a minor naming improvement for consistency.

The EffectiveCost method correctly uses the updated EffectiveFeeWei method, which in turn uses the renamed EffectiveGasPriceWeiPerGas. The logic remains correct, calculating the effective cost by adding the transaction value to the effective fee.

For consistency with the other renamed methods, consider updating this method name to EffectiveCostWei. This would make it clear that the returned value is in Wei, matching the naming convention of the other methods.

x/evm/tx_data_dynamic_fee_test.go (3)

Line range hint 376-379: Consider adding explanatory comments for magic numbers

In the test cases for TestDynamicFeeTxEffectiveGasPrice, some magic numbers are used (e.g., "5" + strings.Repeat("0", 12)). Consider adding comments to explain the significance of these values or using named constants for better readability.


Line range hint 70-73: TODO comment needs to be addressed

The TestDynamicFeeTxCopy function contains a TODO comment about testing for different pointers. This should be implemented to ensure the Copy method works correctly.

Would you like assistance in implementing this test case?


Line range hint 605-615: Consider adding a specific test for the new method name

While the functionality is tested, there's no explicit test case that verifies the new method name EffectiveGasPriceWeiPerGas. Consider adding a test case that specifically checks if this method exists and functions correctly.

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

256-256: LGTM! Consider enhancing error logging for base fee retrieval.

The change from EffectiveGasPriceWei to EffectiveGasPriceWeiPerGas is appropriate and aligns with the PR objective of using the effective gas price in RefundGas. This modification provides a more precise calculation of the effective gas price for dynamic fee transactions.

Consider enhancing the error logging when fetching the base fee fails. Instead of just logging the error, it might be helpful to include the transaction hash in the log message for easier debugging. For example:

b.logger.Error("fetch basefee failed, node is pruned?", "txHash", hash.Hex(), "height", res.Height, "error", err)

This addition would make it easier to correlate the error with the specific transaction causing the issue.

x/evm/msg.go (3)

270-278: LGTM! Consider improving error handling.

The renaming of GetEffectiveFee to EffectiveFeeWei enhances clarity by explicitly stating the unit of the returned value. The implementation looks correct.

Consider handling the error returned by UnpackTxData instead of silently returning nil. For example:

 func (msg MsgEthereumTx) EffectiveFeeWei(baseFee *big.Int) *big.Int {
 	txData, err := UnpackTxData(msg.Data)
 	if err != nil {
-		return nil
+		// Log the error or handle it appropriately
+		return big.NewInt(0)
 	}
 	return txData.EffectiveFeeWei(baseFee)
 }

279-287: LGTM! Consider improving error handling.

The renaming of GetEffectiveGasPrice to EffectiveGasPriceWeiPerGas enhances clarity by explicitly stating the unit of the returned value. The implementation looks correct, and the updated comment provides clear information about the method's purpose and return value unit.

Similar to the previous method, consider handling the error returned by UnpackTxData instead of silently returning nil. For example:

 func (msg MsgEthereumTx) EffectiveGasPriceWeiPerGas(baseFeeWei *big.Int) *big.Int {
 	txData, err := UnpackTxData(msg.Data)
 	if err != nil {
-		return nil
+		// Log the error or handle it appropriately
+		return big.NewInt(0)
 	}
 	return txData.EffectiveGasPriceWeiPerGas(baseFeeWei)
 }

270-287: Summary: Improved clarity in gas fee-related methods

The changes in this file focus on renaming two methods related to gas fees and updating their comments:

  1. GetEffectiveFeeEffectiveFeeWei
  2. GetEffectiveGasPriceEffectiveGasPriceWeiPerGas

These changes improve clarity by explicitly stating the units of the returned values (Wei and Wei per unit gas, respectively). The implementations remain correct, and the updates align with the PR objectives of addressing gas fee-related issues.

Consider applying similar naming conventions to other gas and fee-related methods throughout the codebase for consistency. This will enhance overall code readability and maintainability.

CHANGELOG.md (3)

Line range hint 323-376: Non-breaking improvements and refactoring

This section includes a wide range of improvements, including SDK migration, test coverage enhancements, and dependency updates.

The non-breaking nature of these changes is positive, allowing for easier adoption. The focus on improving test coverage and refactoring is commendable.

Consider grouping similar changes together (e.g., all test improvements, all dependency updates) to make the changelog more readable. Also, for significant refactoring like the one in the oracle module, it might be helpful to briefly mention the benefits or motivations behind the change.


Line range hint 418-1110: Historical release information

The changelog provides detailed information about previous releases, including features, bug fixes, and breaking changes.

The level of detail for each release is commendable, allowing users to understand the evolution of the project over time.

To improve readability and help users quickly find information about specific releases:

  1. Consider adding a table of contents at the beginning of the changelog.
  2. For releases with breaking changes, consider adding a "Breaking Changes" subsection to make these more visible.
  3. Use consistent formatting across all releases (some older releases use different header levels or formatting styles).

Line range hint 1-1110: Overall CHANGELOG.md review

The changelog provides a comprehensive history of changes across multiple releases, including features, bug fixes, breaking changes, and dependency updates.

The level of detail and regular updates to the changelog are commendable, providing users and developers with valuable information about the project's evolution.

To further improve the changelog:

  1. Consider adding a brief summary at the top of each release, highlighting the most significant changes.
  2. Standardize the formatting and structure across all releases for consistency.
  3. For breaking changes, consider adding migration guides or links to relevant documentation.
  4. Group similar types of changes together within each release (e.g., all API changes, all new features, all bug fixes).
  5. For major releases or those with significant changes, consider adding a "Highlights" section to draw attention to key improvements or changes.
x/evm/keeper/gas_fees.go (1)

Line range hint 91-99: Remove commented-out code to maintain code clarity

The block of commented-out code related to gas fee cap comparisons is no longer active. Keeping such code can cause confusion and clutter. If this logic is not needed anymore, consider removing it to improve code readability.

Apply this diff to remove the unnecessary code:

-    // gasFeeCapMicronibi := evm.WeiToNative(txData.GetGasFeeCapWei())
-    // if baseFeeMicronibi != nil && gasFeeCapMicronibi.Cmp(baseFeeMicronibi) < 0 {
-    // 	baseFeeWei := evm.NativeToWei(baseFeeMicronibi)
-    // 	return nil, errors.Wrapf(errortypes.ErrInsufficientFee,
-    // 		"the tx gasfeecap is lower than the tx baseFee: %s (gasfeecap), %s (basefee) wei per gas",
-    // 		txData.GetGasFeeCapWei(),
-    // 		baseFeeWei,
-    // 	)
-    // }
x/evm/msg_test.go (1)

701-701: Nitpick: Rename variable for improved readability

The variable effFee is abbreviated and may not be immediately clear to readers. Consider renaming it to effectiveFee for better readability and to adhere to Go naming conventions.

Apply this diff to rename the variable:

-effFee = tx.EffectiveFeeWei(big.NewInt(0))
+effectiveFee = tx.EffectiveFeeWei(big.NewInt(0))
x/evm/keeper/grpc_query.go (1)

150-150: Reminder: Address the TODO comment

There's a TODO comment indicating that both units should be displayed. Please ensure this enhancement is implemented.

Would you like assistance in implementing this feature or opening a GitHub issue to track this task?

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

129-129: Clarify the comment for 'weiPerGas' field

The comment // Comes from EffectiveGasPriceWeiPerGas might be unclear. Consider rephrasing it to provide more clarity on the source or calculation of weiPerGas.


195-195: Rephrase test case name for clarity and professionalism

Consider changing the test case name from "fee collector is broke" to "fee collector has insufficient funds" to enhance clarity and maintain a professional tone.


137-145: Consider renaming 'bal' to 'balance' for increased readability

In the fundFeeCollectorEvmBal function, renaming the parameter bal to balance can enhance code readability and make the purpose of the variable clearer.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 62d4b1a and 15c6c78.

📒 Files selected for processing (29)
  • .gitignore (1 hunks)
  • CHANGELOG.md (1 hunks)
  • app/evmante/evmante_can_transfer.go (2 hunks)
  • app/evmante/evmante_gas_consume.go (1 hunks)
  • app/evmante/evmante_mempool_fees.go (2 hunks)
  • app/evmante/evmante_validate_basic.go (2 hunks)
  • app/evmante/interfaces.go (1 hunks)
  • e2e/evm/test/contract_send_nibi.test.ts (2 hunks)
  • e2e/evm/test/native_transfer.test.ts (2 hunks)
  • eth/rpc/backend/backend.go (1 hunks)
  • eth/rpc/backend/tx_info.go (1 hunks)
  • justfile (1 hunks)
  • x/evm/const.go (1 hunks)
  • x/evm/keeper/gas_fees.go (1 hunks)
  • x/evm/keeper/gas_fees_test.go (2 hunks)
  • x/evm/keeper/grpc_query.go (4 hunks)
  • x/evm/keeper/keeper.go (1 hunks)
  • x/evm/keeper/msg_server.go (2 hunks)
  • x/evm/keeper/vm_config.go (1 hunks)
  • x/evm/msg.go (1 hunks)
  • x/evm/msg_test.go (1 hunks)
  • x/evm/statedb/config.go (1 hunks)
  • x/evm/tx.go (1 hunks)
  • x/evm/tx_data.go (1 hunks)
  • x/evm/tx_data_access_list.go (1 hunks)
  • x/evm/tx_data_dynamic_fee.go (2 hunks)
  • x/evm/tx_data_dynamic_fee_test.go (1 hunks)
  • x/evm/tx_data_legacy.go (1 hunks)
  • x/evm/tx_data_legacy_test.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • x/evm/statedb/config.go
🧰 Additional context used
🔇 Additional comments (32)
app/evmante/interfaces.go (2)

30-30: Ensure consistent updates across the codebase.

The change from GetBaseFee to BaseFeeMicronibiPerGas in the EVMKeeper interface may have wider implications:

  1. Update all implementations of this interface.
  2. Modify any mock objects used in tests.
  3. Update relevant documentation and comments.

To help identify affected areas, run the following script:

#!/bin/bash
# Description: Identify potential areas affected by the interface change

# Test: Find files implementing EVMKeeper interface
echo "Files potentially implementing EVMKeeper interface:"
rg --type go 'type\s+\w+\s+struct' -l | xargs rg --type go 'EVMKeeper'

# Test: Find test files that might use mock objects
echo -e "\nTest files potentially using EVMKeeper mocks:"
fd -e go -e _test.go | xargs rg 'mock.+EVMKeeper'

# Test: Find documentation files mentioning GetBaseFee
echo -e "\nDocumentation files mentioning GetBaseFee:"
rg --type md 'GetBaseFee'

Please review the output and make necessary updates to maintain consistency across the codebase.


30-30: Approve the method signature change and verify its usage.

The change from GetBaseFee to BaseFeeMicronibiPerGas is a good improvement as it more accurately describes the method's purpose. This aligns well with the PR objective to use effective gas price in RefundGas.

To ensure this change doesn't break existing functionality, please verify all usages of this method across the codebase. Run the following script to check for any remaining references to the old method name:

This will help identify any places where the method call needs to be updated.

e2e/evm/test/native_transfer.test.ts (1)

41-41: Approve the reordering of balance assertions.

Moving the sender's balance assertion after the debug log is a good change. It improves the logical flow of the test and ensures that all relevant information is logged before the assertion is made. This can make debugging easier if the assertion fails.

x/evm/tx.go (1)

42-42: Approve the change to EffectiveGasPriceWeiPerGas, but verify its impact.

The update from EffectiveGasPriceWei to EffectiveGasPriceWeiPerGas is a good improvement, as it likely provides a more accurate representation of the gas price per unit of gas. This aligns better with Ethereum's gas pricing model and should lead to more precise transaction prioritization.

To ensure this change doesn't have unintended consequences, please verify:

  1. The impact on transaction prioritization in the mempool.
  2. Consistency with other parts of the codebase that may rely on gas price calculations.

Run the following script to check for other occurrences of EffectiveGasPriceWei that might need updating:

If any occurrences are found, please update them accordingly or justify why they should remain unchanged.

✅ Verification successful

Change to EffectiveGasPriceWeiPerGas verified successfully.

All occurrences of EffectiveGasPriceWei have been updated to EffectiveGasPriceWeiPerGas, ensuring consistency across the codebase. This change aligns with Ethereum's gas pricing model and enhances the accuracy of transaction prioritization.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for other occurrences of EffectiveGasPriceWei

# Test: Look for any remaining instances of EffectiveGasPriceWei
rg --type go 'EffectiveGasPriceWei\('

# Test: Check for any tests that might need updating
rg --type go 'TestEffectiveGasPriceWei'

Length of output: 79


Script:

#!/bin/bash
# Description: Use ast-grep to search for all invocations of EffectiveGasPriceWei and related functions

# Search for function calls to EffectiveGasPriceWei
ast-grep --lang go --pattern='txData.EffectiveGasPriceWei($_)' src/...

# Search for function calls to EffectiveGasPriceWeiPerGas to ensure consistency
ast-grep --lang go --pattern='txData.EffectiveGasPriceWeiPerGas($_)' src/...

# Additionally, search for any references to EffectiveGasPrice to capture partial matches
rg --type go 'EffectiveGasPrice' -A 2

Length of output: 5116

e2e/evm/test/contract_send_nibi.test.ts (1)

13-13: LGTM: Import addition enhances precision

The addition of parseEther import is appropriate and consistent with its usage later in the file. This change enhances the precision of balance comparisons, which is a good practice when dealing with cryptocurrency amounts.

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

Line range hint 30-35: Approve the base fee retrieval method change.

The change from k.GetBaseFee(ctx) to k.BaseFeeMicronibiPerGas(ctx) aligns with the PR objective to use the effective gas price. This new method name is more specific and clearly indicates that it returns the base fee in micronibi per gas unit.

To ensure consistency across the codebase, please run the following script to check for any remaining usages of GetBaseFee:

This will help identify if there are any other occurrences of GetBaseFee that might need to be updated for consistency.

✅ Verification successful

Base fee retrieval method change verified successfully.

All instances of GetBaseFee have been removed, and BaseFeeMicronibiPerGas is consistently used across the codebase as intended.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining usages of GetBaseFee method

# Test: Search for GetBaseFee method calls
echo "Searching for GetBaseFee method calls:"
rg --type go 'GetBaseFee\s*\(' -g '!vendor/'

# Test: Search for BaseFeeMicronibiPerGas method calls
echo "Searching for BaseFeeMicronibiPerGas method calls:"
rg --type go 'BaseFeeMicronibiPerGas\s*\(' -g '!vendor/'

Length of output: 1258

app/evmante/evmante_validate_basic.go (1)

92-92: LGTM. Verify impact on dependent code.

The change from GetBaseFee to BaseFeeMicronibiPerGas looks good and provides more clarity on the unit of measurement. However, it's important to ensure that this change is consistent throughout the codebase.

Please run the following script to check for any other occurrences of GetBaseFee that might need updating:

✅ Verification successful

Verified: No other occurrences of GetBaseFee found.

The search did not reveal any additional instances of GetBaseFee in the codebase. Therefore, the replacement to BaseFeeMicronibiPerGas is consistent and should not affect other parts of the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other occurrences of GetBaseFee in the codebase

# Search for GetBaseFee method calls
echo "Searching for GetBaseFee method calls:"
rg --type go 'GetBaseFee\('

# Search for GetBaseFee method definitions
echo "Searching for GetBaseFee method definitions:"
ast-grep --lang go --pattern 'func $_ GetBaseFee($_) $_'

Length of output: 274

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

117-119: Approve renaming and suggest verification of usage

The renaming of GetBaseFee to BaseFeeMicronibiPerGas is a good improvement. It makes the function's purpose clearer and follows a more descriptive naming convention. The updated comment accurately reflects the function's behavior.

To ensure all references to this function have been updated, please run the following verification script:

This script will help identify any places where the old function name might still be in use and confirm the correct usage of the new name throughout the codebase.

✅ Verification successful

Function Renaming Verified Successfully

All instances of GetBaseFee have been successfully renamed to BaseFeeMicronibiPerGas. No remaining references to GetBaseFee were found, and the new function is correctly used throughout the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all references to the renamed function have been updated

# Test 1: Search for any remaining instances of GetBaseFee
echo "Searching for remaining instances of GetBaseFee:"
rg --type go 'GetBaseFee'

# Test 2: Verify correct usage of BaseFeeMicronibiPerGas
echo "Verifying correct usage of BaseFeeMicronibiPerGas:"
rg --type go 'BaseFeeMicronibiPerGas'

Length of output: 1342

x/evm/tx_data_legacy.go (2)

200-200: Approve method call update

The update to use EffectiveGasPriceWeiPerGas is consistent with the method renaming and maintains the correct functionality of calculating the effective fee.


Line range hint 1-207: Summary of changes in x/evm/tx_data_legacy.go

The changes in this file primarily focus on improving clarity and consistency in gas price calculations for legacy transactions. The renaming of EffectiveGasPriceWei to EffectiveGasPriceWeiPerGas and the subsequent updates in related methods enhance the code's readability and maintainability. These modifications align well with the PR's objective of improving gas fee handling.

app/evmante/evmante_gas_consume.go (1)

71-71: LGTM! Verify consistent usage across the codebase.

The change from GetBaseFee to BaseFeeMicronibiPerGas improves clarity and specificity. It aligns well with the variable name and its purpose.

To ensure consistency, please run the following script to check for any remaining usages of GetBaseFee that might need updating:

This script will help identify any inconsistencies in method usage and verify the update in the EVMKeeper interface.

✅ Verification successful

Renaming Verified Successfully

The method EffectiveGasPriceWei has been successfully renamed to EffectiveGasPriceWeiPerGas. All implementations of the TxData interface have been updated accordingly, and no occurrences of the old method name remain in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for occurrences of the old method name 'EffectiveGasPriceWei'

# Search for the old method name
echo "Searching for occurrences of 'EffectiveGasPriceWei':"
rg --type go 'EffectiveGasPriceWei'

# Search for implementations of the TxData interface
echo "Searching for implementations of the TxData interface:"
ast-grep --lang go --pattern 'type $_ struct {
  $$$
}

func ($_ $_) EffectiveGasPriceWei($$$) $$$'

Length of output: 2862


Script:

#!/bin/bash
# Description: Verify renaming of 'EffectiveGasPriceWei' to 'EffectiveGasPriceWeiPerGas'

# Check for any remaining occurrences of the old method name 'EffectiveGasPriceWei'
echo "Checking for any remaining occurrences of 'EffectiveGasPriceWei':"
rg --type go 'EffectiveGasPriceWei'

# Verify that all implementations of the TxData interface use 'EffectiveGasPriceWeiPerGas'
echo "Verifying implementations of the TxData interface use 'EffectiveGasPriceWeiPerGas':"
ast-grep --lang go --pattern 'func ($_ $_) EffectiveGasPriceWeiPerGas($_, $_) *big.Int { $$$ }' .

Length of output: 2459


Script:

#!/bin/bash
# Description: Verify all implementations of 'TxData' interface use 'EffectiveGasPriceWeiPerGas'

# Check for any remaining occurrences of the old method name 'EffectiveGasPriceWei'
echo "Checking for any remaining occurrences of 'EffectiveGasPriceWei':"
rg --type go 'EffectiveGasPriceWei'

# Verify that all implementations of the TxData interface use 'EffectiveGasPriceWeiPerGas'
echo "Verifying that all implementations of the TxData interface use 'EffectiveGasPriceWeiPerGas':"
ast-grep --lang go --pattern 'func ($_ $_) EffectiveGasPriceWeiPerGas($_ *big.Int) *big.Int' .

Length of output: 3838

x/evm/tx_data_legacy_test.go (1)

375-375: LGTM! Verify corresponding implementation change.

The renaming of EffectiveGasPriceWei to EffectiveGasPriceWeiPerGas improves clarity by specifying that the price is "per gas". This change aligns with best practices for method naming.

To ensure consistency, please verify that the corresponding method in the LegacyTx implementation has also been updated. You can run the following command to check:

Ensure that the old method name is not found and the new method name is present in the implementation files.

✅ Verification successful

Verified the method renaming in implementation files.

The method EffectiveGasPriceWei has been successfully renamed to EffectiveGasPriceWeiPerGas in all relevant implementation files, ensuring consistency across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the method name change in the LegacyTx implementation

# Search for the old and new method names in the implementation files
echo "Searching for old method name:"
rg --type go "func \(.*\) EffectiveGasPriceWei\(" x/evm

echo "Searching for new method name:"
rg --type go "func \(.*\) EffectiveGasPriceWeiPerGas\(" x/evm

Length of output: 685

x/evm/tx_data_access_list.go (1)

289-291: LGTM: Improved method naming for clarity

The renaming of EffectiveGasPriceWei to EffectiveGasPriceWeiPerGas enhances clarity by explicitly stating that the returned value is "per gas". This change aligns well with the PR objective of using the effective gas price in RefundGas.

x/evm/tx_data_dynamic_fee.go (3)

Line range hint 294-301: LGTM! Improved method naming and implementation.

The renaming of EffectiveGasPriceWei to EffectiveGasPriceWeiPerGas enhances clarity by explicitly stating the unit of measurement. The implementation correctly follows EIP-1559 rules for calculating the effective gas price. The use of BigIntMax ensures that the effective gas price is never lower than the base fee, which is a crucial aspect of EIP-1559.


306-306: LGTM! Consistent method call update.

The EffectiveFeeWei method has been correctly updated to use the renamed EffectiveGasPriceWeiPerGas method. This change maintains consistency with the earlier renaming while preserving the original logic of calculating the effective fee.


Line range hint 1-311: Overall, the changes improve clarity and consistency in gas price calculations.

The main change, renaming EffectiveGasPriceWei to EffectiveGasPriceWeiPerGas, enhances clarity by explicitly stating the unit of measurement. This change propagates through the file, maintaining consistency. The implementation correctly follows EIP-1559 rules for calculating the effective gas price, which aligns with the PR objectives of improving gas fee handling.

Consider the minor suggestion to rename EffectiveCost to EffectiveCostWei for complete consistency across the file.

x/evm/tx_data_dynamic_fee_test.go (1)

613-613: Method name updated for clarity

The method name has been changed from EffectiveGasPriceWei to EffectiveGasPriceWeiPerGas. This change improves clarity by explicitly stating that the price is in Wei per Gas unit.

To ensure consistency, let's check if this method name has been updated throughout the codebase:

✅ Verification successful

Method name update verified

All instances of the old method name EffectiveGasPriceWei have been successfully updated to EffectiveGasPriceWeiPerGas throughout the codebase. No remaining instances of the old method name were found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining instances of the old method name
rg "EffectiveGasPriceWei\(" --type go

# Search for the new method name to confirm its usage
rg "EffectiveGasPriceWeiPerGas\(" --type go

Length of output: 1513

CHANGELOG.md (3)

Line range hint 378-416: Numerous dependency updates

The changelog lists multiple dependency updates, including security-related packages and core components like gRPC and Cosmos SDK.

Keeping dependencies up-to-date is crucial for security and performance. The update to golang.org/x/crypto is particularly important for addressing potential vulnerabilities.

To ensure these updates don't introduce any unexpected issues, please run the following verification script:

#!/bin/bash
# Verify dependency updates
echo "Running all tests to check for any regressions..."
go test ./... -v

echo "Checking for any deprecated function usage..."
go vet ./...

echo "Verifying build with updated dependencies..."
go build -v ./...

Consider adding a brief note for major version bumps (e.g., bufbuild/buf-setup-action from 1.21.0 to 1.26.1) explaining any significant changes or reasons for the update.


Line range hint 298-321: Changes to Dapp modules

Several changes have been made to the perp, spot, and oracle modules, including new features and fixes.

These changes appear to enhance the functionality and reliability of the Dapp modules. The addition of volume-based rebates and the ability to close markets are particularly noteworthy.

To ensure these changes don't introduce any regressions, please run the following verification script:


Line range hint 135-296: Extensive changes in the EVM module

The changelog lists numerous updates and additions to the EVM module, including new features, refactoring, and bug fixes. This indicates a significant evolution of the EVM functionality within Nibiru.

The extensive work on the EVM module shows a commitment to improving and expanding Ethereum compatibility. However, given the breadth of changes, it's crucial to ensure comprehensive testing has been performed, especially for edge cases and potential interactions between new features.

To confirm the stability and correctness of these changes, please run the following verification script:

app/evmante/evmante_mempool_fees.go (2)

64-64: Confirm correct computation of effective gas price

In the line:

effectiveGasPriceDec := math.LegacyNewDecFromBigInt(
    evm.WeiToNative(ethTx.EffectiveGasPriceWeiPerGas(baseFeeWei)),
)

Ensure that the method EffectiveGasPriceWeiPerGas correctly calculates the effective gas price using baseFeeWei. Also, verify that converting from Wei to micronibi using WeiToNative maintains accuracy. This is crucial for proper fee validation against minGasPrice.

To validate the correct usage and calculation of the effective gas price, run:

#!/bin/bash
# Description: Verify the calculation of effective gas price with base fee adjustments.

# Locate all uses of EffectiveGasPriceWeiPerGas in the codebase.
rg --type go 'EffectiveGasPriceWeiPerGas'

# Confirm the correct definition and usage of baseFeeWei.
rg --type go 'baseFeeWei'

# Check for consistent conversions from Wei to micronibi.
rg --type go 'WeiToNative'

42-43: Ensure consistent unit conversions between micronibi and Wei

In these lines:

baseFeeMicronibi := d.evmKeeper.BaseFeeMicronibiPerGas(ctx)
baseFeeDec := math.LegacyNewDecFromBigInt(baseFeeMicronibi)

You're retrieving the base fee in micronibi per gas and converting it to a decimal using LegacyNewDecFromBigInt. Please verify that the units remain consistent throughout your calculations, especially when comparing the base fee to the minGasPrice, which is also in micronibi. Inconsistent units could lead to incorrect fee validations.

To ensure unit consistency across the codebase, you can run the following script:

✅ Verification successful

Unit conversions between micronibi and Wei are consistently handled. No issues found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent unit conversions between micronibi and Wei.

# Find all instances of unit conversions using NativeToWei and WeiToNative.
rg --type go 'NativeToWei|WeiToNative'

# Check all usages of base fee variables.
rg --type go 'baseFeeMicronibi|baseFeeWei|baseFeeDec'

Length of output: 6823


Script:

#!/bin/bash
# Description: Verify consistent usage of minGasPrice in micronibi.

# Find all declarations and assignments of minGasPrice.
rg --type go 'minGasPrice'

# Find all comparisons involving minGasPrice.
rg --type go 'minGasPrice[\s]*[<>=]'

Length of output: 1546

x/evm/msg_test.go (1)

701-701: Verify the use of zero base fee in EffectiveFeeWei

At line 701, the effective fee is calculated using a base fee of zero:

effFee = tx.EffectiveFeeWei(big.NewInt(0))

In the context of EIP-1559, the base fee is a critical component in fee calculations. Using a zero value may not accurately simulate realistic scenarios and could affect the validity of the test case.

To ensure the test reflects actual conditions, please verify whether passing a base fee of zero is appropriate. You can examine how EffectiveFeeWei is used elsewhere in the codebase to compare typical base fee values.

Run the following script to find all usages of EffectiveFeeWei and their arguments:

This will help determine if a non-zero base fee should be used in this test for accuracy.

✅ Verification successful

Using a zero base fee in EffectiveFeeWei is appropriate for testing edge cases.

Passing a base fee of zero allows the test to verify that the function correctly handles scenarios where no base fee is applied, ensuring robustness in such conditions.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all calls to EffectiveFeeWei and display the arguments passed.

# Search for EffectiveFeeWei function calls in Go files
rg --type go 'EffectiveFeeWei\((.*?)\)'

Length of output: 1505

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

39-39: Updated function call to ApplyEvmTx

The EthereumTx method now calls k.ApplyEvmTx(ctx, msg) with msg directly, simplifying the function call and enhancing code clarity.


47-47: Modified ApplyEvmTx function signature

ApplyEvmTx now accepts txMsg *evm.MsgEthereumTx instead of tx *gethcore.Transaction, streamlining transaction handling by working directly with the message.


49-49: Conversion of txMsg to transaction

The line tx := txMsg.AsTransaction() appropriately converts the txMsg into a *gethcore.Transaction for further processing.


115-116: Use of effective gas price in gas refund calculation

Calculating weiPerGas using txMsg.EffectiveGasPriceWeiPerGas(evmConfig.BaseFee) ensures that the gas refund utilizes the effective gas price, addressing the gas fee handling issue outlined in the PR objectives.

app/evmante/evmante_can_transfer.go (2)

58-60: Ensure nil check for baseFeeMicronibiPerGas is necessary.

At lines 58-60, a nil check is performed on baseFeeMicronibiPerGas. Confirm that this variable can be nil and that this check is required to prevent potential runtime errors.


50-50: Confirm the base fee units passed to AsMessage.

In line 50, baseFeeWeiPerGas is passed to msgEthTx.AsMessage. Ensure that this function expects the base fee in Wei for accurate transaction processing.

Run the following script to check the expected units of AsMessage:

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

208-209: ⚠️ Potential issue

Validate handling of negative 'weiPerGas' in 'RefundGas' method

The test case uses a negative weiPerGas, which is an invalid scenario. Verify that the RefundGas method correctly handles negative weiPerGas values and appropriately returns an error or prevents processing such inputs.

Run the following script to check for input validation of negative gas prices:

eth/rpc/backend/backend.go Outdated Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -129,6 +129,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- [#2056](https://github.com/NibiruChain/nibiru/pull/2056) - feat(evm): add oracle precompile
- [#2065](https://github.com/NibiruChain/nibiru/pull/2065) - refactor(evm)!: Refactor out dead code from the evm.Params
- [#2073](https://github.com/NibiruChain/nibiru/pull/2073) - fix(evm-keeper): better utilize ERC20 metadata during FunToken creation
- [#2xxx](https://github.com/NibiruChain/nibiru/pull/2xxx) - fix(evm-gas-fees): fix(gas-fees): use effective gas price in RefundGas
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

New gas fee refund functionality added

The addition of a fix for gas fee refunds using the effective gas price is a significant change that could impact transaction processing and user costs. This change should be thoroughly tested to ensure it behaves as expected across various transaction types and gas price scenarios.

Please provide more details on the implementation of this fix and its potential impact on users and validators. Consider adding a brief explanation of how the effective gas price is calculated and used in the refund process.

return errors.Wrapf(evm.ErrInvalidRefund, "refunded amount value cannot be negative %d", remaining.Int64())
// Should be impossible since leftoverGas is a uint64. Reaching this case
// would imply a critical error in the effective gas calculation.
return errors.Wrapf(evm.ErrInvalidRefund, "refunded amount value cannot be negative %d", leftoverMicronibi.Int64())
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

Use String() method for error message to avoid potential overflows

When logging leftoverMicronibi in the error message, using Int64() may lead to incorrect values if leftoverMicronibi exceeds the range of an int64. It's safer to use leftoverMicronibi.String() to represent the full big.Int value.

Apply this diff to fix the error message:

-return errors.Wrapf(evm.ErrInvalidRefund, "refunded amount value cannot be negative %d", leftoverMicronibi.Int64())
+return errors.Wrapf(evm.ErrInvalidRefund, "refunded amount value cannot be negative %s", leftoverMicronibi.String())
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return errors.Wrapf(evm.ErrInvalidRefund, "refunded amount value cannot be negative %d", leftoverMicronibi.Int64())
return errors.Wrapf(evm.ErrInvalidRefund, "refunded amount value cannot be negative %s", leftoverMicronibi.String())

x/evm/keeper/grpc_query.go Outdated Show resolved Hide resolved
app/evmante/evmante_can_transfer.go Outdated Show resolved Hide resolved
app/evmante/evmante_can_transfer.go Outdated Show resolved Hide resolved
x/evm/keeper/gas_fees_test.go Show resolved Hide resolved
x/evm/keeper/gas_fees_test.go Outdated Show resolved Hide resolved
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: 8

🧹 Outside diff range and nitpick comments (23)
app/evmante/interfaces.go (1)

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

The modifications to this file align with the PR objectives by simplifying the EVMKeeper interface. This change potentially allows for more direct use of the effective gas price through the standard Keeper methods. However, there are a few points to consider:

  1. The removal of specific methods from the EVMKeeper interface might impact other parts of the codebase that rely on these methods. It's crucial to ensure that all necessary functionality is still accessible through the evmkeeper.Keeper.

  2. The addition of the new import for the tx package suggests potential changes in how transactions are handled. We should verify that this import is actually used within this file or remove it if unused.

  3. The protoTxProvider interface remains unchanged, which is good for maintaining compatibility with existing code.

Consider documenting these interface changes in the project's API documentation or changelog to inform other developers about the simplified EVMKeeper interface and any potential migration steps needed.

app/evmante/evmante_emit_event.go (2)

Line range hint 27-62: Approve the unchanged AnteHandle method with a minor suggestion.

The AnteHandle method remains unchanged and is still valid with the new pointer type for evmKeeper. However, a minor optimization could be made:

Consider caching the result of eeed.evmKeeper.EVMState() at the beginning of the method to avoid multiple dereferences:

 func (eeed EthEmitEventDecorator) AnteHandle(
 	ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler,
 ) (newCtx sdk.Context, err error) {
+	evmState := eeed.evmKeeper.EVMState()
-	txIndex := eeed.evmKeeper.EVMState().BlockTxIndex.GetOr(ctx, 0)
+	txIndex := evmState.BlockTxIndex.GetOr(ctx, 0)

 	// ... rest of the method remains unchanged
 }

This change could potentially improve performance, especially if AnteHandle is called frequently.


Line range hint 1-62: Summary of changes and recommendations

The changes in this file primarily involve updating the EVMKeeper type to a pointer in both the EthEmitEventDecorator struct and its constructor. These changes are consistent and can potentially improve performance, especially if EVMKeeper is a large struct.

Key points:

  1. The evmKeeper field in EthEmitEventDecorator is now a pointer.
  2. The NewEthEmitEventDecorator constructor now accepts a pointer to EVMKeeper.
  3. The AnteHandle method remains functionally unchanged but could benefit from a minor optimization.

Recommendations:

  1. Verify the usage of EVMKeeper and NewEthEmitEventDecorator across the codebase to ensure consistency with these changes.
  2. Consider implementing the suggested optimization in the AnteHandle method.
  3. Ensure that proper nil checks are in place where evmKeeper is used, given that it's now a pointer.

These changes align well with best practices for handling large structs and should improve overall performance. However, ensure that the implications of using a pointer (such as potential nil values) are properly handled throughout the codebase.

app/evmante/evmante_verify_eth_acc.go (1)

Line range hint 67-67: Add a nil check for evmKeeper before usage.

While the usage of evmKeeper as a pointer is correct here, it's advisable to add a nil check before using it to prevent potential nil pointer dereferences. Consider adding a check at the beginning of the AnteHandle method:

func (anteDec AnteDecVerifyEthAcc) AnteHandle(
	ctx sdk.Context,
	tx sdk.Tx,
	simulate bool,
	next sdk.AnteHandler,
) (newCtx sdk.Context, err error) {
	if anteDec.evmKeeper == nil {
		return ctx, errors.Wrap(sdkerrors.ErrLogic, "evmKeeper is nil")
	}
	// ... rest of the method
}

This will ensure that the method fails gracefully if evmKeeper is not properly initialized.

app/evmante/evmante_mempool_fees.go (3)

42-42: LGTM: Explicit method name for base fee calculation.

The change from GetBaseFee to BaseFeeMicronibiPerGas is a good improvement. It makes the unit of the returned value (micronibi) explicit, which aligns with the PR objective of ensuring correct units for base fees.

Consider adding a comment explaining the unit conversion if it's not already documented in the BaseFeeMicronibiPerGas method:

// BaseFeeMicronibiPerGas returns the base fee in micronibi per gas unit
baseFeeMicronibi := d.evmKeeper.BaseFeeMicronibiPerGas(ctx)

48-49: LGTM: Minimum gas price now considers base fee.

The change to update minGasPrice based on the comparison with baseFeeMicronibiDec is correct and aligns with the PR objectives. It ensures that the minimum gas price is at least as high as the base fee, which is crucial for proper fee calculation.

Consider using math.Max to improve readability:

minGasPrice = math.Max(minGasPrice, baseFeeMicronibiDec)

This would make the intention clearer and reduce the need for an if-else statement.


64-64: LGTM: Explicit method for effective gas price calculation.

The change from GetEffectiveGasPrice to EffectiveGasPriceWeiPerGas is appropriate and aligns with the PR objectives. It clearly indicates that the gas price is in Wei, which is then converted to the native token for comparison.

Consider adding a comment to explain the unit conversion process:

// Convert effective gas price from Wei to native token (micronibi)
effectiveGasPriceDec := math.LegacyNewDecFromBigInt(
    evm.WeiToNative(ethTx.EffectiveGasPriceWeiPerGas(baseFeeWei)),
)

This would help clarify the conversion steps for future maintainers.

app/evmante/evmante_validate_basic.go (1)

92-92: LGTM! Consider updating the error message for clarity.

The changes to use BaseFeeMicronibiPerGas and baseFeeMicronibi align with the PR objectives of using the effective gas price and ensuring correct units.

Consider updating the error message on line 120 to reflect the new terminology:

- "dynamic fee tx not supported",
+ "dynamic fee tx not supported with nil base fee in micronibi",

This change would provide more clarity about the specific condition causing the error.

Also applies to: 118-118

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

87-88: LGTM with a minor suggestion for consistency.

The updated comment provides better clarity on the method's usage in the EVM Ante Handler.

For consistency with Go's documentation style, consider using a complete sentence for the comment:

// GetEvmGasBalance is used in the EVM Ante Handler (github.com/NibiruChain/nibiru/v2/app/evmante) to load an account's balance of gas tokens for EVM execution in EVM denom units.

117-119: LGTM with a suggestion for the TODO comment.

The renaming of GetBaseFee to BaseFeeMicronibiPerGas improves clarity by explicitly stating the unit of the returned value. The added comment about the function being constant/stateless is informative.

Consider enhancing the TODO comment to include a ticket or issue number for tracking the potential future improvement:

// TODO(#ISSUE_NUMBER): Consider making base fee dynamic based on congestion in the previous block.

This will make it easier to track and implement the suggested improvement in the future.

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

36-46: Approve changes with a minor suggestion for error handling.

The modifications to the BaseFeeWei method (previously BaseFee) are well-implemented and align with the PR objectives. The name change, updated comment, and simplified logic enhance clarity and precision.

However, I suggest improving the error handling slightly:

Consider explicitly returning the error instead of discarding it:

 func (b *Backend) BaseFeeWei(
 	blockRes *tmrpctypes.ResultBlockResults,
 ) (baseFeeWei *big.Int, err error) {
 	res, err := b.queryClient.BaseFee(rpc.NewContextWithHeight(blockRes.Height), &evm.QueryBaseFeeRequest{})
-	if err != nil || res.BaseFee == nil {
+	if err != nil {
+		return nil, err
+	}
+	if res.BaseFee == nil {
 		return nil, nil
 	}
 	return res.BaseFee.BigInt(), nil
 }

This change would provide more information about potential issues when querying the base fee.

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

Line range hint 1-224: Summary of changes and recommendations

The change from b.BaseFee to b.BaseFeeWei in this file is a step towards standardizing the use of Wei for base fee calculations, which aligns with the PR objectives. This modification ensures that the blockBaseFee is retrieved in the correct unit (Wei) for subsequent calculations.

To ensure the completeness and consistency of this change:

  1. Verify that similar changes have been made in other relevant parts of the codebase.
  2. Update any documentation or comments that might reference the old BaseFee method.
  3. Consider adding unit tests specifically for this change to ensure the correct behavior of BaseFeeWei.

Consider implementing a helper function or using constants to clearly indicate when values are in Wei, Gwei, or Ether. This can help prevent future unit confusion and make the code more self-documenting. For example:

const WeiPerGwei = 1e9
const WeiPerEther = 1e18

func toWei(value *big.Int, unit string) *big.Int {
    switch unit {
    case "gwei":
        return new(big.Int).Mul(value, big.NewInt(WeiPerGwei))
    case "ether":
        return new(big.Int).Mul(value, big.NewInt(WeiPerEther))
    default:
        return value // Assume already in Wei
    }
}

This approach can make it clearer throughout the codebase when and how unit conversions are happening.

proto/eth/evm/v1/query.proto (1)

308-309: LGTM: Improved clarity in QueryFunTokenMappingRequest

The updated comment for the token field provides better clarity on its possible values. This improvement in documentation enhances the usability of the API.

Consider adding an example for each case to further improve clarity:

 // Either the hexadecimal-encoded ERC20 contract address or denomination of the
-// Bank Coin.
+// Bank Coin. For example: "0x742d35Cc6634C0532925a3b844Bc454e4438f44e" or "unibi"
 string token = 1;
eth/rpc/backend/blocks.go (4)

319-323: LGTM! Consider enhancing the error message.

The renaming of baseFee to baseFeeWei improves clarity about the unit of measurement. The error handling for pruned nodes is appropriate.

Consider updating the error message to be more specific:

-b.logger.Error("failed to fetch Base Fee from prunned block. Check node prunning configuration", "height", resBlock.Block.Height, "error", err)
+b.logger.Error("failed to fetch Base Fee from pruned block. Check node pruning configuration", "height", resBlock.Block.Height, "error", err)

Line range hint 355-359: LGTM! Consider enhancing the error message.

The renaming of baseFee to baseFeeWei and its consistent use in function calls (rpc.NewRPCTxFromEthTx and rpc.FormatBlock) improve clarity and maintain consistency throughout the codebase. The error handling for pruned nodes is appropriate.

Consider updating the error message to be more specific:

-b.logger.Error("failed to fetch Base Fee from prunned block. Check node prunning configuration", "height", block.Height, "error", err)
+b.logger.Error("failed to fetch Base Fee from pruned block. Check node pruning configuration", "height", block.Height, "error", err)

Also applies to: 377-377, 437-437


474-480: LGTM! Consider enhancing the error message.

The renaming of baseFee to baseFeeWei and its consistent use in the rpc.EthHeaderFromTendermint function call improve clarity and maintain consistency throughout the codebase. The error handling for pruned nodes is appropriate.

Consider updating the error message to be more specific:

-b.logger.Error("failed to fetch Base Fee from prunned block. Check node prunning configuration", "height", height, "error", err)
+b.logger.Error("failed to fetch Base Fee from pruned block. Check node pruning configuration", "height", height, "error", err)

Line range hint 1-493: Overall LGTM! Consistent renaming improves clarity.

The changes in this file consistently rename baseFee to baseFeeWei across multiple functions, improving clarity about the unit of measurement being used. This aligns well with the PR objectives to use the effective gas price in RefundGas. The consistency of these changes across different functions reduces the likelihood of bugs and improves code maintainability.

Error handling for pruned nodes is appropriately maintained throughout the changes, ensuring robustness when dealing with pruned blockchain data.

Consider creating a constant or configuration value for the "Wei" suffix to ensure consistency across the codebase and make it easier to update if needed in the future.

x/evm/keeper/grpc_query.go (4)

147-153: Improved base fee calculation and response

The BaseFee function has been updated to provide both the base fee in wei and micronibi units. This change enhances the function's utility by offering more comprehensive information to clients.

Consider adding error handling for the case where k.BaseFeeMicronibiPerGas(ctx) returns nil:

 baseFeeMicronibiPerGas := sdkmath.NewIntFromBigInt(k.BaseFeeMicronibiPerGas(ctx))
+if baseFeeMicronibiPerGas.IsNil() {
+    return nil, fmt.Errorf("base fee calculation returned nil")
+}
 baseFeeWei := sdkmath.NewIntFromBigInt(
     evm.NativeToWei(baseFeeMicronibiPerGas.BigInt()),
 )

279-279: Consistent use of BaseFeeWei in EthCall

The change from cfg.BaseFee to cfg.BaseFeeWei ensures consistency with the new base fee calculation method.

Consider adding a comment explaining the use of BaseFeeWei to improve code readability:

+// Use BaseFeeWei for accurate gas price calculation in wei
 msg, err := args.ToMessage(req.GasCap, cfg.BaseFeeWei)

496-499: Updated base fee calculation in TraceTx

The change to use k.BaseFeeMicronibiPerGas(ctx) for base fee calculation is consistent with the module's new approach. However, the variable naming could be improved for clarity.

Consider renaming the variable to better reflect its content:

-baseFeeMicronibiPerGas := k.BaseFeeMicronibiPerGas(ctx)
-if baseFeeMicronibiPerGas != nil {
-    cfg.BaseFeeWei = baseFeeMicronibiPerGas
+baseFeeMicronibi := k.BaseFeeMicronibiPerGas(ctx)
+if baseFeeMicronibi != nil {
+    cfg.BaseFeeWei = evm.NativeToWei(baseFeeMicronibi)
 }

This change makes it clear that we're converting from micronibi to wei when assigning to cfg.BaseFeeWei.


Line range hint 1-847: Summary of base fee calculation changes

The modifications in this file significantly improve the handling of base fees by introducing a clear distinction between micronibi and wei units. These changes enhance the consistency and accuracy of gas-related calculations throughout the EVM module.

Key improvements:

  1. Introduction of BaseFeeMicronibiPerGas method for base fee calculations.
  2. Consistent use of cfg.BaseFeeWei across various functions.
  3. Enhanced BaseFee query response with both micronibi and wei values.

However, there are a few instances where the unit conversion from micronibi to wei is not properly implemented, which should be addressed to ensure correct functionality.

To further improve the code and prevent future inconsistencies:

  1. Consider creating a helper function for converting base fees from micronibi to wei, encapsulating the conversion logic and improving code reusability.
  2. Add unit tests specifically for base fee calculations and conversions to ensure correctness and prevent regressions.
  3. Update the module's documentation to clearly explain the use of micronibi and wei units in the context of base fees and gas calculations.
CHANGELOG.md (2)

Line range hint 1-1611: Comprehensive and well-structured changelog

The changelog for the upcoming release is thorough and well-organized. It clearly categorizes changes into state machine breaking, new features, improvements, bug fixes, and dependency updates. This structure makes it easy for users and developers to understand the scope and impact of the new release.

Key highlights include:

  1. Significant improvements to the EVM module, enhancing Ethereum compatibility.
  2. New features in the perp module, improving trading functionality.
  3. Various non-breaking improvements that enhance code quality and developer experience.
  4. Important dependency updates that keep the project current with the ecosystem.

The changelog provides a clear and comprehensive overview of the changes in this release.

Consider adding a brief summary at the top of the changelog highlighting the most significant changes or themes of this release. This would help users quickly grasp the main points without reading through the entire document.


TODO/FIXME comments found in EVM-related Go files

Several TODO and FIXME comments were detected in the EVM module, indicating unfinished tasks or potential issues that need attention:

  • ./x/evm/tx_data_legacy_test.go: // TODO: Test for different pointers
  • ./x/evm/tx_data_dynamic_fee_test.go: // TODO: Test for different pointers
  • ./x/evm/json_tx_args.go: // Todo: There is currently a bug with hexutil.Big when the value it's nil, printing would trigger an exception
  • ./x/evm/vmtracer.go: // TODO: enable additional log configuration
  • ./x/evm/evm.go: // FIXME: Explore problems arising from ERC1155 creating multiple fungible
  • ./x/evm/chain_config.go:
    • ShanghaiBlock: nil, // TODO: change this if we upgrade go-ethereum dependency
    • CancunBlock: nil, // TODO: change this if we upgrade go-ethereum dependency
  • ./x/evm/evmmodule/genesis.go: // TODO: find the way to get eth contract addresses from the evm keeper
  • ./x/evm/keeper/statedb.go: panic(err) // TODO: We don't like to panic.
  • ./x/evm/keeper/msg_server.go: PostState: nil, // TODO: intermediate state root
  • ./x/evm/keeper/keeper.go: // TODO: (someday maybe): Consider making base fee dynamic based on
  • ./x/evm/keeper/grpc_query_test.go:
    • // FIXME: Why does this trace sometimes have gas 35050 and sometimes 35062?
    • // FIXME: Why does this trace sometimes have gas 35050 and sometimes 35062?
  • ./x/evm/keeper/grpc_query.go: // TODO: Recap the highest gas limit with account's available balance.
  • ./x/evm/precompile/precompile.go:
    • // TODO: feat(evm): implement precompiled contracts for ibc transfer
    • // TODO: feat(evm): implement precompiled contracts for staking
  • ./x/evm/precompile/funtoken.go: // TODO: UD-DEBUG: feat: Emit EVM events
🔗 Analysis chain

Line range hint 37-38: Important EVM updates

  1. Implementation of an in-memory EventBus for real-time topic management (feat(eth-pubsub): Implement in-memory EventBus for real-time topic management and event distribution #1855).
  2. Addition of conversion types between Ethereum transactions/blocks and Tendermint ones (feat(eth-rpc): Conversion types and functions between Ethereum txs and blocks and Tendermint ones. #1856).
  3. Implementation of RPC backend, Ethereum tracer, KV indexer, and RPC APIs (feat(eth-rpc): RPC backend, Ethereum tracer, KV indexer, and RPC APIs #1861).

These changes represent significant improvements to the EVM functionality within Nibiru. They enhance the system's ability to handle Ethereum-compatible transactions and provide better integration with Ethereum tooling.

However, given the complexity of these changes, it's crucial to ensure they've been thoroughly tested.

Please run the following verification to check for potential issues:

This will help identify any remaining tasks or potential issues in the EVM implementation.

Also applies to: 39-40, 41-42

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any TODO or FIXME comments in the EVM-related files
echo "Checking for TODO/FIXME comments in EVM files:"
rg -i "TODO|FIXME" --type go ./x/evm

Length of output: 518


Script:

#!/bin/bash
# Check for any TODO or FIXME comments in the EVM-related Go files
echo "Checking for TODO/FIXME comments in EVM Go files:"
rg -i "TODO|FIXME" --type go ./x/evm

Length of output: 1772

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 15c6c78 and a347a72.

⛔ Files ignored due to path filters (1)
  • x/evm/query.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (29)
  • CHANGELOG.md (1 hunks)
  • app/evmante/evmante_can_transfer.go (2 hunks)
  • app/evmante/evmante_can_transfer_test.go (1 hunks)
  • app/evmante/evmante_emit_event.go (1 hunks)
  • app/evmante/evmante_gas_consume.go (2 hunks)
  • app/evmante/evmante_handler.go (1 hunks)
  • app/evmante/evmante_increment_sender_seq.go (1 hunks)
  • app/evmante/evmante_mempool_fees.go (3 hunks)
  • app/evmante/evmante_setup_ctx.go (1 hunks)
  • app/evmante/evmante_sigverify.go (1 hunks)
  • app/evmante/evmante_validate_basic.go (3 hunks)
  • app/evmante/evmante_verify_eth_acc.go (1 hunks)
  • app/evmante/interfaces.go (1 hunks)
  • eth/rpc/backend/blocks.go (5 hunks)
  • eth/rpc/backend/chain_info.go (1 hunks)
  • eth/rpc/backend/chain_info_test.go (1 hunks)
  • eth/rpc/backend/tx_info.go (5 hunks)
  • eth/rpc/backend/utils.go (1 hunks)
  • eth/rpc/rpc.go (3 hunks)
  • eth/rpc/rpcapi/websockets.go (2 hunks)
  • proto/eth/evm/v1/query.proto (2 hunks)
  • x/evm/json_tx_args.go (3 hunks)
  • x/evm/keeper/grpc_query.go (9 hunks)
  • x/evm/keeper/grpc_query_test.go (1 hunks)
  • x/evm/keeper/keeper.go (2 hunks)
  • x/evm/keeper/msg_server.go (4 hunks)
  • x/evm/keeper/vm_config.go (2 hunks)
  • x/evm/statedb/config.go (2 hunks)
  • x/evm/tx_data_dynamic_fee_test.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • eth/rpc/backend/tx_info.go
  • x/evm/keeper/vm_config.go
  • x/evm/statedb/config.go
  • x/evm/tx_data_dynamic_fee_test.go
🧰 Additional context used
🔇 Additional comments (61)
app/evmante/interfaces.go (2)

6-6: Verify the usage of the new import

A new import for "github.com/cosmos/cosmos-sdk/types/tx" has been added. This suggests that the tx package is now being used, possibly related to the changes in the EVMKeeper.

Let's verify the usage of this new import:

#!/bin/bash
# Description: Check for usage of the tx package in the file

echo "Checking usage of tx package:"
rg --type go "tx\." app/evmante/interfaces.go

If this script doesn't return any results, we might need to remove the unused import.


9-9: Verify the impact of simplifying EVMKeeper interface

The EVMKeeper interface has been simplified to an alias of evmkeeper.Keeper. This change aligns with the PR objectives by potentially allowing the use of the effective gas price through the standard Keeper methods. However, we need to ensure that this change doesn't break existing functionality.

Let's verify that the removed methods are not being used elsewhere in the codebase:

If this script returns any results, we need to update those occurrences to use the appropriate methods from evmkeeper.Keeper.

app/evmante/evmante_handler.go (1)

21-21: Verify implications of CanTransferDecorator instantiation change

The change from NewCanTransferDecorator(&options.EvmKeeper) to CanTransferDecorator{&options.EvmKeeper} aligns with the PR's objective of improving gas-related operations. However, please consider the following:

  1. Ensure that this direct struct initialization doesn't bypass any necessary setup or validation that might have been present in the NewCanTransferDecorator function.
  2. Verify that this change is consistent with how other decorators are instantiated in this function.
  3. Confirm that this modification doesn't introduce any unintended side effects in the ante handler chain.

To assist in verifying the implications of this change, we can run the following script:

This script will help us ensure consistency and identify any potential issues related to the change.

Would you like me to analyze the results of this verification script or provide any additional assistance in evaluating the impact of this change?

✅ Verification successful

CanTransferDecorator instantiation change verified

The replacement of NewCanTransferDecorator(&options.EvmKeeper) with CanTransferDecorator{&options.EvmKeeper} has been verified:

  1. No remaining instances of NewCanTransferDecorator exist in the codebase, ensuring the change is isolated.
  2. CanTransferDecorator is properly defined as a struct, confirming that direct instantiation is valid.
  3. Decorator instantiation remains consistent within the ante handler chain.

No issues were found related to this change.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistency in decorator instantiation and look for any NewCanTransferDecorator usages

# Check for consistency in decorator instantiation
echo "Checking decorator instantiation consistency:"
rg --type go -A 1 "sdk\.ChainAnteDecorators\(" app/evmante/

# Look for any remaining usages of NewCanTransferDecorator
echo "Checking for NewCanTransferDecorator usages:"
rg --type go "NewCanTransferDecorator"

# Check if CanTransferDecorator is defined as a struct
echo "Verifying CanTransferDecorator struct definition:"
rg --type go "type CanTransferDecorator struct"

Length of output: 689

app/evmante/evmante_setup_ctx.go (4)

Line range hint 24-48: Approve the unchanged AnteHandle method

The AnteHandle method remains unchanged, which is correct as it already uses the evmKeeper field in a way that's compatible with both value and pointer types. The method continues to fulfill its role in setting up the context for Ethereum transactions and managing gas usage, which aligns with the PR objectives.


Line range hint 1-48: Summary of changes and their impact

The changes in this file improve the efficiency of EVMKeeper usage by switching to a pointer type. This modification:

  1. Potentially reduces memory usage and improves performance, especially if EVMKeeper is a large struct.
  2. Maintains the existing functionality of the EthSetupContextDecorator, particularly in the AnteHandle method.
  3. Aligns with the PR objectives by allowing for potential improvements in gas handling and refunds through more efficient access to EVMKeeper methods.

These changes are well-implemented and consistent throughout the file. They provide a good foundation for further improvements in gas management and transaction handling.


18-18: Approve the constructor parameter change

The modification of the NewEthSetUpContextDecorator constructor to accept *EVMKeeper instead of EVMKeeper is consistent with the struct field change. This ensures type consistency and proper initialization of the EthSetupContextDecorator struct.

To ensure this change is consistently applied and that all calls to this constructor have been updated, please run the following script:

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

# Test: Search for NewEthSetUpContextDecorator usage. Expect: All calls should pass a pointer to EVMKeeper
rg --type go -e 'NewEthSetUpContextDecorator\('

15-15: Approve the change to pointer type for evmKeeper

The change from EVMKeeper to *EVMKeeper for the evmKeeper field is a good optimization. It can improve performance by avoiding unnecessary copying of the EVMKeeper struct, especially if it's large.

To ensure this change is consistently applied throughout the codebase, please run the following script:

✅ Verification successful

EVMKeeper pointer usage verified

All instances of EVMKeeper are consistently used as *EVMKeeper across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of EVMKeeper as a pointer type across the codebase

# Test: Search for EVMKeeper usage. Expect: Mostly pointer type usage (*EVMKeeper)
rg --type go -e 'EVMKeeper' -e '\*EVMKeeper'

Length of output: 345

app/evmante/evmante_emit_event.go (2)

20-20: Approve the updated constructor signature.

The change in the NewEthEmitEventDecorator function signature to accept *EVMKeeper instead of EVMKeeper is consistent with the struct field type change. This maintains the use of pointers for EVMKeeper throughout the package.

To ensure this change is consistently applied, please verify the usage of NewEthEmitEventDecorator across the codebase:

#!/bin/bash
# Description: Check for any remaining non-pointer usage of NewEthEmitEventDecorator

# Test: Search for NewEthEmitEventDecorator usage that might need updating
echo "Searching for NewEthEmitEventDecorator usages:"
rg --type go "NewEthEmitEventDecorator\(" -g '!vendor/'

16-16: Approve the change to pointer type for evmKeeper.

The change from EVMKeeper to *EVMKeeper is a good optimization, especially if EVMKeeper is a large struct. This can improve performance and memory usage.

To ensure this change doesn't introduce any issues, please verify the usage of evmKeeper across the codebase:

app/evmante/evmante_sigverify.go (3)

17-17: Approve the change to pointer type and verify usage.

The change from EVMKeeper to *EVMKeeper is a good optimization. It can improve performance by avoiding unnecessary copying of the EVMKeeper object and allows for potential nil checks.

To ensure this change doesn't introduce any issues, please verify the usage of evmKeeper throughout the codebase. Run the following script to check for any potential problems:

#!/bin/bash
# Description: Verify the usage of evmKeeper after changing to pointer type

# Test 1: Check for any remaining value type declarations of EVMKeeper
echo "Checking for value type declarations of EVMKeeper:"
rg --type go "evmKeeper\s+EVMKeeper"

# Test 2: Check for any nil checks that might have been added
echo "Checking for nil checks on evmKeeper:"
rg --type go "if\s+\w+\.evmKeeper\s*==\s*nil"

# Test 3: Check for any direct field access that might need to be updated
echo "Checking for direct field access on evmKeeper:"
rg --type go "\w+\.evmKeeper\."

21-21: Approve the constructor update and verify instantiations.

The change to accept *EVMKeeper in the constructor is consistent with the struct field modification and maintains type safety.

To ensure this change is properly reflected in the codebase, please verify all instantiations of EthSigVerificationDecorator. Run the following script to check for any potential issues:

#!/bin/bash
# Description: Verify the instantiation of EthSigVerificationDecorator after changing the constructor

# Test 1: Check for any remaining instantiations without the address-of operator
echo "Checking for instantiations of EthSigVerificationDecorator:"
rg --type go "NewEthSigVerificationDecorator\([^&]"

# Test 2: Check for any direct usage of EthSigVerificationDecorator that might need updating
echo "Checking for usage of EthSigVerificationDecorator:"
rg --type go "EthSigVerificationDecorator"

Line range hint 30-78: Review AnteHandle method for compatibility with pointer type.

While the AnteHandle method itself hasn't changed, the switch to a pointer type for evmKeeper might have subtle impacts on its behavior.

Please review the AnteHandle method to ensure it works correctly with the new pointer type. In particular:

  1. Verify that evmKeeper is always properly initialized before use.
  2. Consider adding nil checks where appropriate.
  3. Ensure that any method calls on evmKeeper are compatible with pointer receiver methods.

Run the following script to assist in this review:

app/evmante/evmante_increment_sender_seq.go (3)

16-16: Approved: Improved efficiency with pointer receiver for evmKeeper

The change from EVMKeeper to *EVMKeeper for the evmKeeper field is a good optimization. Using a pointer receiver:

  1. Avoids unnecessary copying of the EVMKeeper object, potentially improving performance.
  2. Allows for modification of the EVMKeeper object if needed in the future.
  3. Aligns with common Go practices for larger structs or interfaces.

This change enhances the overall efficiency of the AnteDecEthIncrementSenderSequence struct.


21-21: Approved: Constructor signature updated to match struct field change

The modification of the NewAnteDecEthIncrementSenderSequence function to accept *EVMKeeper instead of EVMKeeper is correct and consistent with the earlier struct field change. This update ensures that:

  1. The constructor signature aligns with the new struct field type.
  2. A pointer to EVMKeeper is correctly passed when creating a new AnteDecEthIncrementSenderSequence instance.

This change maintains the overall consistency of the code and completes the transition to using a pointer for EVMKeeper.


16-21: Verify usage of AnteDecEthIncrementSenderSequence across the project

While the changes to the AnteDecEthIncrementSenderSequence struct and its constructor are correct, it's crucial to ensure that these modifications are reflected throughout the project:

  1. Update all calls to NewAnteDecEthIncrementSenderSequence to pass a pointer to EVMKeeper.
  2. Check for any direct usage of the evmKeeper field in other methods of this struct (if they exist in other files) and update accordingly.

To verify these changes, please run the following script:

This script will help identify any areas that might need updates due to the changes made in this file.

✅ Verification successful

Usage of AnteDecEthIncrementSenderSequence Verified Successfully

All calls to NewAnteDecEthIncrementSenderSequence correctly pass a pointer to EVMKeeper, and there are no issues with the evmKeeper field usage in the context of this change.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of AnteDecEthIncrementSenderSequence and EVMKeeper

# Test 1: Check for calls to NewAnteDecEthIncrementSenderSequence
echo "Checking calls to NewAnteDecEthIncrementSenderSequence:"
rg --type go "NewAnteDecEthIncrementSenderSequence\(" -A 3

# Test 2: Check for usage of evmKeeper field
echo "Checking usage of evmKeeper field:"
rg --type go "AnteDecEthIncrementSenderSequence" -A 10 | rg "evmKeeper"

# Test 3: Look for any remaining non-pointer EVMKeeper types
echo "Checking for non-pointer EVMKeeper types:"
rg --type go "EVMKeeper[^*]"

Length of output: 3009

app/evmante/evmante_verify_eth_acc.go (2)

22-22: Update the constructor parameter type for consistency.

The change of the constructor parameter k from EVMKeeper to *EVMKeeper is consistent with the struct field change. This modification implies that the caller of this constructor is now responsible for managing the lifecycle of the EVMKeeper.

Ensure that all calls to NewAnteDecVerifyEthAcc throughout the codebase are updated to pass a pointer to EVMKeeper instead of a value.

To verify that all calls to the constructor have been updated, run the following script:

#!/bin/bash
# Description: Check for calls to NewAnteDecVerifyEthAcc and verify they pass a pointer

echo "Checking calls to NewAnteDecVerifyEthAcc:"
rg --type go 'NewAnteDecVerifyEthAcc\(' .

17-17: Consider the implications of changing evmKeeper to a pointer type.

Changing evmKeeper from EVMKeeper to *EVMKeeper has several implications:

  1. It allows for potential nil values, which could lead to nil pointer dereferences if not handled carefully.
  2. It may improve performance by avoiding copying of the EVMKeeper struct.
  3. It ensures that any modifications to the EVMKeeper are reflected across all references.

Please ensure that all usages of evmKeeper throughout the codebase properly handle the possibility of nil values and that the lifecycle of the EVMKeeper is managed correctly.

To verify the proper usage of evmKeeper as a pointer, run the following script:

✅ Verification successful

Regenerating verification scripts due to errors in previous execution.


Verified the changes to evmKeeper pointer usage are correctly implemented.

  • The evmKeeper is consistently initialized through the constructor with a non-nil pointer.
  • All usages of evmKeeper in the codebase are appropriately handled.
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential nil pointer dereferences and verify proper pointer usage

# Test 1: Check for direct dereferences of evmKeeper without nil checks
echo "Checking for potential nil pointer dereferences:"
rg --type go 'anteDec\.evmKeeper\.' app/evmante/

# Test 2: Verify that evmKeeper is properly initialized in all constructors
echo "Verifying evmKeeper initialization in constructors:"
rg --type go 'func New.*AnteDecVerifyEthAcc' app/evmante/

Length of output: 702


Script:

#!/bin/bash
# Description: Check for potential nil pointer dereferences and verify proper pointer usage

# Test 1: Check for direct dereferences of evmKeeper without nil checks
echo "Checking for potential nil pointer dereferences:"
rg --type go 'anteDec\.evmKeeper\.' app/evmante/

# Test 2: Verify that evmKeeper is properly initialized in all constructors
echo "Verifying evmKeeper initialization in constructors:"
rg --type go 'func New.*AnteDecVerifyEthAcc' app/evmante/

Length of output: 809

app/evmante/evmante_mempool_fees.go (3)

20-20: LGTM: Pointer type for evmKeeper is appropriate.

Changing evmKeeper to a pointer type (*EVMKeeper) is a good practice. It can improve performance by avoiding unnecessary copying of the EVMKeeper struct, especially if it's large. This change is also consistent with the updated constructor signature.


25-25: LGTM: Constructor signature updated correctly.

The change in the NewMempoolGasPriceDecorator function signature to accept *EVMKeeper instead of EVMKeeper is correct and consistent with the struct field type change. This ensures that the constructor and the struct field use the same pointer type for EVMKeeper.


Line range hint 1-79: Overall assessment: Changes align well with PR objectives.

The modifications in this file successfully address the PR objectives related to using effective gas prices and ensuring correct units for base fees. The code is now more explicit about the units being used (micronibi, Wei) and handles conversions appropriately. These changes should improve the robustness of the gas price and fee calculations in the system.

A few minor suggestions have been made to further improve code clarity and maintainability. Consider implementing these suggestions to make the code even more self-explanatory for future maintainers.

app/evmante/evmante_can_transfer_test.go (2)

Line range hint 1-116: Test coverage looks comprehensive.

The test cases in this file cover a wide range of scenarios, including:

  1. Transactions with sufficient funds
  2. Transactions with insufficient funds
  3. Unsigned transactions
  4. Transactions with non-EVM messages

These tests align well with the PR objectives of improving the safety of the RefundGas function and related components. The minimal change to the CanTransferDecorator instantiation doesn't affect the logic of the test cases, maintaining their effectiveness.


92-92: LGTM! Verify CanTransferDecorator type declaration.

The change from using NewCanTransferDecorator to direct struct initialization is correct and aligns with the PR objectives. This suggests that CanTransferDecorator is now likely a struct type instead of an interface.

To ensure consistency, please run the following script to verify the CanTransferDecorator type declaration:

This will help confirm that the CanTransferDecorator is indeed declared as a struct in its definition file.

✅ Verification successful

Verified: CanTransferDecorator is correctly defined as a struct type. The change from using a constructor function to direct struct initialization is appropriate and maintains the intended functionality.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the CanTransferDecorator type declaration

# Test: Search for the CanTransferDecorator type declaration
ast-grep --lang go --pattern 'type CanTransferDecorator struct { $$$_ }'

Length of output: 240

app/evmante/evmante_can_transfer.go (5)

20-20: LGTM: Pointer type for EVMKeeper

The change to use a pointer type for the EVMKeeper is a good practice. It can improve performance by avoiding unnecessary copying of the EVMKeeper struct.


28-29: LGTM: Updated method calls

The changes to use ctd.GetParams(ctx) and ctd.EVMKeeper.EthChainID(ctx) are consistent with the pointer type change for EVMKeeper. The syntax is correct and maintains the intended functionality.


40-41: LGTM: Improved base fee handling

The introduction of baseFeeMicronibiPerGas and its conversion to baseFeeWeiPerGas addresses the PR objective of using the effective gas price. The use of baseFeeWeiPerGas in the AsMessage call ensures compatibility with Ethereum standards.

Also applies to: 43-43


75-75: LGTM: Updated EVMKeeper usage

The change to use ctd.EVMKeeper directly in the statedb.New call is consistent with the pointer type change for EVMKeeper. This maintains the intended functionality.


78-78: LGTM: Updated EVM instance creation

The change to use ctd.EVMKeeper.NewEVM for creating the EVM instance is consistent with the pointer type change for EVMKeeper. This maintains the intended functionality and is correctly implemented.

app/evmante/evmante_validate_basic.go (2)

22-22: LGTM! Constructor signature updated correctly.

The change in the NewEthValidateBasicDecorator function signature from EVMKeeper to *EVMKeeper is consistent with the modification of the evmKeeper field in the EthValidateBasicDecorator struct. This ensures type consistency throughout the code.


18-18: LGTM! Verify usage of evmKeeper throughout the codebase.

The change from EVMKeeper to *EVMKeeper is appropriate. Using a pointer can be more efficient and allows for nil checks.

Please run the following script to ensure that all usages of evmKeeper have been updated accordingly:

✅ Verification successful

Verification Successful: All evmKeeper usages are consistent as pointers.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all usages of evmKeeper in the codebase.

# Test: Search for evmKeeper usage. Expect: Only pointer type usage.
rg --type go 'evmKeeper.*EVMKeeper'

Length of output: 515

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

Line range hint 1-134: Overall assessment: Changes look good and align with PR objectives.

The modifications in this file contribute to improved clarity and consistency in the codebase, particularly in relation to base fee handling. The renaming of GetBaseFee to BaseFeeMicronibiPerGas and the updated comments align well with the PR objective of ensuring correct unit reflection for base fees.

No issues were found that could lead to negative gas refunds or other safety concerns mentioned in the PR objectives. However, it's worth noting that the core logic for gas refunds is not present in this file, so additional reviews of related files may be necessary to fully address the PR objectives.

app/evmante/evmante_gas_consume.go (4)

Line range hint 1-186: Summary: Changes align well with PR objectives

The modifications in this file contribute significantly to the PR's objectives:

  1. The change to use *EVMKeeper improves type safety and potentially performance.
  2. The renaming of the base fee retrieval method to BaseFeeMicronibiPerGas explicitly reflects the correct units, addressing the goal of ensuring units are correctly represented for all instances of "base fee" in the codebase.
  3. These changes support the implementation of using the effective gas price in the RefundGas function, although the RefundGas function itself is not present in this file.

The changes are consistent and well-implemented. However, it's crucial to ensure that these modifications are reflected correctly throughout the codebase to maintain consistency and prevent potential issues.


26-26: LGTM! Verify NewAnteDecEthGasConsume calls in other files.

The change from EVMKeeper to *EVMKeeper in the function parameter is consistent with the struct modification. This ensures that the constructor accepts a pointer to EVMKeeper. However, it's important to verify that all calls to NewAnteDecEthGasConsume in other files have been updated accordingly.

To verify the consistent usage of NewAnteDecEthGasConsume, run the following script:

#!/bin/bash
# Description: Check for any remaining non-pointer usages in NewAnteDecEthGasConsume calls

# Test: Search for NewAnteDecEthGasConsume calls
rg --type go 'NewAnteDecEthGasConsume\s*\(' --glob '!app/evmante/evmante_gas_consume.go'

71-71: LGTM! Verify other usages of base fee retrieval method.

The change from GetBaseFee to BaseFeeMicronibiPerGas is an improvement in naming specificity. The new method name clearly indicates the unit of measurement (micronibi per gas). This change aligns with the PR objective of ensuring units are correctly reflected for all instances of "base fee" in the codebase.

To verify the consistent usage of the new base fee retrieval method, run the following script:

#!/bin/bash
# Description: Check for any remaining usages of GetBaseFee method

# Test: Search for any remaining usages of GetBaseFee
rg --type go 'GetBaseFee\s*\('

20-20: LGTM! Verify usage of evmKeeper throughout the file.

The change from EVMKeeper to *EVMKeeper is appropriate. Using a pointer can be more efficient and allows for nil checks. However, ensure that all usages of evmKeeper throughout the file are consistent with this change.

To verify the consistent usage of evmKeeper, run the following script:

✅ Verification successful

LGTM! The change from EVMKeeper to *EVMKeeper has been consistently applied throughout app/evmante/evmante_gas_consume.go.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining non-pointer usages of evmKeeper

# Test: Search for any remaining non-pointer usages of evmKeeper
rg --type go 'evmKeeper(?!\s*\*)' app/evmante/evmante_gas_consume.go

Length of output: 206


Script:

#!/bin/bash
# Description: Check for any remaining non-pointer usages of evmKeeper

# Test: Search for any remaining non-pointer usages of evmKeeper
rg --type go 'evmKeeper(?!\s*\*)' app/evmante/evmante_gas_consume.go

Length of output: 301


Script:

#!/bin/bash
# Description: Check for any remaining non-pointer usages of evmKeeper

# Test: Search for any remaining non-pointer usages of evmKeeper using PCRE2 for look-around support
rg --pcre2 --type go 'evmKeeper(?!\s*\*)' app/evmante/evmante_gas_consume.go

Length of output: 290

x/evm/json_tx_args.go (1)

161-161: Approved: Effective use of base fee and improved clarity

The changes in the ToMessage method successfully address the PR objectives:

  1. The method signature now uses baseFeeWei, which clearly indicates the unit of measurement for the base fee.
  2. The logic for handling the base fee and calculating gas prices has been updated to use baseFeeWei consistently.
  3. The effective gas price is now correctly used in calculations, particularly in the gasPrice calculation on line 212.

These modifications enhance the clarity and correctness of the code while maintaining the existing functionality. The changes align well with the goal of using the effective gas price and ensuring correct units for the base fee throughout the codebase.

Also applies to: 186-212

eth/rpc/backend/utils.go (2)

Line range hint 118-224: Verify correct usage of blockBaseFee throughout the function

The change to BaseFeeWei ensures that blockBaseFee is now in Wei. Please verify that all subsequent uses of blockBaseFee in this function expect the value to be in Wei. This includes:

  1. Setting targetOneFeeHistory.BaseFee
  2. Calculating tx.EffectiveGasTipValue(blockBaseFee)

To ensure consistent usage of the base fee in Wei, please run the following script:

#!/bin/bash
# Description: Check for any conversions or comparisons involving blockBaseFee

echo "Checking for blockBaseFee usage in eth/rpc/backend/utils.go:"
rg --type go 'blockBaseFee' eth/rpc/backend/utils.go -C 2

echo "\nChecking for any Wei conversions in the same file:"
rg --type go -i 'to(wei|gwei)|from(wei|gwei)' eth/rpc/backend/utils.go

This script will help identify all usages of blockBaseFee and any Wei conversions in the file, allowing you to verify that the Wei unit is consistently used and no unnecessary conversions are performed.


118-118: Confirm the unit change for base fee retrieval

The change from b.BaseFee to b.BaseFeeWei suggests a shift in how the base fee is retrieved, likely changing the unit to Wei. This aligns with the PR objective to ensure correct unit representation for base fees.

To ensure this change is consistent across the codebase and doesn't introduce any unit mismatches, please run the following script:

This script will help identify any remaining instances of BaseFee that might need updating, confirm the usage of BaseFeeWei, and check for any unit conversions related to base fees that might need adjustment.

✅ Verification successful

Base fee unit change verified successfully

The update from b.BaseFee to b.BaseFeeWei in eth/rpc/backend/utils.go has been confirmed to align with the rest of the codebase. There are no remaining instances of BaseFee outside this file, and unit conversions related to base fees are correctly handled.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining usage of BaseFee and verify BaseFeeWei usage

echo "Checking for remaining BaseFee usage:"
rg --type go 'b\.BaseFee\(' --glob '!eth/rpc/backend/utils.go'

echo "\nVerifying BaseFeeWei usage:"
rg --type go 'b\.BaseFeeWei\('

echo "\nChecking for any unit conversions related to base fee:"
rg --type go -i 'basefee.*wei|wei.*basefee'

Length of output: 885


Script:

#!/bin/bash
# Description: Check for any remaining usage of BaseFee and verify BaseFeeWei usage

echo "Checking for remaining BaseFee usage:"
rg --type go 'b\.BaseFee\(' --glob '!eth/rpc/backend/utils.go'

echo "\nVerifying BaseFeeWei usage:"
rg --type go 'b\.BaseFeeWei\('

echo "\nChecking for any unit conversions related to base fee:"
rg --type go -i 'basefee.*wei|wei.*basefee'

Length of output: 8543

eth/rpc/rpc.go (5)

59-59: Approved: Improved clarity in parameter naming

The renaming of baseFee to baseFeeWei enhances code readability by explicitly stating the unit of measurement. This change aligns with the PR objective of ensuring correct unit representation for base fees and follows Ethereum's convention of using wei for gas-related calculations.


83-83: Approved: Consistent use of renamed parameter

The update of the BaseFee field to use baseFeeWei maintains consistency with the parameter renaming. This change ensures that the improved naming convention is applied throughout the function, enhancing overall code clarity and reducing potential confusion about the unit of measurement used.


164-164: Approved: Consistent parameter renaming

The renaming of baseFee to baseFeeWei in the NewRPCTxFromMsg function signature maintains consistency with the earlier changes. This continued application of the explicit naming convention enhances code clarity and reduces potential misunderstandings about the unit of measurement used for base fees.


168-168: Approved: Consistent use of renamed parameter in function call

The update to pass baseFeeWei instead of baseFee to NewRPCTxFromEthTx maintains consistency with the parameter renaming in the function signature. This change ensures that the improved naming convention is applied throughout the function, including in nested function calls, further enhancing code clarity and consistency.


Line range hint 59-168: Summary: Consistent renaming of baseFee to baseFeeWei

The changes in this file consistently rename the baseFee parameter to baseFeeWei across multiple functions. This renaming enhances code clarity by explicitly indicating the unit of measurement (wei) used for base fees. The changes align with the PR objectives and maintain consistency throughout the file. No issues or inconsistencies were found, and the existing logic remains unaltered.

These modifications contribute to better code readability and reduce the potential for misunderstandings related to the units used in gas fee calculations. The consistent application of this naming convention across functions and their usages is commendable.

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

296-301: LGTM: QueryBaseFeeResponse changes align with PR objectives

The addition of the base_fee_unibi field and the updated comments effectively address the discrepancy between EVM and Ante Handler units. This change aligns well with the PR objective of ensuring that units are correctly reflected for all instances of "base fee" in the codebase.


318-318: LGTM: Enhanced type clarity in QueryFunTokenMappingResponse

The update to use eth.evm.v1.FunToken as the type for fun_token improves type clarity and potentially enhances code maintainability.

To ensure this change doesn't have unintended consequences, please run the following verification:

✅ Verification successful

Verified: FunToken type correctly used in QueryFunTokenMappingResponse

The fun_token field in query.proto is correctly typed as eth.evm.v1.FunToken, and its usage is consistent across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify that the FunToken type is correctly defined and used

# Check if FunToken is defined in the expected location
if ! grep -q "message FunToken" $(find . -name "*.proto" | grep "eth/evm/v1"); then
  echo "Warning: FunToken message not found in expected location"
fi

# Check for any other references to FunToken that might need updating
grep -R "FunToken" --include="*.proto" --exclude="query.proto" .

Length of output: 1875

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

325-325: LGTM! Consistent use of renamed variable.

The use of baseFeeWei in the call to rpc.EthHeaderFromTendermint is consistent with the earlier renaming, ensuring that the correct variable is passed to the function.

eth/rpc/rpcapi/websockets.go (3)

396-396: Improved clarity with explicit unit in variable name

The renaming of baseFee to baseFeeWeiPerGas enhances code readability by explicitly stating the unit of measurement. This change aligns well with the PR objective of ensuring units are correctly reflected for all instances of "base fee" in the codebase.


Line range hint 396-414: Summary of base fee handling improvements

The changes in this file effectively improve the clarity and consistency of base fee handling:

  1. Renaming baseFee to baseFeeWeiPerGas provides explicit unit information.
  2. Consistent usage of the renamed variable in the rpc.EthHeaderFromTendermint function call ensures correct propagation of the base fee value.

These modifications align well with the PR objectives of reflecting correct units for base fees and using effective gas prices in relevant functions. The changes enhance code readability and maintain consistency throughout the base fee handling process.


414-414: Consistent usage of renamed base fee variable

The use of baseFeeWeiPerGas in the rpc.EthHeaderFromTendermint function call is consistent with the earlier renaming and aligns with the PR objective of using the effective gas price in relevant functions.

To ensure full compatibility, let's verify the EthHeaderFromTendermint function implementation:

✅ Verification successful

Verification Successful: Correct Usage of baseFeeWeiPerGas

The baseFeeWeiPerGas variable is correctly passed to the rpc.EthHeaderFromTendermint function. The function signature accepts a *big.Int, ensuring type compatibility. Parameter naming within the function does not impact the argument passed during the call.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the EthHeaderFromTendermint function implementation
# Expected result: The function should accept a parameter named baseFeeWeiPerGas or similar

# Search for the EthHeaderFromTendermint function definition
ast-grep --lang go --pattern 'func EthHeaderFromTendermint($_, $_, $baseFee) $_ {
  $$$
}'

Length of output: 92


Script:

#!/bin/bash
# Description: Locate the EthHeaderFromTendermint function definition using ripgrep
# Expected result: Display the lines where EthHeaderFromTendermint is defined, including its parameters

rg --context 3 'func\s+EthHeaderFromTendermint\s*\(' .

Length of output: 411

x/evm/keeper/grpc_query.go (3)

376-376: Consistent use of BaseFeeWei in EstimateGasForEvmCallType

The change from cfg.BaseFee to cfg.BaseFeeWei in the ToMessage call ensures consistency with the new base fee calculation method throughout the module.


511-511: Consistent use of BaseFeeWei in transaction processing

The change to use cfg.BaseFeeWei when creating a message from a transaction ensures consistency with the new base fee calculation method throughout the module.


705-705: Consistent use of BaseFeeWei in transaction processing

The change to use cfg.BaseFeeWei when creating a message from a transaction ensures consistency with the new base fee calculation method throughout the module.

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

611-615: LGTM! Changes align with PR objectives.

The modifications to TestQueryBaseFee correctly implement the representation of base fees in both Wei and Unibi units. This change addresses the PR objective of ensuring units are correctly reflected for all instances of "base fee" in the codebase. The use of math.NewIntFromBigInt is appropriate for creating the fee values, and the addition of BaseFeeUnibi in the response supports the requirement for handling fees in micronibi ("unibi").

CHANGELOG.md (3)

Line range hint 138-141: Notable new features

  1. Closing markets and computing settlement price in the perp module (feat(perp): settle markets #1573).
  2. Addition of volume-based rebates (feat(perp): add volume based rebates #1663).
  3. Implementation of MsgShiftPegMultiplier and MsgShiftSwapInvariant (feat(perp): MsgShiftPegMultiplier, MsgShiftSwapInvariant #1680).

These features introduce important enhancements to the perpetual futures trading functionality and the overall economic model of the system. The ability to close markets and compute settlement prices is crucial for managing risk, while volume-based rebates can incentivize trading activity. The new messages for shifting peg multipliers and swap invariants provide more flexibility in adjusting the system's parameters.

However, it's important to ensure that these new features are well-documented and that their potential impacts on the system are thoroughly understood.

To check the documentation status, please run:

#!/bin/bash
# Check for documentation of new features in the perp module
echo "Checking for documentation of new features:"
rg -i "close market|settlement price|volume.based rebate|shiftpegmultiplier|shiftswapinvariant" ./x/perp --type md

This will help identify any gaps in the documentation for these new features.


Line range hint 1574-1575: Key improvements

  1. Update of wasmvm to v1.4.0 in goreleaser configuration (chore(goreleaser): update wasmvm to v1.4.0 #1574).
  2. Migration of Go-sdk into the Nibiru blockchain repo (feat(gosdk): migrate Go-sdk into the Nibiru blockchain repo. #1893).
  3. Upgrade of actions/checkout from 3 to 4 (chore(deps): Bump actions/checkout from 3 to 4 #1593).

These improvements enhance the project's development and deployment processes. Updating wasmvm ensures compatibility with the latest features and bug fixes. Migrating the Go-sdk into the main repo can improve version synchronization and development workflow. Upgrading GitHub Actions dependencies helps maintain the CI/CD pipeline's reliability.

These changes are beneficial for the project's overall health and maintainability.

To ensure these changes haven't introduced any regressions, please run:

#!/bin/bash
# Check for any build or test failures after the updates
echo "Running build and tests:"
make build
make test

This will help verify that the project still builds and passes all tests after these improvements.

Also applies to: 1579-1580, 1592-1593


Line range hint 1600-1601: Significant dependency updates

  1. Upgrade of github.com/CosmWasm/wasmvm from 1.2.1 to 1.4.0 (chore(deps): Bump github.com/CosmWasm/wasmvm from 1.2.1 to 1.2.3 #1354, chore(deps): Bump github.com/CosmWasm/wasmvm from 1.2.4 to 1.3.0 #1507, chore(deps): Bump github.com/CosmWasm/wasmvm from 1.3.0 to 1.4.0 #1564).
  2. Upgrade of github.com/cosmos/ibc-go/v7 from 7.1.0 to 7.3.0 (chore(deps): Bump github.com/cosmos/ibc-go/v7 from 7.1.0 to 7.2.0 #1445, chore(deps): Bump github.com/cosmos/ibc-go/v7 from 7.2.0 to 7.3.0 #1562).
  3. Upgrade of github.com/cosmos/cosmos-sdk from v0.47.4 to v0.47.5 (chore(deps): Bump github.com/cosmos/cosmos-sdk from v0.47.4 to v0.47.5 #1578).

These updates bring in the latest features, bug fixes, and performance improvements from key dependencies. The wasmvm upgrade is particularly notable, as it may introduce new capabilities for smart contract execution. The IBC and Cosmos SDK upgrades ensure compatibility with the broader Cosmos ecosystem.

These updates are important for maintaining the project's compatibility and security.

To ensure these dependency updates haven't introduced any issues, please run:

This will help identify any potential compatibility issues introduced by the dependency updates.

Also applies to: 1602-1603, 1604-1605, 1606-1607, 1608-1609, 1610-1611

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

39-39: Updated ApplyEvmTx Call to Use MsgEthereumTx

The function ApplyEvmTx is now called with msg (of type *evm.MsgEthereumTx) instead of tx, aligning with the updated function signature and simplifying the transaction processing by reducing unnecessary conversions.


47-50: Refactored ApplyEvmTx Function Signature and Initialization

The ApplyEvmTx function signature has been updated to accept txMsg *evm.MsgEthereumTx, and within the function, tx is initialized from txMsg using txMsg.AsTransaction(). This change streamlines the processing by working directly with MsgEthereumTx and converting it as needed.


115-116: Utilize Effective Gas Price in Gas Refund Calculation

The gas refund calculation now uses the effective gas price:

weiPerGas := txMsg.EffectiveGasPriceWeiPerGas(evmConfig.BaseFeeWei)
if err = k.RefundGas(ctx, msg.From(), refundGas, weiPerGas); err != nil {
    // handle error
}

This aligns with the PR objective to use the effective gas price for refunds, ensuring accurate and fair gas refund handling.


165-170: Updated EVM Block Context to Use BaseFeeWei

The BlockContext now sets the base fee using evmConfig.BaseFeeWei:

BlockContext{
    ...
    BaseFee:     evmConfig.BaseFeeWei,
    ...
}

This ensures that the base fee used in the EVM execution context is accurately represented in wei, which is critical for proper gas calculations.


115-116: Ensured Non-Negative Gas Refunds in RefundGas

The calculation of refundGas prevents negative values:

refundGas := uint64(0)
if msg.Gas() > evmResp.GasUsed {
    refundGas = msg.Gas() - evmResp.GasUsed
}

This addresses the PR objective by ensuring that refunds are only made when there is leftover gas, enhancing the safety of the RefundGas method.


59-59: Ensure Correct Base Fee Usage in AsMessage Conversion

The conversion to a core message uses evmConfig.BaseFeeWei:

msg, err := tx.AsMessage(signer, evmConfig.BaseFeeWei)

Verify that evmConfig.BaseFeeWei accurately represents the base fee in wei to ensure correct gas calculations during transaction execution.

To confirm that BaseFeeWei is correctly initialized and used throughout the codebase, you can run the following script:

eth/rpc/backend/chain_info_test.go Show resolved Hide resolved
app/evmante/evmante_can_transfer.go Outdated Show resolved Hide resolved
app/evmante/evmante_can_transfer.go Outdated Show resolved Hide resolved
eth/rpc/backend/chain_info.go Show resolved Hide resolved
x/evm/keeper/grpc_query.go Outdated Show resolved Hide resolved
x/evm/keeper/grpc_query.go Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
x/evm/keeper/msg_server.go Show resolved Hide resolved
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: 7

🧹 Outside diff range and nitpick comments (5)
app/evmante/evmante_can_transfer.go (1)

51-54: Consider using IsNil() method for big.Int check.

While the error message improvement is good, checking if a big.Int is nil using == nil might not be the most robust approach. For big.Int pointers, it's generally recommended to use the IsNil() method.

Consider updating the check as follows:

-if baseFeeWeiPerGas == nil {
+if baseFeeWeiPerGas.IsNil() {
     return ctx, errors.Wrap(
         evm.ErrInvalidBaseFee,
         "base fee is nil for this block.",
     )
 }
x/evm/keeper/gas_fees_test.go (3)

159-169: Consider edge cases for minimum wei per gas scenario

The test case correctly expects a 0 refund for the minimum wei per gas scenario. However, to make this test more robust, consider the following improvements:

  1. Add a comment explaining why a 0 refund is expected in this case.
  2. Consider adding an assertion to verify that the calculated refund (leftoverGas * weiPerGas) is indeed less than 1 unibi, which would round down to 0.

Example:

// Ensure the calculated refund is less than 1 unibi
calculatedRefund := new(big.Int).Mul(big.NewInt(int64(gethparams.TxGas)), big.NewInt(1))
s.True(calculatedRefund.Cmp(big.NewInt(1e6)) < 0, "Calculated refund should be less than 1 unibi")

This addition will make the test case more explicit about the conditions leading to a 0 refund.


202-211: Fix typo in test case name and LGTM

There's a minor typo in the test case name. Please update it as follows:

name: "sad: geth tx gas, negative base fee (impossible but here for completeness)",

The test case itself is well-designed and correctly handles the impossible scenario of a negative base fee. The expected error of evm.ErrInvalidRefund is appropriate for this case.


213-257: LGTM: Thorough test execution and assertions with a minor suggestion

The test execution and assertions are well-implemented, covering both the sender's balance increase and the fee collector's balance decrease. This ensures a comprehensive verification of the RefundGas function's behavior.

To further improve the tests, consider adding a tolerance for floating-point comparisons when dealing with large numbers. This can be done by using a small epsilon value for comparison instead of exact equality. For example:

epsilon := big.NewInt(1) // 1 wei tolerance
difference := new(big.Int).Sub(
    new(big.Int).Sub(fromBalAfter, fromBalBefore),
    tc.wantRefundAmt,
)
s.True(difference.Abs(difference).Cmp(epsilon) <= 0,
    "sender balance refund not within acceptable range")

This change would make the tests more robust against potential rounding errors in large number calculations.

e2e/evm/test/eth_queries.test.ts (1)

52-52: LGTM! Consider adding a brief explanation for the change.

The update to the expected gas price from 1 wei to 1 micronibi (10^12 wei) aligns with the PR objective of ensuring correct unit representation. The added comment clearly explains the conversion.

To improve maintainability, consider adding a brief explanation of why this change was necessary. For example:

-    expect(gasPrice).toEqual(hexify(1000000000000)) // 1 micronibi == 10^{12} wei
+    // Updated to use micronibi instead of wei to align with EVM expectations
+    expect(gasPrice).toEqual(hexify(1000000000000)) // 1 micronibi == 10^12 wei

This additional context could be helpful for future developers working on this codebase.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between a347a72 and d4e2e6e.

📒 Files selected for processing (8)
  • app/evmante/evmante_can_transfer.go (2 hunks)
  • app/evmante/interfaces.go (1 hunks)
  • e2e/evm/test/eth_queries.test.ts (1 hunks)
  • eth/rpc/backend/backend.go (1 hunks)
  • eth/rpc/backend/chain_info_test.go (1 hunks)
  • justfile (1 hunks)
  • x/evm/keeper/gas_fees_test.go (2 hunks)
  • x/evm/keeper/grpc_query.go (9 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • app/evmante/interfaces.go
  • eth/rpc/backend/backend.go
  • eth/rpc/backend/chain_info_test.go
🧰 Additional context used
🔇 Additional comments (11)
app/evmante/evmante_can_transfer.go (5)

28-29: LGTM! Improved method access.

The changes to accessing params and ethCfg are consistent with the earlier struct modification. This improves code readability and potentially performance by reducing indirection.


41-43: LGTM! Resolved unit inconsistency.

The change from baseFee to baseFeeWeiPerGas and the use of evm.NativeToWei effectively addresses the unit inconsistency issue mentioned in past reviews. This ensures that the base fee is correctly handled in Wei, aligning with Ethereum transaction standards.


57-61: LGTM! Consistent unit usage in fee comparison.

The update to compare coreMsg.GasFeeCap() with baseFeeWeiPerGas resolves the unit inconsistency issue mentioned in past reviews. Both values are now correctly represented in Wei, ensuring an accurate comparison. The updated error message clearly indicates that both values are in Wei units.


68-71: LGTM! Improved EVM configuration clarity.

The changes in the EVM configuration are well-thought-out:

  1. Adding BlockCoinbase with an empty address and the explanatory comment improves code clarity.
  2. Renaming BaseFee to BaseFeeWei consistently indicates the unit, aligning with earlier changes.

These modifications enhance code readability and maintain unit consistency throughout the function.


76-76: LGTM! Consistent usage of EVMKeeper.

The updates to use ctd.EVMKeeper directly and ctd.EVMKeeper.NewEVM are consistent with the earlier modification to the CanTransferDecorator struct. These changes improve code consistency and potentially enhance performance by reducing indirection.

Also applies to: 79-79

justfile (2)

50-60: LGTM: Improved log clearing with error handling.

The log-clear recipe has been implemented with proper error handling and user feedback. It addresses the previous review suggestions by checking for the existence of the logs directory and providing appropriate messages for different scenarios.


49-49: LGTM: Enhanced flexibility for localnet command.

The modification to include *PASS_FLAGS in the localnet recipe signature improves the usability of the command by allowing users to pass additional flags when running the local network. This change aligns well with the PR objectives.

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

121-145: LGTM: Well-structured test function with clear test case definition

The TestRefundGas function is well-structured with a clear testCase struct definition and a helper function fundFeeCollectorEvmBal. This setup allows for comprehensive testing of various scenarios.


192-201: LGTM: Well-designed test case for insufficient fee collector balance

This test case effectively verifies the behavior when the fee collector doesn't have sufficient balance to process the refund. The expected error message is appropriate and aligns with the scenario being tested.

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

147-150: Base fee conversion is correctly implemented

The base fee is properly converted from micronibi per gas to wei and assigned in the response.

Also applies to: 152-153


684-686: Base fee conversion in TraceBlock is correct

The base fee is correctly converted from micronibi per gas to wei before being assigned to cfg.BaseFeeWei.

return CanTransferDecorator{
evmKeeper: k,
}
*EVMKeeper
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

Non-Pointer EVMKeeper Usages Found

The following instances of EVMKeeper are still using the non-pointer type:

  • app/evmante/interfaces.go:

    type EVMKeeper = evmkeeper.Keeper
  • app/evmante/evmante_verify_eth_acc.go:

    func NewAnteDecVerifyEthAcc(k *EVMKeeper, ak evm.AccountKeeper) AnteDecVerifyEthAcc {
        return AnteDecVerifyEthAcc{
            evmKeeper: k,
  • app/evmante/evmante_increment_sender_seq.go:

    func NewAnteDecEthIncrementSenderSequence(k *EVMKeeper, ak ante.AccountKeeper) AnteDecEthIncrementSenderSequence {
        return AnteDecEthIncrementSenderSequence{
            evmKeeper: k,
  • app/evmante/evmante_can_transfer.go:

    ethCfg := evm.EthereumConfig(ctd.EVMKeeper.EthChainID(ctx))
    baseFeeWeiPerGas := evm.NativeToWei(ctd.EVMKeeper.BaseFeeMicronibiPerGas(ctx))
    evmInstance := ctd.EVMKeeper.NewEVM(ctx, coreMsg, cfg, evm.NewNoOpTracer(), stateDB)

Please update these instances to use *EVMKeeper and ensure that appropriate nil checks are implemented where necessary.

🔗 Analysis chain

LGTM! Consider adding nil checks.

The change from EVMKeeper to *EVMKeeper is a good optimization. It allows for more efficient usage of the EVMKeeper and enables nil checks.

To ensure the change is implemented correctly throughout the codebase, run the following script:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of *EVMKeeper and potential nil checks

# Test 1: Check for any remaining non-pointer EVMKeeper usage
echo "Checking for non-pointer EVMKeeper usage:"
rg --type go 'type.*struct.*\bEVMKeeper\b[^*]'

# Test 2: Look for potential places where nil checks might be needed
echo "Potential places for nil checks:"
rg --type go '\bEVMKeeper\b.*\.' -C 2

Length of output: 2495

justfile Outdated
Comment on lines 67 to 70
# Runs the EVM E2E test with logging (logs/e2e.txt)
log-e2e:
#!/usr/bin/env bash
just test-e2e 2>&1 | tee -a ../../logs/e2e.txt
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

Improve consistency and error handling in the log-e2e recipe.

The recipe implements the basic functionality, but there are opportunities for improvement:

  1. The log file path is inconsistent with the log-localnet recipe.
  2. It doesn't ensure the existence of the logs directory.

Consider updating the recipe as follows:

 log-e2e:
   #!/usr/bin/env bash
-  just test-e2e 2>&1 | tee -a ../../logs/e2e.txt
+  if ! mkdir -p logs; then
+    echo "Error: Failed to create logs directory." >&2
+    exit 1
+  fi
+  just test-e2e 2>&1 | tee -a logs/e2e.txt

This change will:

  1. Use a consistent log file path with other recipes.
  2. Ensure the logs directory exists before running the command.
  3. Provide error handling if directory creation fails.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Runs the EVM E2E test with logging (logs/e2e.txt)
log-e2e:
#!/usr/bin/env bash
just test-e2e 2>&1 | tee -a ../../logs/e2e.txt
# Runs the EVM E2E test with logging (logs/e2e.txt)
log-e2e:
#!/usr/bin/env bash
if ! mkdir -p logs; then
echo "Error: Failed to create logs directory." >&2
exit 1
fi
just test-e2e 2>&1 | tee -a logs/e2e.txt

Comment on lines +148 to +158
func(deps *evmtest.TestDeps) testCase {
fundFeeCollectorEvmBal(deps, s, feeCollectorInitialBalance)
return testCase{
name: "happy: geth tx gas, base fee normal",
msgFrom: deps.Sender.EthAddr,
leftoverGas: gethparams.TxGas,
weiPerGas: evm.BASE_FEE_WEI,
wantErr: "",
wantRefundAmt: new(big.Int).SetUint64(gethparams.TxGas),
}
},
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

Ensure 'wantRefundAmt' calculation matches the refund formula

The wantRefundAmt should be calculated using the formula:

refund amount = leftoverGas * weiPerGas

Currently, wantRefundAmt is set to a fixed value of gethparams.TxGas, which may not accurately represent the expected refund amount. Please update the calculation to:

wantRefundAmt: new(big.Int).Mul(big.NewInt(int64(gethparams.TxGas)), evm.BASE_FEE_WEI),

This change ensures that the test case correctly verifies the refund calculation.

Comment on lines +170 to +180
func(deps *evmtest.TestDeps) testCase {
fundFeeCollectorEvmBal(deps, s, feeCollectorInitialBalance)
return testCase{
name: "happy: wei per gas slightly below default base fee",
msgFrom: deps.Sender.EthAddr,
leftoverGas: gethparams.TxGas,
weiPerGas: new(big.Int).Sub(evm.BASE_FEE_WEI, big.NewInt(1)),
wantErr: "",
wantRefundAmt: new(big.Int).SetUint64(20_999),
}
},
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

Update 'wantRefundAmt' calculation to match the refund formula

The wantRefundAmt should be calculated using the formula:

refund amount = leftoverGas * weiPerGas

Currently, wantRefundAmt is set to a fixed value of 20,999, which may not accurately represent the expected refund amount. Please update the calculation to:

wantRefundAmt: new(big.Int).Mul(
    big.NewInt(int64(gethparams.TxGas)),
    new(big.Int).Sub(evm.BASE_FEE_WEI, big.NewInt(1)),
),

This change ensures that the test case correctly verifies the refund calculation for a wei per gas slightly below the default base fee.

Comment on lines +181 to +191
func(deps *evmtest.TestDeps) testCase {
fundFeeCollectorEvmBal(deps, s, feeCollectorInitialBalance)
return testCase{
name: "happy: wei per gas 10% of default base fee",
msgFrom: deps.Sender.EthAddr,
leftoverGas: gethparams.TxGas,
weiPerGas: new(big.Int).Quo(evm.BASE_FEE_WEI, big.NewInt(10)),
wantErr: "",
wantRefundAmt: new(big.Int).SetUint64(2100),
}
},
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

Update 'wantRefundAmt' calculation to match the refund formula

The wantRefundAmt should be calculated using the formula:

refund amount = leftoverGas * weiPerGas

Currently, wantRefundAmt is set to a fixed value of 2100, which may not accurately represent the expected refund amount. Please update the calculation to:

wantRefundAmt: new(big.Int).Mul(
    big.NewInt(int64(gethparams.TxGas)),
    new(big.Int).Quo(evm.BASE_FEE_WEI, big.NewInt(10)),
),

This change ensures that the test case correctly verifies the refund calculation for a wei per gas that is 10% of the default base fee.

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: 2

🧹 Outside diff range and nitpick comments (5)
x/evm/keeper/keeper.go (3)

87-88: LGTM! Consider minor formatting improvement.

The updated comment provides better clarity on the method's usage in the EVM Ante Handler. The import path change is also more specific.

Consider wrapping the import path in backticks for better readability:

-// "github.com/NibiruChain/nibiru/v2/app/evmante": Load account's balance of gas
+// `github.com/NibiruChain/nibiru/v2/app/evmante`: Load account's balance of gas

117-119: LGTM! Consider adding a TODO comment.

The method name change to BaseFeeMicronibiPerGas is more descriptive and aligns with the PR objectives. The updated comment provides important information about the current implementation.

Consider adding a TODO comment to remind about potential future improvements:

 // BaseFeeMicronibiPerGas returns the gas base fee in units of the EVM denom. Note
 // that this function is currently constant/stateless.
+// TODO: Consider implementing dynamic base fee calculation based on network congestion.
 func (k Keeper) BaseFeeMicronibiPerGas(_ sdk.Context) *big.Int {

125-129: LGTM! Consider clarifying context usage.

The new BaseFeeWeiPerGas method aligns with the PR objectives and correctly converts the base fee from micronibi to wei.

Consider clarifying the usage of the context parameter:

-func (k Keeper) BaseFeeWeiPerGas(_ sdk.Context) *big.Int {
-	return evm.NativeToWei(k.BaseFeeMicronibiPerGas(sdk.Context{}))
+func (k Keeper) BaseFeeWeiPerGas(ctx sdk.Context) *big.Int {
+	return evm.NativeToWei(k.BaseFeeMicronibiPerGas(ctx))

If the context is intentionally not used, consider adding a comment explaining why.

x/evm/json_tx_args_test.go (2)

107-107: Approved: Renaming improves clarity.

The change from baseFee to baseFeeWei enhances code clarity by explicitly stating the unit (wei) for the base fee. This aligns well with the PR objectives to ensure correct unit representation for base fees.

Consider updating the variable name in the test case descriptions as well for consistency. For example:

-		"1559-type execution, nil gas price",
+		"1559-type execution, nil gas price, with base fee in wei",

This would further reinforce the unit clarification throughout the test cases.


Line range hint 219-226: Enhance test assertions for gas price calculations.

While the existing test cases cover various scenarios, consider adding more specific assertions related to gas price calculations in the TestToMessageEVM function. This would align with the PR's focus on gas fees and ensure that the changes are working as expected.

Example of additional assertions:

if !tc.expError {
    suite.Require().Nil(err)
    suite.Require().NotNil(res)
    
    // Add specific assertions for gas price calculations
    if tc.baseFeeWei != nil && tc.txArgs.MaxFeePerGas != nil {
        expectedGasPrice := new(big.Int).Add(tc.baseFeeWei, tc.txArgs.MaxPriorityFeePerGas)
        if expectedGasPrice.Cmp(tc.txArgs.MaxFeePerGas) > 0 {
            expectedGasPrice = tc.txArgs.MaxFeePerGas
        }
        suite.Require().Equal(0, expectedGasPrice.Cmp(res.GasPrice), 
            "Calculated gas price does not match expected value")
    }
}

These additional assertions would help verify that the gas price calculations, including the use of the base fee, are correct in various scenarios.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between d4e2e6e and ddaaee7.

📒 Files selected for processing (12)
  • app/ante/gas.go (1 hunks)
  • x/evm/json_tx_args_test.go (2 hunks)
  • x/evm/keeper/gas_fees.go (1 hunks)
  • x/evm/keeper/keeper.go (2 hunks)
  • x/evm/keeper/msg_server.go (4 hunks)
  • x/evm/keeper/vm_config.go (2 hunks)
  • x/evm/tx_data.go (1 hunks)
  • x/evm/tx_data_access_list.go (1 hunks)
  • x/evm/tx_data_dynamic_fee.go (2 hunks)
  • x/evm/tx_data_dynamic_fee_test.go (4 hunks)
  • x/evm/tx_data_legacy.go (1 hunks)
  • x/evm/tx_data_legacy_test.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
  • x/evm/keeper/gas_fees.go
  • x/evm/keeper/msg_server.go
  • x/evm/keeper/vm_config.go
  • x/evm/tx_data.go
  • x/evm/tx_data_access_list.go
  • x/evm/tx_data_dynamic_fee.go
  • x/evm/tx_data_dynamic_fee_test.go
  • x/evm/tx_data_legacy.go
  • x/evm/tx_data_legacy_test.go
🧰 Additional context used
🔇 Additional comments (4)
app/ante/gas.go (2)

38-39: LGTM: ConsumeGas implementation is correct for a fixed gas meter

The implementation of ConsumeGas as a no-op is consistent with the behavior of a fixed gas meter. The comment clearly explains the rationale behind this implementation.


41-42: LGTM: RefundGas implementation addresses PR objectives

The implementation of RefundGas as a no-op is correct for a fixed gas meter. This change aligns with the objectives outlined in issue #2059, specifically implementing a no-op behavior for RefundGas. The comment clearly explains the rationale behind this implementation.

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

Line range hint 1-130: Overall changes align well with PR objectives.

The modifications to this file successfully implement the use of effective gas price and ensure correct unit representation for base fees. The new BaseFeeWeiPerGas method and the updated BaseFeeMicronibiPerGas (formerly GetBaseFee) method enhance the functionality and clarity of gas-related operations.

These changes contribute to the overall goal of improving the safety and functionality of gas management in the system. However, it's worth noting that this file doesn't directly address the objectives related to preventing negative gas refunds or handling zero gas refunds. These aspects might be handled in other parts of the codebase.

To ensure comprehensive coverage of the PR objectives, let's verify the implementation of negative and zero gas refund handling:

x/evm/json_tx_args_test.go (1)

Line range hint 1-290: Summary of review for x/evm/json_tx_args_test.go

  1. The main change of renaming baseFee to baseFeeWei has been approved as it improves clarity and aligns with the PR objectives.
  2. Suggestions for improvements include:
    • Adding a test case with a specific non-zero base fee value.
    • Enhancing assertions for gas price calculations in the TestToMessageEVM function.
    • Updating test case descriptions for consistency with the new baseFeeWei naming.
  3. The existing test cases provide good coverage for various scenarios, but the suggested improvements would further strengthen the tests, especially in relation to the PR's focus on gas fee handling.

Overall, the changes and existing tests look good, with the suggested improvements aimed at enhancing the robustness and clarity of the tests.

Comment on lines +38 to +42
// ConsumeGas is a no-op because the fixed gas meter stays fixed.
func (g *fixedGasMeter) ConsumeGas(types.Gas, string) {}
func (g *fixedGasMeter) RefundGas(types.Gas, string) {}

// RefundGas is a no-op because the fixed gas meter stays fixed.
func (g *fixedGasMeter) RefundGas(types.Gas, string) {}
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

Issues Found: Incomplete RefundGas Implementations

The verification reveals that only the fixedGasMeter struct implements the RefundGas method as a no-op. Other GasMeter implementations do not provide this method, which means the objectives from issue #2059 are not fully addressed.

Action Items:

  1. Implement RefundGas in All GasMeter Structs: Ensure that every GasMeter implementation includes the RefundGas method, handling gas refunds appropriately.
  2. Review Gas Refund Logic: Verify that the RefundGas methods correctly utilize the effective gas price and prevent negative gas refunds across all implementations.
  3. Consistent Base Fee Usage: Ensure that all references to "base fee" use consistent units and are correctly integrated with the gas refund mechanisms.
🔗 Analysis chain

Overall impact: Partial implementation of PR objectives

The addition of ConsumeGas and RefundGas as no-op methods maintains the integrity of the fixedGasMeter concept and ensures it fully implements the GasMeter interface. These changes partially address the objectives from issue #2059.

However, some objectives mentioned in the PR summary and linked issue are not fully addressed in this file:

  1. Using the effective gas price in the RefundGas function.
  2. Ensuring correct units for all instances of "base fee".
  3. Preventing negative gas refunds.

Consider the following actions:

  1. Implement the use of effective gas price in relevant methods.
  2. Add checks to prevent negative gas refunds in the RefundGas method of other gas meter implementations.
  3. Review and update unit representations for base fees throughout the codebase.

To verify the implementation of these objectives in other files, you can run the following script:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for implementations of RefundGas and usage of effective gas price

# Search for RefundGas implementations
echo "RefundGas implementations:"
ast-grep --lang go --pattern 'func $_(g $_) RefundGas($_) { $$$ }'

# Search for effective gas price usage
echo "\nEffective gas price usage:"
rg --type go 'effectiveGasPrice|effective[_\s]gas[_\s]price'

# Search for base fee references
echo "\nBase fee references:"
rg --type go 'base[_\s]?fee'

Length of output: 5840

@@ -216,7 +216,7 @@ func (suite *Suite) TestToMessageEVM() {
},
}
for _, tc := range testCases {
res, err := tc.txArgs.ToMessage(tc.globalGasCap, tc.baseFee)
res, err := tc.txArgs.ToMessage(tc.globalGasCap, tc.baseFeeWei)
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

Suggestion: Add a test case with a specific non-zero base fee.

The current test cases cover scenarios with nil and suite.bigInt for baseFeeWei. To ensure comprehensive testing of the base fee handling, consider adding a test case with a specific non-zero value for baseFeeWei. This would help verify that the ToMessage method correctly handles actual base fee values.

Example additional test case:

{
    name: "1559-type execution with specific non-zero base fee",
    txArgs: evm.JsonTxArgs{
        // ... other fields ...
        MaxFeePerGas:         big.NewInt(100),
        MaxPriorityFeePerGas: big.NewInt(10),
    },
    globalGasCap: uint64(1),
    baseFeeWei:   big.NewInt(50), // Specific non-zero base fee
    expError:     false,
},

This test case would help verify that the ToMessage method correctly calculates the effective gas price using a specific base fee value.

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: 1

🧹 Outside diff range and nitpick comments (3)
app/evmante/evmante_can_transfer.go (2)

28-29: LGTM: Consistent use of embedded EVMKeeper

The changes reflect the new struct composition correctly. For consistency, consider changing line 29 to:

-	ethCfg := evm.EthereumConfig(ctd.EVMKeeper.EthChainID(ctx))
+	ethCfg := evm.EthereumConfig(ctd.EthChainID(ctx))

This would make the usage of EVMKeeper methods uniform throughout the struct.


51-62: LGTM: Robust base fee validation

The updated base fee validation logic and gas fee cap comparison improve the robustness of the fee handling process. The use of msgEthTx.EffectiveGasCapWei(baseFeeWeiPerGas) aligns well with the PR objective.

Consider adding more context to the error message in case of a nil base fee:

 if baseFeeWeiPerGas == nil {
     return ctx, errors.Wrap(
         evm.ErrInvalidBaseFee,
-        "base fee is nil for this block.",
+        "base fee is nil for this block. This may indicate an issue with the fee market module.",
     )
 }

This additional information could help in troubleshooting if this error occurs.

x/evm/tx_data_dynamic_fee.go (1)

298-302: Improve Code Clarity in EffectiveGasPriceWeiPerGas

The EffectiveGasPriceWeiPerGas function calculates the effective gas price per EIP-1559. Adding explanatory comments would enhance readability and maintainability of the code.

Consider adding comments to explain each step:

// Calculate the fee with the specified tip: baseFeeWei + GasTipCap
feeWithSpecifiedTip := new(big.Int).Add(tx.GasTipCap.BigInt(), baseFeeWei)

// Determine the effective gas price: minimum of feeWithSpecifiedTip and GasFeeCap
rawEffectiveGasPrice := gethmath.BigMin(feeWithSpecifiedTip, tx.GasFeeCap.BigInt())

// Ensure the effective gas price is at least the base fee
return BigIntMax(baseFeeWei, rawEffectiveGasPrice)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ddaaee7 and 3e9cbc6.

📒 Files selected for processing (9)
  • app/evmante/evmante_can_transfer.go (2 hunks)
  • eth/rpc/rpcapi/eth_api_test.go (1 hunks)
  • x/evm/keeper/msg_ethereum_tx_test.go (2 hunks)
  • x/evm/keeper/msg_server.go (5 hunks)
  • x/evm/msg.go (1 hunks)
  • x/evm/tx_data.go (2 hunks)
  • x/evm/tx_data_access_list.go (2 hunks)
  • x/evm/tx_data_dynamic_fee.go (3 hunks)
  • x/evm/tx_data_legacy.go (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • x/evm/keeper/msg_ethereum_tx_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
  • x/evm/keeper/msg_server.go
  • x/evm/msg.go
  • x/evm/tx_data.go
  • x/evm/tx_data_legacy.go
🧰 Additional context used
🔇 Additional comments (14)
app/evmante/evmante_can_transfer.go (6)

20-20: LGTM: Efficient use of pointer type for EVMKeeper

The change from EVMKeeper to *EVMKeeper is a good optimization. It allows for more efficient usage of the EVMKeeper and enables nil checks if needed.


41-43: LGTM: Improved base fee handling

The introduction of baseFeeWeiPerGas and its usage in the AsMessage call aligns well with the PR objective to use the effective gas price. The conversion from micronibi to wei ensures correct units are used throughout the transaction processing.


69-72: LGTM: Consistent EVM configuration

The updates to the EVM configuration initialization improve clarity and ensure consistent use of wei units for base fees. The expanded comment about the empty coinbase provides helpful context for future maintainers.


77-77: LGTM: Consistent EVMKeeper usage

The change from ctd.evmKeeper to ctd.EVMKeeper ensures consistent access to the EVMKeeper throughout the method, aligning with the earlier struct modification.


80-80: LGTM: Consistent EVMKeeper usage in EVM instance creation

The change from ctd.evmKeeper.NewEVM to ctd.EVMKeeper.NewEVM ensures consistent access to the EVMKeeper when creating a new EVM instance, aligning with the earlier struct modification.


Line range hint 1-97: Overall assessment: Well-implemented changes

The modifications in this file successfully address the PR objectives, particularly in improving the handling of base fees and ensuring consistent use of the EVMKeeper. The code quality has generally improved, with better clarity and robustness in fee validation and EVM configuration.

Key improvements:

  1. Efficient use of pointer type for EVMKeeper
  2. Consistent handling of base fees in wei units
  3. More robust fee validation process
  4. Clearer EVM configuration

Minor suggestions have been made for further improvements, but these are not critical. Great job on the implementation!

x/evm/tx_data_access_list.go (5)

188-190: LGTM: New method EffectiveGasFeeCapWei correctly implemented

The new method EffectiveGasFeeCapWei is a valuable addition that aligns with EIP-1559 logic. It correctly calculates the effective gas fee cap by taking the maximum of the base fee and the transaction's gas fee cap. This implementation ensures that the transaction always pays at least the base fee, which is crucial for proper fee market behavior.


293-296: LGTM: Method renamed for clarity and correctly implemented

The renaming of EffectiveGasPriceWei to EffectiveGasPriceWeiPerGas improves the method's clarity by explicitly stating that the price is "per gas". The implementation remains correct, ensuring that the effective gas price is at least the base fee. This change aligns well with the PR objective to use the effective gas price in RefundGas.


300-300: LGTM: Method updated to use new naming convention

The EffectiveFeeWei method has been correctly updated to use the renamed EffectiveGasPriceWeiPerGas method. This change maintains consistency with the earlier renaming while preserving the correct logic for calculating the effective fee.


303-307: LGTM: Method renamed and updated for EIP-1559 compatibility

The renaming of EffectiveCost to EffectiveCostWei improves clarity by specifying the unit. The implementation has been correctly updated to use EffectiveFeeWei instead of Fee, ensuring that the effective cost calculation takes into account the base fee. This change is crucial for EIP-1559 compatibility and aligns well with the PR objectives.


Line range hint 1-307: Summary: Excellent improvements for EIP-1559 compatibility

The changes in this file significantly enhance the gas price and fee calculations for AccessListTx, aligning them with EIP-1559 requirements. The introduction of the EffectiveGasFeeCapWei method, along with the renaming and updates to EffectiveGasPriceWeiPerGas, EffectiveFeeWei, and EffectiveCostWei, ensure proper handling of base fees and effective gas prices. These improvements directly contribute to the PR's objective of using effective gas prices in the RefundGas function, enhancing the overall robustness of the transaction processing logic.

eth/rpc/rpcapi/eth_api_test.go (1)

388-388: Approved: Assertion change aligns with expected behavior

The modification from Require().Errorf to Require().NoErrorf correctly reflects the expected behavior after the changes implemented in this PR. This assertion now verifies that a transaction receipt can be successfully retrieved after sending a transaction, which is consistent with the improved gas handling and transaction processing.

This change enhances the test's ability to confirm the proper functioning of the transaction lifecycle, particularly in relation to gas fee calculations and refunds.

x/evm/tx_data_dynamic_fee.go (2)

310-310: Update Function Call Consistency

The EffectiveFeeWei method correctly updates the call to EffectiveGasPriceWeiPerGas. The change maintains consistency with the renamed function.


313-314: Consistent Naming for EffectiveCostWei

Renaming EffectiveCost to EffectiveCostWei enhances clarity by explicitly indicating the unit of measure. The implementation correctly reflects this change.

Comment on lines +170 to +172
func (tx *DynamicFeeTx) EffectiveGasFeeCapWei(baseFeeWei *big.Int) *big.Int {
return BigIntMax(baseFeeWei, tx.GetGasFeeCapWei())
}
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

Potential Logical Error in EffectiveGasFeeCapWei Calculation

The function EffectiveGasFeeCapWei currently returns the maximum of baseFeeWei and tx.GetGasFeeCapWei(). However, according to EIP-1559 specifications, the gas fee cap (GasFeeCap) should serve as an upper limit on the gas price, not a minimum. By taking the maximum, this function might produce a gas fee higher than the specified cap, which could lead to unintended behavior.

Consider modifying the function to return the minimum of baseFeeWei and GasFeeCapWei to ensure that the effective gas fee cap does not exceed the user's specified maximum.

Apply this diff to correct the logic:

-func (tx *DynamicFeeTx) EffectiveGasFeeCapWei(baseFeeWei *big.Int) *big.Int {
-	return BigIntMax(baseFeeWei, tx.GetGasFeeCapWei())
+func (tx *DynamicFeeTx) EffectiveGasFeeCapWei(baseFeeWei *big.Int) *big.Int {
+	return gethmath.BigMin(baseFeeWei, tx.GetGasFeeCapWei())
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (tx *DynamicFeeTx) EffectiveGasFeeCapWei(baseFeeWei *big.Int) *big.Int {
return BigIntMax(baseFeeWei, tx.GetGasFeeCapWei())
}
func (tx *DynamicFeeTx) EffectiveGasFeeCapWei(baseFeeWei *big.Int) *big.Int {
return gethmath.BigMin(baseFeeWei, tx.GetGasFeeCapWei())
}

Copy link

codecov bot commented Oct 13, 2024

Codecov Report

Attention: Patch coverage is 72.53521% with 39 lines in your changes missing coverage. Please review.

Project coverage is 65.07%. Comparing base (62d4b1a) to head (b99631b).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
x/evm/msg.go 12.50% 7 Missing ⚠️
x/evm/keeper/msg_server.go 62.50% 4 Missing and 2 partials ⚠️
x/evm/tx_data_access_list.go 25.00% 6 Missing ⚠️
app/evmante/evmante_can_transfer.go 61.53% 3 Missing and 2 partials ⚠️
app/evmante/evmante_mempool_fees.go 66.66% 1 Missing and 1 partial ⚠️
eth/rpc/backend/tx_info.go 66.66% 2 Missing ⚠️
eth/rpc/rpcapi/websockets.go 0.00% 2 Missing ⚠️
x/evm/tx_data_dynamic_fee.go 60.00% 2 Missing ⚠️
x/evm/tx_data_legacy.go 60.00% 2 Missing ⚠️
app/ante/gas.go 0.00% 1 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2076      +/-   ##
==========================================
+ Coverage   65.06%   65.07%   +0.01%     
==========================================
  Files         270      270              
  Lines       17068    17091      +23     
==========================================
+ Hits        11105    11122      +17     
- Misses       5023     5030       +7     
+ Partials      940      939       -1     
Files with missing lines Coverage Δ
app/evmante/evmante_emit_event.go 100.00% <100.00%> (ø)
app/evmante/evmante_gas_consume.go 69.56% <100.00%> (ø)
app/evmante/evmante_handler.go 100.00% <100.00%> (ø)
app/evmante/evmante_increment_sender_seq.go 70.96% <100.00%> (ø)
app/evmante/evmante_setup_ctx.go 66.66% <100.00%> (ø)
app/evmante/evmante_sigverify.go 100.00% <100.00%> (ø)
app/evmante/evmante_verify_eth_acc.go 93.10% <100.00%> (ø)
eth/rpc/backend/backend.go 85.71% <ø> (ø)
eth/rpc/backend/blocks.go 52.09% <100.00%> (ø)
eth/rpc/backend/utils.go 65.98% <100.00%> (ø)
... and 22 more

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

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3e9cbc6 and b99631b.

📒 Files selected for processing (3)
  • e2e/evm/test/contract_send_nibi.test.ts (2 hunks)
  • e2e/evm/test/native_transfer.test.ts (3 hunks)
  • justfile (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • e2e/evm/test/contract_send_nibi.test.ts
  • e2e/evm/test/native_transfer.test.ts
🧰 Additional context used
🔇 Additional comments (2)
justfile (2)

50-60: LGTM! Improvements implemented as suggested.

The log-clear recipe has been implemented with proper error handling and user feedback. It checks for the existence of the logs directory and its contents before attempting to clear it, providing appropriate messages for different scenarios.


50-80: Summary of justfile changes

The new recipes added to the justfile align well with the PR objectives of introducing commands for improved usability, particularly for logging local network outputs and running end-to-end tests. These additions will enhance the development workflow and debugging capabilities.

While the basic functionality is implemented, there are opportunities to improve error handling and consistency across the new recipes. Implementing the suggested changes will further enhance the robustness and user-friendliness of these commands.

Overall, these changes contribute positively to the project's development tooling and align with the stated objectives of the pull request.

Comment on lines +61 to +65
# Runs "just localnet" with logging (logs/localnet.txt)
log-localnet:
#!/usr/bin/env bash
mkdir -p logs
just localnet 2>&1 | tee -a logs/localnet.txt
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

Enhance error handling in the log-localnet recipe.

While the recipe implements the basic functionality, it could benefit from improved error handling as suggested in the previous review. Consider updating the recipe to handle potential errors when creating the logs directory:

 log-localnet:
   #!/usr/bin/env bash
-  mkdir -p logs
+  if ! mkdir -p logs; then
+    echo "Error: Failed to create logs directory." >&2
+    exit 1
+  fi
   just localnet 2>&1 | tee -a logs/localnet.txt

This change will ensure that the script exits with an error message if it fails to create the logs directory.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Runs "just localnet" with logging (logs/localnet.txt)
log-localnet:
#!/usr/bin/env bash
mkdir -p logs
just localnet 2>&1 | tee -a logs/localnet.txt
# Runs "just localnet" with logging (logs/localnet.txt)
log-localnet:
#!/usr/bin/env bash
if ! mkdir -p logs; then
echo "Error: Failed to create logs directory." >&2
exit 1
fi
just localnet 2>&1 | tee -a logs/localnet.txt

Comment on lines +67 to +70
# Runs the EVM E2E test with logging (logs/e2e.txt)
log-e2e:
#!/usr/bin/env bash
just test-e2e 2>&1 | tee -a logs/e2e.txt
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

Improve consistency and error handling in the log-e2e recipe.

The recipe implements the basic functionality, but there are opportunities for improvement:

  1. The log file path is inconsistent with the log-localnet recipe.
  2. It doesn't ensure the existence of the logs directory.

Consider updating the recipe as follows:

 log-e2e:
   #!/usr/bin/env bash
-  just test-e2e 2>&1 | tee -a logs/e2e.txt
+  if ! mkdir -p logs; then
+    echo "Error: Failed to create logs directory." >&2
+    exit 1
+  fi
+  just test-e2e 2>&1 | tee -a logs/e2e.txt

This change will:

  1. Use a consistent log file path with other recipes.
  2. Ensure the logs directory exists before running the command.
  3. Provide error handling if directory creation fails.

Committable suggestion was skipped due to low confidence.

Comment on lines +72 to +80
# Runs the EVM E2E tests
test-e2e:
#!/usr/bin/env bash
source contrib/bashlib.sh
log_info "Make sure the localnet is running! (just localnet)"

cd e2e/evm
just test

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

Enhance error handling and provide more context in the test-e2e recipe.

While the recipe implements the basic functionality, it could benefit from improved error handling and more informative logging:

Consider updating the recipe as follows:

 test-e2e:
   #!/usr/bin/env bash
   source contrib/bashlib.sh
   log_info "Make sure the localnet is running! (just localnet)"
 
-  cd e2e/evm
-  nvm use
-  just test
+  if ! cd e2e/evm; then
+    log_error "Failed to change directory to e2e/evm. Aborting."
+    exit 1
+  fi
+
+  if ! command -v nvm &> /dev/null; then
+    log_error "nvm is not installed. Please install nvm and try again."
+    exit 1
+  fi
+
+  if ! nvm use; then
+    log_error "Failed to switch Node.js version using nvm. Aborting."
+    exit 1
+  fi
+
+  log_info "Running E2E tests..."
+  if ! just test; then
+    log_error "E2E tests failed."
+    exit 1
+  fi
+
+  log_success "E2E tests completed successfully."

These changes will:

  1. Handle potential errors when changing directory.
  2. Check if nvm is installed before using it.
  3. Handle potential errors when switching Node.js versions.
  4. Provide more informative logging throughout the process.
  5. Handle potential test failures and provide appropriate exit codes.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Runs the EVM E2E tests
test-e2e:
#!/usr/bin/env bash
source contrib/bashlib.sh
log_info "Make sure the localnet is running! (just localnet)"
cd e2e/evm
just test
# Runs the EVM E2E tests
test-e2e:
#!/usr/bin/env bash
source contrib/bashlib.sh
log_info "Make sure the localnet is running! (just localnet)"
if ! cd e2e/evm; then
log_error "Failed to change directory to e2e/evm. Aborting."
exit 1
fi
if ! command -v nvm &> /dev/null; then
log_error "nvm is not installed. Please install nvm and try again."
exit 1
fi
if ! nvm use; then
log_error "Failed to switch Node.js version using nvm. Aborting."
exit 1
fi
log_info "Running E2E tests..."
if ! just test; then
log_error "E2E tests failed."
exit 1
fi
log_success "E2E tests completed successfully."

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.

[bug] Make RefundGas safer (potential issue)
1 participant