RegisterInterfaces registers service Msg type_urls #7671

amaury1093 · 2020-10-26T11:38:51Z

Description

closes: #7647

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2020-10-26T13:55:56Z

Codecov Report

Merging #7671 into master will increase coverage by 0.00%.
The diff coverage is 72.72%.

@@           Coverage Diff           @@
##           master    #7671   +/-   ##
=======================================
  Coverage   54.11%   54.12%           
=======================================
  Files         611      611           
  Lines       38616    38631   +15     
=======================================
+ Hits        20898    20909   +11     
- Misses      15585    15590    +5     
+ Partials     2133     2132    -1

types/msg_service.go

…647-msgserver

baseapp/msg_service_router.go

clevinson

utACK

clevinson · 2020-10-26T22:51:34Z

x/ibc/applications/transfer/types/codec.go

 )

 // RegisterInterfaces register the ibc transfer module interfaces to protobuf
 // Any.
 func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
 	registry.RegisterImplementations((*sdk.Msg)(nil), &MsgTransfer{})
+
+	msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc)


We should make sure this addition is accounted for in the "building a module" docs that @blushi is updating in #7660

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

…k into am-7647-msgserver

…647-msgserver

amaury1093 · 2020-10-27T12:04:00Z

baseapp/msg_service_router.go

+// This function PANICs if it is called before the the service `Msg`s have been
+// registered using RegisterInterfaces.


I believe "running RegisterInterfaces before running RegisterService" is also worth mentioning in docs.

baseapp/msg_service_router.go

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

robert-zaremba

Left few comments and approving.

robert-zaremba · 2020-10-28T18:50:24Z

baseapp/msg_service_router.go

+// This function PANICs if it is called before the the service `Msg`s have been
+// registered using RegisterInterfaces.


robert-zaremba · 2020-10-28T18:53:29Z

types/msgservice/msg_service.go

+			if !ok {
+				// We panic here because there is no other alternative and the app cannot be initialized correctly
+				// this should only happen if there is a problem with code generation in which case the app won't
+				// work correctly anyway.


good comment 👍

robert-zaremba · 2020-10-28T18:55:26Z

types/msgservice/msg_service.go

+				panic(fmt.Errorf("can't register request type %T for service method %s", i, fqMethod))
+			}
+
+			registry.RegisterCustomTypeURL((*sdk.MsgRequest)(nil), fqMethod, msg)


TBH, I don't know why we are typed nil using it in SDK. It's good when we will like to optimize for memory allocation. But here this is called / created only once per app lifetime. So how about using new(sdk.MsgRequest)?

robert-zaremba · 2020-10-28T18:55:50Z

types/msgservice/msg_service.go

+// gRPC NOOP interceptor
+func noopInterceptor(_ context.Context, _ interface{}, _ *grpc.UnaryServerInfo, _ grpc.UnaryHandler) (interface{}, error) {
+	return nil, nil
+}


We already have it in baseapp/msg_service_router.go. Maybe we can move it somewhere and export it?

I actually moved this function from baseapp/msg_service_router.go to here 👍

robert-zaremba · 2020-10-28T18:59:59Z

x/auth/client/cli/cli_test.go

+		fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastBlock),
+		fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()),
+		fmt.Sprintf("--gas=%d", flags.DefaultGasLimit),
+	)


this is an integration test. Not related to this PR, because other code in this test is doing a similar things. In the future we will need to mark this tests as such and skip them if we just want to do unit-tests.

* Add RegisterMsgServiceDesc * Refactor newSendTxMsgServiceCmd * Add test * Register in all modules * Remove RegisterMsgServiceDesc from baseapp * Add explicit error message * Add comment * Update comment * Add test * Update comment * Remove duplicate import * Fix lint * Forgot vesting * Fix lint * Fix test * Put in types/module * Put in types/msgservice * Add comment about panic * Update baseapp/msg_service_router.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Update baseapp/msg_service_router.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Update baseapp/msg_service_router.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> * Add comment * Add better test * Update baseapp/msg_service_router.go Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com> Co-authored-by: Robert Zaremba <robert@zaremba.ch> Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Add RegisterMsgServiceDesc

132e5eb

amaury1093 force-pushed the am-7647-msgserver branch 2 times, most recently from 8e01a7e to 132e5eb Compare October 26, 2020 13:35

Refactor newSendTxMsgServiceCmd

c986595

amaury1093 added 2 commits October 26, 2020 15:44

Add test

3027b94

Register in all modules

03eca89

amaury1093 commented Oct 26, 2020

View reviewed changes

types/msg_service.go Outdated Show resolved Hide resolved

amaury1093 added 12 commits October 26, 2020 16:33

