Extend ClientConn on TxBuilder #7630

amaury1093 · 2020-10-22T14:26:42Z

Description

This PR introduces a clunky UX to manage service Msgs with TxBuilder, see #7630 (comment). It should be ultimately replaces by a custom code generator #8270.

Update x/bank/cli/cli_test.go

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

codecov · 2020-10-22T14:41:28Z

Codecov Report

Merging #7630 (7afe557) into master (6fb5e1f) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #7630      +/-   ##
==========================================
- Coverage   61.82%   61.77%   -0.05%     
==========================================
  Files         609      611       +2     
  Lines       35110    35136      +26     
==========================================
  Hits        21707    21707              
- Misses      11121    11147      +26     
  Partials     2282     2282

Impacted Files	Coverage Δ
x/auth/legacy/legacytx/msg_service.go	`0.00% <0.00%> (ø)`
x/auth/tx/builder.go	`74.80% <ø> (ø)`
x/auth/tx/msg_service.go	`0.00% <0.00%> (ø)`

…541-txbuilder

amaury1093 · 2020-10-23T12:22:38Z

client/tx_config.go

@@ -34,6 +36,7 @@ type (
 	// signatures, and provide canonical bytes to sign over. The transaction must
 	// also know how to encode itself.
 	TxBuilder interface {
+		gogogrpc.ClientConn


I don't think an update to ADR 020 is really needed, wdyt?

Probably not. But just wondering is it better to change the interface, or create a separate struct that implements ClientConn wraps TxBuilder?

I like the idea of creating ClientConn. I was confused with the Invoke method initially. Maybe we can call the new interface: MsgSrvClient?

I'm not sure here. Are you proposing:

type MsgSrvClient struct { txBuilder TxBuilder } var _ gogogrpc.ClientConn = MsgSrvClient{}

? I feel yet another struct creates more confusion. At some point we should revisit #7630 (comment), which imo is the ideal way forward.

codec/types/any.go

Co-authored-by: Alessio Treglia <[email protected]>

alessio

ACK

aaronc

The unfortunate part of this is that it only allows setting one Msg. We should add another function AppendMsg to TxBuilder and use that instead so that multiple Msgs can be added.

…k into am-7541-txbuilder

amaury1093 · 2020-10-23T14:31:42Z

Ah, you're right. I added AppendMsgs. Then I realized that method makes SetMsgs kind of useless, so I removed SetMsgs in my last commit.

codec/types/any.go

robert-zaremba · 2020-10-23T21:25:34Z

codec/types/any.go

+// Ex:
+// This will allow us to pack service methods in Any's using the full method name
+// as the type URL and the request body as the value.
+func NewAnyWithTypeURL(typeURL string, value proto.Message) (*Any, error) {


This function looks dangerous.

Is the protobuf generated code already providing it? Maybe there is a method already for this?

Is the protobuf generated code already providing it? Maybe there is a method already for this?

I don't think so, because we have our own implementation of Any.

codec/types/any.go

x/auth/tx/msg_service.go

x/auth/legacy/legacytx/msg_service.go

…541-txbuilder

x/auth/tx/msg_service.go

x/auth/legacy/legacytx/msg_service.go

client/tx_config.go

aaronc · 2020-10-26T13:20:53Z

client/tx_config.go

@@ -34,6 +36,7 @@ type (
 	// signatures, and provide canonical bytes to sign over. The transaction must
 	// also know how to encode itself.
 	TxBuilder interface {
+		gogogrpc.ClientConn


Probably not. But just wondering is it better to change the interface, or create a separate struct that implements ClientConn wraps TxBuilder?

aaronc · 2020-10-26T13:22:08Z

@amaurymartiny I would say let's hold off on merging this until we're done with 0.40 backport issues. What do you think?

amaury1093 · 2020-10-26T13:28:18Z

@amaurymartiny I would say let's hold off on merging this until we're done with 0.40 backport issues. What do you think?

Ah ok. I kind of wanted to get this in to have the tests i write for #7647 using this already, not strictly necessary but slightly cleaner imo to avoid having this in other tests.

aaronc · 2020-10-26T13:41:51Z

@amaurymartiny I would say let's hold off on merging this until we're done with 0.40 backport issues. What do you think?

Ah ok. I kind of wanted to get this in to have the tests i write for #7647 using this already, not strictly necessary but slightly cleaner imo to avoid having this in other tests.

Ah okay, well let's not remove SetMsgs then

amaury1093 · 2020-10-26T15:01:44Z

Actually no, I may have found a way not to DRY in #7647. Would also like to revisit:

Probably not. But just wondering is it better to change the interface, or create a separate struct that implements ClientConn wraps TxBuilder?

as it still feels a bit weird to write:

bankMsgClient := types.NewMsgClient(txBuilder)
_, err = bankMsgClient.Send(context.Background(), msg) // seems like we're actually sending. `context.Background()` is also useless.

we can probably build a wrapper somewhere to hide the awkwardness.

Putting to draft for now.

aaronc · 2020-10-29T02:17:06Z

as it still feels a bit weird to write:

bankMsgClient := types.NewMsgClient(txBuilder)
_, err = bankMsgClient.Send(context.Background(), msg) // seems like we're actually sending. `context.Background()` is also useless.

I think we're going to want to do custom code generation for this eventually. The grpc code generator isn't that complex and protobuf recommends people write their own code generation plugins anyway. Ideally we just something that takes into account that the call is async and maybe even that we're using client.Context instead of context.Context (or don't even need a context). Anyway, I think this is fine for now but just thoughts for the future.

…541-txbuilder

alessio · 2020-12-11T11:26:47Z

x/auth/tx/msg_service.go

@@ -19,6 +19,11 @@ func (w *wrapper) Invoke(_ gocontext.Context, method string, args, reply interfa
 		return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "%T should implement %T", args, (*sdk.MsgRequest)(nil))
 	}

+	err := req.ValidateBasic()
+	if err != nil {
+		return err


shouldn't we use sdkerrors.Wrapf here?

I thumbed up, but maybe it's not necessary. err should already be a SDK error. I used sdkerrors.Wrapf 3 lines above because there's no underlying error.

…541-txbuilder

amaury1093 · 2021-01-11T14:44:57Z

x/auth/legacy/legacytx/msg_service.go

+// Invoke implements the grpc ClientConn.Invoke method. This is so that we can
+// use ADR-031 service `Msg`s with StdTxBuilder. Invoking this method will
+// **append** the service Msg into the TxBuilder's Msgs array.
+// TODO Full amino support still needs to be added as part of https://github.com/cosmos/cosmos-sdk/issues/7541.


This PR was originally set up in #7541. It introduces a clunky UX to manage service Msgs with TxBuilder, see #7630 (comment). It should be ultimately replaced by a custom code generator #8270.

However, as I understand, #8270 is targetted for 0.42, whereas this PR might be useful for 0.41's x/fee_grant and x/authz (see #8061 (comment), #7629 (comment), both use a temporary ServiceMsgClientConn struct for now).

Should we:

Merge this, and have a slightly clunky txBuilder for now?

Close this, use temporary ServiceMsgClientConn wherever we need service Msgs, and wait for Custom protobuf service code generator #8270?

Talking offline with the Regen team, I'll close this PR.

TxBuilder won't be able to handle service Msgs (until Custom protobuf service code generator #8270)

If we want to use service msgs, we use the temporary ServiceMsgClientConn.

amaury1093 added 2 commits October 22, 2020 16:24

Add invoke & NewStream

9c8486d

Remove validate basic

2c36489

amaury1093 mentioned this pull request Oct 22, 2020

Add ClientConn to TxBuilder to support ADR 031 #7541

Closed

6 tasks

Update test

097c13f

amaury1093 added 6 commits October 22, 2020 18:30

WIP

b0e89dc

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

55c4a88

…541-txbuilder

NewAnyWithWithTypeURL

7b44e68

Add todo comment for amino

7b1e26a

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

2306164

…541-txbuilder

Add Changelog

12cba1e

amaury1093 commented Oct 23, 2020

View reviewed changes

Remove println

73af333

amaury1093 marked this pull request as ready for review October 23, 2020 12:36

amaury1093 requested review from aaronc, alessio and alexanderbez as code owners October 23, 2020 12:36

amaury1093 requested review from blushi, anilcse and robert-zaremba October 23, 2020 12:37

alessio reviewed Oct 23, 2020

View reviewed changes

codec/types/any.go Outdated Show resolved Hide resolved

Update codec/types/any.go

383b3e8

Co-authored-by: Alessio Treglia <[email protected]>

alessio approved these changes Oct 23, 2020

View reviewed changes

aaronc requested changes Oct 23, 2020

View reviewed changes

amaury1093 added 3 commits October 23, 2020 15:58

Add AppendMsgs

24cdb72

Rename SetMsgs to AppendMsgs

70c0ebf

Merge branch 'am-7541-txbuilder' of ssh://github.com/cosmos/cosmos-sd…

626319a

…k into am-7541-txbuilder

Merge branch 'master' into am-7541-txbuilder

3b75af0

robert-zaremba requested changes Oct 23, 2020

View reviewed changes

amaury1093 added 2 commits October 26, 2020 10:33

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

9a68044

…541-txbuilder

Address review comments

92679a4

amaury1093 requested review from aaronc and robert-zaremba October 26, 2020 09:39

robert-zaremba requested changes Oct 26, 2020

View reviewed changes

x/auth/tx/msg_service.go Show resolved Hide resolved

x/auth/legacy/legacytx/msg_service.go Show resolved Hide resolved

amaury1093 added 2 commits October 26, 2020 14:12

Add comment

4e2f3a5

Typo

dc1613b

aaronc reviewed Oct 26, 2020

View reviewed changes

amaury1093 marked this pull request as draft October 26, 2020 15:04

Merge branch 'master' into am-7541-txbuilder

e5c6106

amaury1093 added 5 commits December 11, 2020 11:55

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

b6209de

…541-txbuilder

Use SetMsgs

173b75d

remove AppendMsg

7b9da51

Add comments and CHANHGELOG

8083644

Add validate basic

ade84b5

alessio reviewed Dec 11, 2020

View reviewed changes

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

7afe557

…541-txbuilder

amaury1093 marked this pull request as ready for review December 15, 2020 17:15

amaury1093 mentioned this pull request Jan 11, 2021

Add fee grant module #8061

Merged

9 tasks

amaury1093 commented Jan 11, 2021

View reviewed changes

amaury1093 closed this Jan 11, 2021

amaury1093 deleted the am-7541-txbuilder branch January 11, 2021 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend ClientConn on TxBuilder #7630

Extend ClientConn on TxBuilder #7630

amaury1093 commented Oct 22, 2020 •

edited

Loading

codecov bot commented Oct 22, 2020 •

edited

Loading

amaury1093 Oct 23, 2020

aaronc Oct 26, 2020

robert-zaremba Oct 26, 2020

robert-zaremba Dec 11, 2020

amaury1093 Dec 15, 2020 •

edited

Loading

alessio left a comment

aaronc left a comment

amaury1093 commented Oct 23, 2020

robert-zaremba Oct 23, 2020

amaury1093 Oct 26, 2020 •

edited

Loading

aaronc Oct 26, 2020

aaronc commented Oct 26, 2020

amaury1093 commented Oct 26, 2020 •

edited

Loading

aaronc commented Oct 26, 2020

amaury1093 commented Oct 26, 2020 •

edited

Loading

aaronc commented Oct 29, 2020 •

edited

Loading

alessio Dec 11, 2020

amaury1093 Dec 15, 2020 •

edited

Loading

amaury1093 Jan 11, 2021 •

edited

Loading

amaury1093 Jan 11, 2021

Extend ClientConn on TxBuilder #7630

Extend ClientConn on TxBuilder #7630

Conversation

amaury1093 commented Oct 22, 2020 • edited Loading

Description

codecov bot commented Oct 22, 2020 • edited Loading

Codecov Report

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 Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

aaronc left a comment

Choose a reason for hiding this comment

amaury1093 commented Oct 23, 2020

Choose a reason for hiding this comment

amaury1093 Oct 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Oct 26, 2020

amaury1093 commented Oct 26, 2020 • edited Loading

aaronc commented Oct 26, 2020

amaury1093 commented Oct 26, 2020 • edited Loading

aaronc commented Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

amaury1093 Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

amaury1093 Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 commented Oct 22, 2020 •

edited

Loading

codecov bot commented Oct 22, 2020 •

edited

Loading

amaury1093 Dec 15, 2020 •

edited

Loading

amaury1093 Oct 26, 2020 •

edited

Loading

amaury1093 commented Oct 26, 2020 •

edited

Loading

amaury1093 commented Oct 26, 2020 •

edited

Loading

aaronc commented Oct 29, 2020 •

edited

Loading

amaury1093 Dec 15, 2020 •

edited

Loading

amaury1093 Jan 11, 2021 •

edited

Loading