chore(evm): add scaling on buildTx fees #2906
Conversation
WalkthroughThe pull request updates the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/evm/types/msg_test.go (2)
95-96: Suggestion: Define constants for large numeric values for clarityIn lines 95-96,
GasPriceandGasFeeCapare set tobig.NewInt(1e18). Defining a constant for1e18(which represents 1 Ether in Wei) can improve readability and maintainability.Here's how you might apply this:
+const OneEtherInWei = 1e18 ... evmTx := &types.EvmTxArgs{ Nonce: 0, To: &suite.to, GasLimit: 100000, - GasPrice: big.NewInt(1e18), - GasFeeCap: big.NewInt(1e18), + GasPrice: big.NewInt(OneEtherInWei), + GasFeeCap: big.NewInt(OneEtherInWei), GasTipCap: big.NewInt(0), Input: []byte("test"), }
140-145: Suggestion: Use constants for scaling factors to enhance readabilityIn lines 143-144, the hardcoded value
1e12is used to scaleexpFeeAmt. Defining a constant for this scaling factor improves code clarity and maintainability.Here's how you might apply this:
+const ScaleFactor = 1e12 ... if cfg.Decimals == types.SixDecimals { - scaledAmt := expFeeAmt.QuoRaw(1e12) + scaledAmt := expFeeAmt.QuoRaw(ScaleFactor) expFee = sdk.NewCoins(sdk.NewCoin(baseDenom, scaledAmt)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- x/evm/types/msg.go (1 hunks)
- x/evm/types/msg_test.go (3 hunks)
🔇 Additional comments (5)
x/evm/types/msg.go (1)
344-348: Verify the impact of fee conversion from 18 decimalsThe changes in the
BuildTxmethod include an optimization for thefeesslice initialization and a new step to convert fees from 18 decimals. While the optimization is a good practice, the fee conversion requires careful consideration:
The
feesslice initialization with a capacity of 1 is a minor optimization that slightly improves memory allocation efficiency.The new line
fees = ConvertCoinsFrom18Decimals(fees)could significantly impact fee calculations. This change assumes that the input fees are in 18 decimal places and converts them to a different precision.Please confirm the following:
- Is this conversion necessary for all cases where
BuildTxis called?- Are there any scenarios where fees might already be in the correct decimal precision?
- How does this change align with the rest of the system's fee handling?
To verify the impact and usage of this new conversion, you can run the following script:
This will help ensure consistency in fee handling across the codebase and identify any potential issues with the new conversion step.
✅ Verification successful
Fee conversion from 18 decimals is consistently implemented across the codebase.
The introduction of
ConvertCoinsFrom18Decimalsin theBuildTxmethod aligns with its usage in other modules, ensuring uniform fee handling and accurate fee calculations throughout the system.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of fee handling in the codebase echo "Searching for fee handling occurrences:" rg --type go -i '(fee|fees).*decimal' echo "\nSearching for ConvertCoinsFrom18Decimals usage:" rg --type go 'ConvertCoinsFrom18Decimals' echo "\nSearching for BuildTx method calls:" rg --type go 'BuildTx'Length of output: 4029
CHANGELOG.md (4)
Line range hint
70-76: State Machine Breaking changes require careful considerationThe v16.0.0 release introduces several state machine breaking changes:
- Addition of secp256r1 curve precompile (RIP-7212)
- New ClaimRewards custom transaction in the distribution precompile
- Addition of bech32 conversion precompile
- Implementation of CreateValidator function in the staking precompile
- Addition of bank precompile
- Reduction of inflation by 2/3
These changes will require careful planning for the upgrade process and may impact existing applications. Ensure all validators and node operators are aware of these changes and their potential implications.
🧰 Tools
🪛 Markdownlint
68-68: Expected: 120; Actual: 150
Line length(MD013, line-length)
Line range hint
78-81: API Breaking changes require updates to dependent applicationsThe v16.0.0 release includes two API breaking changes:
- Renaming of the
inflationmodule toinflation/v1- Deprecation of the legacy EIP-712 ante handler
Developers of applications or tools that interact with Evmos should update their code to reflect these changes. In particular, any code that references the inflation module or uses the legacy EIP-712 ante handler will need to be modified.
🧰 Tools
🪛 Markdownlint
68-68: Expected: 120; Actual: 150
Line length(MD013, line-length)
Line range hint
83-124: Significant improvements across multiple areasThe v16.0.0 release includes a wide range of improvements:
- Enhanced testing: Lint tests for consistency, add integration tests for precompile transactions.
- CI improvements: Added Golang dependency vulnerability checker.
- New features: Added bank precompile, implemented CreateValidator function for staking precompile.
- Performance optimizations: Improved EVM extensions and transaction handling.
- Documentation updates: Added audits page, updated Swagger configuration.
- Dependency updates: Bumped various dependencies including IBC-Go and Cosmos-SDK.
These improvements enhance the overall quality and functionality of the Evmos project. Developers should review the changes relevant to their work, particularly new features like the bank precompile and staking precompile improvements.
🧰 Tools
🪛 Markdownlint
68-68: Expected: 120; Actual: 150
Line length(MD013, line-length)
Line range hint
126-132: Important bug fixes across multiple modulesThe v16.0.0 release includes several critical bug fixes:
- Fixed Ledger supported algorithms to only include
EthSecp256k1.- Updated default node snapshot interval to 5000.
- Fixed governance proposals query by removing deprecated incentives module proposals.
- Corrected Swagger configuration to use the right version of proto dependencies.
- Fixed an issue in the revenue module allowing transactions with 0 gasPrice to precompiled contracts.
These fixes address important issues that could have affected system functionality and user experience. Users and developers should take note of these resolved issues, particularly if they were impacted by any of these bugs in previous versions.
🧰 Tools
🪛 Markdownlint
68-68: Expected: 120; Actual: 150
Line length(MD013, line-length)
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
Reviewers Checklist
All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.
I have...
Unreleasedsection inCHANGELOG.mdSummary by CodeRabbit
Improvements
eth_headerByHashRPC call.Bug Fixes
evmosd testnet init-filescommand.addGrantcommand.Documentation