Remove RegisterMsgServiceDesc from baseapp

63562ef

Add explicit error message

c1f6923

Add comment

44e8ac4

Update comment

80def81

Add test

0f6facd

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-7…

5564ad2

…647-msgserver

Update comment

5d8612d

Remove duplicate import

fb43263

Fix lint

0edf85a

Forgot vesting

049bdfa

Fix lint

dd5b42f

Fix test

8710259

amaury1093 marked this pull request as ready for review October 26, 2020 16:29

amaury1093 requested review from aaronc, alessio and alexanderbez as code owners October 26, 2020 16:29

amaury1093 requested review from robert-zaremba, anilcse, blushi and clevinson October 26, 2020 16:29

amaury1093 added backport/0.40.x (Stargate) C:Encoding labels Oct 26, 2020

aaronc reviewed Oct 26, 2020

View reviewed changes

baseapp/msg_service_router.go Show resolved Hide resolved

amaury1093 added 2 commits October 26, 2020 18:03

Put in types/module

196414e

Put in types/msgservice

295f625

robert-zaremba mentioned this pull request Oct 26, 2020

Limit and remove panic usage in the code. #7409

Closed

5 tasks

Add comment about panic

8cd8466

robert-zaremba reviewed Oct 26, 2020

View reviewed changes

baseapp/msg_service_router.go Outdated Show resolved Hide resolved

baseapp/msg_service_router.go Outdated Show resolved Hide resolved

baseapp/msg_service_router.go Outdated Show resolved Hide resolved

robert-zaremba requested changes Oct 26, 2020

View reviewed changes

baseapp/msg_service_router.go Show resolved Hide resolved

clevinson approved these changes Oct 26, 2020

View reviewed changes

amaury1093 and others added 9 commits October 27, 2020 10:53

Update baseapp/msg_service_router.go

9aa2f82

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

Update baseapp/msg_service_router.go

7b32d47

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

Update baseapp/msg_service_router.go

817c0ae

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

Add comment

a2d09ea

Merge branch 'master' into am-7647-msgserver

4ba80f4

Merge branch 'master' into am-7647-msgserver

4401ccf

Add better test

5e5d453

Merge branch 'am-7647-msgserver' of ssh://github.com/cosmos/cosmos-sd…

3a2f2d8

…k into am-7647-msgserver

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-7…

b442ee3

…647-msgserver

amaury1093 commented Oct 27, 2020

View reviewed changes

amaury1093 mentioned this pull request Oct 27, 2020

Add GetTx gRPC endpoint #7688

Merged

11 tasks

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 27, 2020

Merge branch 'master' into am-7647-msgserver

d6d740c

blushi reviewed Oct 28, 2020

View reviewed changes

baseapp/msg_service_router.go Outdated Show resolved Hide resolved

Update baseapp/msg_service_router.go

5da8fb1

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

robert-zaremba approved these changes Oct 28, 2020

View reviewed changes

Merge branch 'master' into am-7647-msgserver

a57edcc

mergify bot merged commit 82f15f3 into master Oct 28, 2020

mergify bot deleted the am-7647-msgserver branch October 28, 2020 19:20

clevinson mentioned this pull request Oct 29, 2020

msg_service_router: reduce code complexity #7570

Merged

9 tasks

clevinson mentioned this pull request Oct 29, 2020

[backports]: v0.40.0-rc2 Cherry-picks #7734

Closed

45 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RegisterInterfaces registers service Msg type_urls #7671

RegisterInterfaces registers service Msg type_urls #7671

amaury1093 commented Oct 26, 2020 •

edited

Loading

codecov bot commented Oct 26, 2020 •

edited

Loading

clevinson left a comment

clevinson Oct 26, 2020

amaury1093 Oct 27, 2020

robert-zaremba Oct 28, 2020

robert-zaremba left a comment

robert-zaremba Oct 28, 2020

robert-zaremba Oct 28, 2020

robert-zaremba Oct 28, 2020

robert-zaremba Oct 28, 2020

amaury1093 Oct 28, 2020 •

edited

Loading

robert-zaremba Oct 28, 2020

		// This function PANICs if it is called before the the service `Msg`s have been
		// registered using RegisterInterfaces.

RegisterInterfaces registers service Msg type_urls #7671

RegisterInterfaces registers service Msg type_urls #7671

Conversation

amaury1093 commented Oct 26, 2020 • edited Loading

Description

codecov bot commented Oct 26, 2020 • edited Loading

Codecov Report

clevinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 commented Oct 26, 2020 •

edited

Loading

codecov bot commented Oct 26, 2020 •

edited

Loading

amaury1093 Oct 28, 2020 •

edited

Loading