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

doc: Updates related to ServiceMsg, sdk.Msg and Msg service #9294

Merged
merged 6 commits into from
May 12, 2021

Conversation

robert-zaremba
Copy link
Collaborator

@robert-zaremba robert-zaremba commented May 11, 2021

Description

In this PR I'm trying to distinguish sdk.Msg from Msg service as well as remove leftovers of ServiceMsg.
There are many places in documentation which were not very clear - I hope this will improve them.

closes: #9208
related to: baseapp audit


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

@robert-zaremba robert-zaremba added the T:Docs Changes and features related to documentation. label May 11, 2021
Copy link
Contributor

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

Looking good! A lot of minor suggestions and fixes.

Co-authored-by: Ryan Christoffersen <12519942+ryanchrypto@users.noreply.github.com>
@robert-zaremba
Copy link
Collaborator Author

Thank you @ryanchrypto for looking at the PRs and improving it.

Copy link
Contributor

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

LGTM overall, proposing to rename "service RPC" to "service method"

@@ -139,7 +139,9 @@ Modules must implement [interfaces](../building-modules/module-manager.md#applic

### `Msg` Services

Each module defines two [Protobuf services](https://developers.google.com/protocol-buffers/docs/proto#services): one `Msg` service to handle messages, and one gRPC `Query` service to handle queries. If we consider the module as a state-machine, then a `Msg` is a state transition. A `Msg` service is a Protobuf service defining all possible `Msg`s a module exposes. Note that `Msg`s are bundled in [`transactions`](../core/transactions.md), and each transaction contains one or multiple `messages`.
Each module defines two [Protobuf services](https://developers.google.com/protocol-buffers/docs/proto#services): one `Msg` service to handle messages, and one gRPC `Query` service to handle queries. If we consider the module as a state-machine, then a `Msg` service is a set of state transition RPC methods.
Each Protobuf `Msg` service RPC is 1:1 related to a Protobuf request type, which must implement `sdk.Msg` interface.
Copy link
Contributor

@amaury1093 amaury1093 May 12, 2021

Choose a reason for hiding this comment

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

So in the protobuf docs, a service == a RPC service, and that's where RPC is mentioned. What you call "service RPC" here is actually what protobuf calls a method.

My proposal would be to s/service RPC/service method/g (in all docs), what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about it, I thought that we use method because of Go, and in proto each endpoint is marked as RPC. But if in prodobuf docs they use method then let's use it here as well. I'm updating the docs.

@robert-zaremba robert-zaremba requested a review from amaury1093 May 12, 2021 13:30
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label May 12, 2021
@mergify mergify bot merged commit f2cea6a into master May 12, 2021
@mergify mergify bot deleted the robert/docs-update branch May 12, 2021 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:x/authz T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove mentions of ServiceMsg TypeURLs in docs
3 participants