-
Notifications
You must be signed in to change notification settings - Fork 402
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
Stargate msg and query #435
Conversation
Codecov Report
@@ Coverage Diff @@
## master #435 +/- ##
==========================================
- Coverage 55.42% 55.36% -0.07%
==========================================
Files 39 39
Lines 4043 4073 +30
==========================================
+ Hits 2241 2255 +14
- Misses 1619 1627 +8
- Partials 183 191 +8
|
@@ -20,8 +22,8 @@ type DefaultMessageHandler struct { | |||
encoders MessageEncoders | |||
} | |||
|
|||
func NewDefaultMessageHandler(router sdk.Router, channelKeeper types.ChannelKeeper, capabilityKeeper types.CapabilityKeeper, customEncoders *MessageEncoders) DefaultMessageHandler { | |||
encoders := DefaultEncoders(channelKeeper, capabilityKeeper).Merge(customEncoders) | |||
func NewDefaultMessageHandler(router sdk.Router, channelKeeper types.ChannelKeeper, capabilityKeeper types.CapabilityKeeper, unpacker codectypes.AnyUnpacker, customEncoders *MessageEncoders) DefaultMessageHandler { |
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.
good use of the minimal interface
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.
@alpe this should be ready now. I'd like a review, especially on the proto part and how I wire it with the app
@@ -276,6 +288,27 @@ func TestEncoding(t *testing.T) { | |||
}, | |||
}, | |||
}, | |||
// TODO: alpe? can you add an example with sub-interfaces (where the UnpackInterfaces call would be needed) |
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 am not sure how to create such a case. (I also think we only have bank, staking, and distribution registered here in the tests).
If you could add such a test to trigger this that would be great. Or if it is a pain, please open up an issue on it and explain more or less what is needed. I can put that down lower on the priority list
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 found Any
types in sdk (master).
- MsgGrantAuthorizationRequest
- MsgGrantFeeAllowance
- MsgSubmitProposal
- MsgCreateValidator
I guessMsgSubmitProposal
would be a good candidate. I can add a test.
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.
Great work. This will add a very valuable feature to the system. 🚀
@@ -276,6 +288,27 @@ func TestEncoding(t *testing.T) { | |||
}, | |||
}, | |||
}, | |||
// TODO: alpe? can you add an example with sub-interfaces (where the UnpackInterfaces call would be needed) |
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 found Any
types in sdk (master).
- MsgGrantAuthorizationRequest
- MsgGrantFeeAllowance
- MsgSubmitProposal
- MsgCreateValidator
I guessMsgSubmitProposal
would be a good candidate. I can add a test.
protoQuery := banktypes.QueryAllBalancesRequest{ | ||
Address: creator.String(), | ||
} | ||
protoQueryBin, err := proto.Marshal(&protoQuery) |
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.
Clever to test with a different golang protobuf marshaller. 🌻
var simpleBalance wasmvmtypes.AllBalancesResponse | ||
mustParse(t, simpleChain.Data, &simpleBalance) | ||
require.Equal(t, len(expectedBalance), len(simpleBalance.Amount)) | ||
assert.Equal(t, simpleBalance.Amount[0].Amount, expectedBalance[0].Amount.String()) |
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.
nit: no need to change: wrong order of arguments. Here and others (t, expect, got) (to be fair it is not consistent within testify
like assert.Len(t, got, exp)` )
Thanks. If you could add a follow up pr sometime with submit proposal, that would be great Now contracts can vote |
Closes #388