-
Notifications
You must be signed in to change notification settings - Fork 12
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
Simplify Migration Structs #1036
Conversation
Have not tested this btw |
2f7da7e
to
56ddba1
Compare
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.
Wow this feels super clean. Have not tested either and we definitely want @charithabandi to review and test, but LGTM.
I'm going to test this over the weekend since it is such a big change |
Looking into this rn |
Regarding CI failure, on the "unhealthy" node1 I see:
So it never gets healthy, at least not in 30 sec |
Yeah we got it sorted, it was really really stupid. I realized I was testing with a binary that was built from main, not this branch 🤦 . As soon as I tested with this branch I saw the error |
95489e3
to
e1416a9
Compare
Ok just finished testing this. I feel good about it |
cmd/kwild/server/migration.go
Outdated
logger.Info("Genesis state already downloaded", log.String("genesis snapshot", snapshotFileName)) | ||
|
||
if !validateGenesisState(snapshotFileName, genesisCfg.DataAppHash) { |
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.
validateGenesisState
could use an update. I don't think it should itself call failBuild
(it's not in build.go and PrepareForMigration
can return errors without having to panic). Also, a bunch of logic there is obsoleted now that we're ensuring it's existence here. The empty apphash logic in validateGenesisState
is also a little questionable given the current changes, plus the bytes.Equal
in the final return covers the length an nilness checks implicitly.
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.
Oh yeah. Im just going to change this whole function since it is a bit convoluted in its usage
test/driver/operator/cli_driver.go
Outdated
var res display.TxHashResponse | ||
err := o.runCommand(ctx, &res, "migrate", "propose", activationHeight.String(), migrationDuration.String(), chainID) | ||
err := o.runCommand(ctx, &res, "migrate", "propose", "activation-period", activationHeight.String(), "duration", migrationDuration.String()) |
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.
This doesn't need --
on activation-period
and duration
?
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.
Oh whoops. Currently we do not test the cli for this, but yes you are correct
Is this good to go? @charithabandi @jchappelow |
I am doing staging e2e this evening, but can merge. Any issues can be resolved after |
Cool, thanks. Trying to get it merged b/c I know there will be merge conflicts with #1040 |
They're minimal (I've resolved locally), but that's true |
This PR simplifies migration structs. There are three key changes:
chain-id
from the migration proposals, since they are effectively a suggestion anyways.core/types
, but didn't to keep it simple.