[ValSet-Pref] Add `WithdrawDelegationRewards` message #3686

stackman27 · 2022-12-10T23:34:07Z

Part of : #2579

What is the purpose of the change

Adds new message WithdrawDelegationRewards.

WithdrawDelegationRewards withdraws all the delegation rewards from the validator in the val-set.
Delegation reward is collected by the validator and in doing so, they can charge commission to the delegators.
Rewards are calculated per period, and is updated each time validator delegation changes.

Brief Changelog

n/a

Testing and Verifying

Added msg_server test for to check functionality

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/keeper_test.go

x/valset-pref/msg_server_test.go

ValarDragon · 2022-12-20T18:41:41Z

x/valset-pref/types/expected_interfaces.go

@@ -19,3 +19,10 @@ type StakingInterface interface {
 type BankKeeper interface {
 	GetBalance(ctx sdk.Context, addr sdk.AccAddress, denom string) sdk.Coin
 }
+
+type CommunityPoolKeeper interface {


Suggested change

type CommunityPoolKeeper interface {

// For testing only

type DistributionKeeper interface {

Why for testing only?

Its only used in testing? The main logic shouldn't use it?

makes sense

We are using WithdrawDelegationRewards(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress) (sdk.Coins, error) in code that falls under this interface

ValarDragon

logic LGTM, but needs more entries in table test case, at minimum for no valset set, no delegations, no valset explicitly set, one existing delegation

stackman27 · 2022-12-20T21:45:42Z

@ValarDragon @czarcas7ic added more tests coverage as you suggested

ValarDragon · 2022-12-21T17:27:22Z

x/valset-pref/msg_server_test.go

+			name:          "Withdraw all rewards with existing delegation",
+			delegator:     sdk.AccAddress([]byte("addr1---------------")),
+			coinToStake:   sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(20_000_000)), // delegate 20osmo
+			setValSet:     true,


Should be false, right?

or we need a test case with both this set, and it not set?

in this context "existing delegation" is "existing valset delegations"

The WithdrawLogic also only check rewards from existing valset delegations

Oh, I would expect it to work s.t. if I have no valset explicitly set, it infers my valset is my existing delegation set, and withdraws from everyone via that

Yea i can get that to work, the only thing is that GetDelegatorDelegations(ctx sdk.Context, delegator sdk.AccAddress, maxRetrieve uint16) takes in uint16 and we might have to pass math.MaxUint16 that could get a little expensive

Just added withdraw existing delegation reward!

stackman27 · 2022-12-22T03:19:14Z

@ValarDragon @czarcas7ic added test cases and logic for existing staking position that's not valset. lmk what you think

ValarDragon · 2022-12-22T18:45:44Z

x/valset-pref/validator_set.go

+	delegations := k.stakingKeeper.GetDelegatorDelegations(ctx, delegator, math.MaxUint16)
+
+	// get the existing validator set preference
+	existingSet, found := k.GetValidatorSetPreference(ctx, delegatorAddr)


I guess I'm confused -- I thought the module design was always such that GetValidatorSetPreference returns your existing staking distribution if you haven't set one.

Is that not the case or only in query logic?

Unfortunately that is not the case, GetValidatorSetPreference only returns val-set staking distributions

ok, imo we need to change that (which I thought was the initial design for valset pref)

for sure, it wont be a massive change tbh. Logic will be something like.

func GetValidatorSetPreference() dels{ valsetDels, vsFound := getvalsetDels(...) if !vsFound { existingstakingDels, stFound := getstakingDels(...) if !stFound { return err ("No delegations") } return existingstakingDels } return valsetDels }

Few questions;

What about cases where we need both existing valset and staking position or either or? 2. For instance: In SetValSet, we donot care about existingstakingDels, same with delegateToValSet, but undelegate, withdraw and redelegate will need both

Also non valset positions will not have weights but the current code expects weights for every validator

if a valset is set return that. (ignore staking position, that may diverge)

You can derive the weight on the fly based on ratio of staked amounts

I'm down to merge this PR, but lets do the above as a release blocking feature.

Can you make an issue? Then we can merge this PR

added issue #3848

czarcas7ic

LGTM, now that issue has been created, I will merge this

github-actions bot added the C:app-wiring Changes to the app folder label Dec 10, 2022

stackman27 requested review from czarcas7ic, alexanderbez and mattverse December 10, 2022 23:34

stackman27 added the V:state/breaking State machine breaking PR label Dec 10, 2022

stackman27 force-pushed the sis/MsgDel-reward branch from e96b5e4 to ef78748 Compare December 13, 2022 03:19

czarcas7ic reviewed Dec 14, 2022

View reviewed changes

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

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

ValarDragon reviewed Dec 20, 2022

View reviewed changes

ValarDragon approved these changes Dec 20, 2022

View reviewed changes

stackman27 added 4 commits December 20, 2022 13:00

started work

d00a751

withdraw reward message added

aa2da21

updated gomod

45adaad

adam and dev feedback

1cbc820

stackman27 force-pushed the sis/MsgDel-reward branch from ef78748 to 1cbc820 Compare December 20, 2022 21:44

ValarDragon reviewed Dec 21, 2022

View reviewed changes

stackman27 added 3 commits December 21, 2022 12:27

nit

0ee104c

added existing staking position

f125b82

nit

787644c

stackman27 requested a review from czarcas7ic December 22, 2022 03:18

ValarDragon reviewed Dec 22, 2022

View reviewed changes

czarcas7ic approved these changes Dec 23, 2022

View reviewed changes

czarcas7ic merged commit 58dfab3 into main Dec 23, 2022

czarcas7ic deleted the sis/MsgDel-reward branch December 23, 2022 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ValSet-Pref] Add `WithdrawDelegationRewards` message #3686

[ValSet-Pref] Add `WithdrawDelegationRewards` message #3686

stackman27 commented Dec 10, 2022

ValarDragon Dec 20, 2022 •

edited

Loading

stackman27 Dec 20, 2022

ValarDragon Dec 21, 2022

stackman27 Dec 21, 2022

stackman27 Dec 22, 2022

ValarDragon left a comment

stackman27 commented Dec 20, 2022

ValarDragon Dec 21, 2022

ValarDragon Dec 21, 2022

stackman27 Dec 21, 2022

ValarDragon Dec 21, 2022

stackman27 Dec 21, 2022

stackman27 Dec 22, 2022

stackman27 commented Dec 22, 2022

ValarDragon Dec 22, 2022

stackman27 Dec 22, 2022 •

edited

Loading

ValarDragon Dec 23, 2022

stackman27 Dec 23, 2022 •

edited

Loading

ValarDragon Dec 23, 2022

ValarDragon Dec 23, 2022

stackman27 Dec 23, 2022

czarcas7ic left a comment

	type CommunityPoolKeeper interface {
	// For testing only
	type DistributionKeeper interface {

[ValSet-Pref] Add WithdrawDelegationRewards message #3686

[ValSet-Pref] Add WithdrawDelegationRewards message #3686

Conversation

stackman27 commented Dec 10, 2022

What is the purpose of the change

Brief Changelog

Testing and Verifying

Documentation and Release Note

ValarDragon Dec 20, 2022 • edited Loading

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 left a comment

Choose a reason for hiding this comment

stackman27 commented Dec 20, 2022

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 Dec 22, 2022

Choose a reason for hiding this comment

stackman27 Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 Dec 23, 2022 • edited Loading

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

czarcas7ic left a comment

Choose a reason for hiding this comment

[ValSet-Pref] Add `WithdrawDelegationRewards` message #3686

[ValSet-Pref] Add `WithdrawDelegationRewards` message #3686

ValarDragon Dec 20, 2022 •

edited

Loading

stackman27 Dec 22, 2022 •

edited

Loading

stackman27 Dec 23, 2022 •

edited

Loading