Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Vesting Implementation #2168

Closed
wants to merge 25 commits into from
Closed

WIP: Vesting Implementation #2168

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
dd75176
start vesting implementation
AdityaSripal Aug 28, 2018
7caea52
Merge branch 'develop' of https://github.com/cosmos/cosmos-sdk into a…
AdityaSripal Aug 28, 2018
4543dcf
fix method signatures
AdityaSripal Aug 28, 2018
4c21c59
Switch float to Decimal. Keeper tests
AdityaSripal Sep 1, 2018
5703eae
delay transfer tests
AdityaSripal Sep 4, 2018
0215365
minor fixes and test docs
AdityaSripal Sep 4, 2018
a55164a
fmt
AdityaSripal Sep 4, 2018
bab8b34
Merge branch 'develop' into aditya/vesting
Sep 17, 2018
3a8235d
started changes to spec
AdityaSripal Sep 24, 2018
91547f4
Merge branch 'develop' into aditya/vesting
Sep 26, 2018
778b53d
Fix linting and minor cleanup of banking keeper
Sep 26, 2018
c1935a4
Cleanup vesting spec formatting
Sep 26, 2018
73224dd
Cleanup SendableCoins to make it easier to understand
Sep 26, 2018
d119e0f
fix error log
Sep 26, 2018
63c6f4a
Fix TestSubtractVestingFull
Sep 26, 2018
c1d746f
Actually fix TestSubtractVestingFull and more cleanup
Sep 26, 2018
b3bb5b1
Existing unit test cleanup
Sep 26, 2018
748cd14
More vesting unit test cleanup
Sep 27, 2018
aff8b0e
Add contracts for bank keeper methods
Sep 27, 2018
abd3eec
Update bank keeper interface to include DelegateCoins and DeductFees
Sep 27, 2018
c30da51
Implement unit test for bank keeper delegate coins
Sep 27, 2018
4b9d008
Implement unit test for bank keeper deduct fees
Sep 27, 2018
98d475c
Cleanup testing codec for auth structures
Sep 28, 2018
2412404
Implement LockedCoins
Sep 28, 2018
18aad2d
Merge branch 'develop' into aditya/vesting
Sep 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 132 additions & 2 deletions x/auth/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package auth

import (
"time"
"errors"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -30,11 +31,13 @@ type Account interface {
// AccountDecoder unmarshals account bytes
type AccountDecoder func(accountBytes []byte) (Account, error)

var _ Account = (*BaseAccount)(nil)
var _ VestingAccount = (*ContinuousVestingAccount)(nil)
var _ VestingAccount = (*DelayTransferAccount)(nil)

//-----------------------------------------------------------
// BaseAccount

var _ Account = (*BaseAccount)(nil)

// BaseAccount - base account structure.
// Extend this by embedding this in your AppAccount.
// See the examples/basecoin/types/account.go for an example.
Expand Down Expand Up @@ -115,6 +118,133 @@ func (acc *BaseAccount) SetSequence(seq int64) error {
return nil
}

// VestingAccount is an account that can define a vesting schedule
// Vesting coins can still be delegated, but only transferred after they have vested
type VestingAccount interface {
Account

// Returns true if account is still vesting, else false
// CONTRACT: After account is done vesting, account behaves exactly like BaseAccount
IsVesting(time.Time) bool

// Calculates amount of coins that can be sent to other accounts given the current time
SendableCoins(time.Time) sdk.Coins
// Called on bank transfer functions (e.g. bank.SendCoins and bank.InputOutputCoins)
// Used to track coins that are transferred in and out of vesting account after initialization
TrackTransfers(sdk.Coins)
}

// Implement Vesting Interface. Continuously vests coins linearly from StartTime until EndTime
type ContinuousVestingAccount struct {
BaseAccount
OriginalVestingCoins sdk.Coins // Coins in account on Initialization
TransferredCoins sdk.Coins // Net coins transferred into and out of account. May be negative

// StartTime and EndTime used to calculate how much of OriginalCoins is unlocked at any given point
StartTime time.Time
EndTime time.Time
}

func NewContinuousVestingAccount(addr sdk.AccAddress, originalCoins sdk.Coins, startTime, endTime time.Time) ContinuousVestingAccount {
bacc := BaseAccount{
Address: addr,
Coins: originalCoins,
}
return ContinuousVestingAccount{
BaseAccount: bacc,
OriginalVestingCoins: originalCoins,
StartTime: startTime,
EndTime: endTime,
}
}

// Implements VestingAccount interface.
func (vacc ContinuousVestingAccount) IsVesting(blockTime time.Time) bool {
return blockTime.Unix() > vacc.EndTime.Unix()
}

// Implement Vesting Account interface. Uses time in context to calculate how many coins
// has been released by vesting schedule and then accounts for unlocked coins that have
// 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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Add original coins unlocked by vesting schedule
for _, c := range vacc.OriginalVestingCoins {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

amt := int64(float64(c.Amount.Int64()) * scale)

// 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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if currentAmount < amt {
amt = currentAmount
// prevent double count of transferred coins
amt -= vacc.TransferredCoins.AmountOf(c.Denom).Int64()
}

// Add non-zero coins
if amt != 0 {
coin := sdk.NewInt64Coin(c.Denom, amt)
unlockedCoins = unlockedCoins.Plus(sdk.Coins{coin})
}
}

return unlockedCoins
}

