ADR 031: Protobuf Msg Services #7458

aaronc · 2020-10-05T19:13:24Z

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

…ronc/adr-msg-service

anilcse · 2020-10-05T20:22:36Z

docs/architecture/adr-031-msg-service.md

+for `service` definitions. Newer `Msg` types which only support `service` definitions
+should use the more canonical `Msg...Request` names.


For consistency, may be we can avoid Request suffix for newer Msg's as well

I guess that's okay although I prefer to keep the buf linter defaults where possible.

I would make this case a buf linter exception, for the sake of consistency, and also MsgAbc reads better than MsgAbcRequest imo.

Okay, I guess we can enable a linter exception.

docs/architecture/adr-031-msg-service.md

anilcse · 2020-10-05T20:25:46Z

docs/architecture/adr-031-msg-service.md

+
+To do this, `ServiceMsg` implements the `sdk.Msg` interface and its handler does the
+actual method routing, allowing this feature to be added incrementally on top of
+existing functionality.


docs/architecture/adr-031-msg-service.md

amaury1093

This looks exciting! Added some nits and questions.

docs/architecture/adr-031-msg-service.md

amaury1093 · 2020-10-06T08:35:25Z

docs/architecture/adr-031-msg-service.md

+simplified version of `Msg`, without `Route()`, `Type()` and `GetSignBytes()` which
+are no longer needed:


Do the proto messages (e.g. message MsgSubmitProposal{}) still implement sdk.Msg? We still need GetSignBytes() for the legacy sign mode.

They do as long as we are supporting amino and/or concrete Msg types in protobuf. If we switched to service methods everywhere or for new methods that only work with services only MsgRequest would be needed.

docs/architecture/adr-031-msg-service.md

Co-authored-by: Amaury Martiny <amaury.martiny@protonmail.com> Co-authored-by: Anil Kumar Kammari <anil@vitwit.com>

amaury1093

Nice!

I just have some tiny nits (around naming), so pre-approving 👍

blushi

LGTM, that will be a great improvement!

aaronc · 2020-10-07T15:39:18Z

It would be good to get a review from one (or both) of you for this ADR in particular @alexanderbez @alessio.

aaronc · 2020-10-07T15:43:20Z

For those who have reviewed this, I'm wondering what people's opinion is on the long term best strategy regarding Msg encoding.

Should we keep supporting both concrete Msg types and service methods?

Or should we unify on service methods as a long term strategy?

