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

metadata: Generate runtime outer enums if not present in V14 #1174

Merged
merged 10 commits into from
Sep 25, 2023

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Sep 25, 2023

This PR avoids panics while constructing the suxbt client from an unexpected metadata format.

In doing so, the subxt is more robust in connecting to custom chains that may define different metadata formats (such as wss://c1.hashed.live:443).

Panics originate from two paces while converting the metadata V14 to the latest format V15:
-expectation of extrinsic types in metadata
-expectation of outer enums RuntimeCall and RuntimeError being present

For the former case, errors are propagated back to the user avoiding panic.

In the latter case:

  • RuntimeCall is searched in metadata
  • an extra check is added to ensure it is an enum that was previously taken for granted
  • when the RuntimeCall is not found, the Call enum is searched
  • if neither RuntimeCall nor Call are found, an outer enum is generated

Closes: #1171

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv requested a review from a team as a code owner September 25, 2023 11:39
@lexnv lexnv self-assigned this Sep 25, 2023
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Comment on lines 339 to 341
let call_enum = find_type("RuntimeCall").or_else(|| find_type("Call"));
let event_enum = find_type("RuntimeEvent").or_else(|| find_type("Event"));
let error_enum = find_type("RuntimeError").or_else(|| find_type("Error"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

One issue: RuntimeCall, RuntimeEvent and RuntimeError are fairly unique names which are unlikely to be used for random other types. but Call, Event and Error are not, and in fact all of these names are used for other enums in the type registry (each pallet could have one or more of these types declared).

Is the Call enum that you found actually one of those per-pallet ones? I have a feeling though that possibly an old version of Substrate used Call rather than RuntimeCall or something like that, but can't remember for sure.

Anyway, in the generated polkakdot metadata, the types we want (RuntimeCall etc) are all at paths like runtime_types::polkadot_runtime::RuntimeCall, runtime_types::polkadot_runtime::RuntimeError and runtime_types::polkadot_runtime::RuntimeEvent. (other types all live in runtime_types but in other places). I wonder whether the _runtime part of the polkadot_runtime path can be used to help find these things. Or else, maybe we just return an error and don't try falling back to anything else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep that makes sense, checking if a Call is indeed the enum we are looking for can become quite tedious :D

I've changed the code to not have fallbacks; propagate the error back to the user and only generate the RuntimeError if not present, which seems to be more straight forward

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Comment on lines 329 to 333
if path.is_none() {
let mut segments = ty.ty.path.segments.clone();
segments.pop();
path = Some(segments);
}
Copy link
Collaborator

@jsdw jsdw Sep 25, 2023

Choose a reason for hiding this comment

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

It's a bit weird saving a path mutably like this I think. More code but IMO clearer would be something like this:

// here, return eg:
Some((ty.id, segments))

// and then:
let call_enumm = find_type("RuntimeCall");
let event_enumm = find_type("RuntimeEvent");
let error_enumm = find_type("RuntimeError");

// Pick a path to save any generated types to; aim to
// put them next to whatever existing type we find first,
// or fall back to dumping it in the "top level" runtime_types
// path.
let path = match (call_enum, event_enum, error_enum) {
    (Some((_,p)), None, None) => p,
    (None, Some((_,p)), None) => p,
    (None, None, Some((_,p))) => p,
    (None, None, None) => vec!["runtime_types".into()]
};

// ...

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'll remove that bit; fun fact I had initially thought of something similar with call.or(event).or(error) :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ooh yeah you're right; using .or would have been much more compact :D

@jsdw
Copy link
Collaborator

jsdw commented Sep 25, 2023

Is there any sensible way to test that we return gracefully (ie without a panic) in the cases where Runtime*'s aren't available; if so that would be really nice :)

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Copy link
Contributor

@tadeohepperle tadeohepperle left a comment

Choose a reason for hiding this comment

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

Nice, good job!

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 good; nice one!

@lexnv lexnv merged commit c2522c6 into master Sep 25, 2023
8 checks passed
@lexnv lexnv deleted the lexnv/generate_outer_enums branch September 25, 2023 15:37
tadeohepperle pushed a commit that referenced this pull request Sep 26, 2023
* metadata: Extend outer enum generation for V14

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Generate outer enums if not present

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Porpagate v14 error instead of panic

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Try to find `RuntimeCall` then `Call` enums

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Ensure the returned type is variant for outer enums

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Replace or with or_else

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Apply clippy

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Return error and generate only `RuntimeError`

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Remove modified path

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata/tests: Check missing runtime types

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
tadeohepperle added a commit that referenced this pull request Sep 26, 2023
* add web feature to subxt codegen

* add getrandom

* remove compile error

* metadata: Generate runtime outer enums if not present in V14 (#1174)

* metadata: Extend outer enum generation for V14

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Generate outer enums if not present

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Porpagate v14 error instead of panic

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Try to find `RuntimeCall` then `Call` enums

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Ensure the returned type is variant for outer enums

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Replace or with or_else

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Apply clippy

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Return error and generate only `RuntimeError`

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata: Remove modified path

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* metadata/tests: Check missing runtime types

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* add empty use of getrandom

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Co-authored-by: James Wilson <james@jsdw.me>
@jsdw jsdw mentioned this pull request Sep 27, 2023
@kylezs
Copy link
Contributor

kylezs commented Jan 4, 2024

We were on 0.31.0 and I was about to report a bug but thought I'd check later versions first, and saw it was fixed here 🙌 thanks.

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.

Panic upon establishing client connection
4 participants