Separate vesting from auth, add custom vesting schedules #5040

karzak · 2019-09-12T15:49:43Z

Fixes #4486

Features

Adds a new vesting account type, PeriodicVestingAccount, which allows for arbitrary vesting schedules.

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.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

…dekunze/ADR-11-generalize-gen-accs

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

….com/cosmos/cosmos-sdk into fedekunze/ADR-11-generalize-gen-accs

karzak · 2019-09-12T15:50:55Z

This is R4R, but currently blocked by #5017

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

codecov · 2019-09-12T15:54:50Z

Codecov Report

Merging #5040 into master will decrease coverage by 0.07%.
The diff coverage is 58.52%.

@@            Coverage Diff             @@
##           master    #5040      +/-   ##
==========================================
- Coverage   54.94%   54.87%   -0.08%     
==========================================
  Files         302      300       -2     
  Lines       18357    18362       +5     
==========================================
- Hits        10087    10076      -11     
- Misses       7517     7546      +29     
+ Partials      753      740      -13

x/auth/vesting/types/vesting_account.go

x/auth/vesting/types/vesting_account_test.go

x/bank/internal/keeper/keeper.go

alexanderbez

Looking good @karzak, left some more feedback ⚡️

alexanderbez · 2019-10-03T14:50:34Z

x/auth/module.go

