Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
chore: denom traces migration handler #1680
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
chore: denom traces migration handler #1680
Changes from 13 commits
96f3c49
250e45f
bf3b96b
ed4a153
8018627
08e71e7
05f50c4
169ead2
7c46b84
27998dd
952b114
7a1d263
a5b3ff6
b33f4df
f673132
0b2f4c2
78d4616
3084982
b1cd41f
3c07902
6110232
87a97e6
ecbd10a
35ea75e
7660328
cd8c867
0f91661
9a44480
96aabb5
b608e89
b86cdd8
45fdc09
60785a6
38c93bc
515e0e7
a2fe4ae
d999103
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
is the intention that all transfer related migrations will happen here?
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.
yes -- the cosmos sdk has all migrations in a separate directory but imo that might be overkill for us. wdyt?
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.
I think for us it makes sense to have them here, we can worry about additional directories if we end up with a large number of 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.
Just wondering if there's anything we should do if this (unlikely) scenario happens: if there's a corrupt denom trace and that blocks the migration to complete, is there anything we can do or document to help chains?
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.
Maybe it should just panic and then have the chain devs export a snapshot and manually address the situation? What do you think?
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.
i think this is extremely unlikely to happen unless the migration happens from an exported genesis which was manually corrupted -- i think it could make sense rather than panicking the err here to simply pass back to the UpgradeHandler and have the app devs deal with it how they want... does that make sense?
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.
In which case, couldn't we return the error immediately? Instead of continuing to do unnecessary work... I'm assuming that the upgrade handler and
RunMigrations
will likely bubble up the error and revert any changes so I think it seems unnecessary to continue parsing traces if we encounter an error for some unforeseen reason!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.
we do stop the parsing and then return the error in the current code, is there another way to return the error that you think would make more sense?
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.
Ah sorry, I missed the
return true
underneath here due to the thread of comments!