Skip to content
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

track delegation vesting accounts #8946

Conversation

boz
Copy link
Contributor

@boz boz commented Mar 22, 2021

@gsora @alexanderbez what do you guys think of this patch to the account migration?

  1. reset corrupted values to zero
  2. re-run TrackDelegation
  3. save account with SetAccount 😅

@lgtm-com
Copy link

lgtm-com bot commented Mar 22, 2021

This pull request introduces 2 alerts and fixes 2 when merging 80335ad into c0cb1d2 - view on LGTM.com

new alerts:

  • 2 for Useless assignment to local variable

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Mar 22, 2021

This pull request fixes 2 alerts when merging 9a1c74a into c0cb1d2 - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Mar 22, 2021

This pull request fixes 2 alerts when merging 93b2ec9 into c0cb1d2 - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Mar 22, 2021

This pull request fixes 2 alerts when merging c10b189 into c0cb1d2 - view on LGTM.com

fixed alerts:

  • 2 for Useless assignment to local variable

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. Any reason this is preferred over the original approach? Is the original approach broken?

@gsora
Copy link
Contributor

gsora commented Mar 22, 2021

This PR adds a more general test case and a clearing pass in the migration.

Not a completely new approach, but a rather important factor that was missing in #8865, hence the PR-on-PR :D

@boz
Copy link
Contributor Author

boz commented Mar 23, 2021

Looks reasonable to me. Any reason this is preferred over the original approach? Is the original approach broken?

Small change to the previous approach - zero-out the fields and don't clamp down the delegation amount before calling TrackDelegation - this allows for delegation of more than the amount that is/was vesting.

@gsora
Copy link
Contributor

gsora commented Mar 23, 2021

I am merging this, looks fine and adds further refinements to the migration code.

@gsora gsora merged commit b82d06f into cosmos:gsora/track-delegation-vesting-accounts Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants