-
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
validator remove, and fix unauthorized leave #378
Conversation
|
||
// upgradeValidatorsDBfrom1To2 upgrades the validators db from version 1 to 2. | ||
// Just create the removals table and bump the version. | ||
func (vs *validatorStore) upgradeValidatorsDBfrom1To2(ctx context.Context) error { |
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.
since we will likely have breaking changes (given the proposed auth changes), should we just consider this a totally new version and break the old one?
Other things will have to break anyways, so it seems like it may just be easier to still consider this a v1.
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.
Are all existing deployments running at v1 certainly going to be wiped?
We can certainly scrub this upgradeValidatorsDBfrom1To2
, but that makes the existing upgradeValidatorsDBfrom0To1
incorrect and we'd actually have to move the sqlInitRemovalsTableV2
execution into that previous upgrade. That can be done, but I think that's not generally beneficial, and it ends up being more complicated. It also breaks the upgrade tests established for that v0 -> v1 update and we'd need to regenerate test data.
There were also some issues with the general upgrade framework that are sorted though, so it feels like a good test of it all anyway.
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.
Gotcha. Yeah, all deployments will likely have to be wiped. Mostly b/c it looks like we will likely pursue some sort of identity approach that allows EVM users to use just their addresses, instead of having to recover their public keys. This is a pretty large change, since EVM users are the default users.
* authorized leave * remove validator
* authorized leave * remove validator
* authorized leave * remove validator
* authorized leave * remove validator
* authorized leave * remove validator
* authorized leave * remove validator
Resolves both #377 and #365
No integration tests for the
remove
process, but it works in manual testing.Both changes are important for release, but especially. #377