-
Notifications
You must be signed in to change notification settings - Fork 411
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
Wasmvm upgrade 557 #559
Wasmvm upgrade 557 #559
Conversation
Codecov Report
@@ Coverage Diff @@
## master #559 +/- ##
==========================================
- Coverage 59.61% 59.55% -0.06%
==========================================
Files 45 45
Lines 5220 5242 +22
==========================================
+ Hits 3112 3122 +10
- Misses 1884 1891 +7
- Partials 224 229 +5
|
// FIXME: do we want to error here on invalid events | ||
// and reserve all _* keys for our use (not contract) |
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.
We do!
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.
Same for the min length check in newCustomEvents
above. Why silently ignore events with invalid type length?
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.
This was Alex's choice.
I left a message on the v0.15.0 upgrade PR to that effect. (See #548 (comment) )
I would make that change in a separate PR as it is unrelated to v0.16.0. But good not to miss it.
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.
Could you create a ticket for those two places? Then we can merge here.
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 added #560 for this. Will take on it in this next week
@@ -60,8 +76,20 @@ func (k Keeper) OnConnectChannel( | |||
env := types.NewEnv(ctx, contractAddr) | |||
querier := k.newQueryHandler(ctx, contractAddr) | |||
|
|||
msg := wasmvmtypes.IBCChannelConnectMsg{} | |||
if counterpartyVersion == "" { |
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 really know this codebase, but would it be possible to rely on something more meaningful for this?
if msgConfirm {
// ..
}
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 question, captured in #561
LGTM outside of my one comment! |
Closes #557
Currently tested versions:
Work done:
IbcChannel*Msg
typesIbcChannel
but in some of those messagescontract_address
event attribute (forwasm
type) to_contract_address
._*
attributes and thus conflict with any special "trusted" attributes from the runtime.IbcBasicResponse
andIbcReceiveResponse
(not just attributes)