[ValSet-Pref] Add Msgs for Delegate and Undelegate Tokens #3260

stackman27 · 2022-11-07T01:43:44Z

Part of : #2579

What is the purpose of the change

This message MsgDelegateToValidatorSet allows delegators to delegate to a set of defined validator set. For ex: delegate 10osmo with validator-set {ValA -> 0.5, ValB -> 0.3, ValC -> 0.2} our delegate logic would attempt to delegate 5osmo to A , 2osmo to B, 3osmo to C

Brief Changelog

n/a

Testing and Verifying

Added tests for TestDelegateToValidatorSet

Documentation and Release Note

Does this pull request introduce a new feature or user-facing behavior changes? (yes / no)
Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? (yes / no)
How is the feature or change documented? (not applicable / specification (x/<module>/spec/) / Osmosis docs repo / not documented)

x/valset-pref/validator_set.go

faddat · 2022-11-08T19:35:10Z

why?

not that I have any issue with it, it seems cool, but curious on thinking here

stackman27 · 2022-11-08T23:15:36Z

why?

not that I have any issue with it, it seems cool, but curious on thinking here

Hi @faddat! This feature can technically be done off-chain today with front-ends, having preference lists stored locally, but Having this on chain:

eases front-end code burden that is already very high in cosmos
reduces front-end lock-in, giving you the same UX across multiple front-ends
Opens door for features such as auto-staking

x/valset-pref/msg_server_test.go

x/valset-pref/validator_set.go

mattverse

Looking good so far!
Main logic looks lgtm, left some minor comments for the first round of review

x/valset-pref/README.md

x/valset-pref/validator_set.go

x/valset-pref/msg_server_test.go

mattverse · 2022-11-15T13:56:17Z

x/valset-pref/msg_server_test.go

