-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Cleanup remains of ServiceMsg #9236
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9236 +/- ##
==========================================
+ Coverage 60.20% 60.28% +0.08%
==========================================
Files 591 591
Lines 37017 36937 -80
==========================================
- Hits 22285 22269 -16
+ Misses 12758 12698 -60
+ Partials 1974 1970 -4
|
// response_fullname is the protobuf fullname of the given sdk.ServiceMsg response | ||
string response_fullname = 4; | ||
// msg_type_url contains the TypeURL of a sdk.Msg. | ||
string msg_type_url = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fdymylja Does this simplification make sense to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AmauryM perfect!
R4R! |
Can you fix the conflicts @AmauryM ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - there is still some mention of ServiceMsg
in some of the documentation, it's only a few places, maybe it can be changed in this PR?
I found mention of it in:
- x/authz/exported - authorization.go (comment on Accept method)
- x/authz/spec - 01_concepts
- x/authz/spec - 02_state
there's a separate issue for this #9208 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, could you fix the conflicts?
The `ValidateBasic` function has been called in the `GenerateOrBroadcastTxWithFactory` function, so we no longer need to add the `ValidateBasic` function when writing Cli. Related PRs can be viewed at: cosmos/cosmos-sdk#9236 (comment)
The `ValidateBasic` function has been called in the `GenerateOrBroadcastTxWithFactory` function, so we no longer need to add the `ValidateBasic` function when writing Cli. Related PRs can be viewed at: cosmos/cosmos-sdk#9236 (comment)
Description
This PR is a follow-up from #9139, as not to overload that PR, with lower-priority cleanups related to the removal of ServiceMsg TypeURLs.
ref: #9139 (comment)
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.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes