Vesting Spec #1875

AdityaSripal · 2018-07-30T02:38:00Z

Linked to github-issue with discussion and accepted design
Updated all relevant documentation (docs/)
Updated all relevant code comments
Wrote tests
Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
Updated cmd/gaia and examples/

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)

codecov · 2018-07-30T02:48:37Z

Codecov Report

Merging #1875 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           develop   #1875   +/-   ##
=======================================
  Coverage     63.5%   63.5%           
=======================================
  Files          118     118           
  Lines         7006    7006           
=======================================
  Hits          4449    4449           
  Misses        2301    2301           
  Partials       256     256

rigelrozanski · 2018-07-30T05:28:27Z

docs/spec/auth/vesting.md

+`{"BlockLock": BlockLock}` gets created and put in initial state. Otherwise if `Type == "base"` a base account is created 
+and the `BlockLock` attribute of corresponding `GenesisAccount` is ignored. `InitChain` will panic on any other account types.
+
+### Pros and Cons


lol I'm not sure that a pro's con's section really belongs in a spec - but not sure where it belongs beyond a proposal github issue

zmanian · 2018-07-30T05:29:32Z

docs/spec/auth/vesting.md

+
+This paper specifies changes to the auth and bank modules to implement vested accounts for the Cosmos Hub. 
+The requirements for this vested account is that it should be capable of being initialized during genesis with
+a starting balance X coins and a vesting blocknumber N. The owner of this account should be able to delegate to validators and vote,


It would be way better if this used BFT time instead of block number

rigelrozanski · 2018-07-30T05:34:42Z

docs/spec/auth/vesting.md

+    // It is upto handler to use these appropriately
+    GetParams() map[string]interface{}
+    SetParams(map[string]interface{}) error
+}


shouldn't this be more like GetParams([]byte) []byte and SetParams([]byte, []byte)
p.s. golang maps are a non-deterministic type meaning we can't use them in the state-machine

rigelrozanski · 2018-07-30T05:35:09Z

docs/spec/auth/vesting.md

+
+The `VestedAccount` will be an implementation of `Account` interface that wraps `BaseAccount` with 
+`Type() => "vested` and params, `GetParams() => {"BlockLock": blockN (int64)}`. 
+`SetParams` will be disabled as we do not want to update params after vested account initialization.


I'm wondering if we can just eliminate Type (are we introducing it?) and only have a block-lock param - if it exists then we know it's vested, or rather just locked - which is the important part

Yea I want to introduce it. If we just want to support this specific case, we can. I think we should add the Type tho, because we may want to add other types of Account. And people building on top of sdk should be able to create multiple account types as well.

For example, in the future we may want to support NLocked Accounts where each month a new amount of coins get unlocked, or maybe some other feature. I don't think we should add cruft to BaseAccount every time we want something new and specific to our application.

imo, it makes sense to create different Account types that define their own parameters and have handlers that can handle specific account types.

Will move a lot of this into a proposal issue as you suggested and rewrite.
Thanks for the review!

rigelrozanski

Left a few comments throughout - I like the approach you've proposed of extending the account

This document is structured more like a proposal for these features, and it's placed in a funny location given that it affects bank and auth.

I think maybe this PR should actually be modifying the existing spec documents which are relevant, and also having a new document talking about vesting, but not changes to other modules. Like, let's phrase this as more definite changes and implementation requirements - not a proposal

jaekwon · 2018-07-30T22:53:17Z

docs/spec/auth/vesting.md

