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.

Sign up for GitHub

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

Jump to bottom

Finish ValSet Pref Unbonding #6630

Merged
merged 4 commits into from
Oct 5, 2023
Merged

Finish ValSet Pref Unbonding #6630

merged 4 commits into from
Oct 5, 2023

Conversation

mattverse
Copy link
Member

Closes: #XXX

What is the purpose of the change

Finishes the val set pref unbonding PR, mainly adds test cases, minor refactoring and better code documentation included.

Note that this PR is pointed towards dev/valset-pref-unbond-issues branch, not the main branch

Testing and Verifying

This change added tests

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@mattverse mattverse added V:state/compatible/no_backport State machine compatible PR, depends on prior breaks A:no-changelog labels Oct 4, 2023
@mattverse mattverse mentioned this pull request Oct 4, 2023
Merged
@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Oct 4, 2023
@p0mvn p0mvn mentioned this pull request Oct 4, 2023
Closed
czarcas7ic
czarcas7ic reviewed Oct 4, 2023
View reviewed changes
x/valset-pref/keeper.go Outdated Show resolved Hide resolved
czarcas7ic
czarcas7ic reviewed Oct 4, 2023
View reviewed changes
x/valset-pref/validator_set.go
@@ -195,7 +193,6 @@ func (k Keeper) UndelegateFromValidatorSet(ctx sdk.Context, delegatorAddr string

// Step 6
Copy link
Member

Choose a reason for hiding this comment

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

Can we give a synopsis of Step 6 and Step 7 in the comments like the others? Was nice/helpful

Copy link
Member Author

Choose a reason for hiding this comment

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

I mentioned this on slack, but I personally don't understand if this part of the algorithm is even reachable, thus should be deleted. Will hold this comment for now and either add more comment or delete this once I gain context

x/valset-pref/validator_set.go Outdated Show resolved Hide resolved
x/valset-pref/validator_set_test.go Outdated Show resolved Hide resolved
x/valset-pref/validator_set_test.go Show resolved Hide resolved
x/valset-pref/validator_set_test.go Show resolved Hide resolved
czarcas7ic
czarcas7ic approved these changes Oct 4, 2023
View reviewed changes
Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

Going to approve since this is just merging into another branch, but would like to review the branch being merged into main with more scrutiny.

@czarcas7ic czarcas7ic self-assigned this Oct 5, 2023
@czarcas7ic
Copy link
Member

Merging this, but going to transfer the comments about not reaching steps 5-7 / not testing them either, and will be a blocker for final merge. Thanks for getting this done!

@czarcas7ic czarcas7ic merged commit 9e98292 into dev/valset-pref-unbond-issues Oct 5, 2023
1 check passed
@czarcas7ic czarcas7ic deleted the mattverse/valset-pref-unbond branch October 5, 2023 17:20
AlpinYukseloglu pushed a commit that referenced this pull request Oct 13, 2023
@ValarDragon @stackman27
@devbot-wizard @czarcas7ic @p0mvn @mattverse
Fix: Fix valset pref unbonding bugs (#5967)
e12011b 
* Add helper to get existing delegations along with valset pref

* Lay out more of the logic nuance

* Add pseudo-code needed

* algorithm v1

* added algorithm docs

* fixed all test

* removed unwanted files

* remove more code

* added more tests

* update changelog

* added test and addressed feedback

* Update x/valset-pref/validator_set.go

* Update x/valset-pref/validator_set.go

* Update x/valset-pref/README.md

* Minor cleanup

* re-use validator gets

* Refactor

* Highlight bug

* fixed the issue

* added comments

* update changelog

* fixed the issue

* fixed go test

* fixed test

* lint

* Finish ValSet Pref Unbonding (#6630)

* Add Test cases and fix

* Update x/valset-pref/keeper.go

* Adams comment

* add error types and tidy test

---------

Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>

* feat: unbond with rebalanced val set weights (#6685)

* initial push

* add todo

* msg server test

* add godoc

* custom error

* update godoc

* update proto and remove duplication

* add more TODOs

* use min when calculating amt to withdraw from last val

* Update x/valset-pref/validator_set.go

---------

Co-authored-by: stackman27 <sis1001@berkeley.edu>
Co-authored-by: devbot-wizard <141283918+devbot-wizard@users.noreply.github.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: roman <roman@osmosis.team>
Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
mergify bot pushed a commit that referenced this pull request Oct 13, 2023
@ValarDragon @mergify
Fix: Fix valset pref unbonding bugs (#5967)
0570258 
* Add helper to get existing delegations along with valset pref

* Lay out more of the logic nuance

* Add pseudo-code needed

* algorithm v1

* added algorithm docs

* fixed all test

* removed unwanted files

* remove more code

* added more tests

* update changelog

* added test and addressed feedback

* Update x/valset-pref/validator_set.go

* Update x/valset-pref/validator_set.go

* Update x/valset-pref/README.md

* Minor cleanup

* re-use validator gets

* Refactor

* Highlight bug

* fixed the issue

* added comments

* update changelog

* fixed the issue

* fixed go test

* fixed test

* lint

* Finish ValSet Pref Unbonding (#6630)

* Add Test cases and fix

* Update x/valset-pref/keeper.go

* Adams comment

* add error types and tidy test

---------

Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>

* feat: unbond with rebalanced val set weights (#6685)

* initial push

* add todo

* msg server test

* add godoc

* custom error

* update godoc

* update proto and remove duplication

* add more TODOs

* use min when calculating amt to withdraw from last val

* Update x/valset-pref/validator_set.go

---------

Co-authored-by: stackman27 <sis1001@berkeley.edu>
Co-authored-by: devbot-wizard <141283918+devbot-wizard@users.noreply.github.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: roman <roman@osmosis.team>
Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
(cherry picked from commit e12011b)
czarcas7ic pushed a commit that referenced this pull request Oct 13, 2023
@mergify @ValarDragon
Fix: Fix valset pref unbonding bugs (#5967) (#6696)
f5acb01 
* Add helper to get existing delegations along with valset pref

* Lay out more of the logic nuance

* Add pseudo-code needed

* algorithm v1

* added algorithm docs

* fixed all test

* removed unwanted files

* remove more code

* added more tests

* update changelog

* added test and addressed feedback

* Update x/valset-pref/validator_set.go

* Update x/valset-pref/validator_set.go

* Update x/valset-pref/README.md

* Minor cleanup

* re-use validator gets

* Refactor

* Highlight bug

* fixed the issue

* added comments

* update changelog

* fixed the issue

* fixed go test

* fixed test

* lint

* Finish ValSet Pref Unbonding (#6630)

* Add Test cases and fix

* Update x/valset-pref/keeper.go

* Adams comment

* add error types and tidy test

---------

Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>

* feat: unbond with rebalanced val set weights (#6685)

* initial push

* add todo

* msg server test

* add godoc

* custom error

* update godoc

* update proto and remove duplication

* add more TODOs

* use min when calculating amt to withdraw from last val

* Update x/valset-pref/validator_set.go

---------

Co-authored-by: stackman27 <sis1001@berkeley.edu>
Co-authored-by: devbot-wizard <141283918+devbot-wizard@users.noreply.github.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: roman <roman@osmosis.team>
Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
(cherry picked from commit e12011b)

Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
pysel pushed a commit that referenced this pull request Oct 17, 2023
@ValarDragon @stackman27
@devbot-wizard @czarcas7ic @p0mvn @mattverse @pysel
Fix: Fix valset pref unbonding bugs (#5967)
75d57aa 
* Add helper to get existing delegations along with valset pref

* Lay out more of the logic nuance

* Add pseudo-code needed

* algorithm v1

* added algorithm docs

* fixed all test

* removed unwanted files

* remove more code

* added more tests

* update changelog

* added test and addressed feedback

* Update x/valset-pref/validator_set.go

* Update x/valset-pref/validator_set.go

* Update x/valset-pref/README.md

* Minor cleanup

* re-use validator gets

* Refactor

* Highlight bug

* fixed the issue

* added comments

* update changelog

* fixed the issue

* fixed go test

* fixed test

* lint

* Finish ValSet Pref Unbonding (#6630)

* Add Test cases and fix

* Update x/valset-pref/keeper.go

* Adams comment

* add error types and tidy test

---------

Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>

* feat: unbond with rebalanced val set weights (#6685)

* initial push

* add todo

* msg server test

* add godoc

* custom error

* update godoc

* update proto and remove duplication

* add more TODOs

* use min when calculating amt to withdraw from last val

* Update x/valset-pref/validator_set.go

---------

Co-authored-by: stackman27 <sis1001@berkeley.edu>
Co-authored-by: devbot-wizard <141283918+devbot-wizard@users.noreply.github.com>
Co-authored-by: Adam Tucker <adam@osmosis.team>
Co-authored-by: roman <roman@osmosis.team>
Co-authored-by: Matt, Park <45252226+mattverse@users.noreply.github.com>
Co-authored-by: Adam Tucker <adamleetucker@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@czarcas7ic czarcas7ic czarcas7ic approved these changes

Assignees

@czarcas7ic czarcas7ic

Labels
A:no-changelog C:docs Improvements or additions to documentation V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mattverse @czarcas7ic