-
Notifications
You must be signed in to change notification settings - Fork 284
metadata: Generate runtime outer enums if not present in V14 #1174
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
Conversation
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
metadata/src/from_into/v14.rs
Outdated
| 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")); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 <[email protected]>
metadata/src/from_into/v14.rs
Outdated
| if path.is_none() { | ||
| let mut segments = ty.ty.path.segments.clone(); | ||
| segments.pop(); | ||
| path = Some(segments); | ||
| } |
There was a problem hiding this comment.
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()]
};
// ...There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
Is there any sensible way to test that we return gracefully (ie without a panic) in the cases where |
Signed-off-by: Alexandru Vasile <[email protected]>
Signed-off-by: Alexandru Vasile <[email protected]>
tadeohepperle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, good job!
jsdw
left a comment
There was a problem hiding this 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!
* metadata: Extend outer enum generation for V14 Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Generate outer enums if not present Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Porpagate v14 error instead of panic Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Try to find `RuntimeCall` then `Call` enums Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Ensure the returned type is variant for outer enums Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Replace or with or_else Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Apply clippy Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Return error and generate only `RuntimeError` Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Remove modified path Signed-off-by: Alexandru Vasile <[email protected]> * metadata/tests: Check missing runtime types Signed-off-by: Alexandru Vasile <[email protected]> --------- Signed-off-by: Alexandru Vasile <[email protected]>
* 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 <[email protected]> * metadata: Generate outer enums if not present Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Porpagate v14 error instead of panic Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Try to find `RuntimeCall` then `Call` enums Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Ensure the returned type is variant for outer enums Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Replace or with or_else Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Apply clippy Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Return error and generate only `RuntimeError` Signed-off-by: Alexandru Vasile <[email protected]> * metadata: Remove modified path Signed-off-by: Alexandru Vasile <[email protected]> * metadata/tests: Check missing runtime types Signed-off-by: Alexandru Vasile <[email protected]> --------- Signed-off-by: Alexandru Vasile <[email protected]> * add empty use of getrandom --------- Signed-off-by: Alexandru Vasile <[email protected]> Co-authored-by: Alexandru Vasile <[email protected]> Co-authored-by: James Wilson <[email protected]>
|
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. |
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
RuntimeCallandRuntimeErrorbeing presentFor the former case, errors are propagated back to the user avoiding panic.
In the latter case:
RuntimeCallis searched in metadataRuntimeCallis not found, theCallenum is searchedRuntimeCallnorCallare found, an outer enum is generatedCloses: #1171