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 ServiceMsgs from ADR-031 #9139

Merged
merged 56 commits into from
Apr 30, 2021
Merged

Remove ServiceMsgs from ADR-031 #9139

merged 56 commits into from
Apr 30, 2021

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Apr 19, 2021

Description

  • completely remove ServiceMsg TypeURLs, without supporting backwards compatbility
  • emit events with leading /, e.g. message.action='/cosmos.bank.v1beta1.MsgSend'

closes: #9063
closes: #8864
closes: #9172


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

@fdymylja
Copy link
Contributor

@AmauryM just some small feedback, I'd create a function in types:

func MsgName(msg Msg) { return proto.MessageName(msg) }

Since the message name has become important and we might see it more often, I think it would be better to have this functionality self contained in the types package rather than importing proto whenever the name of a Msg is needed.

@codecov
Copy link

codecov bot commented Apr 20, 2021

Codecov Report

Merging #9139 (e95ae32) into master (6fbded9) will increase coverage by 0.05%.
The diff coverage is 64.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9139      +/-   ##
==========================================
+ Coverage   60.06%   60.11%   +0.05%     
==========================================
  Files         595      595              
  Lines       37355    37184     -171     
==========================================
- Hits        22437    22355      -82     
+ Misses      12929    12848      -81     
+ Partials     1989     1981       -8     
Impacted Files Coverage Δ
codec/types/interface_registry.go 57.95% <ø> (-4.55%) ⬇️
server/rosetta/client_online.go 0.00% <0.00%> (ø)
types/codec.go 66.66% <ø> (+16.66%) ⬆️
types/simulation/types.go 0.00% <0.00%> (ø)
types/tx_msg.go 0.00% <0.00%> (ø)
x/authz/keeper/msg_server.go 0.00% <0.00%> (ø)
x/authz/types/codec.go 0.00% <0.00%> (ø)
x/authz/types/generic_authorization.go 60.00% <ø> (-11.43%) ⬇️
x/bank/client/testutil/cli_helpers.go 50.00% <ø> (+41.89%) ⬆️
x/feegrant/types/filtered_fee.go 0.00% <0.00%> (ø)
... and 29 more

@amaury1093 amaury1093 marked this pull request as ready for review April 20, 2021 14:44
@github-actions github-actions bot added T: ADR An issue or PR relating to an architectural decision record T:Docs Changes and features related to documentation. labels Apr 28, 2021
Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Great simplification and great work!
Left few comments.

CHANGELOG.md Outdated Show resolved Hide resolved
baseapp/baseapp.go Outdated Show resolved Hide resolved
baseapp/msg_service_router.go Outdated Show resolved Hide resolved
// 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.
panic(fmt.Errorf("can't register request type %T for service method %s", i, fqMethod))
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we don't return an error?

Copy link
Contributor Author

@amaury1093 amaury1093 Apr 29, 2021

Choose a reason for hiding this comment

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

If we return an error, then the app will continue running, with potential encoding errors that are hard to debug down the road.

// NOTE: This is how we pull the concrete request type for each handler for registering in the InterfaceRegistry.
// This approach is maybe a bit hacky, but less hacky than reflecting on the handler object itself.
// We use a no-op interceptor to avoid actually calling into the handler itself.
_, _ = methodHandler(nil, context.Background(), func(i interface{}) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there context available in sd? We should use it instead of context.Background

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not, sd is just a descriptor.

// ValidateBasic does a simple validation check that
// doesn't require access to any other information.
ValidateBasic() error

// Get the canonical byte representation of the Msg.
GetSignBytes() []byte

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shall we update the code generator as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure which code generator you're talking about. The one in regen?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought these functions are generated (didn't check it).

}
}
s.Msgs = stdTxMsgs
s.Msgs = msgs
return nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

shall we update the interface to not return an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's update TxBuilder once, when we decide on a good TxBuilder DX (#8138)

