[R4R]: Vesting Account(s) Implementation

[alexanderbez]

Implementation of the vesting account specification (per #2589):

• Updated and cleaned up specification. As implementation went along, I found some structural mistakes and found room to DRY/clean things up.
• Implemented sdk.MaxInt and sdk.MaxUint.
• Implemented continuous and delayed (discrete) vesting account types

---

• 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 entries in PENDING.md with issue #

• rereviewed 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)

[codecov]

Codecov Report

Merging #2694 into develop will increase coverage by 0.56%.
The diff coverage is 84.73%.

@@             Coverage Diff             @@
##           develop    #2694      +/-   ##
===========================================
+ Coverage    55.18%   55.75%   +0.56%     
===========================================
  Files          134      134              
  Lines         9526     9702     +176     
===========================================
+ Hits          5257     5409     +152     
- Misses        3938     3949      +11     
- Partials       331      344      +13

[alexanderbez]

Rebased.

Addressed

[fedekunze]

ditto

[zmanian]

This is an extremely good candidate for the hard fork game of stakes upgrade after merged.

[cwgoes]

I think this could be merged. Does anyone else want to review it?

[jaekwon]

Don't merge.

[jaekwon]

TrackDelegation/TrackUndelegation are weird names. DelegateCoins/UndelegateCoins seems fine?

[alexanderbez]

Ehhh, but you're not actually delegating or undelegating. You're simply performing some accounting where the actual delegation/undelegation is performed upstream (i.e. stake keeper => accnt keeper). I think Track* is just fine imho.

[sunnya97]

If we don't prioritize Undelegating into VestedCoins, couldn't we get rid of tracking of DelegatedVested vs DelegatedVesting? Wouldn't that simplify things a lot?

Furthermore, can we change the name of bankKeeper.DelegateCoins() to bankKeeper.SubtractVesting() or something, cause that's really what the desired functionality of this feature is. For example, you should be able to use VestingCoins for governance deposits.

[alexanderbez]

@sunnya97

If we don't prioritize Undelegating into VestedCoins, couldn't we get rid of tracking of DelegatedVested vs DelegatedVesting? Wouldn't that simplify things a lot?

Yes, technically I think so. But why not support undelegating? It works.

Furthermore, can we change the name of bankKeeper.DelegateCoins() to bankKeeper.SubtractVesting() or something, ...

Hmmm, why? For example, the stake keeper calls DelegateCoins, so you'd like to call it SubtractVesting? What if the account isn't a vesting account? I'm not in favor of this renaming.

For example, you should be able to use VestingCoins for governance deposits.

I think we talked about that and we decided not to support such a feature.

---

I still need to address @jaekwon's comments :)

[sunnya97]

Essentially I want to reduce the breaking of the abstraction barriers here between stake and bank. I don't like that right now bank knows about staking. Currently the dependency is that stake knows about bank, not the other way around.

So you'd like to call it SubtractVesting? What if the account isn't a vesting account? I'm not in favor of this renaming.

Hmm, you're right. I don't like that either. An alternative: A bank.VestedOnlyKeeper, that still meets the bank.Keeper interface definition, but caps at VestedCoins for SubtractCoins. So keepers that should have access to Vesting coins get the normal Keeper, while ones that don't get the VestedOnlyKeeper. The converse of this is also fine (VestingAccessibleKeeper & NormalKeeper).

But why not support undelegating? It works.

I was just thinking, it gets rid of the tracking of delegated coins, simplifying this module significantly. I can see that intuitively, prioritizing vested coins makes logical sense. But, is the increase complexity worth it? Idk, fine either way, but could we at least call it OutstandingVesting instead of DelegatedVesting?

I think we talked about that and we decided not to support such a feature.

Oh, I didn't realize that. What was the reason for this? Was this discussion recorded somewhere?

[alexanderbez]

Ok @jaekwon and @cwgoes, I've updated the code and spec to address the major comments, namely:

• AddCoinByDenom no longer panics
• Removed TrackDelegation and TrackUndelegation from the Account interface. The x/bank keeper now simply calls private auxiliary functions: trackDelegation and trackUndelegation which do type checking.
• Updated spec and fixed a few typos

At this point, I think the vesting spec and implementation are in a good place -- lmk your thoughts.

[cwgoes]

@alexanderbez Thanks; your changes look fine to me - my approval stands.

[cwgoes]

@jackzampolin Can you check if your suggestions have been addressed?

@jaekwon Did you have any further suggestions or concerns?

[alexanderbez]

@cwgoes I spoke with @jaekwon -- I'll be removing Add/SubCoinsByDenom in favor of the original Plus/Sub logic. Afterwards, it can be merged.

Addressed.