// Implement Vesting Account. Track transfers in and out of account
// Send amounts must be negated
func (vacc *ContinuousVestingAccount) TrackTransfers(coins sdk.Coins) {
vacc.TransferredCoins = vacc.TransferredCoins.Plus(coins)
}

// Implements Vesting Account. Vests all original coins after EndTime but keeps them
// all locked until that point.
type DelayTransferAccount struct {
BaseAccount
TransferredCoins sdk.Coins // Any received coins are sendable immediately

// All coins unlocked after EndTime
EndTime time.Time
}

func NewDelayTransferAccount(addr sdk.AccAddress, originalCoins sdk.Coins, endTime time.Time) DelayTransferAccount {
bacc := BaseAccount{
Address: addr,
Coins: originalCoins,
}
return DelayTransferAccount{
BaseAccount: bacc,
EndTime: endTime,
}
}

// Implements VestingAccount
func (vacc DelayTransferAccount) IsVesting(blockTime time.Time) bool {
return blockTime.Unix() > vacc.EndTime.Unix()
}

// Implements VestingAccount. If Time < EndTime return only net transferred coins
// 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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Return net transferred coins
// If positive, then those coins are sendable
return vacc.TransferredCoins
}

// If EndTime has passed, DelayTransferAccount behaves like BaseAccount
return vacc.BaseAccount.GetCoins()
Copy link
Contributor

@ValarDragon ValarDragon Sep 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@ValarDragon ValarDragon Sep 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

// Implement Vesting Account. Track transfers in and out of account
// Send amounts must be negated
func (vacc *DelayTransferAccount) TrackTransfers(coins sdk.Coins) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the spirit of minimizing contracts, can we have two separate methods, "TrackSend", "TrackReceive" and have these functions handle negating.

vacc.TransferredCoins = vacc.TransferredCoins.Plus(coins)
}

//----------------------------------------
// Wire

Expand Down
50 changes: 50 additions & 0 deletions x/auth/account_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package auth

