Skip to content

Commit

Permalink
Review feedback: better naming
Browse files Browse the repository at this point in the history
  • Loading branch information
alpe committed Sep 21, 2022
1 parent e04d1a2 commit 4c85b0d
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 20 deletions.
27 changes: 14 additions & 13 deletions x/wasm/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +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 the given account. The method returns true when the account
// balance is handled. Unsupported account types are rejected by returning false
PruneBalances(ctx sdk.Context, existingAccount authtypes.AccountI) (handled bool, err 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 Down Expand Up @@ -101,7 +102,7 @@ type Keeper struct {
gasRegister GasRegister
maxQueryStackSize uint32
acceptedAccountTypes map[reflect.Type]struct{}
coinPruner CoinPruner
accountPruner AccountPruner
}

// NewKeeper creates a new contract Keeper instance
Expand Down Expand Up @@ -140,7 +141,7 @@ func NewKeeper(
wasmVM: wasmer,
accountKeeper: accountKeeper,
bank: NewBankCoinTransferrer(bankKeeper),
coinPruner: NewVestingCoinBurner(bankKeeper),
accountPruner: NewVestingCoinBurner(bankKeeper),
portKeeper: portKeeper,
capabilityKeeper: capabilityKeeper,
messenger: NewDefaultMessageHandler(router, channelKeeper, capabilityKeeper, bankKeeper, cdc, portSource),
Expand Down Expand Up @@ -313,7 +314,7 @@ func (k Keeper) instantiate(ctx sdk.Context, codeID uint64, creator, admin sdk.A
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
switch handled, err := k.coinPruner.PruneBalances(ctx, existingAcct); {
switch handled, err := k.accountPruner.CleanupExistingAccount(ctx, existingAcct); {
case err != nil:
return nil, nil, sdkerrors.Wrap(err, "prune balance")
case !handled:
Expand Down Expand Up @@ -1148,9 +1149,9 @@ func (c BankCoinTransferrer) TransferCoins(parentCtx sdk.Context, fromAddr sdk.A
return nil
}

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

// VestingCoinBurner default implementation for CoinPruner to burn the coins
// VestingCoinBurner default implementation for AccountPruner to burn the coins
type VestingCoinBurner struct {
bank types.BankKeeper
}
Expand All @@ -1163,9 +1164,9 @@ func NewVestingCoinBurner(bank types.BankKeeper) VestingCoinBurner {
return VestingCoinBurner{bank: bank}
}

// PruneBalances accepts only vesting account types to burns all their original vesting coin balances.
// Other account types will be rejected with a types.ErrAccountExists
func (b VestingCoinBurner) PruneBalances(ctx sdk.Context, existingAcc authtypes.AccountI) (handled bool, err error) {
// 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 {
return false, nil
Expand Down
2 changes: 1 addition & 1 deletion x/wasm/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2324,7 +2324,7 @@ func TestCoinBurnerPruneBalances(t *testing.T) {

// when
noGasCtx := ctx.WithGasMeter(sdk.NewGasMeter(0)) // should not use callers gas
gotHandled, gotErr := NewVestingCoinBurner(keepers.BankKeeper).PruneBalances(noGasCtx, existingAccount)
gotHandled, gotErr := NewVestingCoinBurner(keepers.BankKeeper).CleanupExistingAccount(noGasCtx, existingAccount)
// then
if spec.expErr != nil {
require.ErrorIs(t, gotErr, spec.expErr)
Expand Down
6 changes: 3 additions & 3 deletions x/wasm/keeper/options.go
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
6 changes: 3 additions & 3 deletions x/wasm/keeper/options_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,10 @@ func TestConstructorOptions(t *testing.T) {
assert.Equal(t, exp, k.acceptedAccountTypes)
},
},
"coin pruner": {
srcOpt: WithCoinPruner(VestingCoinBurner{}),
"account pruner": {
srcOpt: WithAccountPruner(VestingCoinBurner{}),
verify: func(t *testing.T, k Keeper) {
assert.Equal(t, VestingCoinBurner{}, k.coinPruner)
assert.Equal(t, VestingCoinBurner{}, k.accountPruner)
},
},
}
Expand Down

0 comments on commit 4c85b0d

Please sign in to comment.