WIP: Vesting Implementation #2168

AdityaSripal · 2018-08-28T01:00:51Z

Starting implementation for Vesting

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

…ditya/vesting

cwgoes

Good start, see comments.

cwgoes · 2018-08-29T12:28:59Z

x/auth/account.go

+// already been transferred or delegated
+func (vacc ContinuousVestingAccount) SendableCoins(blockTime time.Time) sdk.Coins {
+	unlockedCoins := vacc.TransferredCoins
+	scale := float64(blockTime.Unix() - vacc.StartTime.Unix()) / float64(vacc.EndTime.Unix() - vacc.StartTime.Unix())


Floats are dangerous - we should use sdk.Dec here.

oh yeah floats are non-deterministic, we pretty much can't use them anywhere

cwgoes · 2018-08-29T12:29:40Z

x/auth/account.go

+
+		// Must constrain with coins left in account
+		// Since some unlocked coins may have left account due to delegation
+		currentAmount := vacc.GetCoins().AmountOf(c.Denom).Int64()


We should do all this match on the sdk.Int type, not convert to int64.

cwgoes · 2018-08-29T12:30:14Z

x/auth/account.go

+	scale := float64(blockTime.Unix() - vacc.StartTime.Unix()) / float64(vacc.EndTime.Unix() - vacc.StartTime.Unix())
+
+	// Add original coins unlocked by vesting schedule
+	for _, c := range vacc.OriginalVestingCoins {


I don't understand why we need this loop - can't we just subtract TransferredCoins from OriginalVestingCoins?

cwgoes · 2018-08-29T12:31:53Z

x/bank/keeper.go

@@ -175,6 +175,19 @@ func addCoins(ctx sdk.Context, am auth.AccountMapper, addr sdk.AccAddress, amt s
 // SendCoins moves coins from one account to another
 // NOTE: Make sure to revert state changes from tx on error
 func sendCoins(ctx sdk.Context, am auth.AccountMapper, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) (sdk.Tags, sdk.Error) {
+	// check if sender is vesting account
+	blockTime := ctx.BlockHeader().Time
+	vacc, ok := am.GetAccount(ctx, fromAddr).(auth.VestingAccount)


Is there a way we can implement this which just implements BaseAccount differently and returns SendableCoins() for GetCoins()?

Hmm... wouldn't that entail changing the Account interface?

Account's an interface, we can implement a different GetCoins() for the VestingAccount struct?

Don't believe we can without a lot of changes to bank and other modules that depend on it.

For example, bank keeper's subtractCoins method needs GetCoins to return the full amount in the account. Otherwise the staking handler will only allow you to delegate unlocked coins

I actually think this is important to do though. A large premise of the SDK is that you should be able to add your own account types that can support all the features you want within your other modules. Here this is a sign that the API we provide is insufficient, or we haven't tried to make vesting fit into the correct API. We want anyone to be able to achieve the functionality they want with their custom accounts without modding baseapp like this.

I think we should refactor things accordingly so that vesting can be its own full module, not an add-on within auth.

Perhaps we add a "UseableCoins" function to account that takes in the context. This would have utility outside of just vesting, e.g. for covenants and the like.

x/bank/keeper.go

codecov · 2018-09-04T18:55:41Z

Codecov Report

Merging #2168 into develop will increase coverage by 0.4%.
The diff coverage is 90.74%.

@@            Coverage Diff             @@
##           develop    #2168     +/-   ##
==========================================
+ Coverage    61.52%   61.93%   +0.4%     
==========================================
  Files          122      122             
  Lines         7486     7587    +101     
==========================================
+ Hits          4606     4699     +93     
- Misses        2561     2569      +8     
  Partials       319      319

addressed comments

cwgoes

I still just don't like putting this logic in x/bank, I think it's the wrong approach to abstraction: it places the requirement to manage token tracking correctly on the caller, and it increases the surface area for bugs in any other modules which might utilize a VestingAccount.

What about changing the account interface itself, so that instead of having GetCoins and SetCoins, an account has AddCoins, SubtractCoins, GetSpendableCoins, and GetTotalCoins (or something, but you get the idea). That way we could encapsulate the vesting logic cleanly within auth and guarantee that any module calling methods on the account can't handle vesting accounts incorrectly by accident.

Is this too much of an interface change? Replies / other ideas welcome.

rigelrozanski · 2018-09-07T18:53:45Z

Action items based on conversation with @jaekwon @cwgoes @alexanderbez

Add Delegate and Undelegate Coins to the bank keeper (very similar to Subtract and Add Coins but different implications for vesting coins)
- update everything in x/stake to use this
more intelligent subtract coins to prevent vested coins to be subtracted - using type switch in bank keeper
make sure that add coins is positive

AdityaSripal · 2018-09-08T01:54:44Z

it places the requirement to manage token tracking correctly on the caller

I think to some extent this is unavoidable because the token updating behavior of the account changes depending on who the caller is (banking vs staking module). I do think there are ways to alleviate this by changing the Account interface tho.

Maybe we can allow the Account interface to distinguish between a coin update that is caused by a transfer and one that is caused by something else (i.e. staking)

So either we could have in Account interface:

SetCoins, GetCoins, TransferCoins(amt)
and any caller who intends to transfer coins (rather than an isolated set coins) should call setcoins.

OR

SetCoins(oldargs, bool transfer), GetCoins(bool transfer)

where the SetCoins and GetCoins changes behavior depending on whether the caller intends to set/get because of a transfer.

=========================================================================

Add Delegate and Undelegate Coins to the bank keeper (very similar to Subtract and Add Coins but different implications for vesting coins)

I think this is an ok solution, but do we also want vesting accounts to bond governance deposits to create proposals. If so, allowing a special case for staking only seems strange.

more intelligent subtract coins to prevent vested coins to be subtracted - using type switch in bank keeper

Don't we want DelegateCoins to subtract vesting coins from the account? If so, changing subtract coins is wrong and we should be changing the transfer-specific functions as I have done.

AdityaSripal · 2018-09-08T02:11:53Z

Another point on Action Items:

If only DelegateCoins can subtract vesting coins, then an account with only vesting coins will not be able to create a delegate transaction because there are no vested coins to submit for the transaction fee.

There needs to be a way for feeKeeper to subtract vestingcoins for the fee if no vested coins are available.

ValarDragon · 2018-09-08T19:03:22Z

x/auth/account.go

+// Else return all coins in account (like BaseAccount)
+func (vacc DelayTransferAccount) SendableCoins(blockTime time.Time) sdk.Coins {
+	// Check if ctx.Time < EndTime
+	if blockTime.Unix() < vacc.EndTime.Unix() {


whats the reason for casting these both to unix? They should be comparable in time.Time format. (e.g. time1.Before(time2)

ValarDragon · 2018-09-08T19:04:24Z

x/auth/account.go

+	}
+
+	// If EndTime has passed, DelayTransferAccount behaves like BaseAccount
+	return vacc.BaseAccount.GetCoins()


Is deleting the vesting account and making a base-account w/ the same address something thats easy to do? (I feel like doing this makes one less edge-case to be concerned with)

It was in the original spec but later removed because it is technically unnecessary. The thinking was it takes up a little bit more space, and one could always transfer all coins into a baseaccount after vesting.

Fair point about the edge case. It is easy to do but makes the code more complicated.

I think it will be easier to verify correctness of auto-transitioning a vesting account to a base account than it will be verify correctness of a vesting account once done vesting. (I also think it makes the mental model of this far simpler, and consequently safer) Hence why I think its probs worth doing.

The address would remain the same, so this change wouldn't be noticeable by the other modules as well.

Fair point about the edge case. It is easy to do but makes the code more complicated.

I don't see why it adds a non-neglible increase to code complexity. Isn't basically just setting whats in the account mapper to a new base account with the correct number of tokens?

alexanderbez · 2018-09-28T18:55:06Z

@jaekwon et al, summarizing our discussion from earlier today:

Delegating coins from a vesting account currently supports delegating with both vestING (locked) and vestedED (plus received) coins.

The problem arises when the account decides to Undelegate. Any coins delegated that were vestING should be returned as vestING (what about rewards/inflation?).

One solution was to only allow for delegation of vestING coins, but this comes with its own challenges, primarily around the annoyance of having to move funds around to delegate vestED coins.
Another arose from the misnomer of vested/vesting coins...these coins actually have no such direct property. Maybe introduce some meta fields on a coin?

AdityaSripal · 2018-09-28T21:50:18Z

@alexanderbez Not making vested/vesting a property of the coin IS what makes undelegating easy

Instead whenever someone tries to withdraw from account, the calculation is made dynamically to determine how many of the account's coins are vesting vs unlocked.

If Vesting becomes a property of the coin, I think implementing something like ContinuousVestingAccount becomes much more complicated

alexanderbez · 2018-09-28T21:58:02Z

@AdityaSripal agreed -- we still need to come up with a better solution.

alexanderbez · 2018-10-04T23:17:06Z

@AdityaSripal @cwgoes @rigelrozanski @ValarDragon

Had a discussion with Jae earlier and the following was suggested to update in the spec:

Removing the notion of Transferred Coins.
Adding a new field, DelegatedVestingCoins.

With that in mind, a vesting account can be thought to have the following notation:

DelegatedVestingCoins: Total amount of delegated coins that were vesting at delegation
OriginalVestedCoins: Total amount of original vesting coins. This is immutable.
LockedCoins: The amount of OriginalVestedCoins that is currently locked according to the vesting schedule.
UnlockedCoins: The amount of OriginalVestedCoins that is currently unlocked (vested) according to the vesting schedule.
Total: The amount of coins returns by account.GetCoins (i.e. the total amount of coins in the base embedded account).

The following operations on a vesting account can be defined as such:

Receiving coins

Adds to the Total (i.e. account.SetCoins(...).

Sending coins

A vesting account can only send what is available. This is defined as an amount up to:
Total + DelegatedVestingCoins - LockedCoins. This amount will be deducted from Total.

Note, the account may have Total go below the amount of LockedCoins due to delegation.

Delegating coins

A vesting account can delegate from its vesting pool and vested pool. Assuming it wants to delegate D coins, first, determine how many of the vesting coins it can delegate:LockedCoins - DelegatedVestingCoins = X. Add min(X, D) to DelegatedVestingCoins.

Presuming the account has enough, deduct D from Total.

Undelegating coins

When D coins go back to a vesting account due to an undelegation, we must determine how many of those coins go back as vesting. This is determined by: min(DelegatedVestingCoins, D) = X. We then deduct X from DelegatedVestingCoins.

Finally, D is added to the Total.

I've ran through a few examples, albeit late at night, and didn't see anything obviously wrong, but would like thoughts on this.

jaekwon · 2018-10-04T23:25:58Z

here's another representation of the same:


Types of coins:
- vesting coins
  - delegated vesting coins
  - remaining vesting coins
- vested coins
- excess coins


DV: Delegated Vesting
UN: How much is unlocked and spendable of OV
OV: Original vesting amount -- does not change
 L: portion of UV is vesting
UL: portion of UV is vested
BC: BaseAccount.Coins = OV - transferred (can be negative) - delegated (DV + other)

|==========DV   
|============<<L==========OV original vesting coins
               |==========UL "unlocked coins"
|====================BC


# When delegate I want to delegate D coins

1. L-DV => X, how many vesting coins we can still delegate
2. DV += min(X, D)
3. BC -= D

# When undelegating D coins,

1. min(DV, D) => X, how many undelegating coins to neutralize DV
2. DV -= X
3. BC += D

# When transferring coins,

1. allow transferring up to min((BC+DV)-L, BC)

AdityaSripal · 2018-10-05T00:08:32Z

it looks like the Vesting period freezes during delegation.

If all the coins I have in an account are vesting with 1 month left, then I delegate them out for 2 months. When they come back into my account, it looks like they are still vesting? Shouldn't it be undelegated by that point since the vesting continues during delegation?

I feel like the desired behavior should be that coins continue vesting while they are also used as delegation. Is there a reason this behavior was changed or am I just wrong about the above?

cwgoes · 2018-10-05T10:33:57Z

I've ran through a few examples, albeit late at night, and didn't see anything obviously wrong, but would like thoughts on this.

Questions:

When are we recalculating LockedCoins / UnlockedCoins? (I think this relates to @AdityaSripal's question above).
This doesn't really respect the notion that "particular" coins are delegated - if I delegate 2 vesting coins to A, then 2 vesting coins to B, wait for 2/4 of my total coins to unlock, and undelegate from B, I immediately "get" spendable coins, but (arguably) the coins that I delegated to B should be "still vesting" if I had delegated the earliest-release coins first to A. Maybe we're OK with this but worth noting.

cwgoes · 2018-10-05T18:36:26Z

@jaekwon Also point 2 above is more relevant if the delegation in question gets slashed.

cwgoes · 2018-10-08T14:22:53Z

Upon further reflection, I think the existence of slashing is a serious problem here. Consider the following sequence of events (all occurring in quick succession, so no change in the vested coin amount):

I start out with 5 free coins and 5 vesting coins.
I delegate 5 coins to validator A and 5 coins to validator B.
1. DelegatedVestingCoins is now 5
Validator A gets slashed by 50%, making my delegation to A now worth 2.5 coins.
I undelegate both delegations.
1. DelegatedVestingCoins is still 5, so 5 of the coins go back as vesting.
I end up with 2.5 free coins and 5 vesting coins.

If validator A had been slashed by 100%, I would have ended up with 5 vesting coins and no free coins - and worse, if both validators got slashed and I later delegated free coins they would be turned back into vesting coins when I undelegated.

Possibly we could rectify this by updating DelegatedVestingCoins when a delegation is slashed, but (a) that's nontrivial since the validator's tokens change but the delegation's shares do not, and (b) we still need some way to choose "which" coins (vesting or free) get slashed.

cc @jaekwon @alexanderbez

jaekwon · 2018-10-08T17:43:59Z

When are we recalculating LockedCoins / UnlockedCoins? (I think this relates to @AdityaSripal's question above).

LockedCoins/UnlockedCoins are calculated dynamically based on the constants OriginalVestingCoins and start/end dates.

it looks like the Vesting period freezes during delegation.
If all the coins I have in an account are vesting with 1 month left, then I delegate them out for 2 months. When they come back into my account, it looks like they are still vesting? Shouldn't it be undelegated by that point since the vesting continues during delegation?

See above.

I start out with 5 free coins and 5 vesting coins.

5 vesting, 0 delegated vesting, 10 BC.

I delegate 5 coins to validator A and 5 coins to validator B.

4.99 vesting (time passed), 5 delegated vesting, 5 BC.

DelegatedVestingCoins is still 5, so 5 of the coins go back as vesting.
I end up with 2.5 free coins and 5 vesting coins.

It should now be 4.98 vesting (time passed), 0 delegated vesting, 7.5 BC. This is fine.

I think there was a problem with slashing but not the way you mentioned... I think this is resolved with the new rule for sending/transfering coins: min((BC+DV)-L, BC) instead of (BC+DV)-L. That way, if you have delegated vesting coins that are slashed, it'll stay in DV and be unaccessible even after the vesting period is over.

|=======DV // some of these are forever gone due to slashing
|L========================OV original vesting coins
|=========================UL "unlocked coins"
|BC

cwgoes · 2018-10-08T21:38:04Z

OK, talked with @jaekwon, makes more sense now - note: if delegated vesting coins are slashed, L - DV may be negative, in which case we treat it as zero instead.

I still think "unlocked coins get slashed first" (above example) may be a problem.

jaekwon · 2018-10-09T20:43:33Z

@cwgoes is this what you were referring to?
Added DelegatedFree field.

type Account struct {
    BaseAccount
    OriginalVesting coins
    DelegatedVesting coins
    DelegatedFree coins
    Start datetime
    End datetime
}

BC: BaseAccount.Coins = OV - transferred (can be negative) - delegated (DV + DF)
OV: Original vesting amount (constant)
-  V: portion of OV that is still vesting (derived w/ start/end/now & OV)
DV: Delegated Vesting
DF: Delegated Free


|============<<V==========OV // original vesting coins, V moves left over time
|====================BC // when delegating, DV or DF goes up (and vice versa)
|==========DV
|===DF   

Could also be seen like this:

|============<<V==========OV
|==========DV================BC===DF // all coins including delegated coins

# When delegating D coins

0. check that BC >= D, and that D is positive
1. min(max(V-DV, 0), D) => X, portion of D that is vesting
2. D - X => Y, portion of D that is free
3. DV += X
4. DF += Y
5. BC -= D

# When undelegating D coins,

0. check that (DV+DF) >= D, and that D is positive
1. min(DF, D) => Y, portion of D that should become free (prioritizing free coins)
2. D - Y => X, portion of D that should become vesting
3. DV -= X
4. DF -= Y
5. BC += D

# When transferring coins,

1. allow transferring up to min((BC+DV)-V, BC)

cwgoes · 2018-10-09T21:39:16Z

@jaekwon The above makes sense to me, except:

DV - X => Y, portion of D that is free

I think you mean 2. D - X => Y (D instead of DV).

check that (DV+DF) >= D, and that D is positive

I don't understand why we need this check, shouldn't our existing undelegation logic ensure both?

jaekwon · 2018-10-09T22:24:04Z

I think you mean 2. D - X => Y (D instead of DV).

Yes.

check that (DV+DF) >= D, and that D is positive
I don't understand why we need this check, shouldn't our existing undelegation logic ensure both?

Just a sanity check / assertion, might as well?

cwgoes · 2018-11-02T12:39:44Z

@alexanderbez The plan for vesting has changed enough that I suspect it might be easier to close this PR and open a fresh one - do you agree?

alexanderbez · 2018-11-02T16:45:50Z

I agree 100%. Closing in favor of impending new implementation.

AdityaSripal added 3 commits August 27, 2018 17:57

start vesting implementation

dd75176

Merge branch 'develop' of https://github.com/cosmos/cosmos-sdk into a…

7caea52

…ditya/vesting

fix method signatures

4543dcf

AdityaSripal requested review from cwgoes, ebuchman and rigelrozanski as code owners August 28, 2018 01:00

AdityaSripal added prelaunch labels Aug 28, 2018

alexanderbez changed the title ~~Vesting Implementation~~ WIP: Vesting Implementation Aug 28, 2018

AdityaSripal assigned cwgoes Aug 28, 2018

cwgoes previously requested changes Aug 29, 2018

View reviewed changes

AdityaSripal unassigned cwgoes Aug 30, 2018

Switch float to Decimal. Keeper tests

4c21c59

cwgoes mentioned this pull request Sep 3, 2018

SDK 0.25 / Gaia-9000 release checklist #2220

Closed

23 tasks

AdityaSripal added 3 commits September 3, 2018 23:45

delay transfer tests

5703eae

minor fixes and test docs

0215365

fmt

a55164a

AdityaSripal changed the title ~~WIP: Vesting Implementation~~ R4R: Vesting Implementation Sep 4, 2018

AdityaSripal assigned cwgoes Sep 4, 2018

AdityaSripal added ready-for-review and removed wip labels Sep 4, 2018

AdityaSripal assigned rigelrozanski Sep 4, 2018

cwgoes suggested changes Sep 7, 2018

View reviewed changes

ValarDragon reviewed Sep 8, 2018

View reviewed changes

Aleksandr Bezobchuk added 10 commits September 26, 2018 17:08

Actually fix TestSubtractVestingFull and more cleanup

c1d746f

Existing unit test cleanup

b3bb5b1

More vesting unit test cleanup

748cd14

Add contracts for bank keeper methods

aff8b0e

Update bank keeper interface to include DelegateCoins and DeductFees

abd3eec

Implement unit test for bank keeper delegate coins

c30da51

Implement unit test for bank keeper deduct fees

4b9d008

Cleanup testing codec for auth structures

98d475c

Implement LockedCoins

2412404

Merge branch 'develop' into aditya/vesting

18aad2d

alexanderbez added the S:blocked Status: Blocked label Sep 28, 2018

alexanderbez mentioned this pull request Oct 24, 2018

R4R: Update Vesting Spec #2589

Merged

5 tasks

alexanderbez closed this Nov 2, 2018

cwgoes deleted the aditya/vesting branch November 2, 2018 21:31

WIP: Vesting Implementation #2168

WIP: Vesting Implementation #2168

Conversation

AdityaSripal commented Aug 28, 2018 • edited Loading

cwgoes 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

ValarDragon Sep 8, 2018 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 4, 2018 • edited Loading

Codecov Report

cwgoes left a comment • edited Loading

Choose a reason for hiding this comment

rigelrozanski commented Sep 7, 2018 • edited Loading

AdityaSripal commented Sep 8, 2018

AdityaSripal commented Sep 8, 2018

Choose a reason for hiding this comment

ValarDragon Sep 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

alexanderbez commented Sep 28, 2018 • edited Loading

AdityaSripal commented Sep 28, 2018

alexanderbez commented Sep 28, 2018

alexanderbez commented Oct 4, 2018

Receiving coins

Sending coins

Delegating coins

Undelegating coins

jaekwon commented Oct 4, 2018 • edited Loading

AdityaSripal commented Oct 5, 2018

cwgoes commented Oct 5, 2018 • edited Loading

cwgoes commented Oct 5, 2018 • edited Loading

cwgoes commented Oct 8, 2018

jaekwon commented Oct 8, 2018 • edited Loading

cwgoes commented Oct 8, 2018 • edited Loading

jaekwon commented Oct 9, 2018 • edited Loading

cwgoes commented Oct 9, 2018

jaekwon commented Oct 9, 2018

cwgoes commented Nov 2, 2018

alexanderbez commented Nov 2, 2018

AdityaSripal commented Aug 28, 2018 •

edited

Loading

ValarDragon Sep 8, 2018 •

edited

Loading

codecov bot commented Sep 4, 2018 •

edited

Loading

cwgoes left a comment •

edited

Loading

rigelrozanski commented Sep 7, 2018 •

edited

Loading

ValarDragon Sep 8, 2018 •

edited

Loading

ValarDragon Sep 11, 2018 •

edited

Loading

alexanderbez commented Sep 28, 2018 •

edited

Loading

jaekwon commented Oct 4, 2018 •

edited

Loading

cwgoes commented Oct 5, 2018 •

edited

Loading

cwgoes commented Oct 5, 2018 •

edited

Loading

jaekwon commented Oct 8, 2018 •

edited

Loading

cwgoes commented Oct 8, 2018 •

edited

Loading

jaekwon commented Oct 9, 2018 •

edited

Loading