Add support for permanent locked vesting accounts #8520

clevinson · 2021-02-05T03:47:17Z

Description

closes: #8283

While the SDK currently supports 3 different types of vesting accounts (delayed, continuous, periodic), it only exposes one Msg for creating vesting accounts, parameterized by a delayed boolean, which either creates a "delayed" or "continuous" vesting account.

After speaking with @aaronc about this today, I've realized that we don't actually need sdk.Msg support for creation of PermanentLockedVestingAccount. We discussed a bit the issues with the current vesting logic (as described in #8528), and as a result, i'd like to proceed with the most minimal solution in this PR.

This simply adds support for a new PermanentLockedVestingAccount type. There shouldn't really be any need for creating these accounts after genesis (as @aaronc noted below). In Regen's own use case, we will setup these accounts to hold locked funds of a Group account (for our Community Staking DAOs), and even with these DAOs not fully realized at genesis, we can instantiate the relevant group accounts and correpsonding PermanentLockedVestingAccounts at genesis.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

aaronc · 2021-02-05T04:06:37Z

Not sure we even need to create this sort of thing after genesis, but maybe... Also I'd like to see a more generic way to deal with vesting eventually but that's out of scope.

clevinson · 2021-02-05T16:03:41Z

@aaronc As it currently stands (on the Regen side atleast), we will not have our Community Staking DAOs setup prior to genesis. It's been my understanding that Regen Foundation would hold all tokens for community staking dao's, and distribute them as the organizations are brought onboard (after mainnet).

x/auth/vesting/types/vesting_account.go

x/auth/vesting/types/vesting_account_test.go

clevinson · 2021-02-06T03:11:45Z

@aaronc I've updated this to remove the Msg support. Let's just stick with setting these up with Group accounts for now. I've updated the description in this PR accordingly.

codecov · 2021-02-09T22:39:20Z

Codecov Report

Merging #8520 (1755c6f) into master (9fd866e) will increase coverage by 0.01%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##           master    #8520      +/-   ##
==========================================
+ Coverage   58.80%   58.82%   +0.01%     
==========================================
  Files         583      583              
  Lines       32749    32769      +20     
==========================================
+ Hits        19258    19275      +17     
- Misses      11214    11216       +2     
- Partials     2277     2278       +1

Impacted Files	Coverage Δ
x/auth/vesting/types/vesting_account.go	`88.23% <87.50%> (-0.60%)`	⬇️
x/auth/vesting/types/codec.go	`100.00% <100.00%> (ø)`

proto/cosmos/vesting/v1beta1/tx.proto

x/auth/vesting/types/vesting_account.go

x/auth/vesting/types/vesting_account_test.go

x/auth/vesting/types/vesting_account.go

aaronc · 2021-03-22T15:47:26Z

x/auth/vesting/types/vesting_account_test.go

-
-	// require that all vested coins (50%) are spendable plus any received
-	lockedCoins = cva.LockedCoins(now.Add(12 * time.Hour))
-	require.Equal(t, sdk.Coins{sdk.NewInt64Coin(feeDenom, 500), sdk.NewInt64Coin(stakeDenom, 50)}, lockedCoins)
 }


Why were these deleted?

They are copy-pasted from 4 lines above.

x/auth/vesting/types/vesting_account_test.go

Co-authored-by: Aaron Craelius <aaron@regen.network>

…inson/permanent-vesting-account

…m/cosmos/cosmos-sdk into clevinson/permanent-vesting-account

amaury1093 · 2021-04-13T13:48:50Z

x/auth/vesting/types/common_test.go

