feat: Clawback Continuous Vesting Account #195

hacheigriega · 2024-02-08T21:57:55Z

Motivation

This PR implements a custom vesting account type ClawbackContinuousVestingAccount that wraps the SDK's ContinuousVestingAccount to provide an additional feature of "clawing back" funds to the funder of the account. Note that now this custom type is the only vesting account type our chain supports.

Explanation of Changes

The sender of the create-vesting-account transaction is registered as the funder of the vesting account so that when the funder later sends a clawback transaction, the vesting (= unvested) funds are returned back to the funder. The implementation is complicated by the fact that the funds can be in various states. The funds are clawed-back from different sources in the following order: Unbonded (through standard bank module transfer) -> Unbonding delegations -> Bonded delegations (which may also be going through redelegation process). The transfer logic is taken from Agoric's TransferDelegation and TransferUnbonding. To integrate these staking keeper methods, we wire to the app the custom staking module that wraps the SDK's staking module. However, as evident in x/staking/module.go, the SDK's default staking handles the remaining functionalities of the staking module, and most of the code present in the custom staking module such as the create-validator-vrf transaction remains unactivated.

The clawed back amount is guaranteed to be all of the currently vesting amount, unless the vesting account's delegation has been slashed, and the account doesn't have sufficient unbonded funds to make up for the loss.

How to use the transactions:

tx create-vesting-account [to_address] [amount] [end_time] for creating a ClawbackContinuousVestingAccount
- The "to" account must not already exist.
- The "from" address is registered as the newly created vesting account's funder.
- Add a flag --disable-clawback to disallow clawback. If this flag is present, the funder field of the vesting account will be empty.
tx clawback [address]
- The funds that are still vesting are sent back to the funder's address.

⚠️Note the simulation testing is still being fixed - There is an issue with the simulation app state not being updated.

Testing

Integration testing and simulation testing

Related PRs and Issues

Closes #194

gluax

Just commenting since I'm not a cosmos expert.

It looks like we are replacing the original ContinousVestingAccoutns with Clawbackable ones. I thought we wanted both, more so for users than us; our personal main priority is Clawbacks for launch.

As for the legal stuff bc of licensing clash @jasperdg @mariocao I think GPLv3 is compatible with Apache 2. I believe this means we need to also include the Apache license. But might need to do more than that...

app/genesis.go

proto/sedachain/vesting/v1/tx.proto

gluax · 2024-02-14T19:48:03Z

x/staking/keeper/keeper.go

+// NOTE: This code was taken from
+// https://github.com/agoric-labs/cosmos-sdk/blob/f42d86980ddfc07869846c391a03622cbd7e9188/x/staking/keeper/delegation.go#L701
+// with slight modifications.


IDK who the right person to ask this question to is, but are we in the OK with their licensing and ours not conflicting?

Ah yeah, they are Apache, and we are GPL. I think we have to include a copy of the Apache license now. But I'm no expert

gluax · 2024-02-14T19:51:24Z

proto/sedachain/vesting/v1/tx.proto

+}
+
+// MsgCreateVestingAccount defines a message that creates a vesting account.
+message MsgCreateVestingAccount {


Same question of does this prevent creating an original continuous vesting account? We still want that behavior(although less priority I suppose), on top of being able to create a clawback one.

Or is this a entirely different vesting thing that just additionally gets added?

Currently it cannot create an original continuous vesting account. Adding that would be quite simple though, as we can just add a boolean field to the message to indicate clawback or not.

Honestly, I'm not positive if we want it or not, but I figured from a user perspective, to be able to do both might be nice. idk thoughts @mariocao?

If it is so simple, yes, why not :)

added a flag for disabling clawback.

hacheigriega · 2024-02-14T21:02:47Z

Just commenting since I'm not a cosmos expert.

It looks like we are replacing the original ContinousVestingAccoutns with Clawbackable ones. I thought we wanted both, more so for users than us; our personal main priority is Clawbacks for launch.

As for the legal stuff bc of licensing clash @jasperdg @mariocao I think GPLv3 is compatible with Apache 2. I believe this means we need to also include the Apache license. But might need to do more than that...

Good points. We can support both SDK's ContinousVestingAccount and our ClawbackContinousVestingAccountif we want. Not sure why we would want that though - just thought it would be simpler to support the clawback one only since they're identical except the clawback feature.

jim380 · 2024-02-14T23:40:39Z

When I run test-sim-determinism, MsgClawback fails in ABCI, particularly in RunTx when it’s called in DeliverTx in execModeFinalize mode.

If you run breakpoints or add a panic like below in Clawback, you will see that it never gets hits.

This confirms that the operation had failed in RunTx because msgs only get routed to the module protobuf msg services after a successful RunTx. Cc: @hacheigriega

x/vesting/keeper/msg_server.go

jim380

Looks great. Particularly the core logic in func (m msgServer) Clawback.

jim380 · 2024-02-15T21:15:56Z

cmd/seda-chaind/cmd/genaccounts.go

+					if err != nil {
+						return errors.New("funder of vesting account must be provided using from flag")
+					}


The error message is a bit ambiguous to me. "invalid funder address" would make more sense imo. funder on line 118 would be "" if no funder address was provided in the flag.

Also need to add a check for vestingStart > vestingEnd.