+		expectedShares []sdk.Dec
+		expectPass     bool
+		valSetExists   bool
+	}{


would it be possible to add a test case that deals delegating coins that wouldn't be calculated perfectly under the ratio and test the rounding conditions? Ex) 739 or similar

added it here 45964da

cannot see it due to force push :( , can you point me to the specific test case?

Yup, i changed the set-validator weights to have upto 3 decimal places https://github.com/osmosis-labs/osmosis/pull/3260/files#diff-102acff6e2fd0c8c3b2b95afe0c166f91100a1dcbc1204c40a0ff981823504a3R31

mattverse · 2022-11-15T13:56:45Z

x/valset-pref/msg_server_test.go

+		expectPass     bool
+		valSetExists   bool
+	}{
+		{


Ditto with the comment above regarding jailed validator. Consider adding a test case on it as well

hmm not sure how i can create a test scenario where a validator is jailed and user tries to delegate to that validator 🤔

So i did some digging around and found out that "It is possible to delegate to a jailed validator, the only difference being it will not be added to the power index until it is unjailed" therefore, i dont think we can add a very indepth test

how about unbonded or unbonding validators?

Here's what i found;

unbonded: Validators will be removed from active set but still can receive delegations

unbonding: Validators leaves active set either through slashing, jailing etc, all delegations can be unbonded but will have to wait for unbonding time to exhaust

I am hoping to test all these in either e2e or testnet

stackman27 · 2022-11-18T23:50:25Z

Thank you @mattverse and @czarcas7ic for the feedback. Everything should be resolved now :))

czarcas7ic

This LGTM, well done! Once we get another review here and this gets merged, please ensure to test this on a localosmosis instance with all the applicable CLI commands you used. This way we can easily test come next upgrade!

x/valset-pref/msg_server_test.go

x/valset-pref/validator_set.go

stackman27 · 2022-11-26T08:11:14Z

Thank you @czarcas7ic addressed all you comments and added more test coverage 06f64d

still looking on tombstoned validators and ways to approach them

czarcas7ic · 2022-11-26T16:28:29Z

@stackman27 that comment was more to add it to the TODO, and not a blocker for this PR IMO

mattverse

LGTM!

Left some minor comments I would love to see before merging!

mattverse · 2022-11-28T10:00:36Z

x/twap/api.go

@@ -58,17 +93,16 @@ func (k Keeper) GetArithmeticTwap(
 	if err != nil {
 		return sdk.Dec{}, err
 	}
-	return computeArithmeticTwap(startRecord, endRecord, quoteAssetDenom)
+	return strategy.computeTwap(startRecord, endRecord, quoteAssetDenom)


doesnt this change to getTwapToNow anyways if endTime.Equal(ctx.BlockTime())?

oops this change wasn't supposed to be in this PR, but endTime = BlockTime logic is covered in line 84. We can follow up this conversation in this PR #3529

mattverse · 2022-11-28T10:05:08Z

x/valset-pref/msg_server_test.go

+		expectPass     bool
+		valSetExists   bool
+	}{
+		{


how about unbonded or unbonding validators?

x/valset-pref/validator_set.go

mattverse · 2022-11-28T10:11:06Z

x/valset-pref/msg_server_test.go

+		expectedShares []sdk.Dec
+		expectPass     bool
+		valSetExists   bool
+	}{


cannot see it due to force push :( , can you point me to the specific test case?

ValarDragon · 2022-12-01T20:46:08Z

Merging due to approvals, and this PR not being merged blocking things

github-actions bot added the C:docs Improvements or additions to documentation label Nov 7, 2022

stackman27 added the V:state/compatible/backport State machine compatible PR, should be backported label Nov 7, 2022

stackman27 marked this pull request as ready for review November 7, 2022 01:52

stackman27 requested review from a team and alexanderbez November 7, 2022 01:52

stackman27 commented Nov 7, 2022

View reviewed changes

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

stackman27 requested review from czarcas7ic, mattverse and p0mvn November 7, 2022 01:56

stackman27 changed the title ~~[ValSet-Pref] Created MsgDelegateToValidatorSet~~ [ValSet-Pref] Delegate tokens for a val-set using MsgDelegateToValidatorSet Nov 7, 2022

stackman27 changed the title ~~[ValSet-Pref] Delegate tokens for a val-set using MsgDelegateToValidatorSet~~ [ValSet-Pref] Delegate tokens to a val-set using MsgDelegateToValidatorSet Nov 7, 2022

stackman27 mentioned this pull request Nov 7, 2022

[ValSet-Pref] UnDelegate tokens from val-set using MsgUnDelegateFromValidatorSet #3288

Closed

czarcas7ic reviewed Nov 14, 2022

View reviewed changes

mattverse reviewed Nov 15, 2022

View reviewed changes

stackman27 requested review from czarcas7ic and mattverse November 18, 2022 23:50

stackman27 force-pushed the sis/valset-pref-DelMsg branch from 45964da to 7c6ed74 Compare November 19, 2022 00:21

stackman27 changed the title ~~[ValSet-Pref] Delegate tokens to a val-set using MsgDelegateToValidatorSet~~ [ValSet-Pref] Add Msgs for Delegate and Undelegate Tokens Nov 21, 2022

czarcas7ic approved these changes Nov 26, 2022

View reviewed changes

stackman27 force-pushed the sis/valset-pref-DelMsg branch from fac3784 to 06f64dc Compare November 26, 2022 08:09

github-actions bot added the C:x/twap Changes to the twap module label Nov 26, 2022

mattverse approved these changes Nov 28, 2022

View reviewed changes

stackman27 force-pushed the sis/valset-pref-DelMsg branch from 06f64dc to d2da989 Compare November 29, 2022 02:43

github-actions bot removed the C:x/twap Changes to the twap module label Nov 29, 2022

refactored twap api.go for geometric TWAP

c6ff8b6

stackman27 added 10 commits November 30, 2022 20:19

working delegate tokens function

4d33cb6

added multiplier

7797147

nit

726f574

nit

ae441f4

removed multiplier

9dfcc99

adam comments

b04dcd1

matts feedback

bacbdf5

added undelegate logic

b7c31d0

adams feedback

050a9ee

matts comments

69b24a8

stackman27 force-pushed the sis/valset-pref-DelMsg branch from d2da989 to 69b24a8 Compare December 1, 2022 04:20

ValarDragon added V:state/breaking State machine breaking PR and removed V:state/compatible/backport State machine compatible PR, should be backported labels Dec 1, 2022

ValarDragon merged commit a095e33 into main Dec 1, 2022

ValarDragon deleted the sis/valset-pref-DelMsg branch December 1, 2022 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ValSet-Pref] Add Msgs for Delegate and Undelegate Tokens #3260

[ValSet-Pref] Add Msgs for Delegate and Undelegate Tokens #3260

stackman27 commented Nov 7, 2022

faddat commented Nov 8, 2022

stackman27 commented Nov 8, 2022

mattverse left a comment

mattverse Nov 15, 2022

stackman27 Nov 18, 2022

mattverse Nov 28, 2022

stackman27 Nov 29, 2022

mattverse Nov 15, 2022

stackman27 Nov 18, 2022

stackman27 Nov 22, 2022

mattverse Nov 28, 2022

stackman27 Nov 29, 2022

stackman27 commented Nov 18, 2022

czarcas7ic left a comment

stackman27 commented Nov 26, 2022

czarcas7ic commented Nov 26, 2022

mattverse left a comment

mattverse Nov 28, 2022

stackman27 Nov 29, 2022

mattverse Nov 28, 2022

mattverse Nov 28, 2022

ValarDragon commented Dec 1, 2022

[ValSet-Pref] Add Msgs for Delegate and Undelegate Tokens #3260

[ValSet-Pref] Add Msgs for Delegate and Undelegate Tokens #3260

Conversation

stackman27 commented Nov 7, 2022

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

faddat commented Nov 8, 2022

stackman27 commented Nov 8, 2022

mattverse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 commented Nov 18, 2022

czarcas7ic left a comment

Choose a reason for hiding this comment

stackman27 commented Nov 26, 2022

czarcas7ic commented Nov 26, 2022

mattverse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Dec 1, 2022