import (
"testing"
"fmt"
"time"

"github.com/stretchr/testify/require"

Expand Down Expand Up @@ -107,3 +109,51 @@ func TestBaseAccountMarshal(t *testing.T) {
require.NotNil(t, err)

}

func TestSendableCoinsContinuousVesting(t *testing.T) {
cases := []struct {
blockTime time.Time
transferredCoins sdk.Coins
delegatedCoins sdk.Coins
expectedSendable sdk.Coins
}{
// No tranfers
{time.Unix(0, 0), sdk.Coins(nil), sdk.Coins(nil), sdk.Coins(nil)}, // No coins available on initialization
{time.Unix(500, 0), sdk.Coins(nil), sdk.Coins(nil), sdk.Coins{{"steak", sdk.NewInt(500)}}}, // Half coins available at halfway point
{time.Unix(1000, 0), sdk.Coins(nil), sdk.Coins(nil), sdk.Coins{{"steak", sdk.NewInt(1000)}}}, // All coins availaible after EndTime
{time.Unix(2000, 0), sdk.Coins(nil), sdk.Coins(nil), sdk.Coins{{"steak", sdk.NewInt(1000)}}}, // SendableCoins doesn't linearly increase after EndTime

// Transfers
{time.Unix(0, 0), sdk.Coins{{"steak", sdk.NewInt(100)}}, sdk.Coins(nil), sdk.Coins{{"steak", sdk.NewInt(100)}}}, // Only transferred coins are sendable at time 0.
{time.Unix(500, 0), sdk.Coins{{"photon", sdk.NewInt(1000)}, {"steak", sdk.NewInt(100)}}, sdk.Coins(nil), sdk.Coins{{"photon", sdk.NewInt(1000)}, {"steak", sdk.NewInt(600)}}}, // scheduled coins + transferred coins
{time.Unix(500, 0), sdk.Coins{{"photon", sdk.NewInt(1000)}, {"steak", sdk.NewInt(-100)}}, sdk.Coins(nil), sdk.Coins{{"photon", sdk.NewInt(1000)}, {"steak", sdk.NewInt(400)}}}, // scheduled coins + transferred coins

// Delegations
{time.Unix(500, 0), sdk.Coins(nil), sdk.Coins{{"steak", sdk.NewInt(400)}}, sdk.Coins{{"steak", sdk.NewInt(500)}}}, // All delegated tokens are vesting
{time.Unix(500, 0), sdk.Coins(nil), sdk.Coins{{"steak", sdk.NewInt(800)}}, sdk.Coins{{"steak", sdk.NewInt(200)}}}, // Some delegated tokens were unlocked (300)
{time.Unix(1000, 0), sdk.Coins(nil), sdk.Coins{{"steak", sdk.NewInt(1000)}}, sdk.Coins(nil)}, // All coins are delegated

// Integration Tests: Transfers and Delegations
{time.Unix(0, 0), sdk.Coins{{"photon", sdk.NewInt(10)}, {"steak", sdk.NewInt(10)}}, sdk.Coins{{"steak", sdk.NewInt(5)}}, sdk.Coins{{"photon", sdk.NewInt(10)}, {"steak", sdk.NewInt(10)}}}, // Delegate some of transferred tokens
{time.Unix(500, 0), sdk.Coins{{"steak", sdk.NewInt(10)}}, sdk.Coins{{"steak", sdk.NewInt(400)}}, sdk.Coins{{"steak", sdk.NewInt(510)}}},
{time.Unix(500, 0), sdk.Coins{{"steak", sdk.NewInt(10)}}, sdk.Coins{{"steak", sdk.NewInt(800)}}, sdk.Coins{{"steak", sdk.NewInt(210)}}},
{time.Unix(500, 0), sdk.Coins{{"steak", sdk.NewInt(10)}}, sdk.Coins{{"steak", sdk.NewInt(1005)}}, sdk.Coins{{"steak", sdk.NewInt(5)}}},

{time.Unix(500, 0), sdk.Coins{{"steak", sdk.NewInt(-10)}}, sdk.Coins{{"steak", sdk.NewInt(400)}}, sdk.Coins{{"steak", sdk.NewInt(490)}}},
{time.Unix(500, 0), sdk.Coins{{"steak", sdk.NewInt(-10)}}, sdk.Coins{{"steak", sdk.NewInt(800)}}, sdk.Coins{{"steak", sdk.NewInt(190)}}},
{time.Unix(500, 0), sdk.Coins{{"steak", sdk.NewInt(-10)}}, sdk.Coins{{"steak", sdk.NewInt(990)}}, sdk.Coins(nil)},
}

for i, c := range cases {
_, _, addr := keyPubAddr()
vacc := NewContinuousVestingAccount(addr, sdk.Coins{{"steak", sdk.NewInt(1000)}}, time.Unix(0, 0), time.Unix(1000, 0))
coins := vacc.GetCoins().Plus(c.transferredCoins)
coins = coins.Minus(c.delegatedCoins) // delegation is not tracked
vacc.SetCoins(coins)
vacc.TrackTransfers(c.transferredCoins)

sendable := vacc.SendableCoins(c.blockTime)
require.Equal(t, c.expectedSendable, sendable, fmt.Sprintf("Expected sendablecoins is incorrect for testcase %d: {Transferred: %s, Delegated: %s, Time: %d",
i, c.transferredCoins, c.delegatedCoins, c.blockTime.Unix()))
}
}
40 changes: 40 additions & 0 deletions x/bank/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK got it

Copy link
Contributor

@ValarDragon ValarDragon Sep 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ok && vacc.IsVesting(blockTime) {
// check if account has enough unlocked coins
sendableCoins := vacc.SendableCoins(blockTime)
if !sendableCoins.IsGTE(amt) {
return nil, sdk.ErrInsufficientCoins(fmt.Sprintf("Vesting account does not have enough unlocked coins: %s < %s", sendableCoins, amt))
}
// Track the transfer amount (send negated)
vacc.TrackTransfers(amt.Negative())
}

_, subTags, err := subtractCoins(ctx, am, fromAddr, amt)
if err != nil {
return nil, err
Expand All @@ -184,16 +197,35 @@ func sendCoins(ctx sdk.Context, am auth.AccountMapper, fromAddr sdk.AccAddress,
if err != nil {
return nil, err
}
// check if receiver is a vesting account
vacc, ok = am.GetAccount(ctx, fromAddr).(auth.VestingAccount)
if ok && vacc.IsVesting(blockTime) {
// Track the transfer amount
vacc.TrackTransfers(amt)
}

return subTags.AppendTags(addTags), nil
}

// InputOutputCoins handles a list of inputs and outputs
// NOTE: Make sure to revert state changes from tx on error
func inputOutputCoins(ctx sdk.Context, am auth.AccountMapper, inputs []Input, outputs []Output) (sdk.Tags, sdk.Error) {
blockTime := ctx.BlockHeader().Time
allTags := sdk.EmptyTags()

for _, in := range inputs {
// Check if sender is vesting account
vacc, ok := am.GetAccount(ctx, in.Address).(auth.VestingAccount)
if ok && vacc.IsVesting(blockTime) {
AdityaSripal marked this conversation as resolved.
Show resolved Hide resolved
// check if vesting account has enough unlocked coins
sendableCoins := vacc.SendableCoins(blockTime)
if !sendableCoins.IsGTE(in.Coins) {
return nil, sdk.ErrInsufficientCoins(fmt.Sprintf("Vesting account does not have enough unlocked coins: %s < %s", sendableCoins, in.Coins))
}
// Track the transfer amount (send negated)
vacc.TrackTransfers(in.Coins.Negative())
}

_, tags, err := subtractCoins(ctx, am, in.Address, in.Coins)
if err != nil {
return nil, err
Expand All @@ -206,6 +238,14 @@ func inputOutputCoins(ctx sdk.Context, am auth.AccountMapper, inputs []Input, ou
if err != nil {
return nil, err
}

// check if receiver is a vesting account
vacc, ok := am.GetAccount(ctx, out.Address).(auth.VestingAccount)
if ok && vacc.IsVesting(blockTime) {
// Track the transfer amount
vacc.TrackTransfers(out.Coins)
}

allTags = allTags.AppendTags(tags)
}

Expand Down