Skip to content
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

Merged
merged 5 commits into from
Feb 5, 2021
Merged

Add WasmMsg::Migrate variant #768

merged 5 commits into from
Feb 5, 2021

Conversation

ethanfrey
Copy link
Member

@ethanfrey ethanfrey commented Feb 5, 2021

Closes #761

After merging, this needs to be exposed in wasmvm and handled in wasmd

@ebuchman thanks for the suggestion

@ethanfrey ethanfrey added this to In progress in CosmWasm VM via automation Feb 5, 2021
new_code_id: u64,
/// msg is the json-encoded MigrateMsg struct that will be passed to the new code
msg: Binary,
},
Copy link
Member

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].

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

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

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(),
}),
}
but tests keep working. Not sure what this does in the end

Copy link
Member

@webmaster128 webmaster128 Feb 5, 2021

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.

CosmWasm VM automation moved this from In progress to Reviewer approved Feb 5, 2021
@ethanfrey ethanfrey merged commit 62912ea into main Feb 5, 2021
CosmWasm VM automation moved this from Reviewer approved to Done Feb 5, 2021
@ethanfrey ethanfrey deleted the wasm-msg-migrate branch February 5, 2021 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Add WasmMsg::Migrate variant
2 participants