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

Use scale-typegen as a backend for the codegen #1260

Merged

Conversation

tadeohepperle
Copy link
Contributor

@tadeohepperle tadeohepperle commented Nov 13, 2023

This PR uses scale-typegen to generate the runtime_types module and structs for calls, events, etc.
Let's see if the pipeline works, I manually checked the differences in the polkadot.rs file to make sure the differences are valid (almost none).

Recursive Derives

PR also includes adjustments to the subxt macro to allow for recursive derives. Shown in the setup_config_custom.rs example.

Fixes #1268
Fixes #1247 (indirectly)

Type Aliases

I tried to reconcile the conflicts between the master and this branch, and implemented the alias modules a bit differently, I hope it does not break anything.

@tadeohepperle tadeohepperle requested a review from a team as a code owner November 13, 2023 17:15
@tadeohepperle tadeohepperle force-pushed the tadeohepperle/implement-scale-typegen-for-type-generation branch from 7db70a1 to 03591a6 Compare November 21, 2023 14:01
tadeohepperle and others added 8 commits November 28, 2023 18:41
* lightclient: Close wasm socket while dropping from connecting state

Signed-off-by: Alexandru Vasile <[email protected]>

* lightclient: Construct one time only closures

Signed-off-by: Alexandru Vasile <[email protected]>

* testing: Enable console logs for lightclient WASM testing

Signed-off-by: Alexandru Vasile <[email protected]>

* lightclient: Separate wakes and check connectivity on poll_read

Signed-off-by: Alexandru Vasile <[email protected]>

* lightclient: Close the socket depending on internal state

Signed-off-by: Alexandru Vasile <[email protected]>

* Revert "lightclient: Separate wakes and check connectivity on poll_read"

This reverts commit 8660940.

* lightclient: Return pending if socket is opening from poll_read

Signed-off-by: Alexandru Vasile <[email protected]>

* lightclient: Close the socket on `poll_close`

Signed-off-by: Alexandru Vasile <[email protected]>

* lightclient: Reset closures on Drop to avoid recursive invokation

Signed-off-by: Alexandru Vasile <[email protected]>

* lightclient: Close the socket if not already closing

Signed-off-by: Alexandru Vasile <[email protected]>

---------

Signed-off-by: Alexandru Vasile <[email protected]>
* cli: Add chainSpec command

Signed-off-by: Alexandru Vasile <[email protected]>

* cli/chainSpec: Move to dedicated module

Signed-off-by: Alexandru Vasile <[email protected]>

* cli: Compute the state root hash

Signed-off-by: Alexandru Vasile <[email protected]>

* cli: Remove code substitutes

Signed-off-by: Alexandru Vasile <[email protected]>

* artifacts: Update polkadot.json

Signed-off-by: Alexandru Vasile <[email protected]>

* scripts: Generate the chain spec

Signed-off-by: Alexandru Vasile <[email protected]>

* cli: Remove testing artifacts

Signed-off-by: Alexandru Vasile <[email protected]>

* cli: Fix clippy

Signed-off-by: Alexandru Vasile <[email protected]>

* cli: Apply rustfmt

Signed-off-by: Alexandru Vasile <[email protected]>

* cli: Introduce feature flag for smoldot dependency

Signed-off-by: Alexandru Vasile <[email protected]>

* cli: Rename chain-spec to chain-spec-pruning

Signed-off-by: Alexandru Vasile <[email protected]>

* scripts: Update chain-spec command

Signed-off-by: Alexandru Vasile <[email protected]>

---------

Signed-off-by: Alexandru Vasile <[email protected]>
@tadeohepperle tadeohepperle requested a review from a team as a code owner November 29, 2023 13:02
codegen/src/api/mod.rs Outdated Show resolved Hide resolved
codegen/src/api/mod.rs Outdated Show resolved Hide resolved
tadeohepperle and others added 3 commits November 30, 2023 16:56
* update crypto dependencies, adjust keypair

* add scale_info::TypeInfo derive in some places

* add multi signature derive
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that the changes have not affected the aliases and other code generations (except for additional docs, which should be fine). Just to double check, is this file generated from the latest polkadot with all the code changes included right? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I ran the polkadot.rs file generation command after implementing all the code changes in this PR. :)

