-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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 19 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 |
---|---|---|
|
@@ -68,11 +68,25 @@ func NewAnyWithValue(value proto.Message) (*Any, error) { | |
return any, nil | ||
} | ||
|
||
// Pack packs the value x in the Any or returns an error. This also caches | ||
// the packed value so that it can be retrieved from GetCachedValue without | ||
// unmarshaling | ||
func (any *Any) Pack(x proto.Message) error { | ||
any.TypeUrl = "/" + proto.MessageName(x) | ||
// NewAnyWithTypeURL constructs a new Any packed with the value provided | ||
// and a custom TypeURL. It returns an error if that value couldn't be packed. | ||
// This also cachesthe packed value so that it can be retrieved from | ||
// GetCachedValue without unmarshaling. | ||
// | ||
// 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) { | ||
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 function looks dangerous. Is the protobuf generated code already providing it? Maybe there is a method already for this? 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 don't think so, because we have our own implementation of Any. |
||
any := &Any{} | ||
|
||
return any, any.packWithCustomTypeURL(typeURL, value) | ||
} | ||
amaury1093 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// PackWithCustomTypeURL packs the value x in the Any or returns an error, | ||
// allowing for a custom TypeUrl. This also caches the packed value so that it | ||
// can be retrieved fromGetCachedValue without unmarshaling. | ||
func (any *Any) packWithCustomTypeURL(typeURL string, x proto.Message) error { | ||
any.TypeUrl = typeURL | ||
bz, err := proto.Marshal(x) | ||
if err != nil { | ||
return err | ||
|
@@ -84,6 +98,13 @@ func (any *Any) Pack(x proto.Message) error { | |
return nil | ||
} | ||
|
||
// Pack packs the value x in the Any or returns an error. This also caches | ||
// the packed value so that it can be retrieved from GetCachedValue without | ||
// unmarshaling | ||
func (any *Any) Pack(x proto.Message) error { | ||
return any.packWithCustomTypeURL("/"+proto.MessageName(x), x) | ||
} | ||
|
||
// UnsafePackAny packs the value x in the Any and instead of returning the error | ||
// in the case of a packing failure, keeps the cached value. This should only | ||
// be used in situations where compatibility is needed with amino. Amino-only | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
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. | ||
// 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
|
||
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)) | ||
} | ||
|
||
s.AppendMsgs(sdk.ServiceMsg{ | ||
MethodName: method, | ||
Request: req, | ||
}) | ||
|
||
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") | ||
} |
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.