chore(dist-prec): convert balance change entries #2929
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: 1
🧹 Outside diff range and nitpick comments (6)
precompiles/distribution/tx.go (5)
95-96: LGTM! Consider extracting the conversion to a helper function.The conversion of the amount to 18 decimal places ensures consistency in balance changes recorded in the EVM state database. This change aligns well with the PR objective.
Consider extracting the conversion logic to a helper function, as it's likely to be reused in other methods:
func convertTo18Decimals(coins sdk.Coins) *big.Int { return evmtypes.ConvertAmountTo18DecimalsBigInt(coins.AmountOf(evmtypes.GetEVMCoinDenom()).BigInt()) }This would improve code readability and maintainability.
175-176: LGTM! Consider using the proposed helper function.The conversion of the amount to 18 decimal places ensures consistency in balance changes recorded in the EVM state database. This change aligns well with the PR objective.
If you implement the helper function suggested in the previous comment, you could simplify this code to:
convertedAmount := convertTo18Decimals(res.Amount) p.SetBalanceChangeEntries(cmn.NewBalanceChangeEntry(withdrawerHexAddr, convertedAmount, cmn.Add))This would improve code consistency and readability across all methods.
222-223: LGTM! Consider using the proposed helper function.The conversion of the amount to 18 decimal places ensures consistency in balance changes recorded in the EVM state database. This change aligns well with the PR objective.
If you implement the helper function suggested earlier, you could simplify this code to:
convertedAmount := convertTo18Decimals(res.Amount) p.SetBalanceChangeEntries(cmn.NewBalanceChangeEntry(withdrawerHexAddr, convertedAmount, cmn.Add))This would improve code consistency and readability across all methods.
264-265: LGTM! Consider using the proposed helper function. Correct use of 'Sub' noted.The conversion of the amount to 18 decimal places ensures consistency in balance changes recorded in the EVM state database. This change aligns well with the PR objective.
If you implement the helper function suggested earlier, you could simplify this code to:
convertedAmount := convertTo18Decimals(msg.Amount) p.SetBalanceChangeEntries(cmn.NewBalanceChangeEntry(depositorHexAddr, convertedAmount, cmn.Sub))This would improve code consistency and readability across all methods.
Note: The correct use of
cmn.Subhere is appreciated, as it accurately reflects that funds are being removed from the depositor's account when funding the community pool.
Line range hint
1-280: Overall LGTM! Consider creating a helper function for amount conversion.The changes consistently apply the conversion to 18 decimal places across different methods, which aligns well with the PR objective of converting balance change entries. This improves consistency in handling balance changes in the EVM state database.
To further improve code readability and maintainability, consider creating a helper function for the amount conversion:
func convertTo18Decimals(coins sdk.Coins) *big.Int { return evmtypes.ConvertAmountTo18DecimalsBigInt(coins.AmountOf(evmtypes.GetEVMCoinDenom()).BigInt()) }This function could be used across all methods, reducing code duplication and improving consistency.
CHANGELOG.md (1)
53-53: Consider breaking down long line for better readabilityThis line exceeds the recommended maximum line length of 120 characters. Consider breaking it into multiple lines for improved readability.
- - (precompiles) [#2929](https://github.com/evmos/evmos/pull/2929) Distribution: scale balance change entries to the statedb journal to support different evm denom precision. + - (precompiles) [#2929](https://github.com/evmos/evmos/pull/2929) Distribution: scale balance change entries + to the statedb journal to support different evm denom precision.🧰 Tools
🪛 Markdownlint
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- precompiles/distribution/tx.go (4 hunks)
🧰 Additional context used
🪛 Markdownlint
CHANGELOG.md
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
🔇 Additional comments (13)
CHANGELOG.md (13)
55-56: Improvement in performance of eth_headerByHash RPC methodThis improvement likely enhances the efficiency of retrieving block headers by hash, which can be beneficial for applications that frequently query historical block data.
Line range hint
58-58: Fix for communityPool being nil in inflation moduleThis bug fix prevents potential issues with the inflation module when the community pool is not initialized properly.
🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
Line range hint
59-59: Removal of duplicate precompile address definitionsThis improvement in the EVM module helps maintain cleaner and more maintainable code by eliminating redundant definitions.
🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
Line range hint
60-60: Minor improvements in precompiles related codeThese improvements likely enhance the overall quality and efficiency of the precompiles implementation.
🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
Line range hint
61-61: CLI command update for pruningThe change from
PruningCmdtoCmdsuggests an update in the CLI interface for the pruning functionality, possibly aligning with newer standards or improving usability.🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
Line range hint
62-62: Removal of unnecessary fee ante handlerThis change simplifies the ante handler logic by removing a redundant fee handler and using the Cosmos SDK one instead. This can lead to more consistent fee handling across the application.
🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
Line range hint
63-63: Repository version bump to v20This indicates a major version update for the application, which likely includes significant changes and improvements.
🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
Line range hint
64-64: Fix for testnet initialization commandThis bug fix ensures that the
evmosd testnet init-filescommand correctly handles validator addresses, improving the reliability of testnet setups.🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
Line range hint
65-65: Improvement in EVM gas estimationThis change caps the highest gas limit with the account's available balance, which can prevent issues with gas estimation for accounts with insufficient funds.
🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
Line range hint
1-1137: Comprehensive changelog demonstrating significant project evolutionThis changelog provides a detailed account of the Evmos project's development across multiple versions. Key observations:
- Frequent updates addressing various aspects of the system, including state machine changes, API improvements, and bug fixes.
- Consistent focus on enhancing EVM compatibility and interoperability.
- Regular dependency updates, keeping the project aligned with the latest developments in the Cosmos ecosystem.
- Attention to performance improvements and security fixes.
- Continuous refinement of CLI tools and developer experience.
The changes documented here reflect a mature project with a strong commitment to improvement and adaptation to the evolving blockchain landscape. Developers and node operators should pay close attention to state machine breaking changes and upgrade their systems accordingly.
🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
Line range hint
57-57: Update to Interpreter interface for custom Opcodes supportThis change in the EVM module allows for more flexibility in implementing custom opcodes. This can be powerful for blockchain-specific optimizations or features.
To ensure this change is properly implemented and doesn't introduce security risks, please run the following verification:
#!/bin/bash # Check for changes in the EVM module related to the Interpreter grep -R "Interpreter" ./x/evm --include="*.go" # Look for any new or modified opcodes grep -R "OpCode" ./x/evm --include="*.go"🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
Line range hint
66-66: Introduction of BankWrapper for managing EVM coins with different decimalsThis significant change allows for more flexible handling of EVM coins with varying decimal places, which can be crucial for interoperability with different token standards.
To ensure this change is properly implemented and doesn't introduce unintended consequences, please run the following verification:
#!/bin/bash # Check for the implementation of BankWrapper grep -R "BankWrapper" . --include="*.go" --exclude-dir={vendor,build} # Look for usage of BankWrapper in other parts of the codebase grep -R "bankWrapper" . --include="*.go" --exclude-dir={vendor,build}🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
Line range hint
68-75: Critical bug fixes across multiple modulesThis section includes several important bug fixes:
- Update to Ledger-supported algorithms for EthSecp256k1.
- Adjustment of default node snapshot interval.
- Fix for the
evmosd versioncommand output.- Correction of snapshot configuration.
- Proper setup of gRPC node service.
- Fix for unbound metrics and removal of continuously increasing labels.
These fixes address various issues ranging from hardware wallet support to system configuration and monitoring. They contribute to the overall stability and reliability of the Evmos network.
To ensure these bug fixes are properly implemented and don't introduce new issues, please run the following verification:
#!/bin/bash # Check for changes related to Ledger support grep -R "Ledger" . --include="*.go" --exclude-dir={vendor,build} # Look for snapshot-related configurations grep -R "snapshot" . --include="*.go" --include="*.toml" --exclude-dir={vendor,build} # Check for gRPC service setup grep -R "grpc.NewServer" . --include="*.go" --exclude-dir={vendor,build} # Investigate metrics-related changes grep -R "prometheus" . --include="*.go" --exclude-dir={vendor,build}🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
53-53: Consider breaking long line into multiple linesThis line exceeds the recommended maximum line length of 120 characters. Consider breaking it into multiple lines for better readability.
You could split the line like this:
- (precompiles) [#2929](https://github.com/evmos/evmos/pull/2929) Distribution: scale balance change entries to the statedb journal to support different evm denom precision.🧰 Tools
🪛 Markdownlint
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- CHANGELOG.md (1 hunks)
🧰 Additional context used
🪛 Markdownlint
CHANGELOG.md
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
🔇 Additional comments (2)
CHANGELOG.md (2)
Line range hint
1-53: LGTM: Well-documented changelog entriesThe changelog entries for the unreleased version are well-organized and provide clear information about the changes. They are categorized into "State Machine Breaking", "Improvements", and "Bug Fixes", which helps users and developers understand the impact of the changes.
🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
Line range hint
1-1165: LGTM: Well-structured and informative changelogThe CHANGELOG.md file is well-structured and provides a comprehensive history of changes for the project. It follows best practices by:
- Organizing entries by version, with the most recent at the top.
- Including release dates for each version.
- Categorizing changes (e.g., "State Machine Breaking", "API Breaking", "Improvements", "Bug Fixes").
- Providing links to relevant pull requests and issues.
This format makes it easy for users and developers to understand the project's evolution and the impact of each release.
🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
There was a problem hiding this comment.
Good job @ramacarlucho! It is not part of the decimals but could you also remove this line from the precompiles to avoid keeping unused vars?
evmos/precompiles/distribution/distribution.go
Lines 20 to 21 in f93a227
Now all prec addresses are handled here:
evmos/x/evm/types/precompiles.go
Lines 11 to 18 in f93a227
|
The only change that we can do is checking if the amount |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
53-53: Consider breaking down long changelog entriesThis changelog entry is quite long and contains multiple changes. Consider breaking it down into smaller, more focused entries for better readability.
For example, you could split it into separate entries for each affected component (precompiles, distribution, etc.):
- (precompiles) [#2929](https://github.com/evmos/evmos/pull/2929) Distribution: scale balance change entries to the statedb journal to support different EVM denom precision. - (distribution) [#2929](https://github.com/evmos/evmos/pull/2929) Add support for different EVM denom precision in balance change entries.🧰 Tools
🪛 Markdownlint
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- CHANGELOG.md (1 hunks)
🧰 Additional context used
🪛 Markdownlint
CHANGELOG.md
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
🔇 Additional comments (3)
CHANGELOG.md (3)
57-59: LGTM: Improvements section looks goodThe improvements section is well-structured and provides clear information about the changes made to the precompiles, RPC, and EVM modules.
🧰 Tools
🪛 Markdownlint
59-59: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
1-63: Overall changelog reviewThe CHANGELOG.md file provides a comprehensive overview of the changes in the upcoming release. It covers important aspects such as state machine breaking changes, improvements, and bug fixes across various modules of the Evmos project.
Some suggestions for improvement:
- Consider breaking down long entries into more focused, smaller entries for better readability.
- Ensure consistency in the level of detail provided for each change.
- Double-check that all significant changes are properly documented and categorized.
Overall, the changelog is informative and well-structured, providing valuable information for users and developers about the evolving state of the Evmos project.
🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
59-59: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
61-63: Verify the impact of bug fixesThe bug fixes section includes changes to the CLI and dependencies. These seem important for the correct functioning of the software.
Please ensure that these bug fixes have been thoroughly tested, especially the change in the default node snapshot interval. You can verify this by running the following command:
🧰 Tools
🪛 Markdownlint
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
59-59: Expected: 120; Actual: 136
Line length(MD013, line-length)
Signed-off-by: Ramiro Carlucho <ramirocarlucho@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
53-54: Consider breaking long lines for better readabilityThe lines describing the distribution precompile changes are quite long (173 and 166 characters respectively). Consider breaking these lines into multiple lines for better readability. For example:
- (precompiles) [#2929](https://github.com/evmos/evmos/pull/2929) Distribution: scale balance change entries to the statedb journal to support different EVM denom precision. - (precompiles) [#2927](https://github.com/evmos/evmos/pull/2927) Erc20: scale balance change entries to the statedb journal to support different EVM denom precision.This change would improve the overall readability of the changelog.
🧰 Tools
🪛 Markdownlint
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
54-54: Expected: 120; Actual: 166
Line length(MD013, line-length)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- CHANGELOG.md (1 hunks)
🧰 Additional context used
🪛 Markdownlint
CHANGELOG.md
56-56: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
54-54: Expected: 120; Actual: 166
Line length(MD013, line-length)
🔇 Additional comments (4)
CHANGELOG.md (4)
Line range hint
59-65: Significant State Machine Breaking ChangesThis version includes several critical state machine breaking changes:
- Dependency updates (IBC-Go, Cosmos-SDK)
- Removal of the
x/recoverymodule- Addition of the
bankprecompile- Removal of the
x/incentivesmodule and burning of the incentives pool balance- Removal of
x/claimsparams and migration ofEVMChannelsparam- Addition of
BurnDecoratortoPostHandler- Reduction of inflation by 2/3
These changes will require careful planning and coordination during the upgrade process. Ensure that all validators and node operators are aware of these changes and have updated their software accordingly.
🧰 Tools
🪛 Markdownlint
56-56: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
54-54: Expected: 120; Actual: 166
Line length(MD013, line-length)
61-61: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
67-93: Notable ImprovementsThis version introduces several important improvements:
- Addition of
VoteWeightedtransaction to the governance precompile- Performance enhancements for
eth_headerByHash- Support for custom Opcodes in the
Interpreterinterface- Refactoring of Stride and Osmosis outposts
- Addition of channel selector based on ChainID
- Updates to various CLI commands and configurations
These improvements enhance the functionality, performance, and developer experience of the Evmos platform. The refactoring and optimizations should lead to better overall system efficiency.
🧰 Tools
🪛 Markdownlint
56-56: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
54-54: Expected: 120; Actual: 166
Line length(MD013, line-length)
61-61: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
95-99: Important Bug FixesThis version addresses three significant bugs:
- Fixed the governance proposals query by removing proposals related to the deprecated
x/incentivesmodule- Removed the deprecated
cosmos.params.v1beta1/ParameterChangeProposalhandler- Fixed an issue preventing users from sending transactions with gasPrice of 0 to precompiled contracts
These bug fixes improve the stability and correctness of the Evmos platform. Users and developers should be aware of these changes, especially those interacting with governance proposals or precompiled contracts.
🧰 Tools
🪛 Markdownlint
56-56: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
54-54: Expected: 120; Actual: 166
Line length(MD013, line-length)
61-61: Expected: 120; Actual: 136
Line length(MD013, line-length)
Line range hint
1-99: Well-structured and Informative ChangelogThe CHANGELOG.md file is well-organized and provides clear, detailed information about the changes in each version of Evmos. Key points:
- Consistent formatting across versions
- Clear categorization of changes (State Machine Breaking, Improvements, Bug Fixes)
- Links to corresponding GitHub pull requests for each change
- Chronological order with the most recent version at the top
This structure makes it easy for users, developers, and node operators to understand the changes and their potential impact. It's particularly helpful for planning upgrades and understanding the evolution of the Evmos platform.
To further improve the changelog:
- Consider adding a brief summary at the top of each version, highlighting the most critical changes
- For major versions, consider adding upgrade instructions or links to upgrade guides
Overall, this changelog is a valuable resource for the Evmos community.
🧰 Tools
🪛 Markdownlint
56-56: Expected: 1; Actual: 2
Multiple consecutive blank lines(MD012, no-multiple-blanks)
51-51: Expected: 120; Actual: 175
Line length(MD013, line-length)
53-53: Expected: 120; Actual: 173
Line length(MD013, line-length)
54-54: Expected: 120; Actual: 166
Line length(MD013, line-length)
61-61: Expected: 120; Actual: 136
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
EthAccountwithBaseAccount.eth_estimateGas.x/feemarketto use Decimal for BaseFee management.eth_headerByHash.