feat: unbond with rebalanced val set weights #6685

czarcas7ic · 2023-10-12T19:51:53Z

Closes: #XXX

What is the purpose of the change

The current implementation of unbonding from validator set is broken. It is really close to being fixed, but we decided to go with a method that simply determines the new weights of the val set based on the user's current delegations to that val set.

For ex: if my valset is 30/60 to valA and valB, but I have 10 OSMO delgated to valA and 90 to valB, if I call the new undelegate rebalanced method here for 50 OSMO, it undelegates 5 OSMO from valA and 45 OSMO from valB.

I know we have opinions on dead code existing in prod, but I really think it makes sense to just disable the original undelegate message, and post v20 we can make the small tweak needed to fix this and test it, rather than completely removing it and having to rewire it.

Testing and Verifying

Added a gotest to show the above behavior is what actually happens.

czarcas7ic · 2023-10-12T20:38:21Z

x/valset-pref/msg_server.go

+	// ctx := sdk.UnwrapSDKContext(goCtx)
+
+	// err := server.keeper.UndelegateFromValidatorSet(ctx, msg.Delegator, msg.Coin)
+	// if err != nil {
+	// 	return nil, err
+	// }
+
+	return &types.MsgUndelegateFromValidatorSetResponse{}, fmt.Errorf("not implemented, utilize UndelegateFromRebalancedValidatorSet instead")
+}


Disables UndelegateFromValidatorSet in the msg server

czarcas7ic · 2023-10-12T20:39:27Z

x/valset-pref/msg_server_test.go

+// 	}
+// }
+
+func (s *KeeperTestSuite) TestUnDelegateFromRebalancedValidatorSet() {


TestUnDelegateFromRebalancedValidatorSet is identical to the commented out TestUnDelegateFromValidatorSet, because the behavior of UnDelegateFromValidatorSet is technically UnDelegateFromRebalancedValidatorSet.

czarcas7ic · 2023-10-12T20:44:08Z

x/valset-pref/client/cli/tx.go

+// func NewUnDelValSetCmd() (*osmocli.TxCliDesc, *types.MsgUndelegateFromValidatorSet) {
+// 	return &osmocli.TxCliDesc{
+// 		Use:     "undelegate-valset",
+// 		Short:   "UnDelegate tokens from existing valset using delegatorAddress and tokenAmount.",
+// 		Example: "osmosisd tx valset-pref undelegate-valset osmo1... 100stake",
+// 		NumArgs: 2,
+// 	}, &types.MsgUndelegateFromValidatorSet{}
+// }


Disable the CLI for undelegate-valset cmd.

AlpinYukseloglu

Discussed having a separate function offline and I agree this gets us to a preferable place in the tradeoff space (where minimizing last-minute implementation overhead is a high priority).

I left two comments covering what I believe are blocking issues that should be quick to address. The rest looks good to me.

proto/osmosis/valset-pref/v1beta1/tx.proto

x/valset-pref/client/cli/tx.go

x/valset-pref/msg_server.go

x/valset-pref/validator_set_test.go

AlpinYukseloglu · 2023-10-12T23:23:53Z

x/valset-pref/validator_set_test.go

+			}
+
+			// System Under Test
+			err := s.App.ValidatorSetPreferenceKeeper.UndelegateFromRebalancedValidatorSet(s.Ctx, defaultDelegator.String(), sdk.NewCoin(bondDenom, test.undelegateAmt))


Don't want to block this PR, but I think we're actually not testing the main important thing here which is that directly delegating/undelegating from a validator changes the weights when the new message is called.

E.g. if it starts at 0.1/0.9 with 10 tokens in validator A and 90 in validator B, then when you delegate 100 tokens to validator A, then withdrawing rebalanced should yield a distribution of 0.55/0.45 (since now validator A has 110 tokens out of 200 and validator B has 90 out of 200)

Would it be possible for us to add tests covering these? I think this could potentially be done with an additional test field and putting system under test into a loop so it runs multiple times with different conditions.

