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

Codegen for custom values in metadata #1117

Merged
merged 34 commits into from Aug 24, 2023

Conversation

tadeohepperle
Copy link
Contributor

@tadeohepperle tadeohepperle commented Aug 10, 2023

This PR adds static addresses for custom values, accessible via codegen.
So for example, if there is a custom value with name "Foo" referring to a type struct Foo { n: u8, b: bool } and the attached value is scale encoded (e.g. [5,0] for Foo), then we get back a decoded Foo from custom_value_client.at(&static_address)?:

#[subxt::subxt(runtime_metadata_path = "some_metadata.scale")]
pub mod interface {}
let static_address = interface::custom().foo();
let api = OnlineClient::<PolkadotConfig>::new().await?;
let custom_value_client = api.custom_values();
// Now the `at()` function already decodes the value into the Foo type:
let foo = custom_value_client.at(&static_address)?;

After some back and forth, I decided not to add a test for this, because it would involve creating some artificial metadata scale or json file, using it in the subxt macro and then using the interface with it. I think if people are moving to using custom types or they appear in polkadot in the future we should add an integration test for it.

@tadeohepperle tadeohepperle marked this pull request as ready for review August 11, 2023 13:07
@tadeohepperle tadeohepperle requested a review from a team as a code owner August 11, 2023 13:07
Comment on lines 36 to 54
(!custom_value_fns.is_empty()).then(|| {
quote!(
pub fn custom() -> custom::CustomValuesApi {
custom::CustomValuesApi
}

pub mod custom{
use super::#types_mod_ident;

pub struct CustomValuesApi;

impl CustomValuesApi {
#( #custom_value_fns )*
}
}


)
});
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I guess either this or the below could be removed

Suggested change
(!custom_value_fns.is_empty()).then(|| {
quote!(
pub fn custom() -> custom::CustomValuesApi {
custom::CustomValuesApi
}
pub mod custom{
use super::#types_mod_ident;
pub struct CustomValuesApi;
impl CustomValuesApi {
#( #custom_value_fns )*
}
}
)
});

Copy link
Member

@niklasad1 niklasad1 Aug 11, 2023

Choose a reason for hiding this comment

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

I agree with Alex that if custom_value_fns.is_empty() is easier for me to understand here but I always struggle to understand then/then_some on bool's

Comment on lines 73 to 74


Copy link
Contributor

Choose a reason for hiding this comment

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

Some extra spaces here

Suggested change

None
} else {
Some(quote!(
pub fn custom() -> custom::CustomValuesApi {
Copy link
Contributor

@lexnv lexnv Aug 11, 2023

Choose a reason for hiding this comment

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

With this implementation, I wonder if the metadata doesn't expose any custom values, then the users will not have the fn custom() exposed.

Wouldn't it be more consistent to have the pub fn custom() either way? Similar to how we expose pub fn tx() -> TransactionApi, then it would be an implementation detail that the CustomValuesApi doesn't expose anything.

Another benefit of mimicking the API would be that we don't have to collect the let custom_value_fns: Vec<_> values, saving an allocation here and there, since quote! works for iterators as well.

@jsdw
Copy link
Collaborator

jsdw commented Aug 11, 2023

I can think of a couple of ways to test this but I'm not sure which is cleanest offhand:

  1. Generate a metadata bundle in our artifacts folder which has some arbitrary custom values added.
    • There are other tests showing how to construct an instance of a type registry with some types in, so one could do that and then build some metadata with only custom types in and nothing else (for one thing, because merging types is a pain).
    • This could be used in some test by pointing the macro at it and checking that you can pull the custom values out.
    • It could also be used in example docs to show custom values working.
    • Bit of a pain because you need some code to generate that file (at least, I'd prefer this to having some opaque pre-generated file we had no code to re-create. This is essentially a script but would need to be in Rust. Wish it was more "ready" but maybe https://crates.io/crates/cargo-script-mvs could be used to write us a rust script to generate).
  2. Make a UI test; there's already a helper to generate arbitrary metadata and then build a UI test from it IIRC, so that shouldn't be super hard to do.
    • Easier because you don't need a script somewhere to generate the metadata bundle.
    • Worse because we can't use this also for good doc examples.

So yeah probably it's worth doing 1 to generate some metadata and use that for test/doc example purposes. When Substrate has custom metadata appearing in it for whatever reason, we can move to using that instead if we prefer.

codegen/src/types/mod.rs Outdated Show resolved Hide resolved
metadata/src/lib.rs Outdated Show resolved Hide resolved
@jsdw
Copy link
Collaborator

jsdw commented Aug 22, 2023

Looking good overall; just a bunch of small things to fix up re consistency or whatever, and I raised #1134 to do a little followup piece of work to spit out custom metadata for more thorough testing :)

metadata/src/lib.rs Outdated Show resolved Hide resolved
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.

Awesome stuff, good job @tadeohepperle!

Copy link
Contributor

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

👍

@tadeohepperle tadeohepperle merged commit b413e5e into master Aug 24, 2023
9 checks passed
@tadeohepperle tadeohepperle deleted the tadeo-hepperle-custom-values-codegen branch August 24, 2023 07:50
@jsdw jsdw mentioned this pull request Sep 27, 2023
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.

None yet

4 participants