-
Notifications
You must be signed in to change notification settings - Fork 103
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
feat(x/ecocredit): add basket state migration #1397
Conversation
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.
LGTM! nice to be able to use the native register migrations 👍🏻
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.
Tested an upgrade from v4 to v5 and it looks like the upgrade resets the basket fees param to an empty array. Tested updating the param with a chain that start on v5 and it works as expected.
https://hackmd.io/MP6BaYA_SOiQtEomPD8ong
Also, I believe we should be using the consensus version rather than the regen-ledger version for the migration name, same as the sdk. The sdk is still using the sdk version in some modules but are migrating to the use of the consensus version.
https://github.com/cosmos/cosmos-sdk/tree/main/x/gov/migrations
For reference (use of consensus version and using migrator): |
Nice catch. The issue is when we start a new chain SDK module manager sets a consensus version for registered modules (we are not using SDK's module manager for regen modules). The ecocredit module migrations not running because there is no consensus version present in the state. I've updated the upgrade handler to initialize the consensus version for |
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.
Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
Description
Ref: #1358
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
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...
!
in the type prefix if API or client breaking change