@@ -36,6 +37,7 @@ func (AppModuleBasic) Name() string {
 // RegisterCodec registers the auth module's types for the given codec.
 func (AppModuleBasic) RegisterCodec(cdc *codec.Codec) {
 	types.RegisterCodec(cdc)
+	vestingtypes.RegisterCodec(cdc)


Can we avoid requiring auth to register anything related to vesting? Vesting should be completely opt-in if possible.

how'd would you make it opt-in then. By defining a module.go in vesting?

The two options I see are

Define a module.go, which was rejected previously in e905853#r325162521

Require vesting to call RegisterCodec outside the normal module manager pattern, as in ab06e07#r329289047

IMO, registering vesting types in auth doesn't makes sense, as a downstream consumer could build an app that doesn't want vesting, and would find the inclusion of vesting an anti-pattern. The tradeoff is that it needs to be clearly communicated that submodules have to be manually registered.

x/auth/vesting/types/codec.go

x/auth/vesting/types/vesting_period.go

x/bank/internal/keeper/keeper.go

alexanderbez · 2019-10-03T15:06:35Z

x/bank/internal/keeper/keeper_test.go

+	require.Equal(t, origCoins, vacc.GetCoins())
+}
+
+func TestPeriodicVestingAccountSend(t *testing.T) {


Why should these move to auth/vesting? These tests are testing banking functionality with relation to vesting contraints.

x/auth/spec/05_vesting.md

x/auth/vesting/types/vesting_account.go

x/auth/spec/05_vesting.md

x/auth/vesting/types/vesting_account.go

… into kd-separate-vesting

alexanderbez

LGTM @karzak. Final piece missing is a changelog entries.

Under Features:

* [\#4486](https://github.com/cosmos/cosmos-sdk/issues/4486) Introduce new `PeriodicVestingAccount` vesting account type
that allows for arbitrary vesting periods.

Under API Breaking:

* [\#4486](https://github.com/cosmos/cosmos-sdk/issues/4486) Vesting account types decoupled from the `x/auth` module and
now live under `x/auth/vesting`. Applications wishing to use vesting account types must be sure to register types via
`RegisterCodec` under the new vesting package.
* [\#4486](https://github.com/cosmos/cosmos-sdk/issues/4486) The `NewBaseVestingAccount` constructor returns an error
if the provided arguments are invalid.

alexanderbez

ACK 🎉

…schedules

fedekunze and others added 17 commits September 9, 2019 17:23

ADR 11: Generalize Gen Accounts implementation

40ed336

move CLI cmd to auth

90a3882

delete genaccount module

8e92e02

add some tests

e1e4993

more fixes

84f92dd

register codec

19fd7c8

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into fe…

7085b9c

…dekunze/ADR-11-generalize-gen-accs

Apply suggestions from code review

0b6bbb5

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

address comments from review

e6bf7c4

Merge branch 'fedekunze/ADR-11-generalize-gen-accs' of https://github…

ac7aaef

….com/cosmos/cosmos-sdk into fedekunze/ADR-11-generalize-gen-accs

remove genaccounts command

e9d215a

keep legacy genaccount migrations

5a69363

update genutil migration

6c4b702

format

4e5e1a2

migrate

6288793

iterator

fe125bf

feat: separate vesting from auth

e905853

karzak requested review from alessio, alexanderbez, fedekunze, jackzampolin and rigelrozanski as code owners September 12, 2019 15:49

golangcibot reviewed Sep 12, 2019

View reviewed changes

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

fix: lint

fb0e13d

alexanderbez added R4R labels Sep 12, 2019

alexanderbez added 2 commits September 12, 2019 12:22

Update godoc

1198733

Update godoc

f07fabc

lint spec

56a9220

fedekunze requested review from alexanderbez and AdityaSripal October 1, 2019 14:48

AdityaSripal approved these changes Oct 3, 2019

View reviewed changes

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

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

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

x/bank/internal/keeper/keeper.go Show resolved Hide resolved

address review comments

e856664

alexanderbez suggested changes Oct 3, 2019

View reviewed changes

alexanderbez and others added 4 commits October 3, 2019 11:40

Merge branch 'master' into kd-separate-vesting

914981b

address review comments

9eebfb9

update spec to reflect changes

3f482d4

separate vesting codec from auth

f2ffb05

alexanderbez assigned alexanderbez, fedekunze and AdityaSripal Oct 7, 2019

Merge branch 'master' into kd-separate-vesting

6b0629d

alexanderbez suggested changes Oct 7, 2019

View reviewed changes

karzak added 5 commits October 7, 2019 11:47

address review comments

4392ae1

Merge branch 'kd-separate-vesting' of github.com:Kava-Labs/cosmos-sdk…

3fe741e

… into kd-separate-vesting

address review comments

cc6fdd0

update spendable coins name for vesting accounts

1e6c27c

move vesting amount validation to contructor

432fdca

alexanderbez suggested changes Oct 8, 2019

View reviewed changes

karzak and others added 2 commits October 8, 2019 16:21

Update changelog

dd905b0

Merge branch 'master' into kd-separate-vesting

e1f67c9

alexanderbez approved these changes Oct 10, 2019

View reviewed changes

Merge remote-tracking branch 'origin' into kd-separate-vesting

a374f5f

alexanderbez merged commit 64a2741 into cosmos:master Oct 10, 2019

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

Merge PR cosmos#5040: Separate vesting from auth, add custom vesting …

0bb3e3d

…schedules

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate vesting from auth, add custom vesting schedules #5040

Separate vesting from auth, add custom vesting schedules #5040

karzak commented Sep 12, 2019

karzak commented Sep 12, 2019

codecov bot commented Sep 12, 2019 •

edited

Loading

alexanderbez left a comment

alexanderbez Oct 3, 2019

fedekunze Oct 3, 2019

karzak Oct 3, 2019

alexanderbez Oct 3, 2019

alexanderbez left a comment

alexanderbez left a comment

Separate vesting from auth, add custom vesting schedules #5040

Separate vesting from auth, add custom vesting schedules #5040

Conversation

karzak commented Sep 12, 2019

karzak commented Sep 12, 2019

codecov bot commented Sep 12, 2019 • edited Loading

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez Oct 3, 2019

Choose a reason for hiding this comment

fedekunze Oct 3, 2019

Choose a reason for hiding this comment

karzak Oct 3, 2019

Choose a reason for hiding this comment

alexanderbez Oct 3, 2019

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 12, 2019 •

edited

Loading