-
Notifications
You must be signed in to change notification settings - Fork 898
chore(erc20): keep MsgConvertCoin interface registered #2892
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
Conversation
WalkthroughThe pull request introduces updates to the changelog and the Changes
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
https://github.com/Mergifyio backport release/v19.x.x |
https://github.com/Mergifyio backport rama/release-v20 |
✅ Backports have been created
|
✅ Backports have been created
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/erc20/types/codec.go (1)
47-47
: LGTM: MsgConvertCoin registered for backwards compatibilityThe addition of
&MsgConvertCoin{}
to theRegisterInterfaces
function is correct and maintains consistency with the module's architecture. This change ensures proper registration of the message type, supporting backwards compatibility for transaction queries.Consider updating the comment to match the exact wording used for the constant:
- &MsgConvertCoin{}, // keep it for backwards compatiblity when querying txs + &MsgConvertCoin{}, // keep it for backwards compatibility when querying txsThis minor change corrects the spelling of "compatibility" and ensures consistency with the comment on the constant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- x/erc20/types/codec.go (3 hunks)
🔇 Additional comments (33)
x/erc20/types/codec.go (3)
31-31
: LGTM: New constant added for backwards compatibilityThe addition of the
convertCoinName
constant is well-documented and follows the existing naming conventions. This change supports backwards compatibility for transaction queries, which is a good practice for maintaining API stability.
69-69
: LGTM: MsgConvertCoin registered with LegacyAmino codecThe addition of
&MsgConvertCoin{}
to theRegisterLegacyAminoCodec
function is correct and consistent with the registration of other message types. This change ensures that theMsgConvertCoin
type is properly serialized for Amino JSON and EIP-712 compatibility.
31-31
: Summary: MsgConvertCoin interface successfully maintainedThe changes in this file effectively achieve the PR's objective of keeping the MsgConvertCoin interface registered. The modifications include:
- Adding a new constant
convertCoinName
for backwards compatibility.- Registering
MsgConvertCoin 8000
in theRegisterInterfaces
function.- Registering
MsgConvertCoin
with the LegacyAmino codec.These changes ensure that the
MsgConvertCoin
type is properly integrated into the existing codec system while maintaining backwards compatibility for transaction queries. The implementation is consistent with the module's architecture and coding conventions.Also applies to: 47-47, 69-69
CHANGELOG.md (30)
68-71
: New bug fix section addedA new "Bug Fixes" section has been added to the changelog for v16.0.0. This is a good practice as it clearly separates bug fixes from other types of changes.
70-70
: Keep MsgConvertCoin interface registered for backwards compatibilityThis bug fix ensures backwards compatibility by keeping the
MsgConvertCoin
interface registered. It's important to maintain compatibility with existing systems that may rely on this interface.
Line range hint
9-10
: New secp256r1 curve precompile addedThe addition of the secp256r1 curve precompile (RIP-7212) is a significant change that enhances the cryptographic capabilities of the system. This change is state machine breaking and will require careful testing and coordination during the upgrade process.
Line range hint
11-11
: New ClaimRewards custom transaction added to distribution precompileAdding a
ClaimRewards
custom transaction to the distribution precompile is a useful feature that allows for more efficient reward claiming. This change is state machine breaking and will require updates to any systems interacting with reward claiming.
Line range hint
12-13
: New bech32 conversion precompile addedThe addition of a bech32 conversion precompile is a helpful feature for address format conversions. This change is state machine breaking and will require careful integration and testing.
Line range hint
15-15
: New CreateValidator function added to staking precompileAdding the
CreateValidator
function to the staking precompile expands the functionality available through precompiles. This is a state machine breaking change that will require updates to any systems interacting with validator creation.
Line range hint
16-16
: Transaction fee tokens restrictedRestricting transaction fee tokens is a significant change that may affect how users interact with the system. This change should be clearly communicated to all stakeholders, including users and developers.
Line range hint
20-20
: x/recovery module removedThe removal of the
x/recovery
module is a major change that may affect existing functionality. Ensure that any dependencies on this module are addressed and that users are informed about the removal of any related features.
Line range hint
21-21
: bank precompile addedThe addition of a bank precompile is a useful feature that allows for easier interaction with bank module functionality. This change is state machine breaking and will require careful testing and integration.
Line range hint
22-22
: x/incentives module removed and incentives pool balance burnedThe removal of the
x/incentives
module and the burning of the incentives pool balance is a significant change. This may affect existing incentive structures and should be clearly communicated to all stakeholders.
Line range hint
23-24
: x/claims params removed and EVMChannels param migratedThe removal of
x/claims
params and the migration of theEVMChannels
param to thex/evm
module params is a notable change. Ensure that any systems relying on the old parameter structure are updated accordingly.
Line range hint
25-25
: BurnDecorator added to PostHandlerAdding a
BurnDecorator
to thePostHandler
to burn cosmos transaction fees is an important change that affects the economics of the system. This should be clearly explained in the documentation and communicated to users.
Line range hint
26-26
: Channel selector added based on chain IDThe addition of a channel selector based on chain ID is a useful feature for multi-chain environments. Ensure that the selection logic is well-documented and tested.
Line range hint
27-27
: Inflation reduced by 2/3A significant reduction in inflation by 2/3 is a major economic change. This should be thoroughly explained in the documentation, including the rationale behind the change and its expected impacts.
Line range hint
28-29
: Stride and Osmosis outposts refactoredThe refactoring of Stride and Osmosis outposts to hardcode the WEVMOS address is a notable change. Ensure that this doesn't introduce any security vulnerabilities and that the hardcoded address is correctly set.
Line range hint
30-30
: feecollector Burner role added in upgrade handlerAdding the
feecollector
Burner role in the upgrade handler is an important change for fee management. Ensure that the implications of this change are well-documented and that it doesn't introduce any unexpected behavior.
Line range hint
31-31
: Usage incentives pool balance burned during v16 upgradeBurning the usage incentives pool balance during the v16 upgrade is a significant economic change. This should be clearly communicated to all stakeholders, and the implications of this action should be thoroughly explained.
Line range hint
35-35
: inflation module renamed to inflation/v1Renaming the
inflation
module toinflation/v1
is an API breaking change that may require updates to client applications. Ensure that this change is clearly documented and that migration guides are provided for users of the inflation module.
Line range hint
36-36
: Legacy EIP-712 ante handler deprecatedThe deprecation of the legacy EIP-712 ante handler is an important change that may affect existing transactions. Provide clear documentation on the new recommended approach and any necessary migration steps.
Line range hint
40-41
: Lint tests added for consistencyAdding lint tests to ensure consistency between test code and non-test code is a good practice. This will help maintain code quality and readability across the project.
Line range hint
42-42
: Golang dependency vulnerability checker added to CIAdding a Golang dependency vulnerability checker to the CI process is an excellent security improvement. This will help identify and address potential vulnerabilities in dependencies more quickly.
Line range hint
43-44
: Default File store listener added for applicationAdding a default File store listener based on ADR38 is a good improvement for state management and observability. Ensure that the implementation is well-documented and that any performance implications are considered.
Line range hint
45-45
: Audits page added to documentationAdding an audits page to the documentation is a great improvement for transparency and security. This will help users and developers understand the security measures in place.
Line range hint
46-47
: New MsgUpdateVestingFunder message addedThe addition of a new
MsgUpdateVestingFunder
message to update theFunder
field of a clawback vesting account is a useful feature. Ensure that proper access controls are in place for this operation.
Line range hint
62-62
: Fix for TraceTx KVGasConfig setupFixing the
TraceTx
KVGasConfig setup is important for accurate gas estimation and tracing. Ensure that this fix is thoroughly tested with various transaction types.
Line range hint
63-63
: Fix for inflation info returned by epoch mint provision getterCorrecting the inflation info returned by the epoch mint provision getter is crucial for accurate economic calculations. Verify that this fix doesn't have any unintended consequences on inflation-related functionality.
Line range hint
64-64
: Add burned cosmos transactions fees metricAdding a metric for burned cosmos transaction fees is a good improvement for monitoring and analytics. Ensure that this metric is properly documented and integrated with existing monitoring systems.
65-65
: Fix store uploader for x/recovery moduleFixing the store uploader for the
x/recovery
module is important for data integrity during upgrades. Thoroughly test this fix with various upgrade scenarios to ensure it works as expected.
66-66
: Fix unbound metrics and remove labels that keep increasingAddressing unbound metrics and removing continuously increasing labels is crucial for the stability and performance of the metrics system. Verify that this fix doesn't break any existing monitoring or alerting rules.
Line range hint
1-71
: Comprehensive update with significant changesThe v16.0.0 release introduces numerous substantial changes, including state machine breaking modifications, API changes, new features, and important bug fixes. Key points to note:
- Several modules have been removed or significantly altered (e.g., recovery, incentives).
- New precompiles and functions have been added, enhancing functionality.
- Economic parameters have been adjusted, including inflation reduction.
- Various improvements have been made to testing, documentation, and development processes.
- Several critical bug fixes have been implemented.
Given the scope of these changes, it is crucial to:
- Thoroughly test all modifications, especially state machine breaking changes.
- Update all relevant documentation and provide clear upgrade guides.
- Communicate these changes clearly to all stakeholders, including users, developers, and node operators.
- Consider the potential impact on existing integrations and provide necessary migration paths.
Overall, this release represents a significant evolution of the system and should be approached with careful planning and testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @GAtom22 🙌
* chore(erc20): keep MsgConvertCoin interface registered * add changelog entry * fix lint warnings (cherry picked from commit 9c452de) # Conflicts: # CHANGELOG.md # x/erc20/types/codec.go
* chore(erc20): keep MsgConvertCoin interface registered * add changelog entry * fix lint warnings (cherry picked from commit 9c452de)
… (#2894) * chore(erc20): keep MsgConvertCoin interface registered (#2892) * chore(erc20): keep MsgConvertCoin interface registered * add changelog entry * fix lint warnings (cherry picked from commit 9c452de) * update chlog --------- Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com> Co-authored-by: tom <tomasguerraalda@hotmail.com>
… (#2893) * chore(erc20): keep MsgConvertCoin interface registered (#2892) * chore(erc20): keep MsgConvertCoin interface registered * add changelog entry * fix lint warnings (cherry picked from commit 9c452de) # Conflicts: # CHANGELOG.md # x/erc20/types/codec.go * fix conflicts * add methods to comply with Msg interface --------- Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com> Co-authored-by: tom <tomasguerraalda@hotmail.com>
… (#2893) * chore(erc20): keep MsgConvertCoin interface registered (#2892) * chore(erc20): keep MsgConvertCoin interface registered * add changelog entry * fix lint warnings (cherry picked from commit 9c452de) * fix conflicts * add methods to comply with Msg interface --------- Co-authored-by: Tom <54514587+GAtom22@users.noreply.github.com> Co-authored-by: tom <tomasguerraalda@hotmail.com>
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...
Unreleased
section inCHANGELOG.md
Summary by CodeRabbit
MsgConvertCoin
interface remains registered for backwards compatibility, preventing potential issues.convertCoinName
, to maintain backward compatibility when querying transactions.