Use service Msgs in CLI tx commands #8512

amaury1093 · 2021-02-04T11:45:04Z

Description

closes: #8306
closes #8346

Some stuff came up when converting to Service Msgs:

We have some utilities to search votes/proposals/deposits by searching Txs in x/gov/client/utils/query.go. These utils used to search only for txs with legacy Msgs. Now I added support for searching for legacy Msgs and service Msgs. See Use service Msgs in CLI tx commands #8512 (comment)
When using CopyTx to convert proto->amino->proto, the 1st and last proto TxBuilders are not equal, because we lose the ServiceMsg information. See Use service Msgs in CLI tx commands #8512 (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.

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

amaury1093 · 2021-02-09T13:12:35Z

x/gov/client/utils/query.go

+// searchEvents queries txs by events with both `oldMsgType` and `newMsgtype`,
+// merges the results into one *sdk.SearchTxsResult.
+func searchEvents(clientCtx client.Context, oldMsgType, newMsgType string, otherEvents ...string) (*sdk.SearchTxsResult, error) {
+	// Tx are indexed in tendermint via their Msgs `Type()`, which can be:
+	// - via legacy Msgs (amino or proto), their `Type()` is a custom string,
+	// - via ADR-031 service msgs, their `Type()` is the protobuf FQ method name.
+	// In searching for events, we search for both `Type()`s.


This is a bugfix that got uncovered when converting CLI to service Msgs.

Before:

We only had amino Msgs, their Type() was e.g. "submit_proposal". So when searching txs, we were searching message.action='submit_proposal'

Now (since 0.40):

We have amino Msgs, proto Msgs (Type() is same as amino), and service Msgs (Type() is proto FQ method name). So for searching txs we need to search for both old and new Type()s. I'm doing 2 separate searches:

message.action='submit_proposal' // then message.action='/cosmos.gov.v1beta1.Msg/SubmitProposal'

and merging the tx responses.

codecov · 2021-02-11T14:40:13Z

Codecov Report

Merging #8512 (3b1ab58) into master (53637b6) will increase coverage by 0.00%.
The diff coverage is 45.41%.

@@           Coverage Diff            @@
##           master    #8512    +/-   ##
========================================
  Coverage   61.42%   61.43%            
========================================
  Files         658      658            
  Lines       37627    37768   +141     
========================================
+ Hits        23111    23201    +90     
- Misses      12104    12143    +39     
- Partials     2412     2424    +12

Impacted Files	Coverage Δ
types/service_msg.go	`0.00% <0.00%> (ø)`
x/auth/legacy/legacytx/stdsign.go	`82.97% <ø> (ø)`
x/gov/types/msgs.go	`47.11% <ø> (ø)`
x/params/client/cli/tx.go	`0.00% <0.00%> (ø)`
x/staking/client/rest/query.go	`40.66% <0.00%> (-1.21%)`	⬇️
x/staking/types/msg.go	`23.92% <ø> (ø)`
x/gov/client/utils/query.go	`25.72% <25.40%> (-2.15%)`	⬇️
x/staking/client/cli/tx.go	`60.64% <77.77%> (+1.43%)`	⬆️
x/auth/vesting/client/cli/tx.go	`57.14% <80.00%> (+3.29%)`	⬆️
x/bank/client/cli/tx.go	`50.00% <80.00%> (+5.17%)`	⬆️
... and 8 more

…dk into am-8306-svcmsg-cli

atheeshp

LGTM, Can you resolve conflicts

x/auth/legacy/legacytx/stdsign.go

Co-authored-by: atheeshp <59333759+atheeshp@users.noreply.github.com>

…306-svcmsg-cli

client/tx/legacy_test.go

clevinson

Nice one! Generally looks good. I have a few clarifying Q's / smaller nits.

I've gone through everything except the details of the event querying logic (e.g. x/gov/client/utils/query.go). Will finish looking at that and wrap up my review tomorrow.

client/tx/legacy_test.go

clevinson · 2021-02-18T03:54:48Z

x/bank/client/cli/cli_test.go

-func (s *IntegrationTestSuite) TestBankMsgService() {
+// TestLegacyProtoMsgSend uses the legacy proto MsgSend CLI util to make sure
+// legacy proto Msgs are still handled. The real CLI commands use service Msgs.
+func (s *IntegrationTestSuite) TestLegacyProtoMsgSend() {


What's the reason for keeping these tests? What pathways for legacy proto msg's still exist?

What pathways for legacy proto msg's still exist?

We can send proto msgs via GRPC, REST and the TM broadcast endpoint. But all of these are already tested in other parts of the codebase, so I guess this fictive "send proto msgs via CLI" can be removed. I also refactored a bit the code in 015aef2.

clevinson · 2021-02-18T03:59:48Z

x/crisis/client/cli/tx.go

@@ -49,11 +50,14 @@ func NewMsgVerifyInvariantTxCmd() *cobra.Command {
 			senderAddr := clientCtx.GetFromAddress()

 			msg := types.NewMsgVerifyInvariant(senderAddr, moduleName, route)
-			if err := msg.ValidateBasic(); err != nil {


General question for all of these - does ValidateBasic() get called under the hood still somewhere in the new service msg path? Where does that happen? (was having trouble finding)

Yes, when using service msg clients, the ServiceMsgClientConn.Invoke method is called, and it does a ValidateBasic.

x/staking/types/msg.go

clevinson · 2021-02-18T04:11:09Z

x/gov/types/msgs.go

@@ -18,6 +18,12 @@ const (
 	TypeMsgVote           = "vote"
 	TypeMsgVoteWeighted   = "weighted_vote"
 	TypeMsgSubmitProposal = "submit_proposal"
+
+	// These are used for querying events by action.
+	TypeSvcMsgDeposit        = "/cosmos.gov.v1beta1.Msg/Deposit"


Same comment as bellow - are these FQ paths not defined elsehwere already? It feels like this is will be a very easy thing to forget to update when we migrate out of beta, change packages, etc. Yes this shouldn't happen often... but when it does, I worry these const strings may get left behind..

Unfortunately not... In Aaron's custom code generator they are, and i think that's useful.

aaronc · 2021-02-18T16:41:38Z

x/auth/legacy/legacytx/stdsign.go

+		// we just call GetSignBytes on it.
+		svcMsg, ok := msg.(sdk.ServiceMsg)
+		if ok {
+			msgBytes = svcMsg.Request.(sdk.Msg).GetSignBytes()


This cast can fail, we should check

Also we should just implement this on ServiceMsg.GetSignBytes rather than panicking there, right?

yeah, I guess that would work too, and we wouldn't need any additional code here

done in bad79a5

amaury1093 added 2 commits February 4, 2021 12:36

Use service Msgs in CLI tx commands

18a7749

Update comment

348b5b4

amaury1093 added the C:CLI label Feb 4, 2021

amaury1093 added 13 commits February 4, 2021 13:13

Gracefully support amino signing

66f8718

CLI to use svc msg

aa0b6bc

Fix lint

654b1a2

Use fq method name in events

ffa24d3

Update tests

d007cab

Fix quering events

c94d2f1

Add docs

b1a40fa

Fix test build

d8235ad

Merge branch 'master' into am-8306-svcmsg-cli

5c2e79d

Fix events

436847c

Fix search for events

95f734f

Handle old andd new event quering

806f43b

Use merge events

74c348b

amaury1093 commented Feb 9, 2021

View reviewed changes

amaury1093 added 3 commits February 9, 2021 15:05

Better encCfg

9a67986

Add page in search

7083b24

Fix tests

d4b7259

Merge branch 'master' into am-8306-svcmsg-cli

d319ab6

amaury1093 marked this pull request as ready for review February 11, 2021 14:40

amaury1093 requested review from aaronc, alessio and alexanderbez as code owners February 11, 2021 14:40

amaury1093 requested review from aleem1314 and atheeshp February 11, 2021 14:42

amaury1093 added 2 commits February 11, 2021 15:45

Update test and comments

966f3c5

Merge branch 'am-8306-svcmsg-cli' of ssh://github.com/cosmos/cosmos-s…

9b6039e

…dk into am-8306-svcmsg-cli

Merge branch 'master' into am-8306-svcmsg-cli

c761970

atheeshp approved these changes Feb 15, 2021

View reviewed changes

x/auth/legacy/legacytx/stdsign.go Outdated Show resolved Hide resolved

amaury1093 and others added 7 commits February 15, 2021 11:05

Update x/auth/legacy/legacytx/stdsign.go

d64ca54

Co-authored-by: atheeshp <59333759+atheeshp@users.noreply.github.com>

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-8…

232d6fd

…306-svcmsg-cli

Fix conflict for weighted vote

e7879aa

Merge branch 'master' of ssh://github.com/cosmos/cosmos-sdk into am-8…

9886c2a

…306-svcmsg-cli

Make CopyTx actually run

64fdf10

Fix CopyTx

e4e4aa8

Fix test

e63aaac

amaury1093 commented Feb 16, 2021

View reviewed changes

client/tx/legacy_test.go Outdated Show resolved Hide resolved

amaury1093 added 2 commits February 17, 2021 14:42

Add changelog entry

8dfebc5

Remove useless code

dc37529

clevinson self-assigned this Feb 17, 2021

clevinson reviewed Feb 18, 2021

View reviewed changes

amaury1093 added 4 commits February 18, 2021 11:19

Add msgs tests

b90c7e3

Remove testing proto Msg via CLI

015aef2

Fix lint

35dacb1

Fix test

d300f79

aaronc reviewed Feb 18, 2021

View reviewed changes

amaury1093 added 2 commits February 18, 2021 18:10

Implement GetSignBytes on ServiceMsg

bad79a5

Fix %T

3b1ab58

aaronc approved these changes Feb 18, 2021

View reviewed changes

alessio approved these changes Feb 18, 2021

View reviewed changes

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Feb 18, 2021

Merge branch 'master' into am-8306-svcmsg-cli

48a5afa

mergify bot merged commit 73e38e4 into master Feb 18, 2021

mergify bot deleted the am-8306-svcmsg-cli branch February 18, 2021 18:00

amaury1093 mentioned this pull request Mar 3, 2021

Fix typo in querying events #8773

Merged

9 tasks

amaury1093 mentioned this pull request Apr 30, 2021

Cleanup remains of ServiceMsg #9236

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use service Msgs in CLI tx commands #8512

Use service Msgs in CLI tx commands #8512

amaury1093 commented Feb 4, 2021 •

edited

Loading

amaury1093 Feb 9, 2021 •

edited

Loading

codecov bot commented Feb 11, 2021 •

edited

Loading

atheeshp left a comment •

edited

Loading

clevinson left a comment •

edited

Loading

clevinson Feb 18, 2021

amaury1093 Feb 18, 2021

clevinson Feb 18, 2021

amaury1093 Feb 18, 2021

clevinson Feb 18, 2021

amaury1093 Feb 18, 2021

aaronc Feb 18, 2021

aaronc Feb 18, 2021

amaury1093 Feb 18, 2021

amaury1093 Feb 18, 2021

Use service Msgs in CLI tx commands #8512

Use service Msgs in CLI tx commands #8512

Conversation

amaury1093 commented Feb 4, 2021 • edited Loading

Description

amaury1093 Feb 9, 2021 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Feb 11, 2021 • edited Loading

Codecov Report

atheeshp left a comment • edited Loading

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

amaury1093 commented Feb 4, 2021 •

edited

Loading

amaury1093 Feb 9, 2021 •

edited

Loading

codecov bot commented Feb 11, 2021 •

edited

Loading

atheeshp left a comment •

edited

Loading

clevinson left a comment •

edited

Loading