chore(vesting-precompile): scale balanceChange entry amt #2928
Conversation
WalkthroughThe pull request updates the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (2)
CHANGELOG.md (1)
75-75: LGTM! Comprehensive changelog updates.The changelog updates for the "Unreleased" section are well-structured and informative. They cover important categories such as State Machine Breaking changes, Improvements, and Bug Fixes across various modules. This level of detail is crucial for tracking the project's evolution and helping users understand the impact of changes.
Consider breaking the long line (160 characters) into multiple lines for better readability. The general recommendation is to keep lines under 120 characters.
🧰 Tools
🪛 Markdownlint
75-75: Expected: 120; Actual: 160
Line length(MD013, line-length)
precompiles/vesting/tx.go (1)
195-200: Ensure Consistent Conditional Structure Across MethodsIn the
Clawbackmethod, the conditional check combinesisContractCallerandevmDenomAmt.IsPositive()in a singleifstatement:if isContractCaller && evmDenomAmt.IsPositive() { // ... }In contrast, the
FundVestingAccountmethod nests these conditions:if isContractCaller { // ... if evmDenomAmt.IsPositive() { // ... } }For consistency and readability, consider structuring the conditional checks similarly in both methods. This can help prevent confusion and potential maintenance issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- precompiles/vesting/tx.go (2 hunks)
🧰 Additional context used
🪛 Markdownlint
CHANGELOG.md
75-75: Expected: 120; Actual: 160
Line length(MD013, line-length)
🔇 Additional comments (2)
precompiles/vesting/tx.go (2)
126-135: LGTM: Correct Handling of EVM Balance UpdatesThe addition of checking
evmDenomAmt.IsPositive()before scaling and updating the EVM balance ensures that only positive amounts are processed, preventing potential errors related to negative balances. This change improves the robustness of theFundVestingAccountmethod.
195-200: LGTM: Appropriate EVM Balance Update in ClawbackThe updated logic correctly checks if
isContractCalleristrueandevmDenomAmtis positive before scaling the amount and updating the EVM balance. This ensures that the EVM state database accurately reflects the changes made during theClawbackoperation when called from a contract.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
CHANGELOG.md (4)
76-76: Consider breaking long line for better readabilityThe line describing the inflation reduction is quite long (160 characters). While this is not a strict rule for changelogs, consider breaking it into multiple lines for better readability:
- (inflation) [#2137](https://github.com/evmos/evmos/pull/2137) Reduce inflation by 2/3. This change significantly alters the token emission rate, which may impact staking rewards and overall token economics.This suggestion maintains the essential information while improving readability.
🧰 Tools
🪛 Markdownlint
76-76: Expected: 120; Actual: 160
Line length(MD013, line-length)
Line range hint
12-23: Consider adding more context to some entriesSome entries in the State Machine Breaking section could benefit from additional context to help users understand the impact of the changes. For example:
For the entry about the
secp256r1curve precompile, consider adding a brief explanation of its purpose or impact:- (p256-precompile) [#1922](https://github.com/evmos/evmos/pull/1922) Implement [RIP-7212](https://github.com/ethereum/RIPs/blob/master/RIPS/rip-7212.md) `secp256r1` curve precompile. This addition enhances cryptographic capabilities, allowing for more efficient operations on the secp256r1 elliptic curve.For the entry about restricting transaction fee tokens, consider explaining the reasoning or impact:
- (fees) [#1998](https://github.com/evmos/evmos/pull/1998) Restrict transaction fee tokens. This change aims to standardize fee payments and may affect how users interact with the network.Adding this context can help users better understand the significance of these changes.
🧰 Tools
🪛 Markdownlint
73-73: Expected: 120; Actual: 126
Line length(MD013, line-length)
76-76: Expected: 120; Actual: 160
Line length(MD013, line-length)
Line range hint
38-73: Consider grouping related improvements and adding contextThe Improvements section contains a variety of changes. Consider grouping related items and adding context to some entries for better organization and clarity:
Group outpost-related improvements:
Outpost Improvements: - (stride-outpost) [#1912](https://github.com/evmos/evmos/pull/1912) Add Stride outpost interface and ABI. - (stride-outpost) [#1913](https://github.com/evmos/evmos/pull/1913) Add Run function, precompile struct and tx definitions. - (stride-outpost) [#1914](https://github.com/evmos/evmos/pull/1914) Add types, events and common util function. - (osmosis-outpost) [#1915](https://github.com/evmos/evmos/pull/1915) Add Osmosis outpost interface and ABI. - (osmosis-outpost) [#2011](https://github.com/evmos/evmos/pull/2011) Update outpost swap ABI to return IBC next sequence.Add context to significant changes:
- (app) [#1842](https://github.com/evmos/evmos/pull/1842) Add support for [MemIAVL](https://github.com/crypto-org-chain/cronos/wiki/MemIAVL) and [versionDB](https://github.com/crypto-org-chain/cronos/blob/main/versiondb/README.md). This improvement enhances data storage and retrieval efficiency. - (evm) [#2172](https://github.com/evmos/evmos/pull/2172) Add channel selector from chain id. This change improves cross-chain communication by allowing dynamic channel selection based on the chain ID.Clarify the impact of some changes:
- (inflation) [#2137](https://github.com/evmos/evmos/pull/2137) Reduce inflation by 2/3. This significant change to the token emission rate may impact staking rewards and overall token economics.These suggestions aim to improve the changelog's readability and provide more context for users.
🧰 Tools
🪛 Markdownlint
73-73: Expected: 120; Actual: 126
Line length(MD013, line-length)
76-76: Expected: 120; Actual: 160
Line length(MD013, line-length)
75-77: Consider adding more context to bug fix descriptionsThe bug fix descriptions are concise, but adding a bit more context could help users understand the impact of these fixes. Consider expanding the descriptions as follows:
For the app bug fix:
- (app) [#1165](https://github.com/evmos/evmos/pull/1165) Update Ledger supported algorithms to only consist of `EthSecp256k1`. This fix ensures compatibility with Ledger hardware wallets and prevents potential issues with unsupported signing algorithms.For the CLI bug fix:
- (cli) [#1172](https://github.com/evmos/evmos/pull/1172) Update default node snapshot interval to `5000`. This change optimizes the frequency of node state snapshots, balancing between data safety and performance.For the revenue module bug fix:
- (revenue) [#2032](https://github.com/evmos/evmos/pull/2032) Fixed the problem that users cannot send transactions with gasPrice of 0 to precompiled contracts. This fix allows for more flexible gas pricing options when interacting with precompiled contracts.These expanded descriptions provide more context about the nature and impact of each bug fix, which can be helpful for users and developers.
🧰 Tools
🪛 Markdownlint
76-76: Expected: 120; Actual: 160
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
76-76: Expected: 120; Actual: 160
Line length(MD013, line-length)
🔇 Additional comments (1)
CHANGELOG.md (1)
Line range hint
1-2930: Overall, the CHANGELOG.md is well-maintained and informativeThis changelog provides a comprehensive record of changes across multiple versions of the project. It follows good practices by categorizing changes, linking to pull requests, and providing descriptions for each entry. The inclusion of both major and minor versions helps users track the project's evolution over time.
For future changelog entries, consider the following suggestions to further improve its usefulness:
Maintain consistency in the level of detail provided for each entry. Some entries are very detailed, while others are quite brief. Aim for a balance that provides enough context without being overly verbose.
For significant changes, especially those marked as "State Machine Breaking" or "API Breaking", consider adding a brief note about potential impacts on users or developers.
Group related changes together, particularly when there are multiple entries for the same module or feature. This can help readers quickly understand the scope of changes in specific areas.
For bug fixes, when possible, include a brief mention of the impact the bug had and how the fix improves the system.
Consider adding a "Deprecations" or "Upcoming Changes" section for major versions to help users prepare for future changes.
These suggestions can help make the changelog even more valuable as a reference for users, developers, and maintainers of the project.
🧰 Tools
🪛 Markdownlint
73-73: Expected: 120; Actual: 126
Line length(MD013, line-length)
76-76: Expected: 120; Actual: 160
Line length(MD013, line-length)
There was a problem hiding this comment.
Great @GAtom22!
Could you also please remove the unused precompile address?
evmos/precompiles/vesting/vesting.go
Lines 24 to 25 in e98be79
The one used should be this one now:
evmos/x/evm/types/precompiles.go
Lines 11 to 18 in e98be79
|
Nice! 🚀 |
…to GAtom22/vest-prec
Signed-off-by: Tom <54514587+GAtom22@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
CHANGELOG.md (2)
77-77: Consider breaking long line for better readabilityThe line describing the vesting precompile change is quite long (160 characters). Consider breaking it into multiple lines for improved readability.
You could format it like this:
- (precompiles) [#2928](https://github.com/evmos/evmos/pull/2928) Vesting: Add amount scaling on `balanceChange` entry to support different evm denom precision.🧰 Tools
🪛 Markdownlint
77-77: Expected: 120; Actual: 160
Line length(MD013, line-length)
Line range hint
12-77: LGTM with minor suggestions for improvementThe "Unreleased" section of the changelog is well-structured and informative. Here are a few suggestions to enhance it further:
Consider adding brief descriptions to some of the more technical changes. For example, for the entry "Add channel selector from chain id", a short explanation of its purpose or impact would be helpful.
Ensure consistency in punctuation. Some entries end with periods while others don't. It's generally preferred to omit periods at the end of changelog entries for consistency.
For bug fixes, it might be helpful to briefly mention the impact of the bug that was fixed, when it's not obvious from the description.
Overall, the changelog is informative and follows good practices. These minor adjustments could further improve its readability and usefulness.
🧰 Tools
🪛 Markdownlint
74-74: Expected: 120; Actual: 126
Line length(MD013, line-length)
77-77: Expected: 120; Actual: 160
Line length(MD013, line-length)
78-78: Expected: 120; Actual: 160
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
77-77: Expected: 120; Actual: 160
Line length(MD013, line-length)
🔇 Additional comments (1)
CHANGELOG.md (1)
Line range hint
1-2928: Well-maintained and informative changelogThis changelog is well-structured and provides a comprehensive history of changes across multiple versions of the project. It consistently categorizes changes into "State Machine Breaking", "Improvements", and "Bug Fixes", which helps readers quickly understand the nature and impact of each change.
The inclusion of links to pull requests and release tags is particularly helpful, allowing readers to dive deeper into specific changes if needed. The consistent format throughout the file makes it easy to navigate and understand the project's evolution over time.
Keep up the good work in maintaining this changelog. It's a valuable resource for users, developers, and contributors to the project.
🧰 Tools
🪛 Markdownlint
74-74: Expected: 120; Actual: 126
Line length(MD013, line-length)
77-77: Expected: 120; Actual: 160
Line length(MD013, line-length)
78-78: Expected: 120; Actual: 160
Line length(MD013, line-length)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
CHANGELOG.md (7)
79-81: Consider adding more details to the inflation reduction entry.The entry "Reduce inflation by 2/3" is quite brief and may benefit from additional context or explanation. Consider expanding on why this change was made and its potential impact on the network.
🧰 Tools
🪛 Markdownlint
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
Line range hint
82-83: Clarify the impact of hardcoding WEVMOS address.The entry "Refactor
StrideandOsmosisoutposts to hardcode WEVMOS address" could benefit from a brief explanation of why this change was made and its implications for users or developers.🧰 Tools
🪛 Markdownlint
76-76: Expected: 120; Actual: 126
Line length(MD013, line-length)
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
Line range hint
84-85: Consider elaborating on the Burner role addition.The entry "Add
feecollectorBurner role in upgrade handler" might be clearer if it briefly explained the purpose of this role and its impact on the system.🧰 Tools
🪛 Markdownlint
76-76: Expected: 120; Actual: 126
Line length(MD013, line-length)
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
Line range hint
86-87: Provide context for burning incentives pool balance.The entry "Burn the usage incentives pool balance during v16 upgrade" could be improved by briefly explaining the reasoning behind this action and its expected impact on the network.
🧰 Tools
🪛 Markdownlint
76-76: Expected: 120; Actual: 126
Line length(MD013, line-length)
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
Line range hint
89-91: Consider clarifying the impact of renaming the inflation module.The entry "Rename
inflationmodule toinflation/v1" might benefit from a brief explanation of why this change was made and if it has any implications for users or developers.🧰 Tools
🪛 Markdownlint
76-76: Expected: 120; Actual: 126
Line length(MD013, line-length)
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
Line range hint
92-93: Explain the implications of deprecating the legacy EIP-712 ante handler.The entry "Deprecate legacy EIP-712 ante handler" could be improved by briefly mentioning the reason for deprecation and any potential impact on users or developers.
🧰 Tools
🪛 Markdownlint
76-76: Expected: 120; Actual: 126
Line length(MD013, line-length)
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
Line range hint
1-160: Overall, the changelog is well-structured and informative.The changelog for v16.0.0 provides a comprehensive list of changes, improvements, and bug fixes. The Improvements and Bug Fixes sections are particularly well-documented, offering clear and concise descriptions of the changes made.
To further enhance the changelog:
- Consider adding more context to some of the State Machine Breaking changes, particularly those related to inflation reduction, outpost refactoring, and burning of incentives pool balance.
- Maintain consistency in the level of detail provided for each entry across all sections.
- When introducing new roles or significant changes, briefly explain their purpose and potential impact on the system.
These minor improvements would make the changelog even more valuable for users and developers trying to understand the changes in this version.
🧰 Tools
🪛 Markdownlint
76-76: Expected: 120; Actual: 126
Line length(MD013, line-length)
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
80-80: Expected: 120; Actual: 160
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
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
🔇 Additional comments (1)
CHANGELOG.md (1)
Line range hint
95-160: LGTM: Improvements and Bug Fixes sections are well-documented.The Improvements and Bug Fixes sections provide clear and concise descriptions of the changes made. They offer good context and explanations for each item, which is helpful for users and developers.
🧰 Tools
🪛 Markdownlint
76-76: Expected: 120; Actual: 126
Line length(MD013, line-length)
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
CHANGELOG.md (3)
79-80: Consider breaking long lines for better readabilityThe lines describing the changes for the vesting and staking precompiles are quite long (160 characters). Consider breaking them into multiple lines for improved readability. For example:
- (precompiles) [#2928](https://github.com/evmos/evmos/pull/2928) Vesting: Add amount scaling on `balanceChange` entry to support different EVM denom precision. - (precompiles) [#2926](https://github.com/evmos/evmos/pull/2926) Staking: Add amount scaling on `balanceChange` entry to support different EVM denom precision.This change would improve readability while maintaining the information content.
🧰 Tools
🪛 Markdownlint
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
Line range hint
27-47: Consider providing more context for some improvement entriesThe Improvements section covers a wide range of enhancements, which is great. However, some entries could benefit from additional context or explanation:
For the entry "Add 'VoteWeighted' transaction to gov precompile", it would be helpful to briefly explain the purpose or benefit of this addition.
The entry "Refactor to use a function to return
TxDataV2in customGetSingers" could use a brief explanation of why this refactoring was done and what benefits it brings.For "Add support for custom base denom decimals in the ante for EVM txs", it would be useful to mention why this support was added and what problem it solves.
Adding a bit more context to these entries would help readers better understand the significance of these improvements.
🧰 Tools
🪛 Markdownlint
76-76: Expected: 120; Actual: 126
Line length(MD013, line-length)
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
Line range hint
49-50: Approve bug fix with a suggestion for clarityThe bug fix addressing the issue with sending transactions to precompiled contracts is important and well-documented. However, the wording could be slightly improved for clarity:
Current:
- (revenue) [#2032](https://github.com/evmos/evmos/pull/2032) Fixed the problem that users cannot send transactions with gasPrice of 0 to precompiled contracts.Suggested:
- (revenue) [#2032](https://github.com/evmos/evmos/pull/2032) Fixed an issue preventing users from sending transactions with a gasPrice of 0 to precompiled contracts.This minor rewording maintains the meaning while improving readability.
🧰 Tools
🪛 Markdownlint
76-76: Expected: 120; Actual: 126
Line length(MD013, line-length)
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
80-80: Expected: 120; Actual: 160
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
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
80-80: Expected: 120; Actual: 160
Line length(MD013, line-length)
🔇 Additional comments (1)
CHANGELOG.md (1)
Line range hint
11-25: State Machine Breaking changes look comprehensive and well-documentedThe State Machine Breaking section provides a clear overview of significant changes, including:
- Version updates for Cosmos-SDK and IBC-Go
- Removal of the
EthAccounttype- Addition of a governance precompile
- Changes to various modules such as vesting, feemarket, and EVM
Each entry is accompanied by a link to the corresponding pull request, which is helpful for developers who want to dive deeper into specific changes. This section effectively communicates the major breaking changes in this release.
🧰 Tools
🪛 Markdownlint
76-76: Expected: 120; Actual: 126
Line length(MD013, line-length)
79-79: Expected: 120; Actual: 160
Line length(MD013, line-length)
80-80: Expected: 120; Actual: 160
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.Interpreterinterface to support custom Opcodes.BankKeeperto manage EVM coins with varying decimals.Bug Fixes
evmosd testnet init-filesrelated tovalidator_address.Documentation
CHANGELOG.mdwith new entries under "Unreleased" for state machine breaking changes, improvements, and bug fixes.