Load latest metadata version from Wasm blobs. #1859
Conversation
macro/src/wasm_loader.rs
Outdated
| } | ||
|
|
||
| fn decode(encoded_metadata: Vec<u8>) -> WasmMetadataResult<Metadata> { | ||
| Metadata::decode(&mut encoded_metadata.as_ref()).map_err(Into::into) | ||
| // We slice the first byte from the metadata because it's wrapped inside an option and we know that its always `Some` | ||
| let metadata = <Vec<u8>>::decode(&mut &encoded_metadata[1..]).map_err(CodegenError::Decode)?; |
There was a problem hiding this comment.
nit: Would this panic on empty encoded_metadata?
There was a problem hiding this comment.
Yeah, probably, will change
There was a problem hiding this comment.
I think that the following code would work 😄
let metadata = <Option<Vec<u8>>>::decode(&mut &encoded_metadata[..]).map_err(CodegenError::Decode)?;There was a problem hiding this comment.
refactored a bit to also fail on None
There was a problem hiding this comment.
Great job here!
Thinking outloud here, maybe a followup: it would be useful at some point to extend the functionality of this module:
- have a
WasmBlobExecutorwrapper that is handling just the executor building and runtime blob - then we could expose
WasmBlobExecutor::versionswhich calls intoMetadata_metadata_versions - similar for
WasmBlobExecutor::get_metadata(version: ..)
This way we can choose in our macros which version to use (most prob the lastest, but sometimes we may want to fallback to lastest stable instead of unstable)
macro/src/wasm_loader.rs
Outdated
| Ok(versions) | ||
| } | ||
|
|
||
| fn metadata(&mut self) -> WasmMetadataResult<Metadata> { |
There was a problem hiding this comment.
Nit: I'd call this something like load_legacy_metadata or something to be consistent with below (and make clear it's the "old" call :))
There was a problem hiding this comment.
renamed to load_legacy_metadata
macro/src/wasm_loader.rs
Outdated
| decode(encoded_metadata) | ||
| } | ||
|
|
||
| fn load_metadata_at_latest_version( |
There was a problem hiding this comment.
This is a nit and can be ignored, but personally I'd break the interface down a bit so that we maybe just expose basic wrappers around:
Metadata_metadata(),Metadata_metadata_versions()andMetadata_metadata_at_version(version)
These wrapperes just call the associated runtime API and give back the decoded result and nothing more.
In other words, load_metadata_at_latest_version here would just take in a single version to try and load, and the logic for handling versions is in the call_and_decode function rather than here. versions would equally just return the versions and not pre-filter them; we'd filter them also in call_and_decode or whatever.
This way, the "important" logic is all visible in one place and what these functions do is trivial.
Description
closes #1858