Prune vesting accounts balances #1003

alpe · 2022-09-14T10:49:37Z

Prevent an out of gas error when burning accounts balance

Update:
This PR also includes

pruning vesting accounts original denoms only
drop the prune account types list. It is easier to check for the vesting account interface. This should cover future vesting types as well. Chains can still overwrite this behaviour with a custom CoinPruner via Options

codecov · 2022-09-14T11:06:12Z

Codecov Report

Merging #1003 (4c85b0d) into main (e55db8b) will decrease coverage by 0.02%.
The diff coverage is 76.00%.

@@            Coverage Diff             @@
##             main    #1003      +/-   ##
==========================================
- Coverage   60.33%   60.30%   -0.03%     
==========================================
  Files          52       52              
  Lines        6436     6437       +1     
==========================================
- Hits         3883     3882       -1     
- Misses       2266     2268       +2     
  Partials      287      287

Impacted Files	Coverage Δ
x/wasm/keeper/keeper.go	`87.67% <73.91%> (-0.20%)`	⬇️
x/wasm/keeper/options.go	`74.71% <100.00%> (-1.38%)`	⬇️

alpe · 2022-09-14T13:13:42Z

An alternative solution would be to burn only the stakingKeeper.BondDenom() balance when pruning an account.

ethanfrey

Logic seems correct.

Hard to follow, I would like PruneBalances to only take vesting account and make that check explicit in instantiate

x/wasm/keeper/keeper.go

ethanfrey · 2022-09-20T11:36:59Z

x/wasm/keeper/keeper.go

