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.

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

feat: implement true vesting accounts with clawback #155

Merged
merged 35 commits into from
Feb 15, 2022

Conversation

JimLarson
Copy link

Description

Closes: Agoric/agoric-sdk#4085

Implements "true" vesting accounts which are subject to clawback.

True vesting accounts have a lockup schedule and a vesting schedule. Any tokens may be staked, but cannot be withdrawn unless they are both unlocked and vested. At any time, the account which funded the true vesting account may command a "clawback" of the unvested amount. The clawback may result in the transfer of bonded or unbonding tokens.

Staking rewards are encumbered with vesting in proportion to the vested/unvested ratio of the staked tokens.

This PR contains:

  • proto definitions for the new account type and messages
  • updates the vesting app module and message handler to include a staking keeper
  • CLI for account creation and clawback commands
  • message handler methods for the new messages
  • utilities for merging schedules in various ways
  • implementation of new account type with clawback and reward methods
  • hook in distribution keeper for post-reward action on certain accounts
  • staking keeper operations to transfer staked and unbonding amounts

The last is the most critical to review, as it mutates core staking data structures.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • [PENDING] included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

Copy link
Collaborator

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Just posting now so that you have time to act on the bikeshed. Still reviewing the rest of the PR.

docs/core/proto-docs.md Show resolved Hide resolved
proto/cosmos/vesting/v1beta1/tx.proto Outdated Show resolved Hide resolved
x/auth/vesting/client/cli/tx.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Wow! Good stuff!

I didn't find anything that looked suspicious, but there are just a few nits I pointed out.

x/auth/vesting/msg_server.go Outdated Show resolved Hide resolved
x/auth/vesting/msg_server.go Outdated Show resolved Hide resolved
x/auth/vesting/msg_server.go Outdated Show resolved Hide resolved
}

// Clawback removes the unvested amount from a TrueVestingAccount.
// The destination defaults to the funder address, but
Copy link
Collaborator

Choose a reason for hiding this comment

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

Incomplete thought?

Copy link
Author

Choose a reason for hiding this comment

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

Completed.

x/auth/vesting/msg_server.go Outdated Show resolved Hide resolved
x/auth/vesting/types/period.go Outdated Show resolved Hide resolved
x/auth/vesting/types/vesting_account.go Outdated Show resolved Hide resolved
x/auth/vesting/types/vesting_account.go Outdated Show resolved Hide resolved
x/auth/vesting/types/vesting_account.go Outdated Show resolved Hide resolved
@JimLarson
Copy link
Author

I've addressed all but one to-fix comment, so the code is ready for review.

I'll be concurrently working on the tests. Please request particular tests to cover.

Comment on lines +630 to +709
// TransferDelegation changes the ownership of at most the desired number of shares.
// Returns the actual number of shares transferred. Will also transfer redelegation
// entries to ensure that all redelegations are matched by sufficient shares.
// Note that no tokens are transferred to or from any pool or account, since no
// delegation is actually changing state.
Copy link
Collaborator

Choose a reason for hiding this comment

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

My view has been that transfer delegation should be in the upstream staking module. It's useful in a lot of scenarios and I don't think it harms proof of stake security. I think there should be a message type for in the delegation module

Choose a reason for hiding this comment

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

Agree, this is useful and should be upstreamed to the SDK

Copy link
Author

Choose a reason for hiding this comment

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

Filed Agoric/agoric-sdk#4315 for a later PR to surface this feature.

if !found {
panic("validator not found") // shouldn't happen
}
wantShares, err := validator.SharesFromTokensTruncated(want)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should handle a slashing event correctly

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean "this should be changed so that it handles slashing" or "it's great that this already handles slashing"? It looks like validators are never removed if any delegator is still holding shares, but I made the code tolerate a missing validator, reading the delegation as having no value, both here and in similar following code..

Copy link
Collaborator

@zmanian zmanian left a comment

Choose a reason for hiding this comment

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

Looked for the obvious edge case like Clawback after slashing and it does the correct things

Copy link

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

This implementation has a lot of nice additions to the vesting logic. Unfortunately, it's really hard to follow through. I'd highly recommend using specific names for variables, adding more comments on top of the functions and inside them to complement the code and make it more maintainable.

As for the staking and distribution, I think they can be split into a separate PR and upstreamed to the SDK. I highly recommend using a distribution hook for rewards instead of a SmartAccount.