jim380 · 2024-02-15T22:00:29Z

proto/sedachain/vesting/v1/tx.proto

+  // funder_address is the address which funded the account.
+  string funder_address = 1;
+  // account_address is the address of the vesting to claw back from.
+  string account_address = 2;


Would be nice, almost necessary to add a dest_address like Agoric did. Funder could've either rotated her keys, or had her original compromised. We need to give them an option to clawback the funds even if the original key was no longer accessible.

jim380 · 2024-02-15T22:03:22Z

proto/sedachain/vesting/v1/tx.proto

+  repeated cosmos.base.v1beta1.Coin clawed_unbonded = 1 [
+    (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins",
+    (gogoproto.nullable) = false
+  ];
+
+  repeated cosmos.base.v1beta1.Coin clawed_unbonding = 2 [
+    (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins",
+    (gogoproto.nullable) = false
+  ];
+
+  repeated cosmos.base.v1beta1.Coin clawed_bonded = 3 [
+    (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins",
+    (gogoproto.nullable) = false
+  ];


nice addition!

jim380 · 2024-02-15T22:56:48Z

x/vesting/keeper/msg_server.go

+
+	// NOTE: errors checked during msg validation
+	vestingAccAddr := sdk.MustAccAddressFromBech32(msg.AccountAddress)
+	funderAddr := sdk.MustAccAddressFromBech32(msg.FunderAddress)


inb4 would be good to add a destination address like Agoric does.

jim380 · 2024-02-15T23:12:24Z

x/vesting/keeper/msg_server.go

+
+	toClawBack := coinsMin(vestingAccount.GetVestingCoins(ctx.BlockTime()), total) // might have been slashed
+
+	// Write now now so that the bank module sees unvested tokens are unlocked.


duplicate now

jim380 · 2024-02-15T23:34:08Z

x/vesting/keeper/msg_server.go

+func (m msgServer) Clawback(goCtx context.Context, msg *types.MsgClawback) (*types.MsgClawbackResponse, error) {
+	ctx := sdk.UnwrapSDKContext(goCtx)
+
+	// NOTE: errors checked during msg validation


Where is this msg validation?

hacheigriega added 9 commits February 8, 2024 16:57

feat: initial implementation of clawback continuous vesting

d00699b

feat: vesting bug fixes and simple integration testing

d52dbfb

feat: adding missing files from prev commit

ace7837

test: add integration and simulation tests for vesting

e8a1b9b

test: more comprehensive integration testing and clawback code cleanup

76ccb87

test: adding delegate and clawback sim ops

4d94950

fix: test and fix clawback with redelegation

dcb9413

refactor: relocate integration testing utils

cf1d521

refactor: lint and improve error handling

c0abc24

hacheigriega marked this pull request as ready for review February 14, 2024 18:22

hacheigriega requested a review from a team February 14, 2024 18:22

gluax reviewed Feb 14, 2024

View reviewed changes

fix: temporary fix by copying over staking autocli code

b23fee8

hacheigriega added 2 commits February 14, 2024 17:09

chore: lint

43d0cfb

chore: proto lint

78d3b2a

test: deactivate future ops in sim test for now

f9286d4

mariocao reviewed Feb 15, 2024

View reviewed changes

x/vesting/keeper/msg_server.go Outdated Show resolved Hide resolved

hacheigriega added 2 commits February 15, 2024 11:42

feat: add flag for disabling clawback

b274cb5

chore: update add-genesis-account command to use clawback vesting acc

3a60cd9

gluax approved these changes Feb 15, 2024

View reviewed changes

hacheigriega merged commit 3a60cd9 into main Feb 15, 2024
13 of 16 checks passed

hacheigriega deleted the hy/vesting branch February 15, 2024 22:22

jim380 reviewed Feb 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Clawback Continuous Vesting Account #195

feat: Clawback Continuous Vesting Account #195

hacheigriega commented Feb 8, 2024 •

edited

Loading

gluax left a comment

gluax Feb 14, 2024

gluax Feb 14, 2024

gluax Feb 14, 2024

gluax Feb 14, 2024

hacheigriega Feb 14, 2024

gluax Feb 14, 2024

mariocao Feb 15, 2024

hacheigriega Feb 15, 2024

hacheigriega commented Feb 14, 2024

jim380 commented Feb 14, 2024 •

edited

Loading

jim380 left a comment

jim380 Feb 15, 2024

jim380 Feb 15, 2024

jim380 Feb 15, 2024

jim380 Feb 15, 2024

jim380 Feb 15, 2024

jim380 Feb 15, 2024

jim380 Feb 15, 2024


		toClawBack := coinsMin(vestingAccount.GetVestingCoins(ctx.BlockTime()), total) // might have been slashed

		// Write now now so that the bank module sees unvested tokens are unlocked.

feat: Clawback Continuous Vesting Account #195

feat: Clawback Continuous Vesting Account #195

Conversation

hacheigriega commented Feb 8, 2024 • edited Loading

Motivation

Explanation of Changes

Testing

Related PRs and Issues

gluax 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

hacheigriega commented Feb 14, 2024

jim380 commented Feb 14, 2024 • edited Loading

jim380 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

hacheigriega commented Feb 8, 2024 •

edited

Loading

jim380 commented Feb 14, 2024 •

edited

Loading