-		}
+func (b VestingCoinBurner) PruneBalances(ctx sdk.Context, existingAcc authtypes.AccountI) error {
+	v, ok := existingAcc.(vestingexported.VestingAccount)
+	if !ok {


Ah, you check here... and only allow vesting accounts.

Either, "leave unchanged", "prune vesting" or reject.

You should definitely rename this function "EnforceVestingAndPruneBalances", or (my preference) do the vesting interface check in the instantiate method and accept vestingexported.VestingAccount as an argument here, which makes it clearer and keeps the 3 cases very explicit in instantiate

This was built with the assumption that also other than SDK account types can exists in a chain. The CoinPruner.PruneBalances method is an extension point that can be used so that chains can handle their account types and/or vesting account types as they want. Another implementation could also be sending the tokens to the community pool or an escrow (not saying this makes sense).
The concrete struct type is VestingCoinBurner. I thought this verbose enough and explicit addressing the SDK vesting account types only (or return error).
I can add some more code doc to this but switching to VestingAccount types only would not work due to the custom account types. Throwing types.ErrAccountExists though would better fit one level where it was before. I will modify the method signature to return a handled bool for this
WDYT?

Ah, okay, so this PruneBalances is not on the Keeper, but can easily be set by an external app. Now I understand the design.

Throwing types.ErrAccountExists though would better fit one level where it was before. I will modify the method signature to return a handled bool for this

No need for handled unless you prefer it as well, I think returning types.ErrAccountExists is nice

You convinced me that we need to keep the logic here. That does make sense.

However, the name is misleading. PruneBalances is what you do when you encounter a vesting account.

Maybe CleanupExistingAccount would be a better name (as is more general and fits what you want as an extension point). And if it is a vesting account, you PruneBalances. Maybe other accounts have different cleanup methods? And if there is no cleanup possible (existing module account), returning some types.ErrCannotReuseAccount error or such sounds good.

👍 CleanupExistingAccount makes sense as a more general term. I prefer the handled result value, too as it brings back the control flow to the caller.

x/wasm/keeper/keeper.go

ethanfrey

Okay, I understand your motivation better.
Left comments for alternate cleanup. Basically just name changes now.

ethanfrey · 2022-09-21T09:16:15Z

x/wasm/keeper/keeper.go

-		}
+func (b VestingCoinBurner) PruneBalances(ctx sdk.Context, existingAcc authtypes.AccountI) error {
+	v, ok := existingAcc.(vestingexported.VestingAccount)
+	if !ok {


Ah, okay, so this PruneBalances is not on the Keeper, but can easily be set by an external app. Now I understand the design.

Throwing types.ErrAccountExists though would better fit one level where it was before. I will modify the method signature to return a handled bool for this

No need for handled unless you prefer it as well, I think returning types.ErrAccountExists is nice

ethanfrey · 2022-09-21T09:18:58Z

x/wasm/keeper/keeper.go

-		}
+func (b VestingCoinBurner) PruneBalances(ctx sdk.Context, existingAcc authtypes.AccountI) error {
+	v, ok := existingAcc.(vestingexported.VestingAccount)
+	if !ok {


You convinced me that we need to keep the logic here. That does make sense.

However, the name is misleading. PruneBalances is what you do when you encounter a vesting account.

Maybe CleanupExistingAccount would be a better name (as is more general and fits what you want as an extension point). And if it is a vesting account, you PruneBalances. Maybe other accounts have different cleanup methods? And if there is no cleanup possible (existing module account), returning some types.ErrCannotReuseAccount error or such sounds good.

alpe · 2022-09-21T11:39:51Z

I have pushed the renaming. Please approve the PR when they make sense now

pinosu · 2022-09-21T13:08:52Z

x/wasm/keeper/keeper_test.go

@@ -2274,3 +2274,66 @@ func TestAppendToContractHistory(t *testing.T) {
 	gotHistory := keepers.WasmKeeper.GetContractHistory(ctx, contractAddr)
 	assert.Equal(t, orderedEntries, gotHistory)
 }
+
+func TestCoinBurnerPruneBalances(t *testing.T) {


pinosu

LGTM 👍

ethanfrey

looks good now

* wasmd_v0.29.1: (94 commits) fix test. fix test. update cosmos-sdk to v45.9, add dragonberry. Changelog updates (CosmWasm#1024) Validate incoming messages Add dependencies for protobuf and remove third_party forlder (CosmWasm#1030) Bump bufbuild/buf-setup-action from 1.7.0 to 1.8.0 (CosmWasm#1006) Add check version wasm (CosmWasm#1029) Make SenderPrivKey field public Bump actions/checkout from 3.0.2 to 3.1.0 Revert module version to 1 as there is no migration Doc ante handler Fix: typos Changelog for v0.29.0-rc1 (CosmWasm#1018) Implement improvements to new address generation (CosmWasm#1014) Prune vesting accounts balances (CosmWasm#1003) Bump github.com/cosmos/ibc-go/v3 from 3.2.0 to 3.3.0 Fix path to proofs in protos Upgrade .a files to 1.1.1 Bump wasmvm to 1.1.1 ... # Conflicts: # .circleci/config.yml # .github/workflows/proto-buf-publisher.yml # CHANGELOG.md # Dockerfile # Makefile # README.md # app/app.go # app/app_test.go # app/export.go # app/sim_test.go # app/test_helpers.go # go.mod # go.sum # scripts/protocgen.sh # third_party/proto/cosmos/base/query/v1beta1/pagination.proto # third_party/proto/cosmos/base/v1beta1/coin.proto # third_party/proto/tendermint/blockchain/types.pb.go # third_party/proto/tendermint/consensus/types.pb.go # third_party/proto/tendermint/consensus/wal.pb.go # third_party/proto/tendermint/p2p/conn.pb.go # third_party/proto/tendermint/privval/types.pb.go # third_party/proto/tendermint/state/types.pb.go # third_party/proto/tendermint/types/types.pb.go # third_party/proto/tendermint/types/validator.pb.go # x/wasm/alias.go # x/wasm/client/cli/genesis_msg.go # x/wasm/client/cli/genesis_msg_test.go # x/wasm/client/cli/query.go # x/wasm/client/cli/tx.go # x/wasm/client/rest/gov.go # x/wasm/handler.go # x/wasm/ibctesting/chain.go # x/wasm/ibctesting/wasm.go # x/wasm/ioutils/ioutil_test.go # x/wasm/ioutils/utils_test.go # x/wasm/keeper/authz_policy.go # x/wasm/keeper/bench_test.go # x/wasm/keeper/contract_keeper.go # x/wasm/keeper/gas_register.go # x/wasm/keeper/gas_register_test.go # x/wasm/keeper/genesis_test.go # x/wasm/keeper/ibc_test.go # x/wasm/keeper/keeper.go # x/wasm/keeper/keeper_test.go # x/wasm/keeper/legacy_querier_test.go # x/wasm/keeper/msg_server.go # x/wasm/keeper/options.go # x/wasm/keeper/options_test.go # x/wasm/keeper/proposal_handler.go # x/wasm/keeper/proposal_integration_test.go # x/wasm/keeper/querier.go # x/wasm/keeper/querier_test.go # x/wasm/keeper/recurse_test.go # x/wasm/keeper/reflect_test.go # x/wasm/keeper/relay_test.go # x/wasm/keeper/staking_test.go # x/wasm/keeper/submsg_test.go # x/wasm/keeper/test_common.go # x/wasm/keeper/testdata/reflect.go # x/wasm/keeper/testdata/reflect.wasm # x/wasm/keeper/wasmtesting/coin_transferrer.go # x/wasm/keeper/wasmtesting/gas_register.go # x/wasm/module.go # x/wasm/module_test.go # x/wasm/simulation/genesis.go # x/wasm/simulation/operations.go # x/wasm/simulation/params.go # x/wasm/types/codec.go # x/wasm/types/events.go # x/wasm/types/exported_keepers.go # x/wasm/types/genesis.pb.go # x/wasm/types/iavl_range_test.go # x/wasm/types/ibc.pb.go # x/wasm/types/params.go # x/wasm/types/proposal.pb.go # x/wasm/types/query.pb.go # x/wasm/types/query.pb.gw.go # x/wasm/types/tx.pb.go # x/wasm/types/types.pb.go

* Prevent out of gas * Prune vesting account denoms only * Fix test state * Move account exists error up again * Review feedback: better naming

alpe requested a review from ethanfrey September 14, 2022 10:49

alpe added 2 commits September 19, 2022 18:00

Prevent out of gas

033b940

Prune vesting account denoms only

45e42f7

alpe force-pushed the 942_gas branch from 51004ca to 45e42f7 Compare September 19, 2022 16:02

alpe changed the title ~~Prevent out of gas~~ Prune vesting accounts Sep 19, 2022

alpe changed the title ~~Prune vesting accounts~~ Prune vesting accounts balances Sep 19, 2022

Fix test state

c777e17

ethanfrey reviewed Sep 20, 2022

View reviewed changes

Move account exists error up again

e04d1a2

alpe requested a review from ethanfrey September 20, 2022 13:00

alpe mentioned this pull request Sep 20, 2022

Implement improvements to new address generation #1014

Merged

ethanfrey reviewed Sep 21, 2022

View reviewed changes

Review feedback: better naming

4c85b0d

alpe requested a review from ethanfrey September 21, 2022 11:39

pinosu reviewed Sep 21, 2022

View reviewed changes

pinosu approved these changes Sep 21, 2022

View reviewed changes

ethanfrey approved these changes Sep 21, 2022

View reviewed changes

alpe merged commit 54fec05 into main Sep 21, 2022

alpe deleted the 942_gas branch September 21, 2022 13:14

faddat mentioned this pull request Oct 2, 2022

sdk 46 neutron-org/wasmd#3

Closed

Magicloud pushed a commit to fpco/wasmd that referenced this pull request Jan 13, 2023

Prune vesting accounts balances (CosmWasm#1003)

5ce2536

* Prevent out of gas * Prune vesting account denoms only * Fix test state * Move account exists error up again * Review feedback: better naming

wojtek-coreum mentioned this pull request Dec 8, 2023

Stop rejecting contract instantiation if account holds funds CoreumFoundation/coreum#738

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prune vesting accounts balances #1003

Prune vesting accounts balances #1003

alpe commented Sep 14, 2022 •

edited

Loading

codecov bot commented Sep 14, 2022 •

edited

Loading

alpe commented Sep 14, 2022

ethanfrey left a comment

ethanfrey Sep 20, 2022

alpe Sep 20, 2022 •

edited

Loading

ethanfrey Sep 21, 2022

ethanfrey Sep 21, 2022

alpe Sep 21, 2022

ethanfrey left a comment

ethanfrey Sep 21, 2022

ethanfrey Sep 21, 2022

alpe commented Sep 21, 2022

pinosu Sep 21, 2022

pinosu left a comment

ethanfrey left a comment

Prune vesting accounts balances #1003

Prune vesting accounts balances #1003

Conversation

alpe commented Sep 14, 2022 • edited Loading

codecov bot commented Sep 14, 2022 • edited Loading

Codecov Report

alpe commented Sep 14, 2022

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey Sep 20, 2022

Choose a reason for hiding this comment

alpe Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

ethanfrey Sep 21, 2022

Choose a reason for hiding this comment

ethanfrey Sep 21, 2022

Choose a reason for hiding this comment

alpe Sep 21, 2022

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey Sep 21, 2022

Choose a reason for hiding this comment

ethanfrey Sep 21, 2022

Choose a reason for hiding this comment

alpe commented Sep 21, 2022

pinosu Sep 21, 2022

Choose a reason for hiding this comment

pinosu left a comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

alpe commented Sep 14, 2022 •

edited

Loading

codecov bot commented Sep 14, 2022 •

edited

Loading

alpe Sep 20, 2022 •

edited

Loading