(I'm personally leaning towards the latter of using service methods.)

amaury1093 · 2020-10-07T16:01:26Z

Should we keep supporting both concrete Msg types and service methods? Or should we unify on service methods as a long term strategy?

Ah, from #7458 (comment) i was under the impression we need to support concrete Msg as long as we support LEGACY_SIGN_MODE_AMINO.

But overall, I'm also leaning towards using service methods everywhere.

robert-zaremba

@aaronc thanks for explaining the goals here. I was confused what this ADR is doing, because at the beginning we are talking about extending Msg type with an expected return type, and only in line 82 there is a mention about creating RPC implementations that encapsulate transaction. In between we are talking about a decision describing a SubmitProposal service (in second line) without mentioning that this is an example of constructing transactions for a x/gov/ module message type.

Overall I really like the idea, but would appreciate if we reword it. We can do it in a second iteration. I will commit a changset here to include an abstract an add a paragraph to Decision section. However having more clarifications would be really good.

+ Adding an abstract + Updating the first paragraph of decision

aaronc · 2020-10-08T18:03:39Z

I will commit a changset here to include an abstract an add a paragraph to Decision section.

Maybe just do that as change request suggestions next time in the PR review? You committed some changes but the PR review is still requesting changes, and I'd like to be able to go through and either approve them or adjust the wording.

docs/architecture/adr-031-msg-service.md

…ronc/adr-msg-service

robert-zaremba

Much better now. Thanks.

amaury1093

Approving the new additions.

amaury1093 · 2020-10-09T10:26:19Z

docs/architecture/adr-031-msg-service.md

+
+## Status
+
+Proposed


Suggested change

Proposed

Accepted

What does it take to make this ADR accepted? I'm in favor of implementing this ASAP, as a clean base for upcoming module development.

We don't actually have a process around ADR status. Let's discuss on today's architecture call.

I would prefer to have it iteratively. We can speedup if needed. Usually a second round for something important is good, and will reach to the wider audience. It might be as simple as changing a status though.

Also, for some cases it will be good to firstly prepare an example implementation / reference. While submitting that example, we can pair it with ADR status update PR. Sometimes it could be useful, because during the implementation we may find that something doesn't work or will require some updates.

aaronc · 2020-10-09T16:57:23Z

This has 6 approvals. I'm inclined to go ahead and merge it. There is a sentiment work on this can and should start soon. Once it's merged, I'll open an issue breaking down implementation steps.

alexanderbez · 2020-10-09T17:07:54Z

6 is more than enough. Merge away 👍

…ronc/adr-msg-service

aaronc · 2020-10-13T16:48:32Z

@zmanian

aaronc added 11 commits October 2, 2020 16:19

Ideas

5469ad4

WIP

f12ed01

Add ADR 031

65b6d35

WIP

55dd534

WIP

3f05f02

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into aa…

acac4ba

…ronc/adr-msg-service

Revert changes

917d9e6

Revert changes

641888c

Revert changes

ae691d2

Updates

3939675

Updates

21915a7

aaronc marked this pull request as ready for review October 5, 2020 19:54

aaronc requested review from alessio and alexanderbez as code owners October 5, 2020 19:54

aaronc requested review from amaury1093, anilcse, blushi, clevinson, ethanfrey and robert-zaremba October 5, 2020 19:54

anilcse reviewed Oct 5, 2020

View reviewed changes

amaury1093 reviewed Oct 6, 2020

View reviewed changes

aaronc and others added 2 commits October 6, 2020 10:41

Apply suggestions from code review

28cb7c2

Co-authored-by: Amaury Martiny <amaury.martiny@protonmail.com> Co-authored-by: Anil Kumar Kammari <anil@vitwit.com>

Merge branch 'master' into aaronc/adr-msg-service

d874b2e

aaronc mentioned this pull request Oct 6, 2020

ADR 030: Authorization Module #7105

Merged

9 tasks

amaury1093 approved these changes Oct 7, 2020

View reviewed changes

blushi approved these changes Oct 7, 2020

View reviewed changes

robert-zaremba requested changes Oct 8, 2020

View reviewed changes

Adding clarifications

12a168d

+ Adding an abstract + Updating the first paragraph of decision

aaronc commented Oct 8, 2020

View reviewed changes

docs/architecture/adr-031-msg-service.md Outdated Show resolved Hide resolved

aaronc added 4 commits October 8, 2020 14:25

Update Context and wording

48147ec

Update wording

48db5d3

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into aa…

3a22fc4

…ronc/adr-msg-service

Address proto naming

b0a1c43

aaronc requested a review from robert-zaremba October 8, 2020 19:31

anilcse approved these changes Oct 8, 2020

View reviewed changes

Merge branch 'master' into aaronc/adr-msg-service

dce4949

aaronc added T: ADR An issue or PR relating to an architectural decision record C:Encoding T: Dev UX UX for SDK developers (i.e. how to call our code) T: Client UX labels Oct 8, 2020

robert-zaremba approved these changes Oct 9, 2020

View reviewed changes

amaury1093 approved these changes Oct 9, 2020

View reviewed changes

Merge branch 'master' into aaronc/adr-msg-service

40914f9

alessio approved these changes Oct 9, 2020

View reviewed changes

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into aa…

625d2f4

…ronc/adr-msg-service

aaronc mentioned this pull request Oct 9, 2020

Implement ADR 031: Protobuf Msg Services #7500

Closed

16 tasks

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

Merge branch 'master' into aaronc/adr-msg-service

05dae26

mergify bot merged commit c7d926d into master Oct 9, 2020

mergify bot deleted the aaronc/adr-msg-service branch October 9, 2020 19:13

amaury1093 mentioned this pull request Oct 14, 2020

Update the ADR process #7498

Closed

4 tasks

clevinson mentioned this pull request Oct 14, 2020

Implement Protobuf Msg Services #7540

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADR 031: Protobuf Msg Services #7458

ADR 031: Protobuf Msg Services #7458

aaronc commented Oct 5, 2020 •

edited

Loading

anilcse Oct 5, 2020

aaronc Oct 6, 2020

amaury1093 Oct 7, 2020

aaronc Oct 7, 2020

anilcse Oct 5, 2020

amaury1093 left a comment

amaury1093 Oct 6, 2020

aaronc Oct 6, 2020

amaury1093 left a comment •

edited

Loading

blushi left a comment

aaronc commented Oct 7, 2020

aaronc commented Oct 7, 2020

amaury1093 commented Oct 7, 2020

robert-zaremba left a comment •

edited

Loading

aaronc commented Oct 8, 2020 •

edited

Loading

robert-zaremba left a comment

amaury1093 left a comment

amaury1093 Oct 9, 2020 •

edited

Loading

aaronc Oct 9, 2020

robert-zaremba Oct 9, 2020

aaronc commented Oct 9, 2020

alexanderbez commented Oct 9, 2020

aaronc commented Oct 13, 2020

		for `service` definitions. Newer `Msg` types which only support `service` definitions
		should use the more canonical `Msg...Request` names.

		simplified version of `Msg`, without `Route()`, `Type()` and `GetSignBytes()` which
		are no longer needed:

ADR 031: Protobuf Msg Services #7458

ADR 031: Protobuf Msg Services #7458

Conversation

aaronc commented Oct 5, 2020 • edited Loading

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 left a comment • edited Loading

Choose a reason for hiding this comment

blushi left a comment

Choose a reason for hiding this comment

aaronc commented Oct 7, 2020

aaronc commented Oct 7, 2020

amaury1093 commented Oct 7, 2020

robert-zaremba left a comment • edited Loading

Choose a reason for hiding this comment

aaronc commented Oct 8, 2020 • edited Loading

robert-zaremba left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Oct 9, 2020

alexanderbez commented Oct 9, 2020

aaronc commented Oct 13, 2020

aaronc commented Oct 5, 2020 •

edited

Loading

amaury1093 left a comment •

edited

Loading

robert-zaremba left a comment •

edited

Loading

aaronc commented Oct 8, 2020 •

edited

Loading

amaury1093 Oct 9, 2020 •

edited

Loading