@@ -43,8 +48,39 @@ message MsgCreatePeriodicVestingAccount {
string to_address = 2 [(gogoproto.moretags) = "yaml:\"to_address\""];
int64 start_time = 3 [(gogoproto.moretags) = "yaml:\"start_time\""];
repeated Period vesting_periods = 4 [(gogoproto.nullable) = false];
bool merge = 5;

Choose a reason for hiding this comment

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

what does merge mean, can you add a comment?

Copy link
Author

Choose a reason for hiding this comment

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

It means to merge the described grant into an existing PeriodicVestingAccount, or create it if it does not exist.

Added a comment here, and for the like-named field in MsgCreateTrueVestingAccount.

Choose a reason for hiding this comment

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

what's the use case for merging?

Copy link
Author

Choose a reason for hiding this comment

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

You'll have to ask @dtribble or @rowgraus for the exact justification. We want merging for TrueVestingAccount too in order to be able to make staking rewards vest.

Comment on lines 62 to 77
string from_address = 1 [(gogoproto.moretags) = "yaml:\"from_address\""];
string to_address = 2 [(gogoproto.moretags) = "yaml:\"to_address\""];

Choose a reason for hiding this comment

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

are these the tx signer and vesting addresses? or the vesting and the "admin"?

Copy link
Author

Choose a reason for hiding this comment

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

Since we decided that creating a ClawbackVestingAccount (revised name, renaming commit will follow) is a non-privileged operation, we don't want to use the term "admin", so we're describing the one making the grant (and signing it) as the "funder". Added descriptions in all the request message fields to make things more clear.

BaseVestingAccount base_vesting_account = 1 [(gogoproto.embed) = true];

// funder_address specifies the account which can perform clawback.
string funder_address = 2;

Choose a reason for hiding this comment

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

nit, I'd rename this to admin

Copy link
Author

Choose a reason for hiding this comment

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

In this implementation we decided not to make it a privileged operation to create a vesting account, so it would be better to have a name that doesn't suggest a privileged role. Open to suggestions other than "funder", though.

Choose a reason for hiding this comment

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

sounds good

// funder_address specifies the account which can perform clawback.
string funder_address = 2;
int64 start_time = 3 [(gogoproto.moretags) = "yaml:\"start_time\""];
repeated Period combined_periods = 4 [(gogoproto.moretags) = "yaml:\"combined_periods\"", (gogoproto.nullable) = false];

Choose a reason for hiding this comment

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

what are combined periods?

Copy link

@liamsi liamsi Jan 17, 2022

Choose a reason for hiding this comment

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

They are explained here: Agoric/agoric-sdk#4085 (comment)
Probably good to have inline comments for these as well IMO.

But I also don't find it very easy to follow.

Copy link

Choose a reason for hiding this comment

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

Can someone answer this:

Let's say someone's tokens are subject to 36 months vesting AND lockup from TGE, released every 6 months.
After month 6, is it:

  • Option 1: 1/6th of "subject-to-lockup" tokens become vested, but only 1/6th of "subject-to-lockup" tokens are unlocked, which means that in fact they can only sell 1/36th of those tokens;

OR

  • Option 2: the lockup runs concurrently to vesting; e.g. for all tokens regardless of vesting, it is the case that no more than x tokens shall be sold according to some lockup schedule. So after 6 months, 1/6th of tokens will be unlocked and unvested.

Or are both scenarios possible/configurable depending on the schedule of each? If so, how?

Copy link
Author

Choose a reason for hiding this comment

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

I'll add comments here too. Combined periods gives the merge of the unlocking schedule and the vesting schedule, to determine when tokens are actually unencumbered. For instance, if I make a clawback grant of 1000 today (2022-01-17) that vests a 1/4th every 3months, but all of it is locked until Nov 1, then we'd have:

Unlocking schedule:
2022-11-01: 1000

Vesting schedule:
2022-04-17: 250
2022-07-17: 250
2022-10-17: 250
2023-01-17: 250

Combined schedule (when tokens are both vested and unlocked):
2022-11-01: 750
2023-01-17: 250

Per a note in vesting_accounts.go, I'm considering dropping the combined_periods field and just querying the two schedules directly. I'd expect the common use case is a vesting schedule with dozens of events, but with a lockup cliff, so this would be just as efficient.

Choose a reason for hiding this comment

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

I think we can remove the combined schedule actually 👍

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I'll implement that.

BTW, this PR is fighting against the prevailing terminology in x/auth/vesting in that it uses "vesting" only to describe the status associated with clawback, and "lockup" to describe the encumbrance that prevents transfer - i.e. what is called "vesting" elsewhere. Let me know if I need to be even more emphatic about this in the comments.

Choose a reason for hiding this comment

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

What would you have instead of the combined schedule then? I think it's important to keep it, because Option 1 of the comment above is unlikely to be the understanding of what most people mean by combined vesting and lockup, or the contractual terms.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, we'll have the same behavior, but we just won't precompute the combined schedule. I.e. to get the unlocked coins at time t, instead of computing ReadSchedule(combined_periods, t), we'd compute min(ReadSchedule(lockup_periods, t), ReadSchedule(vesting_periods, t). (Not the exact API calls, of course, but that's the idea.)


funderString, _ := cmd.Flags().GetString(FlagFunder)

Choose a reason for hiding this comment

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

shouldn't this be the from address?

Copy link
Author

Choose a reason for hiding this comment

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

Ugh - we've got some ugly choices here and I'd appreciate a better suggestion. At clawback time, we're transferring funds from the vesting account to a destination account, based on a message signed by the funder who was the "from" address when the account was created. So at clawback time, "from" is dangerously ambiguous. My proposed solution here is that at clawback time we say "funder" to avoid ambiguity, but at creation time we say "from" for consistency with the other commands to create vesting accounts.

Choose a reason for hiding this comment

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

Tbh I think maintaining from is totally OK since this is the CLI (this will remove additional flags) and I'd expect users to use gRPC (which can use the funder name)

Copy link
Author

Choose a reason for hiding this comment

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

Turns out to be necessary to get the cli tests to work - at least without more poking into it. Done.

// keeping a liability for 25 shares and transferring one for 75 shares.
// Of course, the redelegations themselves can have multiple entries for
// different timestamps, so we're actually working at a finer granularity.
redelegations := k.GetRedelegations(ctx, fromAddr, math.MaxUint16)

Choose a reason for hiding this comment

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

ditto use iterator

Copy link
Author

Choose a reason for hiding this comment

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

It looks like you break the contract if you modify the section of the store you're iterating over. See comment on Iterator() method of KVStore in store/types/store.go.

// SmartRewardAccount is an account with a post-reward processing function.
// Such an account ignores a non-default withdrawal address, as this can
// be implemented in the post-reward processing if desired.
type SmartRewardAccount interface {

Choose a reason for hiding this comment

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

why not introduce Distribution Hooks instead of a new account?

Copy link
Author

Choose a reason for hiding this comment

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

See below.

err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, withdrawAddr, coins)
if err != nil {
return nil, err
}
if isSmart {
smartAcc.PostReward(ctx, coins, k.authKeeper, k.bankKeeper, k.stakingKeeper)

Choose a reason for hiding this comment

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

I think this should be an AfterWithdrawRewards hook.

Copy link
Author

Choose a reason for hiding this comment

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

We also need to prevent the use of a different withdrawAddr when we're dealing with a vesting account with clawback. I could have another hook method, AllowWithdrawAddr(acc authtypes.AccountI) bool - although this is odd, since there are no other hook methods that return a value or affect execution of the calling module.

Vesting accounts themselves are precedent for having modules modify their behavior based on account type - in its interaction with the x/bank module. However, I'm not sure if that's a good precedent to follow.

So I could use the following odd hook interface. I'm not sure if the resulting code is any simpler or more idiomatic:

type interface DistributionHook {
  AllowWithdrawAddr(acc authtypes.AccountI) bool
  AfterReward(ctx sdk.Context, acc authTypes.AccountI, reward sdk.Coins)
}

Or I could keep this as-is. Let me know which you'd prefer.

(BTW, why don't the hook-calling modules allow for an arbitrary number of registered hooks? It doesn't seem any harder.)

type StakingKeeper struct {
    ...
    hooks []types.StakingHooks
    ...
}

func (k Keeper) BeforeFoo(ctx sdk.Context, addr sdk.AccAddress, foo types.Whatever) {
    for _, hook := range k.hooks {
        hook.BeforeFoo(ctx, addr, foo)
    }
}

Copy link
Author

Choose a reason for hiding this comment

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

Okay, so I implemented distribution hooks as I described. It was a little tricky getting the wiring correct to tie it all together, but now it's more general, and I think we'll have some other uses for it.

unbondings := sk.GetUnbondingDelegations(ctx, tva.GetAddress(), math.MaxUint16)
for _, unbonding := range unbondings {
for _, entry := range unbonding.Entries {
unbonded = unbonded.Add(entry.Balance)

Choose a reason for hiding this comment

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

shouldn't this be unbonding?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it should, thanks. I'll expand the test coverage for this.

tokens := validator.TokensFromSharesTruncated(shares).RoundInt()
bonded = bonded.Add(tokens)
}
return

Choose a reason for hiding this comment

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

nit, the return values should be explicit

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vesting accounts with clawback
7 participants