+// Account is a standard account using a sequence number for replay protection
+// and a pubkey for authentication.
+type Account interface {
+    Type() string // returns the type of the account


(btw, should be Vesting not Vested).

I'm not sure that having a top-level type is the right solution.
Would it make more sense to have an optional interface method, like

type VestingAccount interface { Account AssertIsVestingAccount() // existence implies that account is vesting. }

Then you an check with vacc, ok := acc.(VestingAccount); ok

That works well in our case where we are choosing from two possible account types.

I think we should leave the option open to adding more account types in the future, and for developers building on the SDK to have many supported account types.

In those cases, I think having a Type() function that handlers can use to switch their functionality based on the account type is better than trying to type cast for multiple account types.

jaekwon · 2018-07-30T22:53:29Z

docs/spec/auth/vesting.md

+handler can then call `GetParams` to handle the specific account type using the parameters it expects to 
+exist in the parameter map.
+
+The `VestedAccount` will be an implementation of `Account` interface that wraps `BaseAccount` with 


jaekwon · 2018-07-30T22:54:27Z

docs/spec/auth/vesting.md

+}
+```
+
+During `InitChain`, the GenesisAccount's are decoded. If they have `Type == "vested`, a vested account with parameters => 


GenesisAccounts are

jaekwon · 2018-07-30T23:07:48Z

Can someone document why we want to do this, as opposed to having one account that can vest continuously over time? The latter seems much more flexible and I am worried about having to deal with e.g. 24 accounts, or more if we want more fine-grained vesting.

Additional fields OrigTotalCoins, StartVestingTime, and EndVestingTime in the concrete VestingAccount struct, used only for calculating how much is required to stay in the account.
You can add and remove coins from the vesting account, but none of the coins can ever be less than OrigTotalCoins * Max(VectorOfOnes, Now-StartTimeVesting/(EndTimeVesting-StartTimeVesting)). The relevant function here is IsGTE() or IsPositive() after a subtraction.

I think it would require more tooling for us to deal with 24 accounts, than to deal with this more flexible vesting system.

AdityaSripal · 2018-07-31T00:08:28Z

@jaekwon I originally considered doing vesting like a step function, it ends up being a bit more complicated than what you're suggesting. This is because transactions that we want to allow a vesting account to make (like staking) will subtract coins. Thus, vesting accounts balance are allowed to go below the equation you posted.

One thing we could do is to have UnsendableCoins be initialized with the starting balance of the account. When the account delegates or deposits for a vote, the associated handler subtracts the tx amount from UnsendableCoins. Then, the above formula works by replacing OriginalTotalCoins with UnsendableCoins. This will also allow vesting account to send coins earned from fees/inflation.

The downside with this approach is that all handlers for messages we want vesting accounts to be able to call (e.g staking, governance) before fully vesting must be aware of the existence of vesting accounts and update their state correctly. Whereas with the current approach, only the bank module is aware and cares about the existence of vesting accounts.

Not sure how the pros and cons weigh out between the two approaches.

rigelrozanski · 2018-07-31T18:05:56Z

Jae:

Can someone document why we want to do this, as opposed to having one account that can vest continuously over time? The latter seems much more flexible and I am worried about having to deal with e.g. 24 accounts, or more if we want more fine-grained vesting.

^ @zmanian

rigelrozanski · 2018-07-31T18:09:22Z

Just realized I don't think we want any of this.. Account already IS an interface which means we can have multiple implementations of Account - why don't we just implement new a new Account for vesting which is the same as sdk.AccAccount BUT with different GetCoins() sdk.Coins and SetCoins(sdk.Coins) error methods which don't allow the withdrawal of tokens earlier than is allowable?

AdityaSripal · 2018-07-31T18:19:41Z

Yea, realized we shouldn't be changing Account interface as well. Reworking spec

But we can't just implement SetCoins differently. This is because we still want staking handler to be able to subtract coins from our account.

I'll finish up what I have in mind and hopefully that clears things up.

rigelrozanski · 2018-07-31T18:31:36Z

Awesome - yeah sounds good, excited to see your thinking

AdityaSripal · 2018-08-01T01:13:01Z

docs/spec/auth/vesting.md

+The requirements for this vesting account is that it should be capable of being initialized during genesis with
+a starting balance X coins and a vesting blocktime T. The owner of this account should be able to delegate to validators 
+and vote with locked coins, however they cannot send locked coins to other accounts until those coins have been unlocked. 
+The vesting account should also be able to spend any coins it receives from other users or from fees/inflation rewards. 


Old spec was not able to spend coins gained from fees/inflation. New spec will allow vesting account to do so.

wait this is kind of seperate, the ability to spend coins received through the fee-distribution mechanism is independent of the vesting account because the fee-distribution mechanism allows you with withdraw fees to a seperate account - we should keep these distinct and not make the vesting account have special logic for this

Right, the purpose of ReceivedCoins is to makes sure that funds sent to the VestingAccount after initialization are spendable immediately.

AdityaSripal · 2018-08-01T01:14:29Z

docs/spec/auth/vesting.md

+```
+
+During `InitChain`, the GenesisAccounts are decoded. If `EndTime == 0`, a BaseAccount gets created and put in Genesis state. 
+Otherwise a vesting account is created with `StartTime = RequestInitChain.Time`, `EndTime = gacc.EndTime`, and `OriginalCoins = Coins`. 


Flagging for future reference. Since we are using the time in Genesis file as start time, we should make sure creation of Genesis file is close-ish to launch. Otherwise it messes up the slope for continuous vesting.

AdityaSripal · 2018-08-02T22:17:22Z

@jaekwon the checks described in this spec happen "upstream" from the checks that are already in bank's Keeper methods. Thus, I am really calculating the maximum amount of unlocked coins this account can spend.

If the formula written says you are allowed to spend 15 steak, but you only have 10 steak in the account because you have delegated 5 of your unlocked coins, the keeper will still only allow you to spend up to 10 steak.

There's no need to convert. I think it's worthwhile because Vesting accounts take up more space and gas cost for running transactions. Thus, it makes sense to me to do the one-time conversion though it isn't necessary

I don't think its possible to avoid changing bank. This is because we want all the other modules to be able to set account Coins as normal, but bank module needs special logic before it can set Coins. Thus, the logic can't be implemented at the account or mapper level. I'm not sure how a different message type alleviates this though interested to hear the proposal.

Ex for further clarity:

Let's say I start with 50 steak in a vesting account of which 30 is locked.
I have 40 steak delegated to various validators.

I get sent 10 steak by some other users.

The maximum amount of unlocked coins I am allowed to spend calculated by formula in spec: 30 steak
Amount of steak I should be allowed to spend at this moment: 20 steak

In order for a bank MsgSend to succeed, it checks:
Amount less than maximum allowed spend (30 steak)
Amount less than current balance (20 steak)

If both checks pass, the MsgSend succeeds. Else it fails.

addressed comments, added formulas at the bottom for easier verification

alexanderbez · 2018-08-06T15:09:12Z

@AdityaSripal I think the updates help greatly. It seems like the pseudo-code format could be improved a bit.

AdityaSripal · 2018-08-06T20:07:14Z

@alexanderbez anything specific about the pseudocode that was hard to read? I cleaned some of it up to hopefully make it clearer. Let me know if there's still some improvements to be made

alexanderbez · 2018-08-06T22:00:43Z

docs/spec/auth/vesting.md

+`sendCoins` and `inputOutputCoins`. These methods must check that an account is a vesting account using the check described above.
+
+```go
+if Now < vacc.EndTime:


@AdityaSripal I was mostly referring to this 👍

cwgoes

See comments, also:

Can we describe how ContinuousVestingAccount implements BaseAccount (in particular GetCoins)?

cwgoes · 2018-08-14T09:12:27Z

docs/spec/auth/vesting.md

+```go
+type VestingAccount interface {
+    Account
+    AssertIsVestingAccount() // existence implies that account is vesting.


Why do we need this function?

cwgoes · 2018-08-14T09:13:29Z

docs/spec/auth/vesting.md

+
+    // Calculates total amount of unlocked coins released by vesting schedule
+    // May be larger than total coins in account right now
+    TotalUnlockedCoins(sdk.Context) sdk.Coins


Does this need to be a public method?

Yes, it will be used in bank's keeper methods

cwgoes · 2018-08-14T09:14:29Z

docs/spec/auth/vesting.md

+type VestingAccount interface {
+    Account
+    AssertIsVestingAccount() // existence implies that account is vesting.
+    ConvertAccount(sdk.Context) BaseAccount


This just accesses BaseAccount functions, right? Convert is a strange name, it implies that one account is being turned into another, maybe AsBase()?

Originally the plan was to check if account has fully vested (i.e. Now > EndTime) and then simply return the embedded BaseAccount.
This BaseAccount would then replace the completely vested VestingAccount in the Account store. The main benefit would be that BaseAccount would take up less space in store but it is certainly not necessary.

After talking to @jaekwon , removing this functionality for now.

cwgoes · 2018-08-14T09:14:48Z

docs/spec/auth/vesting.md

+// Continuously vests by unlocking coins linearly with respect to time
+type ContinuousVestingAccount struct {
+    BaseAccount
+    OriginalCoins sdk.Coins // Coins in account on Initialization


i.e. that are vesting? VestingCoins?

No upon initialization all coins in OriginalCoins are vesting, true. However as time goes on, some of those coins become vested but OriginalCoins never gets updated.

OriginalCoins denotes all the coins that are in account on initialization, which are the only coins subject to the vesting schedule.
However as these coins go from vesting to vested, OriginalCoins never changes.

The way I understand it, a coin that is unspendable is "vesting" and once it becomes spendable it is "vested". It's entirely possible that this is not correct terminology.

Maybe "OriginalVestingCoins" would make it clearer?

cwgoes · 2018-08-14T09:15:04Z

docs/spec/auth/vesting.md

+    OriginalCoins sdk.Coins // Coins in account on Initialization
+    ReceivedCoins sdk.Coins // Coins received from other accounts
+
+    // StartTime and EndTime used to calculate how much of OriginalCoins is unlocked at any given point


I think these will now be time.Time since we've merged TM 0.23

cwgoes · 2018-08-14T09:16:05Z

docs/spec/auth/vesting.md

+
+// ConvertAccount converts VestingAccount into BaseAccount
+// Will convert only after account has fully vested
+ConvertAccount(vacc ContinuousVestingAccount, ctx sdk.Context) (BaseAccount):


Are we planning to save the converted account or something? Don't we need to deal with any remaining OriginalCoins or ReceivedCoins?

Addressed above, will remove

cwgoes · 2018-08-14T09:16:40Z

docs/spec/auth/vesting.md

+
+// Uses time in context to calculate total unlocked coins
+TotalUnlockedCoins(vacc ContinuousVestingAccount, ctx sdk.Context) sdk.Coins:
+    unlockedCoins := ReceivedCoins + OriginalCoins * (Now - StartTime) / (EndTime - StartTime)


This is continuous, but I think we want discrete vesting (once per month)?

I could be wrong, if you've discussed this with others please disregard

Yeah for now it's continuous because that is what jae and I discussed last. I'm currently trying to confirm what the preferred vesting schedule is for AiB.

Changing the vesting schedule just comes down to changing implementation of this single method.

cwgoes · 2018-08-14T09:18:03Z

docs/spec/auth/vesting.md

+    // NOTE: SendableCoins may be greater than total coins in account 
+    // because coins can be subtracted by staking module
+    // SendableCoins denotes maximum coins allowed to be spent.
+    if msg.Amount > vestingAccount.TotalUnlockedCoins() then fail


What if I've already sent some unlocked coins? I don't think this check is correct.

Yea, originally thought the second check would stop this but it won't.

Fixing this in refactor

cwgoes · 2018-08-14T09:18:34Z

docs/spec/auth/vesting.md

+
+// Account fully vested, convert to BaseAccount
+else:
+    account = ConvertAccount(account) 


See above comment, also I'm not clear on why we need to convert in the first place, the space savings don't matter much since the number of vesting accounts is finite and very small

Yup, removing this functionality

cwgoes · 2018-08-14T09:19:58Z

docs/spec/auth/vesting.md

+
+**Maximum amount of coins spendable right now:**
+
+`min( ReceivedCoins + OriginalCoins - LockedCoins, CurrentCoins )`


This seems to contradict the above definition under keeper changes?

cwgoes · 2018-08-14T09:22:47Z

Discrete vesting is not hard as far as I know, just use time floor div vesting interval.

jaekwon · 2018-08-16T01:55:09Z

docs/spec/auth/vesting.md

+`SentCoins`, `StartTime`, and `EndTime` to calculate how many coins are sendable at any given point. 
+Since the vesting restrictions need to be implemented on a per-module basis, the `ContinuousVestingAccount` implements 
+the `Account` interface exactly like `BaseAccount`. Thus, `ContinuousVestingAccount.GetCoins()` will return the total of 
+both locked coins and unlocked coins currently in the account.


Maybe good to add "Delegated coins are deducted from Account.GetCoins(), but do not count against unlocked coins because they are still at stake and will be reinstated (partially if slashed) after waiting the full unbonding period."

jaekwon · 2018-08-16T02:04:30Z

docs/spec/auth/vesting.md

+originally own should increment `ReceivedCoins` by the amount sent.
+Unlocked coins that are sent to other accounts will increment the vesting account's `SentCoins` attribute.
+
+CONTRACT: Handlers SHOULD NOT update `ReceivedCoins` if they were originally sent from the vesting account. For example, if a vesting account unbonds from a validator, their tokens should be added back to account but `ReceivedCoins` SHOULD NOT be incremented.


Staking handlers SHOULD NOT update ...

jaekwon · 2018-08-16T02:05:30Z

Left comments, otherwise LGTM.

Addressed comments

rigelrozanski

Should merge unless @cwgoes or @alexanderbez have additional comments

cwgoes

utACK

AdityaSripal added 4 commits July 27, 2018 18:35

initial progress on vesting spec

3445a85

simplify spec

39d1cf6

clarify requirements

69d1fe2

improve readability

89494ef

AdityaSripal requested a review from zramsay as a code owner July 30, 2018 02:38

AdityaSripal changed the title ~~WIP: Vesting Spec~~ Vesting Spec Jul 30, 2018

AdityaSripal added spec ready-for-review labels Jul 30, 2018

rigelrozanski reviewed Jul 30, 2018

View reviewed changes

zmanian reviewed Jul 30, 2018

View reviewed changes

rigelrozanski reviewed Jul 30, 2018

View reviewed changes

rigelrozanski previously requested changes Jul 30, 2018

View reviewed changes

Addressed basic comments

41130f8

AdityaSripal added this to In progress in Current Iteration via automation Jul 30, 2018

AdityaSripal moved this from In progress to Review Needed Now in Current Iteration Jul 30, 2018

jaekwon reviewed Jul 30, 2018

View reviewed changes

rigelrozanski mentioned this pull request Jul 31, 2018

Support for different Account types #1886

Closed

4 tasks

New idea for spec

d4d7658

AdityaSripal commented Aug 1, 2018

View reviewed changes

AdityaSripal added 2 commits August 3, 2018 13:12

addressed comments, added formulas for easy verification

10b2e83

improve readability

9c1918e

better pseudocode

d56e3a7

alexanderbez reviewed Aug 6, 2018

View reviewed changes

AdityaSripal added 3 commits August 6, 2018 15:23

even better pseudocode

2ac55eb

Cleaned up spec further

feb9a22

more readable pseudo

7539e21

cwgoes previously requested changes Aug 14, 2018

View reviewed changes

Fix bug, clearer logic

599b8ba

jaekwon reviewed Aug 16, 2018

View reviewed changes

jae comments

c79e130

rigelrozanski approved these changes Aug 22, 2018

View reviewed changes

cwgoes approved these changes Aug 22, 2018

View reviewed changes

cwgoes merged commit 1adb53e into develop Aug 22, 2018

Current Iteration automation moved this from Review Needed Now to Done Aug 22, 2018

cwgoes deleted the aditya/vest-spec branch August 22, 2018 13:07

cwgoes mentioned this pull request Sep 3, 2018

SDK 0.25 / Gaia-9000 release checklist #2220

Closed

23 tasks

This was referenced Oct 2, 2023

[Snyk] Security upgrade postcss from 8.4.24 to 8.4.31 naiba4/cosmos-sdk#1

Open

[Snyk] Security upgrade postcss from 8.4.24 to 8.4.31 naiba4/cosmos-sdk#2

Open

stefan-thothmind mentioned this pull request Oct 2, 2023

[Snyk] Security upgrade postcss from 8.4.21 to 8.4.31 stefan-thothmind/cosmos-sdk#10

Open

keylogic1 mentioned this pull request Oct 6, 2023

[Snyk] Security upgrade postcss from 8.4.27 to 8.4.31 neutron-org/cosmos-sdk#6

Closed


		Maximum amount of coins spendable right now:

		`min( ReceivedCoins + OriginalCoins - LockedCoins, CurrentCoins )`

Vesting Spec #1875

Vesting Spec #1875

Conversation

AdityaSripal commented Jul 30, 2018

codecov bot commented Jul 30, 2018 • edited

Codecov Report

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

rigelrozanski 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

jaekwon commented Jul 30, 2018 • edited

AdityaSripal commented Jul 31, 2018

rigelrozanski commented Jul 31, 2018

rigelrozanski commented Jul 31, 2018

AdityaSripal commented Jul 31, 2018

rigelrozanski commented Jul 31, 2018

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

AdityaSripal commented Aug 2, 2018 • edited

alexanderbez commented Aug 6, 2018

AdityaSripal commented Aug 6, 2018

Choose a reason for hiding this comment

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

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

cwgoes commented Aug 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaekwon commented Aug 16, 2018

rigelrozanski left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 30, 2018 •

edited

jaekwon commented Jul 30, 2018 •

edited

AdityaSripal commented Aug 2, 2018 •

edited

cwgoes commented Aug 14, 2018 •

edited