@@ -167,8 +167,16 @@ impl CodegenBuilder {
&mut self,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'd need to adjust the docs of this function a bit, the warning section specifically

@@ -181,11 +189,19 @@ impl CodegenBuilder {
&mut self,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same doc adjustment here, since we now have the recursive parameter

@@ -168,7 +168,8 @@ fn codegen(
.map_err(|e| eyre!("Cannot parse derive for type {ty_str}: {e}"))?;
let derive = syn::parse_str(&derive)
.map_err(|e| eyre!("Cannot parse derive for type {ty_str}: {e}"))?;
codegen.add_derives_for_type(ty, std::iter::once(derive));
// Note: recursive derives and attributes not supported in the CLI => recursive: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would we want to support recursive derives in the CLI? Might be worth a separate issue to remind us of it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, maybe. Or maybe we just make everything recursive by default in the CLI, I think it is already quite hard to specify derives in the CLI.

Copy link
Member

Choose a reason for hiding this comment

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

Would be great if you could open an issue for it :)

.iter()
.map(|(name, field)| {
let call_arg = if field.is_boxed() {
// Note: fn_arg_type this is relative the type path of the type alias when prefixed with `types::`, e.g. `set_max_code_size::New`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could there be a variant of the metadata where this does not correspond to the expected alias? Or it would be different from the previous format_ident!("{}", name.to_string().to_upper_camel_case())?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, when calling generate_structs_from_variants, which calls generate_type_alias_mod for each such composite, for each field, the field.type_path is replaced by #alias_mod_name::#alias_name, where alias_name is either format_ident!("{}", name.to_string().to_upper_camel_case()) (named fields) or format_ident!("Field{}", i) for unnamed fields, so the logic should be very similar to what you implemented before. I cannot think of a case right now where this is not fulfilled.

let error_type = type_gen.resolve_type_path(error_ty);
let error_ty = type_gen.resolve_type(error_ty);
let error_type = type_gen.resolve_type_path(error_ty)?;
let error_ty = type_gen.resolve_type(error_ty)?;
let docs = &error_ty.docs;
let docs = should_gen_docs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be infered from the type_gen.settings()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, totally! Good catch!

derives: DerivesRegistry,
type_substitutes: TypeSubstitutes,
derives: scale_typegen::DerivesRegistry,
type_substitutes: scale_typegen::TypeSubstitutes,
crate_path: syn::Path,
should_gen_docs: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any particular reason for having this? It is only passed to the error generation, while others use the type_gen settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? Having scale_typegen::DerivesRegistry specified fully, is not necassary, I should just import DerivesRegistry. Maybe I understand the question wrong.

Copy link
Collaborator

@lexnv lexnv Jan 9, 2024

Choose a reason for hiding this comment

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

Ops, I forgot to type should_gen_docs. I do like having the scale_typegen:: prefix 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay, the should_gen_docs we actually need because the typegen settings are not built up yet. They are only built in this very function using the crate_path, should_gen_docs, substitutes and derives :)

@lexnv
Copy link
Collaborator

lexnv commented Jan 9, 2024

The code looks clean and simplifies the code quite a lot! Nice one! 👍

Just some nits about the documentation and inferring the type alias paths

@@ -12,20 +12,20 @@ mod events;
mod runtime_apis;
mod storage;

use scale_typegen::typegen::ir::type_ir::{CompositeFieldIR, CompositeIR, CompositeIRKind};
Copy link
Member

@niklasad1 niklasad1 Jan 10, 2024

Choose a reason for hiding this comment

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

would be nice to export all these types from the root in scale_typegen all the types are already unique and I don't think for instance scale_typegen::typegen::ir::type_ir::CompositeFieldIR is necessary, thus scale_typegen::CompositeFieldIR or ``scale_typegen::ir::CompositeFieldIR` would be sufficient.

Nothing to address here just a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! We already expose some other types at the root, I did not anticipate that we would need easy access to these structs.

codegen/src/api/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@niklasad1 niklasad1 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 to me

@@ -309,12 +309,12 @@ fn storage_differences<'a>(
|e| {
pallet_metadata_1
.storage_hash(e.name())
.expect("storage entry should be present")
.expect("storage entry is in medadata; qed")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.expect("storage entry is in medadata; qed")
.expect("storage entry is in metadata; qed")

},
|e| {
pallet_metadata_2
.storage_hash(e.name())
.expect("storage entry should be present")
.expect("storage entry is in medadata; qed")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.expect("storage entry is in medadata; qed")
.expect("storage entry is in metadata; qed")

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 one! 👍

@tadeohepperle tadeohepperle merged commit fc5a18a into master Jan 11, 2024
11 checks passed
@tadeohepperle tadeohepperle deleted the tadeohepperle/implement-scale-typegen-for-type-generation branch January 11, 2024 15:42
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.

codegen: Recursive derives for types Handling skipped type params in subxt-codegen
3 participants