[ValSet-Pref] Make Redelegate to valset read all your current stake positions #3833
Conversation
x/valset-pref/msg_server_test.go
Outdated
| coinToStake: sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(20_000_000)), | ||
| expectedShares: []sdk.Dec{sdk.NewDec(10_000_000), sdk.NewDec(6_000_000), sdk.NewDec(4_000_000)}, | ||
| valSetDelegationExist: true, | ||
| expectPass: false, // cannot redelegate to same validator set |
There was a problem hiding this comment.
Why is this the design? If I delegate to A (0.2) B (0.5) C (0.3) and want to redelegate to balance to A (0.3) B (0.5) C (0.2), why doesn't this do a single redelegate message from C to A?
There was a problem hiding this comment.
So in the sdk it checks if sourceAddr == destAddr (code)
But the test failure is happening because of our redelegation algorithm, for ex:
Existing Set {A: 0.2, B: 0.5, C: 0.3}, NewSet {A: 0.3, B: 0.5, C: 0.2}
Delegate 20osmo
ExistingSet{A: 4, B: 10, C: 6}, NewSet{A: 6, B: 10, C: 4}, DiffSet: {A:4, B:10, C:6, A: -6, B: -10, C: -4}
Perform Redelegation
A --redelegate--> B (4osmo)
B --redelegate--> B (6osmo) (ERROR: Self delegation)
** Suppose we force the source and destination address to be different**
Perform Redelegation
A --redelegate--> B (4osmo)
B --redelegate--> A (6osmo) (ERROR: Redelegation to ValB is already in progress)
To fix this the algorithm should be aware of the val addresses it's using and also if redelegation is possible from that address which could make things a lil complicated, i can make an issue on this and explore it over holidays
There was a problem hiding this comment.
I thought about this a lot and there's still issues even if we make sourceValAddr != targetValAddr
something like this for instance;
"If I delegate to A (0.2) B (0.5) C (0.3) and want to redelegate to balance to A (0.3) B (0.5) C (0.2), why doesn't this do a single redelegate message from C to A?"
Redelegation has to be done from C --> A, and then A --> C. But A --> C isn't possible because "Redelegation to ValidatorA is in progress and must complete before we perform another redelegation"
stackman27
force-pushed
the
sis/curr-stake-pos
branch
from
7f027b7 to
56a82a1
Compare
|
@ValarDragon this PR is R4R |
|
converting this to draft because, I think its better if we merge this after #3857 |
stackman27
force-pushed
the
sis/curr-stake-pos
branch
2 times, most recently
from
238a121 to
7c0ebd6
Compare
| } | ||
|
|
||
| // reDelegationAmt to is the amount to redelegate, which is the min of diffAmount and target_validator | ||
| reDelegationAmt := sdk.MinDec(target_val.amount.Abs(), diff_val.amount) | ||
| reDelegationAmt := sdk.MinDec(target_val.amount.Abs(), diff_val.amount).TruncateDec() |
There was a problem hiding this comment.
If we truncateDec here, would it not be possible to go into a infinite for loop?
There was a problem hiding this comment.
hrmm i haven't seen any cases for infinite loop yet. I will be simulator testing this feature so, if i find anything i will address it
| @@ -554,11 +529,9 @@ func (suite *KeeperTestSuite) TestRedelegateValidatorSet() { | |||
| del, _ := suite.App.StakingKeeper.GetDelegation(suite.Ctx, test.delegator, valAddr) | |||
| suite.Require().Equal(del.Shares, test.expectedShares[i]) | |||
| } | |||
|
|
|||
| } else { | |||
| suite.Require().Error(err) | |||
There was a problem hiding this comment.
Can we add checks that exisitng delegations pertain?
There was a problem hiding this comment.
hmm so like check if the existing validators amount decreased? I'm a lil confused with what exactly to test
There was a problem hiding this comment.
no the opposite, if redelegation has failed due to an error, we should not have changed positions for delegations, but the delegation for the account should remain the same. Thus we want to check here if this is the case. Would this make sense?
There was a problem hiding this comment.
oh yea, to check if the state reverts right. I have confirmed through CLI testing that it reverts!
stackman27
force-pushed
the
sis/curr-stake-pos
branch
from
9b85371 to
918c1c8
Compare
Closes: #3761
Part of : #2579
What is the purpose of the change
In #3599 the redelegate to (new valset pref list) message redelegates from your current valset.
Instead, we should change this to redelegate from all of your current staked positions. So that way the UX can be "Set new valset preference", "Redelegate everything to this new preference list", and it gracefully handles that you used to stake without a preference list.
Brief Changelog
n/a
Testing and Verifying
testing redelegation with existing non-valset delegation
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? (yes / no)x/<module>/spec/) / Osmosis docs repo / not documented)