ValarDragon · 2023-10-13T11:38:03Z

x/valset-pref/validator_set.go

+		// in the last valset iteration we don't calculate it from shares using decimals and trucation,
+		// we use whats remaining to get more accurate value
+		if len(existingSet.Preferences)-1 == index {
+			amountToUnDelegate = undelegation.Amount.Sub(totalUnDelAmt).ToLegacyDec().TruncateInt()
+		} else {


Lets make amountToUndelegate = Min(amountToUndelegate, amount_staked_to_this_val)

just in case some rounding issue increases it.

Added the logic you described cc584e7

Although I am not quite sure how to force this in a gotest, seems like just a guard rail.

ValarDragon

Logic Lgtm; have not reviewed tests

* 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>

* 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)

* 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>

* 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>

initial push

3782dbf

czarcas7ic added V:state/breaking State machine breaking PR A:backport/v20.x backport patches to v20.x branch labels Oct 12, 2023

github-actions bot added the C:CLI label Oct 12, 2023

czarcas7ic mentioned this pull request Oct 12, 2023

[ValSet]: Implement proper undelegate method #6686

Open

czarcas7ic added the A:no-changelog label Oct 12, 2023

czarcas7ic added 4 commits October 12, 2023 14:11

add todo

5f6f9fb

msg server test

7fd9fab

add godoc

a61563e

custom error

a293300

czarcas7ic commented Oct 12, 2023

View reviewed changes

update godoc

4e2c733

czarcas7ic marked this pull request as ready for review October 12, 2023 20:46

czarcas7ic requested a review from mattverse as a code owner October 12, 2023 20:46

AlpinYukseloglu reviewed Oct 12, 2023

View reviewed changes

proto/osmosis/valset-pref/v1beta1/tx.proto Outdated Show resolved Hide resolved

x/valset-pref/client/cli/tx.go Show resolved Hide resolved

x/valset-pref/msg_server.go Outdated Show resolved Hide resolved

czarcas7ic added 2 commits October 12, 2023 15:22

update proto and remove duplication

948db2b

add more TODOs

fdaa816

AlpinYukseloglu reviewed Oct 12, 2023

View reviewed changes

ValarDragon reviewed Oct 13, 2023

View reviewed changes

use min when calculating amt to withdraw from last val

cc584e7

ValarDragon approved these changes Oct 13, 2023

View reviewed changes

czarcas7ic merged commit ce280e8 into dev/valset-pref-unbond-issues Oct 13, 2023
1 check passed

czarcas7ic deleted the adam/valset-pref-unbond-v20 branch October 13, 2023 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: unbond with rebalanced val set weights #6685

feat: unbond with rebalanced val set weights #6685

czarcas7ic commented Oct 12, 2023 •

edited

czarcas7ic Oct 12, 2023

czarcas7ic Oct 12, 2023 •

edited

czarcas7ic Oct 12, 2023

AlpinYukseloglu left a comment

AlpinYukseloglu Oct 12, 2023

ValarDragon Oct 13, 2023

czarcas7ic Oct 13, 2023

ValarDragon left a comment

feat: unbond with rebalanced val set weights #6685

feat: unbond with rebalanced val set weights #6685

Conversation

czarcas7ic commented Oct 12, 2023 • edited

What is the purpose of the change

Testing and Verifying

czarcas7ic Oct 12, 2023

Choose a reason for hiding this comment

czarcas7ic Oct 12, 2023 • edited

Choose a reason for hiding this comment

czarcas7ic Oct 12, 2023

Choose a reason for hiding this comment

AlpinYukseloglu left a comment

Choose a reason for hiding this comment

AlpinYukseloglu Oct 12, 2023

Choose a reason for hiding this comment

ValarDragon Oct 13, 2023

Choose a reason for hiding this comment

czarcas7ic Oct 13, 2023

Choose a reason for hiding this comment

ValarDragon left a comment

Choose a reason for hiding this comment

czarcas7ic commented Oct 12, 2023 •

edited

czarcas7ic Oct 12, 2023 •

edited