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

Refactor wasm message handlers and query plugin #460

Merged
merged 4 commits into from
Mar 18, 2021

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Mar 16, 2021

This is a refactoring within the wasmvm message handling components and queries.

Design goal was to

  • separation of message handling and encoding - IBC packets were hacked into encoders
  • use of interfaces for decoupling
  • drop of nil instantiation parameters for default plugins

New Options as extension points:

  • WithMessageHandler - replace default message handler
  • WithQueryHandler - replace default query handler
  • WithMessageEncoders - replace default encoders
  • WithQueryPlugins - replace default query plugins

@alpe alpe requested a review from ethanfrey March 16, 2021 16:26
@alpe alpe marked this pull request as draft March 16, 2021 16:26
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks nice. And cool trick with the chaining and ErrUnknownMsg

x/wasm/internal/keeper/handler_plugin_test.go Outdated Show resolved Hide resolved
x/wasm/internal/keeper/handler_plugin.go Show resolved Hide resolved
x/wasm/internal/keeper/handler_plugin.go Show resolved Hide resolved
x/wasm/internal/keeper/keeper.go Show resolved Hide resolved
x/wasm/internal/keeper/options.go Show resolved Hide resolved
x/wasm/internal/keeper/options.go Show resolved Hide resolved
x/wasm/internal/keeper/options.go Outdated Show resolved Hide resolved
@CosmWasm CosmWasm deleted a comment from codecov bot Mar 18, 2021
@codecov
Copy link

codecov bot commented Mar 18, 2021

Codecov Report

Merging #460 (612ff63) into master (ffb7c29) will increase coverage by 1.39%.
The diff coverage is 79.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #460      +/-   ##
==========================================
+ Coverage   55.48%   56.88%   +1.39%     
==========================================
  Files          39       40       +1     
  Lines        4318     4356      +38     
==========================================
+ Hits         2396     2478      +82     
+ Misses       1721     1674      -47     
- Partials      201      204       +3     
Impacted Files Coverage Δ
app/app.go 86.80% <ø> (-0.11%) ⬇️
x/wasm/internal/keeper/options.go 73.33% <61.90%> (-26.67%) ⬇️
x/wasm/internal/keeper/handler_plugin_encoders.go 78.28% <78.28%> (ø)
x/wasm/internal/keeper/handler_plugin.go 85.13% <80.39%> (+8.94%) ⬆️
x/wasm/internal/keeper/query_plugins.go 76.51% <81.25%> (+18.96%) ⬆️
x/wasm/internal/keeper/keeper.go 86.35% <95.65%> (ø)
x/wasm/internal/keeper/relay.go 100.00% <100.00%> (ø)

@alpe alpe marked this pull request as ready for review March 18, 2021 11:48
@alpe alpe merged commit 8e35dc2 into master Mar 18, 2021
@alpe alpe deleted the message_handler_separation branch March 18, 2021 14:31
})
}

// WithMessageHandler is an optional constructor parameter to set a custom message handler.
// WithMessageHandler is an optional constructor parameter to set a custom handler for wasmVM messages.
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker, but an idea for the future.

Now that we chain the handlers, most customizations (outside of tests) would likely be prepending or appending something to the default chain. This should be easy to do.

Right now, making a new chain like the default is exposed as public constructors

return NewMessageHandlerChain(
		NewSDKMessageHandler(router, encoders),
		NewIBCRawPacketHandler(channelKeeper, capabilityKeeper),
	)

But it does require the router, and various keepers to construct it. Those are not exposed on the keeper, we could only extend the default if we do that in app.go where we have the keepers, or some other code that wraps the constuctor of the keeper. Maybe that is the only place it would make sense to customize the handler, but just thinking....

If this makes sense to you, please open a future issue to allow appending/prepending to the default chain. If you think this is do-able with the current API, then I am fine.

Use case: if there is something I cannot express as an Encoder and I want to add support for that Custom message, I want to be able to extend this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants