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

[Valset]: Fix weight calculations to be based on tokens instead of shares #6699

Merged
merged 7 commits into from
Oct 14, 2023

Conversation

AlpinYukseloglu
Copy link
Contributor

@AlpinYukseloglu AlpinYukseloglu commented Oct 13, 2023

Closes: #XXX

What is the purpose of the change

Current valset undelegation logic has a bug where it calculates the amount to undelegate based on validator shares instead of their underlying tokens. Since the value of shares are nonfungible and highly variable across validators, this is a fundamentally incorrect way to calculate the correct portion of a valset that corresponds to a given validator. For instance, if a user has delegated 10 shares each to 10 validators and one of the validators gets slashed for 20% of its stake, the current implementation would continue to treat the slashed validator as 10% of the delegator's set.

This PR fixes valset undelegation logic so it works with tokens instead of shares.

Testing and Verifying

The helper that was causing the root issue is fixed and tested in keeper_test.go

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

@AlpinYukseloglu AlpinYukseloglu added the V:state/breaking State machine breaking PR label Oct 13, 2023
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.

Nice job, I am approving but going to see if you agree with these comments prior to merge.

x/valset-pref/keeper.go Outdated Show resolved Hide resolved
x/valset-pref/keeper.go Show resolved Hide resolved
x/valset-pref/keeper_test.go Show resolved Hide resolved
@github-actions
Copy link
Contributor

Important Notice

This PR modifies an in-repo Go module. It is one of:

  • osmomath
  • osmoutils
  • x/ibc-hooks
  • x/epochs

The dependent Go modules, especially the root one, will have to be
updated to reflect the changes. Failing to do so might cause e2e to fail.

Please follow the instructions below:

  1. Open https://github.com/osmosis-labs/osmosis/actions/workflows/go-mod-auto-bump.yml
  2. Provide the current branch name
  3. On success, confirm if an automated commit corretly updated the go.mod and go.sum files

Please let us know if you need any help.

@czarcas7ic czarcas7ic closed this Oct 14, 2023
@czarcas7ic czarcas7ic reopened this Oct 14, 2023
@czarcas7ic czarcas7ic added the A:backport/v20.x backport patches to v20.x branch label Oct 14, 2023
@czarcas7ic czarcas7ic merged commit cf159ac into main Oct 14, 2023
1 check passed
@czarcas7ic czarcas7ic deleted the alpo/fix-valset-undelegation-shares branch October 14, 2023 03:04
mergify bot pushed a commit that referenced this pull request Oct 14, 2023
…ares (#6699)

* fix weight calculations to be based on tokens instead of shares

* changelog

* add comments and clean up tests

* Auto: update go.mod after push to alpo/fix-valset-undelegation-shares that modified dependencies locally

* switch rounding to bankers

* Auto: update go.mod after push to alpo/fix-valset-undelegation-shares that modified dependencies locally

---------

Co-authored-by: github-actions <[email protected]>
(cherry picked from commit cf159ac)
czarcas7ic pushed a commit that referenced this pull request Oct 14, 2023
…ares (#6699) (#6700)

* fix weight calculations to be based on tokens instead of shares

* changelog

* add comments and clean up tests

* Auto: update go.mod after push to alpo/fix-valset-undelegation-shares that modified dependencies locally

* switch rounding to bankers

* Auto: update go.mod after push to alpo/fix-valset-undelegation-shares that modified dependencies locally

---------

Co-authored-by: github-actions <[email protected]>
(cherry picked from commit cf159ac)

Co-authored-by: Alpo <[email protected]>
Copy link
Member

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

Nice job

pysel pushed a commit that referenced this pull request Oct 17, 2023
…ares (#6699)

* fix weight calculations to be based on tokens instead of shares

* changelog

* add comments and clean up tests

* Auto: update go.mod after push to alpo/fix-valset-undelegation-shares that modified dependencies locally

* switch rounding to bankers

* Auto: update go.mod after push to alpo/fix-valset-undelegation-shares that modified dependencies locally

---------

Co-authored-by: github-actions <[email protected]>
@github-actions github-actions bot mentioned this pull request Apr 1, 2024
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:backport/v20.x backport patches to v20.x branch C:x/val-set-pref V:state/breaking State machine breaking PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants