feat: clawback vesting accounts can return grants to the funder #342
Conversation
The use of the "action" interfaces is to allow account wrappers without creating cyclic package imports. But the mechanism only allows a single sequence of tests, not an arbitrary graph. Therefore we simplify (for now) that a ClawbackVestingAccount is the only account type that supports return-grants. If we add, say, PeriodicVestingAccount in the future, we'll add the ReturnGrants method to the GrantAccount interface, etc.
JimLarson
added
enhancement
| MsgReturnGrants defines a message for a grantee to return the unvested portion of a | ||
| vesting grant to the funder. Currently only applies to ClawbackVesting accounts. |
There was a problem hiding this comment.
Please update for current algorithm.
| MsgReturnGrants defines a message for a grantee to return the unvested portion of a | |
| vesting grant to the funder. Currently only applies to ClawbackVesting accounts. | |
| MsgReturnGrants defines a message for a grantee to return all granted assets, | |
| including delegated, undelegated and unbonding, vested and unvested, | |
| are transferred to the original funder of the account. Might not be complete if | |
| some vested assets have been transferred out of the account. Currently only applies to | |
| ClawbackVesting accounts. |
There was a problem hiding this comment.
This is a generated file, but I made the requested change upstream in proto/cosmos/vesting/v1beta1/tx.proto.
x/auth/vesting/types/msgs.go
Outdated
| @@ -17,6 +17,9 @@ const ( | |||
|
|
|||
| // TypeMsgClawback defines the type value for a MsgClawback. | |||
| TypeMsgClawback = "msg_clawback" | |||
|
|
|||
| // TypeMsgClawback defines the type value for a MsgClawback. | |||
There was a problem hiding this comment.
| // TypeMsgClawback defines the type value for a MsgClawback. | |
| // TypeMsgReturnGrants defines the type value for a MsgReturnGrants. |
| // validator has been removed | ||
| continue | ||
| } | ||
| wantShares, err := validator.SharesFromTokensTruncated(want) |
There was a problem hiding this comment.
What happens if the delegation has fewer shares than the corresponding want? Does wantShares get reduced here, or in the assignment to transferredShares below?
A clarifying comment would be appreciated.
There was a problem hiding this comment.
TransferDelegation() might transfer fewer shares than requested - added a comment.
|
|
||
| // Reduce DelegatedFree/Vesting by actual unbonding/staked amount transferred. | ||
| // The distinction between DF and DV is not significant in practice - only the | ||
| // sum in meaningful. Furthermore, DF/DV is not updated |
There was a problem hiding this comment.
| // sum in meaningful. Furthermore, DF/DV is not updated | |
| // sum is meaningful. Furthermore, DF/DV is not updated |
There was a problem hiding this comment.
Done - and completed the sentence fragment.
| // gotCoins should be zero at this point, unless DF+DV was not accurate | ||
|
|
||
| // If we've transferred everything and still haven't transferred the desired clawback amount, | ||
| // then the account must have most some unvested tokens from slashing. |
There was a problem hiding this comment.
| // then the account must have most some unvested tokens from slashing. | |
| // then the account must have lost some unvested tokens from slashing. |
| toXfer := coinsMin(amt, spendable) | ||
| err := bk.SendCoins(ctx, addr, dest, toXfer) | ||
| if err != nil { | ||
| return err // shouldn't happen, given spendable check |
There was a problem hiding this comment.
This should panic, for clarity that the vesting account state won't be changed on failure.
| return err // shouldn't happen, given spendable check | |
| panic(err) // shouldn't happen, given spendable check |
| // Any unbonding or staked tokens transferred are deducted from | ||
| // the DelegatedFree/Vesting fields. | ||
| // TODO: refactor to de-dup with clawback() | ||
| func (va *BaseVestingAccount) forceTransfer(ctx sdk.Context, amt sdk.Coins, dest sdk.AccAddress, ak AccountKeeper, bk BankKeeper, sk StakingKeeper) error { |
There was a problem hiding this comment.
If this function fails, it will leave the keepers in an inconsistent state, so either:
- don't return any errors, always panic on them here
- always panic on returned error in the caller
A panic will roll back the transaction.
There was a problem hiding this comment.
Done - removed the error return, here and in the parent caller too.
| require.False(t, found) | ||
|
|
||
| // check destination account | ||
| // want 1000fee, 83stake (18 unbonded, 5 unboinding, 60 bonded) |
There was a problem hiding this comment.
| // want 1000fee, 83stake (18 unbonded, 5 unboinding, 60 bonded) | |
| // want 1000fee, 83stake (18 unbonded, 5 unbonding, 60 bonded) |
| require.Equal(t, sdk.NewInt(60), stakeAmt) | ||
| ubd, found := app.StakingKeeper.GetUnbondingDelegation(ctx, dest, valAddr) | ||
| require.True(t, found) | ||
| require.Equal(t, sdk.NewInt(5), ubd.Entries[0].Balance) |
There was a problem hiding this comment.
| require.Equal(t, sdk.NewInt(5), ubd.Entries[0].Balance) | |
| require.Equal(t, 1, len(ubd.Entries)) | |
| require.Equal(t, sdk.NewInt(5), ubd.Entries[0].Balance) |
There was a problem hiding this comment.
I don't see require.Equal(t, 1, len(ubd.Entries)). Did you push it yet?
|
Test failures appear to be unrelated to the content of the PR. |
| require.Equal(t, sdk.NewInt(60), stakeAmt) | ||
| ubd, found := app.StakingKeeper.GetUnbondingDelegation(ctx, dest, valAddr) | ||
| require.True(t, found) | ||
| require.Equal(t, sdk.NewInt(5), ubd.Entries[0].Balance) |
There was a problem hiding this comment.
I don't see require.Equal(t, 1, len(ubd.Entries)). Did you push it yet?
|
Turns out I hadn't pushed everything. I've definitely got to squash this at merge time to hide my shame. |
|
BTW, as a drive-by, the following fixes the data race in diff --git a/types/coin_test.go b/types/coin_test.go
index 8dcd9b25dd..bdc1756328 100644
--- a/types/coin_test.go
+++ b/types/coin_test.go
@@ -21,8 +21,13 @@ type coinTestSuite struct {
ca0, ca1, ca2, cm0, cm1, cm2 sdk.Coin
}
+type serialTestSuite struct {
+ suite.Suite
+}
+
func TestCoinTestSuite(t *testing.T) {
suite.Run(t, new(coinTestSuite))
+ suite.Run(t, new(serialTestSuite))
}
func (s *coinTestSuite) SetupSuite() {
@@ -104,7 +109,8 @@ func (s *coinTestSuite) TestCoinIsValid() {
}
}
-func (s *coinTestSuite) TestCustomValidation() {
+// TestCustomValidation needs to be serial because it mutates the CoinDenomRegex.
+func (s *serialTestSuite) TestCustomValidation() {
newDnmRegex := `[\x{1F600}-\x{1F6FF}]`
sdk.SetCoinDenomRegex(func() string {
return newDnmRegex |
|
Is the name |
|
Filed Agoric/agoric-sdk#8612 for the coins test. Still confused by the proposed solution. |
|
|
Ah, I get it. We're just isolating that test in its own suite, but within the same Go test. |
Adds a message type that allows ClawbackVestingAccounts to return the entire vesting grant to the funder. Unlike clawback, it attempts to return both vested and unvested funds from the initial grant.
Description
Refs: Agoric/agoric-sdk#8570
Adds a message type that allows ClawbackVestingAccounts to return the entire vesting grant to the funder. Unlike clawback, it attempts to return both vested and unvested funds from the initial grant.
Note that this will require an additional PR in Agoric/agoric-sdk to modify the
x/lienaccount wrapper to handle the extra method.Reviewer: please check that I've added the new command and proto definitions to the appropriate registries. Compare to the clawback command/message.
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
!in the type prefix if API or client breaking change