-
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
Extend ClientConn on TxBuilder #7630
Changes from all commits
9c8486d
2c36489
097c13f
b0e89dc
55c4a88
7b44e68
7b1e26a
2306164
12cba1e
73af333
383b3e8
24cdb72
70c0ebf
626319a
3b75af0
9a68044
92679a4
4e2f3a5
dc1613b
e5c6106
b6209de
173b75d
7b9da51
8083644
ade84b5
7afe557
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
package legacytx | ||
|
||
import ( | ||
gocontext "context" | ||
"fmt" | ||
|
||
"google.golang.org/grpc" | ||
|
||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
) | ||
|
||
// 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. | ||
amaury1093 marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+13
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Should we:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Talking offline with the Regen team, I'll close this PR.
|
||
func (s *StdTxBuilder) Invoke(_ gocontext.Context, method string, args, reply interface{}, _ ...grpc.CallOption) error { | ||
req, ok := args.(sdk.MsgRequest) | ||
if !ok { | ||
return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "%T should implement %T", args, (*sdk.MsgRequest)(nil)) | ||
} | ||
|
||
err := req.ValidateBasic() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
msgs := append(s.Msgs, sdk.ServiceMsg{ | ||
MethodName: method, | ||
Request: req, | ||
}) | ||
s.SetMsgs(msgs...) | ||
|
||
return nil | ||
} | ||
|
||
// NewStream implements the grpc ClientConn.NewStream method. | ||
func (s *StdTxBuilder) NewStream(gocontext.Context, *grpc.StreamDesc, string, ...grpc.CallOption) (grpc.ClientStream, error) { | ||
return nil, fmt.Errorf("not supported") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package tx | ||
|
||
import ( | ||
gocontext "context" | ||
"fmt" | ||
|
||
"google.golang.org/grpc" | ||
|
||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
) | ||
|
||
// Invoke implements the grpc ClientConn.Invoke method. This is so that we can | ||
// use ADR-031 service `Msg`s with wrapper. Invoking this method will | ||
// **append** the service Msg into the TxBuilder's Msgs array. | ||
func (w *wrapper) Invoke(_ gocontext.Context, method string, args, reply interface{}, _ ...grpc.CallOption) error { | ||
req, ok := args.(sdk.MsgRequest) | ||
amaury1093 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if !ok { | ||
return sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "%T should implement %T", args, (*sdk.MsgRequest)(nil)) | ||
} | ||
|
||
err := req.ValidateBasic() | ||
if err != nil { | ||
return err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thumbed up, but maybe it's not necessary. |
||
} | ||
|
||
msgs := append(w.GetMsgs(), sdk.ServiceMsg{ | ||
MethodName: method, | ||
Request: req, | ||
}) | ||
w.SetMsgs(msgs...) | ||
|
||
return nil | ||
} | ||
|
||
// NewStream implements the grpc ClientConn.NewStream method. | ||
func (w *wrapper) NewStream(gocontext.Context, *grpc.StreamDesc, string, ...grpc.CallOption) (grpc.ClientStream, error) { | ||
return nil, fmt.Errorf("not supported") | ||
} |
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.
I don't think an update to ADR 020 is really needed, wdyt?
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.
Probably not. But just wondering is it better to change the interface, or create a separate struct that implements ClientConn wraps TxBuilder?
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.
I like the idea of creating ClientConn. I was confused with the
Invoke
method initially. Maybe we can call the new interface:MsgSrvClient
?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.
Ping?
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.
I'm not sure here. Are you proposing:
? I feel yet another struct creates more confusion. At some point we should revisit #7630 (comment), which imo is the ideal way forward.