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 support for generating metadata from runtime wasm files #1720

Merged
merged 12 commits into from
Sep 2, 2024

Conversation

pkhry
Copy link
Contributor

@pkhry pkhry commented Aug 21, 2024

Description

See #1660

Adds wasm_file_path keyword support to subxt::subxt macro to allow extracting metadata from the runtime WASM.

No support for compiling target crate to wasm and using it to generate the Metadata for now.

code taken from polkadot-sdk PR

@pkhry pkhry requested a review from a team as a code owner August 21, 2024 16:59
Cargo.toml Outdated
@@ -115,6 +115,7 @@ which = "5.0.0"
strip-ansi-escapes = "0.2.0"
proptest = "1.5.0"
hex-literal = "0.4.1"
subwasmlib = { git = "https://github.com/chevdor/subwasm", rev = "v0.21.3" }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit; There's no released crate on crates io?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

macro/src/lib.rs Outdated
let subwasm = Subwasm::new(&source.try_into().map_err(to_error)?).map_err(to_error)?;
let mut output = vec![];
subwasm
.write_metadata(OutputFormat::Scale, None, &mut output)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, it would be interesting if we could:

  • decode the metadata in a way that we don't depend on substrate crates (sp-* sc_executor)
  • extract from the wasm blob just the Metadata_metadata() bytes

If that turns out to be too complicated, we can still look into obtaining that information via substrate-crates. Using subwasmlib to fetch the metadata brings quite a tree of dependencies.

For extracting the wasm blob, we'd need to rely on sp_maybe_compressed_blob:
https://github.com/chevdor/subwasm/blob/fae3e13bd1f8680bf32cbf2ed223223583f8d713/libs/wasm-loader/src/lib.rs#L152

For calling into the wasm blob and fetching the metadata, we'd rely onsc-executor:
https://github.com/chevdor/subwasm/blob/fae3e13bd1f8680bf32cbf2ed223223583f8d713/libs/wasm-testbed/src/lib.rs#L131-L147

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rewritten to use direct dependencies in last commit

Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looks good as a first step!

I would try to explore decoding the wasm blob internally, or use the substrate crates directly

edit; I think the end-goal here is to fetch the wasm blob directly from a repo (ie #1660 (comment)), which then justifies the subwasmlib dependency.

@jsdw
Copy link
Collaborator

jsdw commented Aug 22, 2024

edit; I think the end-goal here is to fetch the wasm blob directly from a repo (ie #1660 (comment)), which then justifies the subwasmlib dependency.

The end goal for me here is to point the subxt macro to some WASM runtime file (just as you can also point to the metadata.scale currently) and have it extract the metadata from that and use the metadata to generate an interface.

I would try to explore decoding the wasm blob internally, or use the substrate crates directly

I agree; I think it'd be good to look into how easily we can avoid this subwasm crate and the various dependencies it brings in and to the decoding internally (or using the relevant sp-* crate as long as it doesn't bring in too much itself). So I'm roughly in agreement with #1720 (comment) :)

@pkhry
Copy link
Contributor Author

pkhry commented Aug 22, 2024

edit; I think the end-goal here is to fetch the wasm blob directly from a repo (ie #1660 (comment)), which then justifies the subwasmlib dependency.

The end goal for me here is to point the subxt macro to some WASM runtime file (just as you can also point to the metadata.scale currently) and have it extract the metadata from that and use the metadata to generate an interface.

I would try to explore decoding the wasm blob internally, or use the substrate crates directly

I agree; I think it'd be good to look into how easily we can avoid this subwasm crate and the various dependencies it brings in and to the decoding internally (or using the relevant sp-* crate as long as it doesn't bring in too much itself). So I'm roughly in agreement with #1720 (comment) :)

I've redone things using relevant crates directly and PR unfortunately is still pulling quite a lot of deps.

@pkhry pkhry requested review from niklasad1 and lexnv August 22, 2024 23:19
codegen/src/error.rs Outdated Show resolved Hide resolved
macro/Cargo.toml Outdated
Comment on lines 30 to 34
sc-executor = { workspace = true }
sp-maybe-compressed-blob = { workspace = true }
sp-state-machine = { workspace = true }
sp-io = { workspace = true }
sc-executor-common = { workspace = true }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondfer how many dependencies we end up pulling in here; possibly worth putting behind a feature flag like "runtime_file_path" and then perhaps not enabling it by default (since I think it's probably a bit of an edge case that people would point straight to a runtime WASM blob).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved them to optional and hidden behind feature flag

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of small things re name and putting behind a feature flag but otherwise looks very clean; good job!

@pkhry pkhry requested a review from jsdw August 28, 2024 13:03
"Exclusively one of 'runtime_metadata_path', 'runtime_metadata_insecure_url' or `runtime_path` must be provided"
);
};
let root = std::env::var("CARGO_MANIFEST_DIR").unwrap_or_else(|_| ".".into());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"." -> current_dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think so, I've just taken the default value from 'runtime_metada_path' branch tbh

Cargo.lock Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice call, to add a feature-flag for these wasm deps in substrate

Comment on lines +119 to +120
sc-executor = "0.40.0"
sc-executor-common = "0.35.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very much a Nit, but these are substrate crates too so could be below with the rest :)

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great; nice work!

One tiny thing to do might also be to document this new command in the subct macro docs; I think we have some examples of different attrs and would be good to add one for this too.

@pkhry pkhry merged commit 3866737 into master Sep 2, 2024
13 checks passed
@pkhry pkhry deleted the pkhry/wasm-file-path-macro-support branch September 2, 2024 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants