feat(nervous-system): Enable Root to upgrade canisters using chunked Wasms #3300
Conversation
| (&install_code_args,), | ||
| ) | ||
| .await | ||
| if let Some(ChunkedCanisterWasm { |
There was a problem hiding this comment.
There will need to be validation somewhere that chunked_canister_wasm and wasm_module are mutually exclusive, I think.
There was a problem hiding this comment.
I was thinking about it, too, but couldn't convince myself that this was necessary.
Currently, if there are some bytes in wasm_module and chunked_canister_wasm, then the former would be used and the latter ignored.
Are you sure it's better to reject requests that specify more data than we need?
Either way, I'm happy to add a unit test to make sure this behavior is well specified.
There was a problem hiding this comment.
I think the principle of least surprise should apply. One would not expect to be able to submit two mutually exclusive options into an
API and not get an error. If the API silently ignores one, this could be confusing.
It might be a good change, for future forward compatibility, to make the new field actually support both as an enum, and then translate the old field into the enum as part of the validation, so that we can eventually retire the old field and have the type structure enforce the either-or nature of the arguments.
|
This seems like a good first and necessary step. Can you add a test? |
Done (please see rs/nervous_system/integration_tests/tests/upgrade_sns_controlled_canister_with_large_wasm.rs) |
| sns::swap::await_swap_lifecycle(&pocket_ic, sns.swap.canister_id, Lifecycle::Open) | ||
| .await | ||
| .unwrap(); | ||
| sns::swap::smoke_test_participate_and_finalize( |
There was a problem hiding this comment.
It would be better to not have all our tests do this. This was originally created for the sake of final release qualification and is the slowest part of most tests.
PocketIC tests seem to be a bit slower for some reason
| .upload_chunk( | ||
| store_canister_id.into(), | ||
| // This is a simplification; for now, we assume the Root itself decides to upload | ||
| // some WASM chunks, but eventually this should be triggered via proposal. |
There was a problem hiding this comment.
Actually, it doesn't matter how the chunks get there so long as 1) root is a co-controller and 2) the wasm hash is correct (which is part of the proposal and should be verified). I think that means that you don't need root to upload the chunks necessarily.
This PR enables Root to upgrade canisters using chunked Wasms. This is relevant to both the NNS and the SNSs.