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-encode and scale-decode to encode and decode based on metadata #842

Merged
merged 42 commits into from
Mar 21, 2023

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Feb 28, 2023

Part of a series:

  • scale-encode: new crate to handle encoding using type info.
  • scale-decode: handle decoding using type info.
  • scale-value: update Value type to use scale-encode/decode.
  • !subxt: Start taking advantage of these in Subxt.

This PR:

  • Codegen to derive EncodeAsType and DecodeAsType on generated types (currently as well as Encode and Decode, but eventually Encode/Decode may become entirely optional as we use type information for all encoding and decoding). (derives stuff tweaked to allow attrs to also be added to types).
  • Manually derive EncodeAsType and DecodeAsType (well, IntoVisitor/Visitor) on custom types that need it.
  • Add RootEvent trait and codegen to assist in decoding (with type info) events into a Root event type.
  • Move most things (building transactions, decoding events/storage/constants) to using EncodeAsType and DecodeAsType (ie to encode and decode using type info). Note: Encoding StorageKeys still uses static encoding.
  • Consolidate the addresss/payload types into one type, now that we have one code path that can cater for static and dynamic values.
  • Simplify storage codegen; no more hashers needed (we apply hashing based on runtime metadata for all codepaths now).
  • Remove the generated top level Event type, instead aliasing to the RuntimeEvent that's generated already (in part I did this because I wanted to be able to get hold of the path to the RuntimeCall type, but for now I'm not trying to substitute it since there are other issues there.

Breakage:

  • This PR didn't require much in terms of changes to tests/examples actually; unless you explicitly reference certain types, mostly things will just keep working I hope. A small change to the storage API was needed though; if you need to manually turn a storage address into bytes, the function to do that now lives on the StorageClient (ie api.storage().address_root_bytes())

What's the point of all this?

Previously, static things encoded/decoded to/from bytes based solely on their structure and not based on the actual type information. Dynamic Value types could be used in many places though, and those values would encode and decode based on type information.

This PR takes steps towards unifying these approaches, and allows both static and dynamic values to be encoded in the same sort of way based on type information. This carries a couple of benefits:

  1. Static encoding is now more tolerant w.r.t differences between static types and metadata (eg if we have a u8, and the type information requires a u32, we can now happily encode the u8 as a u32, or if we have a static type with fields that aren't needed, they can safely be ignored). There is no longer a need to use dyanmic Values when that robustness is needed, making static types strictly better.
  2. Static and dynamic encoding can start to share more common logic; this is seen as we collapse Static+Dynamic versions of various structs down into one struct that handles either.
  3. For things like calls to work across runtimes, they need to be encoded dynamically based on pallet/call index. But what if we want to make batch/multisig/nested calls? Currently we can hand craft them using the generated Call enum for the "inner" call, but this enum is hardcoded to the pallet/call indexes of the node it was generated from. Now, everything is encoded dynamically (based on variant name in this case) across the board, so nested calls will "just work" across runtimes (and we can provide a nicer API for this too now).
  4. One code path for encoding static or dynamic things means hopefully more robusty code over two code paths where one (ie the dynamic) may not be so well battle tested)

@jsdw jsdw requested a review from a team March 2, 2023 16:39
source: syn::Path,
target: AbsolutePath,
) -> Result<(), CodegenError> {
let (key, val) = TypeSubstitutes::parse_path_substitution(source, target.0)?;
Copy link
Member

Choose a reason for hiding this comment

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

Ok, this fails only if the parsing fails.

Can't we throw a warning or something if insert is ignored i.e, the given key already exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't bother only because this function is sortof dangling at the moment, but the point of it will be to override the RuntimeCall enum only if the user hasn't already overridden it via some type substitution of their own or whatever (which is necessary because we don't know where it lives at the point of setting up the default substitutions.. although maybe it could and I'll need to look into that).

Maybe it's best just to remove this function for now!

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.

awesome, looking forward to this PR

Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

In general looks fantastic, just a couple of comments/questions.

codegen/src/api/mod.rs Show resolved Hide resolved
subxt/src/metadata/decode_encode_traits.rs Show resolved Hide resolved
codegen/src/types/derives.rs Show resolved Hide resolved
subxt/src/storage/storage_client.rs Outdated Show resolved Hide resolved
testing/test-runtime/build.rs Show resolved Hide resolved
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

It's a shame about losing the substituting of foreign types, but this inconvenience is massively outweighed by the overall improvement here: brilliant stuff 👊

@jsdw
Copy link
Collaborator Author

jsdw commented Mar 21, 2023

It's a shame about losing the substituting of foreign types, but this inconvenience is massively outweighed by the overall improvement here: brilliant stuff 👊

Thank you :)

I'm just writing a small followup PR which adds a small Static util type; at least with this you can, I think, do something like:

#[subxt::subxt(
            runtime_metadata_path = "{}",
            derive_for_all_types = "Eq, PartialEq",
            substitute_type(
                type = "sp_arithmetic::per_things::Perbill",
                with = "Static<::sp_runtime::Perbill>"
)]

And it'll at least provide a workaorund today (but of course having the proper impls will always be more robust, and this will leave a Static type you'll need to dig into)

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