Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imp(vesting): add convert vesting account msg #1400

Merged
merged 37 commits into from
Feb 17, 2023

Conversation

Vvaradinov
Copy link
Contributor

Description

Adds a ConvertVestingAccount msg which converts a vesting account to a regular BaseAccount once it has confirmed that no vesting coins are left.


Closes ENG-1279

@linear
Copy link

linear bot commented Feb 15, 2023

ENG-1279 Convert Vesting Account

Problem

One of the issues with vesting accounts is that when the tokens are fully vested, the account will still have to pay higher tx fees because every time the account will need to check if the tokens are vested.

Scope of Work

  • Automatically convert your VestingAccount into a normal account (default set by the AccountKeeper) if the account is fully vested.
    • No op if the sender doesn't have a vesting account or if the tx is not fully vested or is locked

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #1400 (cd6cb82) into main (592362e) will decrease coverage by 0.03%.
The diff coverage is 63.41%.

❗ Current head cd6cb82 differs from pull request most recent head b07738b. Consider uploading reports for the commit b07738b to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1400      +/-   ##
==========================================
- Coverage   72.42%   72.40%   -0.03%     
==========================================
  Files         260      260              
  Lines       17684    17725      +41     
==========================================
+ Hits        12807    12833      +26     
- Misses       4308     4322      +14     
- Partials      569      570       +1     
Impacted Files Coverage Δ
x/vesting/types/msg.go 87.50% <31.25%> (-9.38%) ⬇️
x/vesting/types/codec.go 21.05% <50.00%> (+1.60%) ⬆️
x/vesting/keeper/msg_server.go 88.97% <86.95%> (-0.20%) ⬇️

@github-actions github-actions bot added the CLI label Feb 16, 2023
x/vesting/client/cli/tx.go Outdated Show resolved Hide resolved
x/vesting/client/cli/tx.go Outdated Show resolved Hide resolved
x/vesting/client/cli/tx.go Outdated Show resolved Hide resolved
x/vesting/keeper/msg_server.go Outdated Show resolved Hide resolved
x/vesting/keeper/msg_server.go Outdated Show resolved Hide resolved
x/vesting/keeper/msg_server.go Outdated Show resolved Hide resolved
x/vesting/types/msg.go Outdated Show resolved Hide resolved
x/vesting/types/msg.go Outdated Show resolved Hide resolved
@Vvaradinov Vvaradinov marked this pull request as ready for review February 16, 2023 10:30
@Vvaradinov Vvaradinov requested a review from a team as a code owner February 16, 2023 10:31
@Vvaradinov Vvaradinov requested review from 0a1c and removed request for a team February 16, 2023 10:31
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor comments. We should also update the spec as part of this PR

x/vesting/keeper/msg_server.go Show resolved Hide resolved
x/vesting/keeper/msg_server.go Outdated Show resolved Hide resolved
x/vesting/keeper/msg_server.go Outdated Show resolved Hide resolved
x/vesting/types/codec.go Show resolved Hide resolved
x/vesting/types/msg.go Show resolved Hide resolved
Vvaradinov and others added 3 commits February 16, 2023 14:32
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
…om/evmos/evmos into Vvaradinov/convert-vesting-account

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK pending spec changes

x/vesting/keeper/msg_server.go Show resolved Hide resolved
x/vesting/keeper/msg_server.go Outdated Show resolved Hide resolved
Vvaradinov and others added 3 commits February 16, 2023 17:05
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry @Vvaradinov, I just noticed that we forgot to add a test case for locked tokens

x/vesting/spec/07_clients.md Outdated Show resolved Hide resolved
x/vesting/spec/03_state_transitions.md Show resolved Hide resolved
x/vesting/keeper/msg_server_test.go Outdated Show resolved Hide resolved
Vvaradinov and others added 3 commits February 16, 2023 17:43
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @Vvaradinov !! Left a small suggestion

x/vesting/client/cli/tx.go Outdated Show resolved Hide resolved
@Vvaradinov Vvaradinov enabled auto-merge (squash) February 17, 2023 11:28
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. Minor comments

x/vesting/client/cli/tx.go Outdated Show resolved Hide resolved
x/vesting/keeper/msg_server.go Outdated Show resolved Hide resolved
x/vesting/keeper/msg_server_test.go Show resolved Hide resolved
x/vesting/spec/03_state_transitions.md Outdated Show resolved Hide resolved
x/vesting/spec/03_state_transitions.md Outdated Show resolved Hide resolved
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@Vvaradinov Vvaradinov merged commit e70e192 into main Feb 17, 2023
@Vvaradinov Vvaradinov deleted the Vvaradinov/convert-vesting-account branch February 17, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants