-
Notifications
You must be signed in to change notification settings - Fork 336
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
Add WasmMsg::Migrate variant #768
Conversation
new_code_id: u64, | ||
/// msg is the json-encoded MigrateMsg struct that will be passed to the new code | ||
msg: Binary, | ||
}, |
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 is by the way a breaking change as the enum is not #[non_exhaustive]
. If we ever want to add new cases in 1.x, we should make it #[non_exhaustive]
.
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.
You mean WasmMsg
? (Which I think is pretty final, we are just finally supporting a feature we added in 0.9).
Or CosmosMsg
in general, which definitely will expand sometime. I guess I could just add this to all the types 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.
This WasmMsg
just brought up the topic. I mean CosmosMsg and all its sub-messages. It is just too easy to forget one type, or require a v2 of an existing type later on. If we need to make a CosmWasm 2.0, just because we did not write #[non_exhaustive]
and use fallback cases in the match, it would be pretty annoying. I guess we do not even match
those as they are decoded in Go directly from JSON.
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.
Huh, I made them non_exhaustive in two commits, and assumed some code would break, at such as places like
cosmwasm/packages/std/src/mock.rs
Lines 301 to 314 in 448671c
match &request { | |
QueryRequest::Bank(bank_query) => self.bank.query(bank_query), | |
QueryRequest::Custom(custom_query) => (*self.custom_handler)(custom_query), | |
QueryRequest::Staking(staking_query) => self.staking.query(staking_query), | |
QueryRequest::Wasm(msg) => self.wasm.query(msg), | |
#[cfg(feature = "stargate")] | |
QueryRequest::Stargate { .. } => SystemResult::Err(SystemError::UnsupportedRequest { | |
kind: "Stargate".to_string(), | |
}), | |
#[cfg(feature = "stargate")] | |
QueryRequest::Ibc(_) => SystemResult::Err(SystemError::UnsupportedRequest { | |
kind: "Ibc".to_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.
This is a surprise I also hit recently. Rust is too clever: crate-internal you always must mach all cases. Clippy also marks fallback as dead code when all cases are matched. Crate-external, you always need a fallback case.
Closes #761
After merging, this needs to be exposed in wasmvm and handled in wasmd
@ebuchman thanks for the suggestion