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 to GRPC message server #366

Merged
merged 7 commits into from
Jan 8, 2021
Merged

Refactor to GRPC message server #366

merged 7 commits into from
Jan 8, 2021

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Jan 8, 2021

Resolves #328

This PR introduces support for the sdk grpc server to handle messages.
It introduces a breaking change to the response data that is returned. Instead of our custom data it is marshalled protobuf now. See tx.proto for service and type definitions

Left to do:

  • Tests for keeper/message server
  • Refactor handler tests to use a MsgServer mock

@codecov
Copy link

codecov bot commented Jan 8, 2021

Codecov Report

Merging #366 (5f8c246) into master (af01d0b) will decrease coverage by 1.17%.
The diff coverage is 14.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #366      +/-   ##
==========================================
- Coverage   57.19%   56.02%   -1.18%     
==========================================
  Files          33       34       +1     
  Lines        3453     3461       +8     
==========================================
- Hits         1975     1939      -36     
- Misses       1327     1376      +49     
+ Partials      151      146       -5     
Impacted Files Coverage Δ
x/wasm/internal/keeper/msg_server.go 0.00% <0.00%> (ø)
x/wasm/internal/types/codec.go 43.75% <0.00%> (-1.42%) ⬇️
x/wasm/internal/types/tx.go 54.00% <ø> (ø)
x/wasm/module.go 22.64% <0.00%> (-0.44%) ⬇️
x/wasm/handler.go 74.28% <86.95%> (+31.42%) ⬆️
x/wasm/internal/keeper/keeper.go 89.97% <0.00%> (-0.59%) ⬇️

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.

Nice work. A few comments on my pass through it.
I do think this is an improvement, and we can shove it into 0.14 so cosmjs works the same for 0.14 and 0.15

x/wasm/genesis_test.go Outdated Show resolved Hide resolved
if msg.Admin != "" {
if adminAddr, err = sdk.AccAddressFromBech32(msg.Admin); err != nil {
return nil, sdkerrors.Wrap(err, "admin")
func hasWasmModuleAttribute(attrs []abci.EventAttribute) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? I am a bit unsure and need to investigate the event/attribute system a bit more.

There were multiple sub-messages merging into each other before causing confusion. I will try some tests in a branch

Copy link
Member

Choose a reason for hiding this comment

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

Saved follow up for #368 but not for 0.14.0 release

x/wasm/internal/keeper/msg_server.go Show resolved Hide resolved
repeated cosmos.base.v1beta1.Coin init_funds = 6 [(gogoproto.nullable) = false, (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"];
}
// MsgInstantiateContractResponse return instantiation result data
message MsgInstantiateContractResponse {
Copy link
Member

Choose a reason for hiding this comment

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

I do like these clear types for return value

x/wasm/internal/types/tx.proto Outdated Show resolved Hide resolved
x/wasm/internal/types/tx.proto Outdated Show resolved Hide resolved
x/wasm/module_test.go Outdated Show resolved Hide resolved
@ethanfrey
Copy link
Member

I am good to merge this. I need to investigate the how events are handled better in the baseapp and tendermint levels.

@ethanfrey ethanfrey marked this pull request as ready for review January 8, 2021 17:07
@ethanfrey ethanfrey merged commit 0093106 into master Jan 8, 2021
@ethanfrey ethanfrey deleted the grpc_service_328 branch January 8, 2021 17:07
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.

Register GRPC message service
2 participants