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.

Sign up for GitHub

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

Jump to bottom

Prune vesting accounts balances #1003

Merged
merged 5 commits into from
Sep 21, 2022
Merged

Prune vesting accounts balances #1003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
033b940
Prevent out of gas
alpe Sep 14, 2022
45e42f7
Prune vesting account denoms only
alpe Sep 19, 2022
c777e17
Fix test state
alpe Sep 20, 2022
e04d1a2
Move account exists error up again
alpe Sep 20, 2022
4c85b0d
Review feedback: better naming
alpe Sep 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 41 additions & 44 deletions x/wasm/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (
"github.com/cosmos/cosmos-sdk/types/address"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
vestingtypes "github.com/cosmos/cosmos-sdk/x/auth/vesting/types"
vestingexported "github.com/cosmos/cosmos-sdk/x/auth/vesting/exported"
paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
"github.com/tendermint/tendermint/libs/log"

Expand Down Expand Up @@ -57,11 +57,13 @@ type CoinTransferrer interface {
TransferCoins(ctx sdk.Context, fromAddr sdk.AccAddress, toAddr sdk.AccAddress, amt sdk.Coins) error
}

// CoinPruner handles the balances for accounts that are pruned on contract instantiate.
// AccountPruner handles the balances and data cleanup for accounts that are pruned on contract instantiate.
// This is an extension point to attach custom logic
type CoinPruner interface {
// PruneBalances handle balances for given address
PruneBalances(ctx sdk.Context, contractAddress sdk.AccAddress) error
type AccountPruner interface {
// CleanupExistingAccount handles the cleanup process for balances and data of the given account. The persisted account
// type is already reset to base account at this stage.
// The method returns true when the account address can be reused. Unsupported account types are rejected by returning false
CleanupExistingAccount(ctx sdk.Context, existingAccount authtypes.AccountI) (handled bool, err error)
}

// WasmVMResponseHandler is an extension point to handles the response data returned by a contract call.
Expand All @@ -82,19 +84,6 @@ var defaultAcceptedAccountTypes = map[reflect.Type]struct{}{
reflect.TypeOf(&authtypes.BaseAccount{}): {},
}

// list of account types that are replaced with base accounts. Chains importing wasmd
// can overwrite this list with the WithPruneAccountTypesOnContractInstantiation option.
//
// contains vesting account types that can be created post genesis
var defaultPruneAccountTypes = map[reflect.Type]struct{}{
reflect.TypeOf(&vestingtypes.DelayedVestingAccount{}): {},
reflect.TypeOf(&vestingtypes.ContinuousVestingAccount{}): {},
// intentionally not added: genesis account types
// reflect.TypeOf(&vestingtypes.BaseVestingAccount{}): {},
// reflect.TypeOf(&vestingtypes.PeriodicVestingAccount{}): {},
// reflect.TypeOf(&vestingtypes.PermanentLockedAccount{}): {},
}

// Keeper will have a reference to Wasmer with it's own data directory.
type Keeper struct {
storeKey sdk.StoreKey
Expand All @@ -113,8 +102,7 @@ type Keeper struct {
gasRegister GasRegister
maxQueryStackSize uint32
acceptedAccountTypes map[reflect.Type]struct{}
pruneAccountTypes map[reflect.Type]struct{}
coinPruner CoinPruner
accountPruner AccountPruner
}

// NewKeeper creates a new contract Keeper instance
Expand Down Expand Up @@ -153,7 +141,7 @@ func NewKeeper(
wasmVM: wasmer,
accountKeeper: accountKeeper,
bank: NewBankCoinTransferrer(bankKeeper),
coinPruner: NewCoinBurner(bankKeeper),
accountPruner: NewVestingCoinBurner(bankKeeper),
portKeeper: portKeeper,
capabilityKeeper: capabilityKeeper,
messenger: NewDefaultMessageHandler(router, channelKeeper, capabilityKeeper, bankKeeper, cdc, portSource),
Expand All @@ -162,7 +150,6 @@ func NewKeeper(
gasRegister: NewDefaultWasmGasRegister(),
maxQueryStackSize: types.DefaultMaxQueryStackSize,
acceptedAccountTypes: defaultAcceptedAccountTypes,
pruneAccountTypes: defaultPruneAccountTypes,
}
keeper.wasmVMQueryHandler = DefaultQueryPlugins(bankKeeper, stakingKeeper, distKeeper, channelKeeper, queryRouter, keeper)
for _, o := range opts {
Expand Down Expand Up @@ -321,17 +308,18 @@ func (k Keeper) instantiate(ctx sdk.Context, codeID uint64, creator, admin sdk.A
if _, accept := k.acceptedAccountTypes[reflect.TypeOf(existingAcct)]; accept {
// keep account and balance as it is
k.Logger(ctx).Info("instantiate contract with existing account", "address", contractAddress.String())
} else if _, clear := k.pruneAccountTypes[reflect.TypeOf(existingAcct)]; clear {
k.Logger(ctx).Info("pruning existing account for contract instantiation", "address", contractAddress.String())
} else {
alpe marked this conversation as resolved.
Show resolved Hide resolved
// consider an account in the wasmd namespace spam and overwrite it.
k.Logger(ctx).Info("pruning existing account for contract instantiation", "address", contractAddress.String())
contractAccount := k.accountKeeper.NewAccountWithAddress(ctx, contractAddress)
k.accountKeeper.SetAccount(ctx, contractAccount)
// also handle balance to not open cases where these accounts are abused and become liquid
if err := k.coinPruner.PruneBalances(ctx, contractAddress); err != nil {
return nil, nil, err
switch handled, err := k.accountPruner.CleanupExistingAccount(ctx, existingAcct); {
case err != nil:
return nil, nil, sdkerrors.Wrap(err, "prune balance")
case !handled:
return nil, nil, types.ErrAccountExists.Wrap("address is claimed by external account")
}
} else { // unknown account type
return nil, nil, types.ErrAccountExists.Wrapf("refusing to overwrite special account type:: %T", existingAcct)
}
} else {
// create an empty account (so we don't have issues later)
Expand Down Expand Up @@ -1161,32 +1149,41 @@ func (c BankCoinTransferrer) TransferCoins(parentCtx sdk.Context, fromAddr sdk.A
return nil
}

var _ CoinPruner = CoinBurner{}
var _ AccountPruner = VestingCoinBurner{}

// CoinBurner default implementation for CoinPruner to burn the coins
type CoinBurner struct {
// VestingCoinBurner default implementation for AccountPruner to burn the coins
type VestingCoinBurner struct {
bank types.BankKeeper
}

// NewCoinBurner constructor
func NewCoinBurner(bank types.BankKeeper) CoinBurner {
// NewVestingCoinBurner constructor
func NewVestingCoinBurner(bank types.BankKeeper) VestingCoinBurner {
if bank == nil {
panic("bank keeper must not be nil")
}
return CoinBurner{bank: bank}
return VestingCoinBurner{bank: bank}
}

// PruneBalances burns all coins owned by the account.
func (b CoinBurner) PruneBalances(ctx sdk.Context, address sdk.AccAddress) error {
if amt := b.bank.GetAllBalances(ctx, address); !amt.IsZero() {
if err := b.bank.SendCoinsFromAccountToModule(ctx, address, types.ModuleName, amt); err != nil {
return sdkerrors.Wrap(err, "prune account balance")
}
if err := b.bank.BurnCoins(ctx, types.ModuleName, amt); err != nil {
return sdkerrors.Wrap(err, "burn account balance")
}
// CleanupExistingAccount accepts only vesting account types to burns all their original vesting coin balances.
// Other account types will be rejected and returned as unhandled.
func (b VestingCoinBurner) CleanupExistingAccount(ctx sdk.Context, existingAcc authtypes.AccountI) (handled bool, err error) {
v, ok := existingAcc.(vestingexported.VestingAccount)
if !ok {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@alpe alpe Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 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.

return false, nil
}
return nil

ctx = ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
coinsToBurn := sdk.NewCoins()
for _, orig := range v.GetOriginalVesting() { // focus on the coin denoms that were setup originally; getAllBalances has some issues
alpe marked this conversation as resolved.
Show resolved Hide resolved
coinsToBurn = append(coinsToBurn, b.bank.GetBalance(ctx, existingAcc.GetAddress(), orig.Denom))
}
if err := b.bank.SendCoinsFromAccountToModule(ctx, existingAcc.GetAddress(), types.ModuleName, coinsToBurn); err != nil {
return false, sdkerrors.Wrap(err, "prune account balance")
}
if err := b.bank.BurnCoins(ctx, types.ModuleName, coinsToBurn); err != nil {
return false, sdkerrors.Wrap(err, "burn account balance")
}
return true, nil
}

type msgDispatcher interface {
Expand Down
75 changes: 69 additions & 6 deletions x/wasm/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,16 @@ import (
"testing"
"time"

"github.com/cosmos/cosmos-sdk/baseapp"

wasmvm "github.com/CosmWasm/wasmvm"
wasmvmtypes "github.com/CosmWasm/wasmvm/types"
"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
stypes "github.com/cosmos/cosmos-sdk/store/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/address"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/cosmos/cosmos-sdk/x/auth/vesting"
vestingtypes "github.com/cosmos/cosmos-sdk/x/auth/vesting/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
distributiontypes "github.com/cosmos/cosmos-sdk/x/distribution/types"
Expand Down Expand Up @@ -645,7 +645,7 @@ func TestInstantiateWithAccounts(t *testing.T) {
deposit: sdk.NewCoins(sdk.NewCoin("denom", sdk.NewInt(1_000))),
expBalance: sdk.NewCoins(sdk.NewCoin("denom", sdk.NewInt(1_000))),
},
"prune listed DelayedVestingAccount gets overwritten": {
"prunable DelayedVestingAccount gets overwritten": {
account: vestingtypes.NewDelayedVestingAccount(
authtypes.NewBaseAccount(contractAddr, nil, 0, 0),
sdk.NewCoins(sdk.NewCoin("denom", sdk.NewInt(1_000))), time.Now().Add(30*time.Hour).Unix()),
Expand All @@ -654,7 +654,7 @@ func TestInstantiateWithAccounts(t *testing.T) {
expAccount: authtypes.NewBaseAccount(contractAddr, nil, lastAccountNumber+2, 0), // +1 for next seq, +1 for spec.account created
expBalance: sdk.NewCoins(sdk.NewCoin("denom", sdk.NewInt(1))),
},
"prune listed ContinuousVestingAccount gets overwritten": {
"prunable ContinuousVestingAccount gets overwritten": {
account: vestingtypes.NewContinuousVestingAccount(
authtypes.NewBaseAccount(contractAddr, nil, 0, 0),
sdk.NewCoins(sdk.NewCoin("denom", sdk.NewInt(1_000))), time.Now().Add(time.Hour).Unix(), time.Now().Add(2*time.Hour).Unix()),
Expand All @@ -663,14 +663,14 @@ func TestInstantiateWithAccounts(t *testing.T) {
expAccount: authtypes.NewBaseAccount(contractAddr, nil, lastAccountNumber+2, 0), // +1 for next seq, +1 for spec.account created
expBalance: sdk.NewCoins(sdk.NewCoin("denom", sdk.NewInt(1))),
},
"prune listed account without balance gets overwritten": {
"prunable account without balance gets overwritten": {
account: vestingtypes.NewContinuousVestingAccount(
authtypes.NewBaseAccount(contractAddr, nil, 0, 0),
sdk.NewCoins(sdk.NewCoin("denom", sdk.NewInt(0))), time.Now().Add(time.Hour).Unix(), time.Now().Add(2*time.Hour).Unix()),
expAccount: authtypes.NewBaseAccount(contractAddr, nil, lastAccountNumber+2, 0), // +1 for next seq, +1 for spec.account created
expBalance: sdk.NewCoins(),
},
"unknown account type creates error": {
"unknown account type is rejected with error": {
account: authtypes.NewModuleAccount(
authtypes.NewBaseAccount(contractAddr, nil, 0, 0),
"testing",
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice test!

parentCtx, keepers := CreateTestInput(t, false, AvailableCapabilities)
amts := sdk.NewCoins(sdk.NewInt64Coin("denom", 100))
senderAddr := keepers.Faucet.NewFundedRandomAccount(parentCtx, amts...)

// create vesting account
var vestingAddr sdk.AccAddress = rand.Bytes(types.ContractAddrLen)
msgCreateVestingAccount := vestingtypes.NewMsgCreateVestingAccount(senderAddr, vestingAddr, amts, time.Now().Add(time.Minute).Unix(), false)
_, err := vesting.NewMsgServerImpl(keepers.AccountKeeper, keepers.BankKeeper).CreateVestingAccount(sdk.WrapSDKContext(parentCtx), msgCreateVestingAccount)
require.NoError(t, err)
myVestingAccount := keepers.AccountKeeper.GetAccount(parentCtx, vestingAddr)
require.NotNil(t, myVestingAccount)

specs := map[string]struct {
setupAcc func(t *testing.T, ctx sdk.Context) authtypes.AccountI
expBalances sdk.Coins
expHandled bool
expErr *sdkerrors.Error
}{
"vesting account - all removed": {
setupAcc: func(t *testing.T, ctx sdk.Context) authtypes.AccountI { return myVestingAccount },
expBalances: sdk.NewCoins(),
expHandled: true,
},
"vesting account with other tokens - only original denoms removed": {
setupAcc: func(t *testing.T, ctx sdk.Context) authtypes.AccountI {
keepers.Faucet.Fund(ctx, vestingAddr, sdk.NewCoin("other", sdk.NewInt(2)))
return myVestingAccount
},
expBalances: sdk.NewCoins(sdk.NewCoin("other", sdk.NewInt(2))),
expHandled: true,
},
"non vesting account - not handled": {
setupAcc: func(t *testing.T, ctx sdk.Context) authtypes.AccountI {
return &authtypes.BaseAccount{Address: myVestingAccount.GetAddress().String()}
},
expBalances: sdk.NewCoins(sdk.NewCoin("denom", sdk.NewInt(100))),
expHandled: false,
},
}
for name, spec := range specs {
t.Run(name, func(t *testing.T) {
ctx, _ := parentCtx.CacheContext()
existingAccount := spec.setupAcc(t, ctx)
// overwrite account in store as in keeper before calling prune
keepers.AccountKeeper.SetAccount(ctx, keepers.AccountKeeper.NewAccountWithAddress(ctx, vestingAddr))

// when
noGasCtx := ctx.WithGasMeter(sdk.NewGasMeter(0)) // should not use callers gas
gotHandled, gotErr := NewVestingCoinBurner(keepers.BankKeeper).CleanupExistingAccount(noGasCtx, existingAccount)
// then
if spec.expErr != nil {
require.ErrorIs(t, gotErr, spec.expErr)
return
}
require.NoError(t, gotErr)
assert.Equal(t, spec.expBalances, keepers.BankKeeper.GetAllBalances(ctx, vestingAddr))
assert.Equal(t, spec.expHandled, gotHandled)
// and no out of gas panic
})
}
}
19 changes: 3 additions & 16 deletions x/wasm/keeper/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,14 @@ func WithCoinTransferrer(x CoinTransferrer) Option {
})
}

// WithCoinPruner is an optional constructor parameter to set a custom type that handles balances
// WithAccountPruner is an optional constructor parameter to set a custom type that handles balances and data cleanup
// for accounts pruned on contract instantiate
func WithCoinPruner(x CoinPruner) Option {
func WithAccountPruner(x AccountPruner) Option {
if x == nil {
panic("must not be nil")
}
return optsFn(func(k *Keeper) {
k.coinPruner = x
k.accountPruner = x
})
}

Expand Down Expand Up @@ -154,19 +154,6 @@ func WithAcceptedAccountTypesOnContractInstantiation(accts ...authtypes.AccountI
})
}

// WithPruneAccountTypesOnContractInstantiation sets the account types that should be cleared. Account types of this list
// will be overwritten with the BaseAccount type and their balance burned, when they exist for an address on contract
// instantiation.
//
// Values should be references and contain the `*vestingtypes.DelayedVestingAccount`, `*vestingtypes.ContinuousVestingAccount`
// as post genesis account types with an open address range.
func WithPruneAccountTypesOnContractInstantiation(accts ...authtypes.AccountI) Option {
m := asTypeMap(accts)
return optsFn(func(k *Keeper) {
k.pruneAccountTypes = m
})
}

func asTypeMap(accts []authtypes.AccountI) map[reflect.Type]struct{} {
m := make(map[reflect.Type]struct{}, len(accts))
for _, a := range accts {
Expand Down
16 changes: 3 additions & 13 deletions x/wasm/keeper/options_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,20 +95,10 @@ func TestConstructorOptions(t *testing.T) {
assert.Equal(t, exp, k.acceptedAccountTypes)
},
},
"prune account types": {
srcOpt: WithPruneAccountTypesOnContractInstantiation(&authtypes.BaseAccount{}, &vestingtypes.ContinuousVestingAccount{}),
"account pruner": {
srcOpt: WithAccountPruner(VestingCoinBurner{}),
verify: func(t *testing.T, k Keeper) {
exp := map[reflect.Type]struct{}{
reflect.TypeOf(&authtypes.BaseAccount{}): {},
reflect.TypeOf(&vestingtypes.ContinuousVestingAccount{}): {},
}
assert.Equal(t, exp, k.pruneAccountTypes)
},
},
"coin pruner": {
srcOpt: WithCoinPruner(CoinBurner{}),
verify: func(t *testing.T, k Keeper) {
assert.Equal(t, CoinBurner{}, k.coinPruner)
assert.Equal(t, VestingCoinBurner{}, k.accountPruner)
},
},
}
Expand Down