@@ -5,6 +5,5 @@ import (
 )

 var (
-	app      = simapp.Setup(false)
-	appCodec = simapp.MakeTestEncodingConfig().Marshaler


amaury1093 · 2021-04-13T14:09:58Z

This PR is r4r again. Would appreciate if @clevinson, @aaronc and @alexanderbez could have a look 🙏

x/auth/vesting/types/vesting_account.go

proto/cosmos/vesting/v1beta1/vesting.proto

robert-zaremba · 2021-04-20T00:54:01Z

proto/cosmos/vesting/v1beta1/vesting.proto

+  option (gogoproto.goproto_getters)  = false;
+  option (gogoproto.goproto_stringer) = false;
+
+  BaseVestingAccount base_vesting_account = 1 [(gogoproto.embed) = true];


we don't need to repeat type name in the attribute name. How about base_account?

I changed PermanentLockedVestingAccount to PermanentLockedAccount, but I disagree with this one: BaseVestingAccount has a field BaseAccount. I dont want to do permanentAccount.BaseAccount.BaseAccount.*

x/auth/vesting/types/vesting_account.go

amaury1093 · 2021-04-20T09:24:59Z

In the latest commits, I changed:

the semantics of EndTime: 0 means no end time, not -1
rename PermanentLockedVestingAccount to PermanentLockedAccount

@robert-zaremba lmk if that lgty.

add basics for permanent locked vesting account support

5fdcff4

clevinson added 2 commits February 5, 2021 18:22

remove sdk.Msg support for PermanentLockedVestingAccount

148f6c2

add tests for PermanentLockedVestingAccount

93d1ba9

clevinson commented Feb 6, 2021

View reviewed changes

x/auth/vesting/types/vesting_account.go Outdated Show resolved Hide resolved

clevinson commented Feb 6, 2021

View reviewed changes

x/auth/vesting/types/vesting_account_test.go Outdated Show resolved Hide resolved

clevinson marked this pull request as ready for review February 6, 2021 03:12

clevinson requested review from aaronc, alessio and alexanderbez as code owners February 6, 2021 03:12

clevinson requested a review from amaury1093 February 17, 2021 17:31

amaury1093 reviewed Feb 19, 2021

View reviewed changes

clevinson added 3 commits March 2, 2021 12:50

remove unecessary tests

697ee2f

drop unecessary create vesting acct msgs

5f96bf0

Merge branch 'master' into clevinson/permanent-vesting-account

efbfd1d

aaronc reviewed Mar 22, 2021

View reviewed changes

amaury1093 and others added 7 commits April 13, 2021 14:21

Update x/auth/vesting/types/vesting_account.go

c88a1da

Co-authored-by: Aaron Craelius <aaron@regen.network>

Update x/auth/vesting/types/vesting_account.go

987ca05

Co-authored-by: Aaron Craelius <aaron@regen.network>

Update x/auth/vesting/types/vesting_account.go

162a1d6

Co-authored-by: Aaron Craelius <aaron@regen.network>

Update x/auth/vesting/types/vesting_account_test.go

06216a3

Co-authored-by: Aaron Craelius <aaron@regen.network>

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into clev…

045f6a5

…inson/permanent-vesting-account

Merge branch 'clevinson/permanent-vesting-account' of ssh://github.co…

670df86

…m/cosmos/cosmos-sdk into clevinson/permanent-vesting-account

Review changes

a2443f4

amaury1093 reviewed Apr 13, 2021

View reviewed changes

amaury1093 added 2 commits April 13, 2021 16:00

Factorize init function

d8f6930

Factorize more

072a6d0

amaury1093 requested review from aaronc, robert-zaremba, anilcse, cyberbono3 and technicallyty April 13, 2021 14:10

cyberbono3 approved these changes Apr 13, 2021

View reviewed changes

blushi assigned robert-zaremba Apr 16, 2021

aaronc reviewed Apr 19, 2021

View reviewed changes

x/auth/vesting/types/vesting_account.go Show resolved Hide resolved

aaronc approved these changes Apr 19, 2021

View reviewed changes

robert-zaremba reviewed Apr 20, 2021

View reviewed changes

amaury1093 added 2 commits April 20, 2021 11:23

Comments

7276244

Merge branch 'master' into clevinson/permanent-vesting-account

a0d197a

amaury1093 added 2 commits April 21, 2021 16:24

Fix build after rename

d202bd3

Merge branch 'master' into clevinson/permanent-vesting-account

1755c6f

robert-zaremba approved these changes Apr 21, 2021

View reviewed changes

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 21, 2021

mergify bot merged commit 603e895 into master Apr 21, 2021

mergify bot deleted the clevinson/permanent-vesting-account branch April 21, 2021 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for permanent locked vesting accounts #8520

Add support for permanent locked vesting accounts #8520

clevinson commented Feb 5, 2021 •

edited

Loading

aaronc commented Feb 5, 2021

clevinson commented Feb 5, 2021

clevinson commented Feb 6, 2021

codecov bot commented Feb 9, 2021 •

edited

Loading

aaronc Mar 22, 2021

amaury1093 Apr 13, 2021 •

edited

Loading

amaury1093 Apr 13, 2021

amaury1093 commented Apr 13, 2021 •

edited

Loading

robert-zaremba Apr 20, 2021

amaury1093 Apr 20, 2021

amaury1093 commented Apr 20, 2021

Add support for permanent locked vesting accounts #8520

Add support for permanent locked vesting accounts #8520

Conversation

clevinson commented Feb 5, 2021 • edited Loading

Description

aaronc commented Feb 5, 2021

clevinson commented Feb 5, 2021

clevinson commented Feb 6, 2021

codecov bot commented Feb 9, 2021 • edited Loading

Codecov Report

aaronc Mar 22, 2021

Choose a reason for hiding this comment

amaury1093 Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

amaury1093 Apr 13, 2021

Choose a reason for hiding this comment

amaury1093 commented Apr 13, 2021 • edited Loading

robert-zaremba Apr 20, 2021

Choose a reason for hiding this comment

amaury1093 Apr 20, 2021

Choose a reason for hiding this comment

amaury1093 commented Apr 20, 2021

clevinson commented Feb 5, 2021 •

edited

Loading

codecov bot commented Feb 9, 2021 •

edited

Loading

amaury1093 Apr 13, 2021 •

edited

Loading

amaury1093 commented Apr 13, 2021 •

edited

Loading