Write back account changes after tracking delegation/undelegation for vesting accounts #8865

gsora · 2021-03-12T16:41:37Z

Description

Before this PR, delegation and undelegation calculations made during accounting operations for vesting accounts were made correct but not written back to the account store.

This PR fixes that behavior.

closes: #8601
closes: #8812

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

orijbot · 2021-03-12T16:42:57Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=77b29c7ccdb94f5bad28f727ac5d6a04

alessio

Sounds like we got this, thank you! A test case is required though

fdymylja · 2021-03-12T17:03:02Z

Do we have a test? IIRC only some vesting account types were failing, and if I'm not mistaken we run tests on vesting accounts delegatio/undelegation tracking (in x/bank I suppose).

Could we add a test here?

On a side note... It feels weird that bank has to update (and test) the vesting accounts types on behalf of auth.

Linking this for context: #8528

fedekunze

ACK, pending changelog

tac0turtle

LGTM, is there anyway to add a regression test?

alessio

Blocking this as we need to introduce a few test cases (and we should not accidentally merge this until that work is done)

orijbot · 2021-03-12T17:12:23Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=af5b275bfb9e5e8e8c20bff726c61db836ea74b3

gsora · 2021-03-12T17:13:17Z

Added a test case in TestDelegateCoins() to test for this specific case.

zmanian · 2021-03-12T17:22:11Z

good find.

orijbot · 2021-03-12T17:23:13Z

Benchmark finished. See the result: https://github.orijtech.com/benchmark/result?id=468365d12c554104a2092360c0f0c6ef

codecov · 2021-03-12T17:27:16Z

Codecov Report

Merging #8865 (34b7961) into master (3a5550a) will increase coverage by 0.01%.
The diff coverage is 62.89%.

@@            Coverage Diff             @@
##           master    #8865      +/-   ##
==========================================
+ Coverage   58.95%   58.97%   +0.01%     
==========================================
  Files         574      576       +2     
  Lines       32220    32377     +157     
==========================================
+ Hits        18996    19095      +99     
- Misses      11004    11041      +37     
- Partials     2220     2241      +21

Impacted Files	Coverage Δ
x/auth/keeper/keeper.go	`49.29% <ø> (+0.68%)`	⬆️
x/auth/keeper/migrations.go	`8.33% <8.33%> (ø)`
x/auth/module.go	`53.65% <60.00%> (-0.40%)`	⬇️
x/auth/legacy/v043/store.go	`67.14% <67.14%> (ø)`
x/bank/keeper/keeper.go	`70.93% <100.00%> (+0.28%)`	⬆️

x/bank/keeper/keeper_test.go

Co-authored-by: Alessio Treglia <alessio@tendermint.com>

orijbot · 2021-03-12T17:40:29Z

Benchmark beginning. Status page: https://github.orijtech.com/benchmark/status?commit=58c1b725e5165e59eeb5e7b94dcaaaa8fa402b0b

alexanderbez · 2021-03-12T18:13:10Z

So was this a regression? What are the repercussions of this on the hub's mainnet?

alessio · 2021-03-12T18:34:12Z

So was this a regression? What are the repercussions of this on the hub's mainnet?

I think @gsora identified this by going through git log and git blame. It'd be interesting to ultimately determine 1. whether this is a regression 2. and if so, which release introduced it.

alessio · 2021-03-12T18:35:52Z

I also suspect this change could be state-breaking. Thus the hub would need to go through an automated upgrade.

alexanderbez · 2021-03-12T18:51:57Z

It must be because it worked on hub 1-3. Specifically, I mean how does this currently affect vesting accounts on the hub right now?

EDIT:

alessio · 2021-03-12T18:56:58Z

It must be because it worked on hub 1-3. Specifically, I mean how does this currently affect vesting accounts on the hub right now?

EDIT: https://github.com/cosmos/cosmos-sdk/blob/v0.37.15/x/bank/internal/keeper/keeper.go#L77-L81

IIUC @gsora managed to reproduce #8812 - I'm unsure about #8601

amaury1093

I scanned through this PR again, imo it's ready to be merged. @robert-zaremba proposed some changes #8865 (comment), but we can make them in a follow-up.

@gsora Could you fix the lint errors?

