fix: x/gov ChargeDeposit delete deposits #15033
Conversation
github-prbot
requested review from
a team,
atheeshp and
likhita-809
and removed request for
a team
| @@ -203,6 +203,8 @@ func (keeper Keeper) ChargeDeposit(ctx sdk.Context, proposalID uint64, destAddre | |||
| return err | |||
| } | |||
| } | |||
|
|
|||
| store.Delete(types.DepositKey(deposit.ProposalId, depositerAddress)) | |||
There was a problem hiding this comment.
Say a function below fails (BurnCoins, GetModuleAddress, FunCommunityPool), what happens to the deposit then?
Isn't it safer to store the deposit keys to delete and delete them at the end of his function?
There was a problem hiding this comment.
I think we might not need to do that, because if we return an error all the changes done during the message execution won't get persisted, right? But now that you mention it I'm not that sure
facundomedica
added
the
backport/v0.47.x
(cherry picked from commit dfb3271) # Conflicts: # x/gov/keeper/deposit.go
|
would be interested to see how this issue landed on main, this is something we should figure out how to prevent in the future |
To answer this, we can see that #13010 added the feature and its simulations. Moreover, long SimApp simulations do not run on PRs because they take 45min up to 1h to run. We do have short sims in CI. |
|
Maybe we can change this with the merge queue, and run sims there too |
|
is there a possibility that a unit test case could of caught the issue? |
|
Probably yes. I reviewed that PR multiple times, my reviews were mostly around adding more tests, review 1, or any comment under review 2. However, towards the end, as a reviewer it's very hard to gauge what was tested, and what not. I wonder if this kind of complex state machine logic could be a good showcase of BDD. |
Description
TL;DR: the code tried to delete all deposits with a prefix, this doesn't work. We need to delete each deposit with its own key.
Previously it was trying to delete a non-existing key, so when comparing the balance of the gov module account vs the amount of deposits they didn't match because the deposit tokens were burnt/refunded but the deposit object was never deleted.
Also included some renamings of variables to make it easier to read
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change