x/authz/client/rest/grpc_query_test.go Outdated Show resolved Hide resolved
@@ -21,26 +19,25 @@ func NewSendAuthorization(spendLimit sdk.Coins) *SendAuthorization {

// MethodName implements Authorization.MethodName.
func (authorization SendAuthorization) MethodName() string {
return "/cosmos.bank.v1beta1.Msg/Send"
return sdk.MsgTypeURL(&MsgSend{})
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's move it to a private (module) var. No need to recreate it. We can update other places as well, where we have similar functions.

Copy link
Contributor Author

@amaury1093 amaury1093 Apr 29, 2021

Choose a reason for hiding this comment

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

I tried this, but it actually doesn't work. proto.MessageName(&MsgSend{}) == "" on the module level. I think it's related to the ordering of which modules are run first. I'm not 100% sure how to make sure proto registration is run before defining a module var in this file.

See for example this failing test: https://github.com/cosmos/cosmos-sdk/pull/9139/checks?check_run_id=2467765703#step:6:155

Copy link
Contributor

@fdymylja fdymylja Apr 30, 2021

Choose a reason for hiding this comment

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

you can import like this:

import _ "gitrhub.com/cosmos-sdk/x/bank/types"

this will cause the proto init function in bank generated pb files to be run and the msg name to be correctly resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, thanks!

Which do you think is clearer? Adding import _, or just recreating a sdk.MsgTypeURL() in each function? I somehow feel the latter is better for readability, and less hidden knowledge.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe RegisterInterfaces can set these values?

require.Equal(t, tc.msg.Route(), types.RouterKey, "unexpected result for tc #%d", i)
require.Equal(t, tc.msg.Type(), types.TypeMsgSubmitEvidence, "unexpected result for tc #%d", i)
require.Equal(t, tc.msg.(legacytx.LegacyMsg).Route(), types.RouterKey, "unexpected result for tc #%d", i)
require.Equal(t, tc.msg.(legacytx.LegacyMsg).Type(), types.TypeMsgSubmitEvidence, "unexpected result for tc #%d", i)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this is LegacyMsg?

Copy link
Contributor Author

@amaury1093 amaury1093 Apr 29, 2021

Choose a reason for hiding this comment

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

We moved sdk.Msg to legacytx.LegacyMsg. So in the SDK all msgs are actually stilllegacytx.LegacyMsg.

@amaury1093
Copy link
Contributor Author

@robert-zaremba I addressed all reviews that made sense to me, left comments for the others.

Copy link
Contributor

@technicallyty technicallyty left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 Good stuff - msgs look much simpler/cleaner with this PR!

x/authz/keeper/keeper_test.go Outdated Show resolved Hide resolved
x/authz/keeper/keeper_test.go Outdated Show resolved Hide resolved
@amaury1093
Copy link
Contributor Author

amaury1093 commented Apr 30, 2021

@robert-zaremba I'm putting automerge on to unblock your work on Authz. I'll also work on a follow-up PR cleaning stuff (#9139 (comment), #9139 (comment), #9139 (comment)), if there are other requested changes we can address them there.

edit; #9236

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 30, 2021
@mergify mergify bot merged commit dfe3e7a into master Apr 30, 2021
@mergify mergify bot deleted the am/adr-31-type-url branch April 30, 2021 11:00
@amaury1093 amaury1093 mentioned this pull request Apr 30, 2021
9 tasks
@robert-zaremba
Copy link
Collaborator

@robert-zaremba I'm putting automerge on to unblock your work on Authz. I'll also work on a follow-up PR cleaning stuff (#9139 (comment), #9139 (comment), #9139 (comment)), if there are other requested changes we can address them there.

edit; #9236

Thanks, it's good.

)

if svcMsg, ok := msg.(sdk.ServiceMsg); ok {
msgFqName = svcMsg.MethodName
Copy link
Contributor

Choose a reason for hiding this comment

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

The message.action is very important for relayers as it is used to distinguish when the next message event starts/ends and what type of event is occurring. Relayers may be connected to chains on the old version and the new version and thus need to be aware of this change to ensure IBC continues to work between chains with different versions

Does the svcMsg.MethodName != sdk.MsgTypeURL(msg)? Or is there no breaking change here for Msgs migrated to proto messages registered on a Msg service (which from my understanding is the case for the current IBC Msgs). I'm a little fuzzy on the exact differences between the legacy messages, the proto message req/resp types and the msg service router, so please excuse me if I used the terminology wrong

Copy link
Contributor Author

@amaury1093 amaury1093 May 7, 2021

Choose a reason for hiding this comment

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

It's just confusing, to me too. I tried to simplify it like this:

In v0.42, these message.actions were emitted:

  1. either message.action='/cosmos.bank.v1beta1.Msg/Send' if the user explicity sent a ServiceMsg tx (note the 2 /)
  2. or message.action='send' for all other messages (99% of the messages)

After this PR:

  • (1) is completely removed
  • (2) has been converted to message.action='/cosmos.bank.v1beta1.MsgSend' (note only 1 /), so legacy message.action='send' are not emitted anymore (unless the app developer didn't wire up proto registration correctly). This is not retroactive though.

Let me know if that works for IBC.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks for the response! This makes sense. This should be fine for relayers, but it will likely require a small change. Relayer should now check for message.action='send' || message.action='/cosmos.bank.v1beta1.MsgSend'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's the approach I'm taking too, e.g. if querying deposits here.

Comment on lines 717 to +723
// legacy sdk.Msg routing
msgRoute := msg.Route()
msgFqName = msg.Type()
// Assuming that the app developer has migrated all their Msgs to
// proto messages and has registered all `Msg services`, then this
// path should never be called, because all those Msgs should be
// registered within the `msgServiceRouter` already.
msgRoute := legacyMsg.Route()
eventMsgName = legacyMsg.Type()
Copy link
Contributor

Choose a reason for hiding this comment

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

Also a sanity check, ledger signed transactions would not end up here if there is a registered proto message? This would be because the difference between the transaction/message layer?

Copy link
Contributor

Choose a reason for hiding this comment

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

For the modules in the SDK is this ever reached? Docs indicate that the handler type will be deprecated, but it seems like now all the SDK modules could remove their legacy Handler if it is no longer used? Just trying to get a better grasp of what is occurring

Copy link
Contributor Author

@amaury1093 amaury1093 May 7, 2021

Choose a reason for hiding this comment

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

For the modules in the SDK is this ever reached?

For SDK modules, this is never reached. I just confirmed by adding a panic in this else if branch, all simapp/integration/CLI tests pass (only some baseapp unit tests panic, because these tests don't register proto msgs).

but it seems like now all the SDK modules could remove their legacy Handler if it is no longer used?

That's also correct, but we're thinking a better AppModule in general, so it might make sense to remove the legacy Handler with that in one go. ref: #9139 (comment)

@amaury1093 amaury1093 mentioned this pull request May 10, 2021
9 tasks
adizere added a commit to informalsystems/hermes that referenced this pull request Jul 15, 2021
romac pushed a commit to informalsystems/hermes that referenced this pull request Aug 19, 2021
* Adapted proto compiler. Regenerated Rust files

* Adapt to newer namespace for apps.

* Manual fix for Staking erroneous compilation

* Workaround: disabled chain upgrade

* Added specific GRPC-web port to prevent conflict on 9091.

* Upgrade protos

* Add support for new message action strings

* Improved output for one-chain script

* CLI for chain upgrade proposal adapted to gaia v5.

* Re-generated proto files with updated proto-compiler (fmt enabled).

* Manual fix for staking::Validators erroneous compilation; added the .applications.transfer generated file

* Quick feature for parametrizing upgraded chain

* Clippy fix. Meta for params for better display.

* Adapted query_upgraded_client_state

* Adapted query_upgraded_consensus_state to ibc-go-v1

* Moved from gRPC to ABCI

* Adapted send_tx_simulate to new def of SimulateRequest

* Added parametrizable upgrade plan name

* Fix unwraps. Retain unbonding period if none specified.

* Missing post-merge bracket

* Removed register and send match arms for rpc events

* Support for new message.actions strings of ICS27

Based on discussion from
cosmos/cosmos-sdk#9139 (comment)
This is not yet tested!

* Update protos sdk(v0.43.0-rc2) & ibc-go(v1.0.0-rc3)

* Revert "Added specific GRPC-web port to prevent conflict on 9091."

This reverts commit abf6dba.

* Add missing fields in upgrade Plan

* Fix clippy warnings

* Added another action field to match intertx MsgSend

* Update protos sdk(v0.43.0) & ibc-go(v1.0.0)

* Update cosmos SDK module version requirement in compatibility.rs

* Add legacy upgrade support (#1289)

* Add legacy upgrade support

* Fix check for legacy

* Update .changelog

* Add TODO

* Apply suggestion

Co-authored-by: Adi Seredinschi <adi@informal.systems>

Co-authored-by: Adi Seredinschi <adi@informal.systems>

* Apply suggestions

* Update .changelog

* Fix .changelog entry

Co-authored-by: Adi Seredinschi <adi@informal.systems>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>
hu55a1n1 added a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Adapted proto compiler. Regenerated Rust files

* Adapt to newer namespace for apps.

* Manual fix for Staking erroneous compilation

* Workaround: disabled chain upgrade

* Added specific GRPC-web port to prevent conflict on 9091.

* Upgrade protos

* Add support for new message action strings

* Improved output for one-chain script

* CLI for chain upgrade proposal adapted to gaia v5.

* Re-generated proto files with updated proto-compiler (fmt enabled).

* Manual fix for staking::Validators erroneous compilation; added the .applications.transfer generated file

* Quick feature for parametrizing upgraded chain

* Clippy fix. Meta for params for better display.

* Adapted query_upgraded_client_state

* Adapted query_upgraded_consensus_state to ibc-go-v1

* Moved from gRPC to ABCI

* Adapted send_tx_simulate to new def of SimulateRequest

* Added parametrizable upgrade plan name

* Fix unwraps. Retain unbonding period if none specified.

* Missing post-merge bracket

* Removed register and send match arms for rpc events

* Support for new message.actions strings of ICS27

Based on discussion from
cosmos/cosmos-sdk#9139 (comment)
This is not yet tested!

* Update protos sdk(v0.43.0-rc2) & ibc-go(v1.0.0-rc3)

* Revert "Added specific GRPC-web port to prevent conflict on 9091."

This reverts commit abf6dba.

* Add missing fields in upgrade Plan

* Fix clippy warnings

* Added another action field to match intertx MsgSend

* Update protos sdk(v0.43.0) & ibc-go(v1.0.0)

* Update cosmos SDK module version requirement in compatibility.rs

* Add legacy upgrade support (informalsystems#1289)

* Add legacy upgrade support

* Fix check for legacy

* Update .changelog

* Add TODO

* Apply suggestion

Co-authored-by: Adi Seredinschi <adi@informal.systems>

Co-authored-by: Adi Seredinschi <adi@informal.systems>

* Apply suggestions

* Update .changelog

* Fix .changelog entry

Co-authored-by: Adi Seredinschi <adi@informal.systems>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>
hu55a1n1 added a commit to cosmos/ibc-rs that referenced this pull request Sep 29, 2022
* Adapted proto compiler. Regenerated Rust files

* Adapt to newer namespace for apps.

* Manual fix for Staking erroneous compilation

* Workaround: disabled chain upgrade

* Added specific GRPC-web port to prevent conflict on 9091.

* Upgrade protos

* Add support for new message action strings

* Improved output for one-chain script

* CLI for chain upgrade proposal adapted to gaia v5.

* Re-generated proto files with updated proto-compiler (fmt enabled).

* Manual fix for staking::Validators erroneous compilation; added the .applications.transfer generated file

* Quick feature for parametrizing upgraded chain

* Clippy fix. Meta for params for better display.

* Adapted query_upgraded_client_state

* Adapted query_upgraded_consensus_state to ibc-go-v1

* Moved from gRPC to ABCI

* Adapted send_tx_simulate to new def of SimulateRequest

* Added parametrizable upgrade plan name

* Fix unwraps. Retain unbonding period if none specified.

* Missing post-merge bracket

* Removed register and send match arms for rpc events

* Support for new message.actions strings of ICS27

Based on discussion from
cosmos/cosmos-sdk#9139 (comment)
This is not yet tested!

* Update protos sdk(v0.43.0-rc2) & ibc-go(v1.0.0-rc3)

* Revert "Added specific GRPC-web port to prevent conflict on 9091."

This reverts commit abf6dba19f8eb5fcc370fcb33edc9bf5a1c4c21a.

* Add missing fields in upgrade Plan

* Fix clippy warnings

* Added another action field to match intertx MsgSend

* Update protos sdk(v0.43.0) & ibc-go(v1.0.0)

* Update cosmos SDK module version requirement in compatibility.rs

* Add legacy upgrade support (#1289)

* Add legacy upgrade support

* Fix check for legacy

* Update .changelog

* Add TODO

* Apply suggestion

Co-authored-by: Adi Seredinschi <adi@informal.systems>

Co-authored-by: Adi Seredinschi <adi@informal.systems>

* Apply suggestions

* Update .changelog

* Fix .changelog entry

Co-authored-by: Adi Seredinschi <adi@informal.systems>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>
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:CLI C:Rosetta Issues and PR related to Rosetta C:Simulations C:x/auth C:x/authz C:x/bank C:x/evidence C:x/feegrant C:x/genutil genutil module issues C:x/gov C:x/staking T: ADR An issue or PR relating to an architectural decision record T:Docs Changes and features related to documentation.
Projects
None yet
8 participants