The interfacer linter has been deprecated.

alessio · 2021-04-04T20:41:50Z

I scanned through this PR again, imo it's ready to be merged. @robert-zaremba proposed some changes #8865 (comment), but we can make them in a follow-up.

@robert-zaremba are you ok with that?

@gsora Could you fix the lint errors?

I took care of the errors

jgimeno

I think we tried as much edge cases as we could, I would merge it. Thanks @gsora

robert-zaremba · 2021-04-06T18:21:59Z

@gsora - could you respond on comments?
I would like to solve the queryFn problem correctly here instead of adding a workaround. But if it's more complex we can do it in a followup issue.

gsora · 2021-04-06T18:57:52Z

Hey all sorry for the delay, got caught up in other issues.

I'd say we can merge this right now and work something out as soon as we have an infra-package communication framework.

robert-zaremba

For the ABCI use, I created an issue to track it and highlight the motivation. So won't block on it.

However, it seams that unbonding is still not handled: https://github.com/cosmos/cosmos-sdk/pull/8865/files#r600474696
cc: @boz

robert-zaremba · 2021-04-07T18:18:32Z

x/auth/legacy/v043/store.go

+	}
+}
+
+func getDelegatorDelegationsSum(ctx sdk.Context, address string, queryServer grpc.Server) (sdk.Coins, error) {


It seams this is still not done.

x/auth/legacy/v043/store.go

alessio · 2021-04-07T19:51:16Z

For the ABCI use, I created an issue to track it and highlight the motivation. So won't block on it.

However, it seams that unbonding is still not handled: https://github.com/cosmos/cosmos-sdk/pull/8865/files#r600474696
cc: @boz

Looks like it was addressed in fef2d00

boz

🔥

robert-zaremba · 2021-04-08T15:09:03Z

Looks like it was addressed in ...

good

… vesting accounts (cosmos#8865) Delegations and undelegations calculations performed during accounting operations for vesting accounts were correct but were not written back to the account store. (cherry picked from commit 93965e0)

write back account changes after tracking delegation/undelegation

a77b0c3

gsora requested review from alessio, jgimeno, sahith-narahari and fdymylja March 12, 2021 16:41

gsora requested review from aaronc and alexanderbez as code owners March 12, 2021 16:41

alessio requested review from amaury1093 and robert-zaremba March 12, 2021 16:57

alessio reviewed Mar 12, 2021

View reviewed changes

alessio requested a review from zmanian March 12, 2021 17:03

fedekunze approved these changes Mar 12, 2021

View reviewed changes

tac0turtle approved these changes Mar 12, 2021

View reviewed changes

alessio previously requested changes Mar 12, 2021

View reviewed changes

add test case for delegated vesting amount

af5b275

alessio reviewed Mar 12, 2021

View reviewed changes

x/bank/keeper/keeper_test.go Outdated Show resolved Hide resolved

remove whitespace

58c1b72

Co-authored-by: Alessio Treglia <alessio@tendermint.com>

amaury1093 approved these changes Apr 2, 2021

View reviewed changes

amaury1093 and others added 2 commits April 2, 2021 17:26

Merge branch 'master' into gsora/track-delegation-vesting-accounts

f928380

Merge branch 'master' into gsora/track-delegation-vesting-accounts

289e4c6

sahith-narahari approved these changes Apr 3, 2021

View reviewed changes

fix linter warnings

5d9d4f9

The interfacer linter has been deprecated.

jgimeno approved these changes Apr 5, 2021

View reviewed changes

sahith-narahari and others added 2 commits April 6, 2021 14:54

Merge branch 'master' into gsora/track-delegation-vesting-accounts

226ca8c

Merge branch 'master' into gsora/track-delegation-vesting-accounts

85b79b2

rename migrate function to MigrateAccount

f342589

robert-zaremba mentioned this pull request Apr 7, 2021

Rollback ABCI use for cross module communication #9070

Closed

4 tasks

robert-zaremba requested changes Apr 7, 2021

View reviewed changes

Merge branch 'master' into gsora/track-delegation-vesting-accounts

34b7961

alessio requested a review from robert-zaremba April 7, 2021 19:51

boz approved these changes Apr 8, 2021

View reviewed changes

robert-zaremba approved these changes Apr 8, 2021

View reviewed changes

alessio merged commit 93965e0 into master Apr 8, 2021

alessio deleted the gsora/track-delegation-vesting-accounts branch April 8, 2021 15:20

amaury1093 mentioned this pull request Apr 9, 2021

No sending and delegate from vesting Account possible. #9013

Closed

tac0turtle mentioned this pull request Apr 14, 2021

backport: #8865 vesting account fix #9113

Closed

robert-zaremba mentioned this pull request Jun 2, 2021

v0.43 Release QA #9116

Closed

42 tasks

amaury1093 mentioned this pull request Sep 15, 2021

Bump Cosmos SDK and ibc-go cosmos/gaia#942

Merged

16 tasks

tomtau mentioned this pull request Sep 24, 2021

fix: removed potential sources of non-determinism in upgrades #10189

Merged

19 tasks

RiccardoM mentioned this pull request Nov 22, 2021

Upgrading to v0.44 breaks vested accounts #10591

Closed

4 tasks

migueldingli1997 mentioned this pull request Feb 10, 2022

Vesting accounts issue ixofoundation/ixo-blockchain#255

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write back account changes after tracking delegation/undelegation for vesting accounts #8865

Write back account changes after tracking delegation/undelegation for vesting accounts #8865

gsora commented Mar 12, 2021

orijbot commented Mar 12, 2021

alessio left a comment

fdymylja commented Mar 12, 2021 •

edited

Loading

fedekunze left a comment

tac0turtle left a comment

alessio left a comment •

edited

Loading

orijbot commented Mar 12, 2021

gsora commented Mar 12, 2021

zmanian commented Mar 12, 2021

orijbot commented Mar 12, 2021

codecov bot commented Mar 12, 2021 •

edited

Loading

orijbot commented Mar 12, 2021

alexanderbez commented Mar 12, 2021

alessio commented Mar 12, 2021

alessio commented Mar 12, 2021

alexanderbez commented Mar 12, 2021 •

edited

Loading

alessio commented Mar 12, 2021

amaury1093 left a comment •

edited

Loading

alessio commented Apr 4, 2021

jgimeno left a comment

robert-zaremba commented Apr 6, 2021

gsora commented Apr 6, 2021

robert-zaremba left a comment

robert-zaremba Apr 7, 2021

alessio commented Apr 7, 2021

boz left a comment

robert-zaremba commented Apr 8, 2021

Write back account changes after tracking delegation/undelegation for vesting accounts #8865

Write back account changes after tracking delegation/undelegation for vesting accounts #8865

Conversation

gsora commented Mar 12, 2021

Description

orijbot commented Mar 12, 2021

alessio left a comment

Choose a reason for hiding this comment

fdymylja commented Mar 12, 2021 • edited Loading

fedekunze left a comment

Choose a reason for hiding this comment

tac0turtle left a comment

Choose a reason for hiding this comment

alessio left a comment • edited Loading

Choose a reason for hiding this comment

orijbot commented Mar 12, 2021

gsora commented Mar 12, 2021

zmanian commented Mar 12, 2021

orijbot commented Mar 12, 2021

codecov bot commented Mar 12, 2021 • edited Loading

Codecov Report

orijbot commented Mar 12, 2021

alexanderbez commented Mar 12, 2021

alessio commented Mar 12, 2021

alessio commented Mar 12, 2021

alexanderbez commented Mar 12, 2021 • edited Loading

alessio commented Mar 12, 2021

amaury1093 left a comment • edited Loading

Choose a reason for hiding this comment

alessio commented Apr 4, 2021

jgimeno left a comment

Choose a reason for hiding this comment

robert-zaremba commented Apr 6, 2021

gsora commented Apr 6, 2021

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba Apr 7, 2021

Choose a reason for hiding this comment

alessio commented Apr 7, 2021

boz left a comment

Choose a reason for hiding this comment

robert-zaremba commented Apr 8, 2021

fdymylja commented Mar 12, 2021 •

edited

Loading

alessio left a comment •

edited

Loading

codecov bot commented Mar 12, 2021 •

edited

Loading

alexanderbez commented Mar 12, 2021 •

edited

Loading

amaury1093 left a comment •

edited

Loading