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

Remove support for ServiceMsgs TypeURLs #9172

Closed
4 tasks
amaury1093 opened this issue Apr 22, 2021 · 2 comments · Fixed by #9139
Closed
4 tasks

Remove support for ServiceMsgs TypeURLs #9172

amaury1093 opened this issue Apr 22, 2021 · 2 comments · Fixed by #9139
Milestone

Comments

@amaury1093
Copy link
Contributor

amaury1093 commented Apr 22, 2021

Summary

Should we still support ServiceMsg TypeURLs in txs in v0.43?

Terminology

1. Msg TypeURL, e.g. "/cosmos.bank.v1beta1.MsgSend"

This is the TypeUrl of a proto message when packed inside an Any. For example: we can pack bank.MsgSend in an Any in txs. We then have any.TypeURL == "/cosmos.bank.v1beta1.MsgSend".

2. ServiceMsg TypeURL, e.g. "/cosmos.bank.v1beta1.Msg/Send"

This is the TypeUrl of an ADR-031 Msg Service method name. In ADR-031, we simulate packing a ServiceMsg in an Any. We then have any.TypeURL == "/cosmos.bank.v1beta1.Msg/Send"

Notice the ServiceMsg TypeURL has 2 / (it defines a service method fully-qualified name) whereas a Msg TypeURL only has one / (it defines a proto message name).

Problem Definition

The SDK currently (as of v0.42) supports txs with both ServiceMsg TypeURLs (routed via Baseapp's msg_service_router) and concrete Msg TypeURLs (routed via the legacy handler). ADR-031's vision was to slowly deprecate Msg TypeURLs.

However, #9063 made a point that ServiceMsg TypeUrls (terminology 2 above) are not protobuf compliant. #9139 was created to deprecate them (more specifically: Baseapp's msg_service_router routes messages using the concrete Msg TypeURLs instead of the ServiceMsg one)

Currently, ServiceMsg TypeURL are still supported, but deprecated, in case some explorers/clients are already using ServiceMsgs.

But should we support&deprecate ServiceMsg TypeURLs in v0.43, or completely remove them?

ref: #9139 (review)

Proposal

Proposal 1: Completely remove ServiceMsg TypeURL support for v0.43.

With documentation, changelog, clear communication etc.

Pros:

  • v0.43 will be protobuf-spec-compliant.
  • ServiceMsg TypeURLs are not yet widely adopted, we can kill it before too many chains upgrade to v0.43.

Cons:

  • Client-breaking change for v0.42->v0.43 upgrades.

Proposal 2: Keep ServiceMsg TypeURL support for v0.43, remove it later (e.g. v0.44).

With deprecation notice.

Pros:

  • Leave time for v0.42 clients/explorers to adapt to this client-breaking change, thanks to deprecation notice.

Cons:

  • Client-breaking change down the road, possibly many chains will already be using ServiceMsg TypeURLs.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@amaury1093 amaury1093 added the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label Apr 22, 2021
@amaury1093 amaury1093 changed the title Remove support for ServiceMsgs Remove support for ServiceMsgs TypeURLs Apr 22, 2021
@amaury1093
Copy link
Contributor Author

I'm in favor of Proposal 1. There's gonna be client breaking changes anyway, so rather soon than later.

@amaury1093
Copy link
Contributor Author

Proposal 1 is implemented in #9139

@aaronc aaronc added in progress and removed backlog T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels Apr 27, 2021
@mergify mergify bot closed this as completed in #9139 Apr 30, 2021
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 a pull request may close this issue.

3 participants