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.

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

Allow better override of wasmVM in x/wasm keeper #1494

Merged
merged 3 commits into from
Jul 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 17 additions & 8 deletions x/wasm/keeper/keeper_cgo.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,10 @@ func NewKeeper(
authority string,
opts ...Option,
) Keeper {
wasmer, err := wasmvm.NewVM(filepath.Join(homeDir, "wasm"), availableCapabilities, contractMemoryLimit, wasmConfig.ContractDebugMode, wasmConfig.MemoryCacheSize)
if err != nil {
panic(err)
}

keeper := &Keeper{
storeKey: storeKey,
cdc: cdc,
wasmVM: wasmer,
wasmVM: nil,
accountKeeper: accountKeeper,
bank: NewBankCoinTransferrer(bankKeeper),
accountPruner: NewVestingCoinBurner(bankKeeper),
Expand All @@ -60,10 +55,24 @@ func NewKeeper(
authority: authority,
}
keeper.wasmVMQueryHandler = DefaultQueryPlugins(bankKeeper, stakingKeeper, distrKeeper, channelKeeper, keeper)
for _, o := range opts {
preOpts, postOpts := splitOpts(opts)
for _, o := range preOpts {
o.apply(keeper)
}
// only set the wasmvm if no one set this in the options
// NewVM does a lot, so better not to create it and silently drop it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for bringing this up! Unfortunately, the implementation conflicts with the WithWasmEngineDecorator now that requires the object to exist.
I will come up with some solution

if keeper.wasmVM == nil {
var err error
keeper.wasmVM, err = wasmvm.NewVM(filepath.Join(homeDir, "wasm"), availableCapabilities, contractMemoryLimit, wasmConfig.ContractDebugMode, wasmConfig.MemoryCacheSize)
if err != nil {
panic(err)
}
}

for _, o := range postOpts {
o.apply(keeper)
}
// not updateable, yet
// not updatable, yet
keeper.wasmVMResponseHandler = NewDefaultWasmVMContractResponseHandler(NewMessageDispatcher(keeper.messenger, keeper))
return *keeper
}
26 changes: 23 additions & 3 deletions x/wasm/keeper/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ func (f optsFn) apply(keeper *Keeper) {
f(keeper)
}

// option that is applied after keeper is setup with the VM. Used for decorators mainly.
type postOptsFn func(*Keeper)

func (f postOptsFn) apply(keeper *Keeper) {
f(keeper)
}

// WithWasmEngine is an optional constructor parameter to replace the default wasmVM engine with the
// given one.
func WithWasmEngine(x types.WasmerEngine) Option {
Expand All @@ -26,7 +33,7 @@ func WithWasmEngine(x types.WasmerEngine) Option {

// WithWasmEngineDecorator is an optional constructor parameter to decorate the default wasmVM engine.
func WithWasmEngineDecorator(d func(old types.WasmerEngine) types.WasmerEngine) Option {
return optsFn(func(k *Keeper) {
return postOptsFn(func(k *Keeper) {
k.wasmVM = d(k.wasmVM)
})
}
Expand All @@ -42,7 +49,7 @@ func WithMessageHandler(x Messenger) Option {
// WithMessageHandlerDecorator is an optional constructor parameter to decorate the wasm handler for wasmVM messages.
// This option should not be combined with Option `WithMessageEncoders` or `WithMessageHandler`
func WithMessageHandlerDecorator(d func(old Messenger) Messenger) Option {
return optsFn(func(k *Keeper) {
return postOptsFn(func(k *Keeper) {
k.messenger = d(k.messenger)
})
}
Expand All @@ -58,7 +65,7 @@ func WithQueryHandler(x WasmVMQueryHandler) Option {
// WithQueryHandlerDecorator is an optional constructor parameter to decorate the default wasm query handler for wasmVM requests.
// This option should not be combined with Option `WithQueryPlugins` or `WithQueryHandler`
func WithQueryHandlerDecorator(d func(old WasmVMQueryHandler) WasmVMQueryHandler) Option {
return optsFn(func(k *Keeper) {
return postOptsFn(func(k *Keeper) {
k.wasmVMQueryHandler = d(k.wasmVMQueryHandler)
})
}
Expand Down Expand Up @@ -189,3 +196,16 @@ func asTypeMap(accts []authtypes.AccountI) map[reflect.Type]struct{} {
}
return m
}

// split into pre and post VM operations
func splitOpts(opts []Option) ([]Option, []Option) {
pre, post := make([]Option, 0), make([]Option, 0)
for _, o := range opts {
if _, ok := o.(postOptsFn); ok {
post = append(post, o)
} else {
pre = append(pre, o)
}
}
return pre, post
}
42 changes: 39 additions & 3 deletions x/wasm/keeper/options_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ import (

func TestConstructorOptions(t *testing.T) {
specs := map[string]struct {
srcOpt Option
verify func(*testing.T, Keeper)
srcOpt Option
verify func(*testing.T, Keeper)
isPostOpt bool
}{
"wasm engine": {
srcOpt: WithWasmEngine(&wasmtesting.MockWasmer{}),
Expand All @@ -37,6 +38,7 @@ func TestConstructorOptions(t *testing.T) {
verify: func(t *testing.T, k Keeper) {
assert.IsType(t, &wasmtesting.MockWasmer{}, k.wasmVM)
},
isPostOpt: true,
},
"message handler": {
srcOpt: WithMessageHandler(&wasmtesting.MockMessageHandler{}),
Expand All @@ -58,6 +60,7 @@ func TestConstructorOptions(t *testing.T) {
verify: func(t *testing.T, k Keeper) {
assert.IsType(t, &wasmtesting.MockMessageHandler{}, k.messenger)
},
isPostOpt: true,
},
"query plugins decorator": {
srcOpt: WithQueryHandlerDecorator(func(old WasmVMQueryHandler) WasmVMQueryHandler {
Expand All @@ -67,6 +70,7 @@ func TestConstructorOptions(t *testing.T) {
verify: func(t *testing.T, k Keeper) {
assert.IsType(t, &wasmtesting.MockQueryHandler{}, k.wasmVMQueryHandler)
},
isPostOpt: true,
},
"coin transferrer": {
srcOpt: WithCoinTransferrer(&wasmtesting.MockCoinTransferrer{}),
Expand Down Expand Up @@ -123,7 +127,10 @@ func TestConstructorOptions(t *testing.T) {
}
for name, spec := range specs {
t.Run(name, func(t *testing.T) {
k := NewKeeper(nil, nil, authkeeper.AccountKeeper{}, &bankkeeper.BaseKeeper{}, stakingkeeper.Keeper{}, nil, nil, nil, nil, nil, nil, nil, nil, "tempDir", types.DefaultWasmConfig(), AvailableCapabilities, "", spec.srcOpt)
opt := spec.srcOpt
_, gotPostOptMarker := opt.(postOptsFn)
require.Equal(t, spec.isPostOpt, gotPostOptMarker)
k := NewKeeper(nil, nil, authkeeper.AccountKeeper{}, &bankkeeper.BaseKeeper{}, stakingkeeper.Keeper{}, nil, nil, nil, nil, nil, nil, nil, nil, "tempDir", types.DefaultWasmConfig(), AvailableCapabilities, "", opt)
spec.verify(t, k)
})
}
Expand All @@ -133,3 +140,32 @@ func setAPIDefaults() {
costHumanize = DefaultGasCostHumanAddress * DefaultGasMultiplier
costCanonical = DefaultGasCostCanonicalAddress * DefaultGasMultiplier
}

func TestSplitOpts(t *testing.T) {
a := optsFn(nil)
b := optsFn(nil)
c := postOptsFn(nil)
d := postOptsFn(nil)
specs := map[string]struct {
src []Option
expPre, expPost []Option
}{
"by type": {
src: []Option{a, c},
expPre: []Option{a},
expPost: []Option{c},
},
"ordered": {
src: []Option{a, b, c, d},
expPre: []Option{a, b},
expPost: []Option{c, d},
},
}
for name, spec := range specs {
t.Run(name, func(t *testing.T) {
gotPre, gotPost := splitOpts(spec.src)
assert.Equal(t, spec.expPre, gotPre)
assert.Equal(t, spec.expPost, gotPost)
})
}
}