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_metadata doesn't return the SCALE-encoded metadata but the SCALE-encoded SCALE-encoded metadata #538

Open
tomaka opened this issue Oct 2, 2022 · 6 comments

Comments

@tomaka
Copy link
Collaborator

tomaka commented Oct 2, 2022

This concerns section C.5.

The title is a bit weird, but when the Metadata_metadata runtime entry point is called, Substrate encodes the metadata, producing a Vec<u8>, then encodes that Vec<u8> again, producing a different Vec<u8>.

In practice, what this means is that Metadata_metadata returns the SCALE-compact-encoded length of the SCALE-encoded metadata, followed with the SCALE-encoded metadata itself.

Note that the JSON-RPC function that returns the metadata correctly returns only the SCALE-encoded metadata. In other words it strips the length.

I would actually suggest to see if this should not be considered a Substrate bug before tweaking the spec.

@bkchr
Copy link
Contributor

bkchr commented Oct 2, 2022

I would actually suggest to see if this should not be considered a Substrate bug before tweaking the spec.

Yes that is intended. The node side should not interpret the metadata and thus, we return SCALE encode the metadata before returning it. The runtime api itself encodes the data again to being able to decode it on the node side.

@tomaka
Copy link
Collaborator Author

tomaka commented Oct 3, 2022

But what you're describing is a Substrate-specific problem, which is that Substrate can't make a runtime call without decoding the output.
If Substrate didn't automatically decode the output (and there's no technical reason why it would always automatically decode the output) then this would be easily solved.

@FlorianFranzen
Copy link
Contributor

FlorianFranzen commented Oct 3, 2022

This initially substrate-specific behavior is part of the spec and mirrored in other implementations as well, so I think it is here to stay.

So if I understood this correctly, the current definition is correct under the assumption of a Runtime API call convention.

@tomaka
Copy link
Collaborator Author

tomaka commented Oct 3, 2022

@FlorianFranzen
I guess you're "technically" correct that it's already explained in the spec. In C.3 we say that data is SCALE-encoded before being returned, and in C.5 we say that we return the SCALE-encoded metadata. So "technically" the text indeed explains that we SCALE-encode the metadata twice.

However IMO this should be explicited, as it's very subtle. As a naive reader, my assumption is that "return the SCALE-encoded metadata" means the same SCALE encoding as what section C.3 mentions, and is just repeated as a convenience.

This is IMO especially important because Metadata_metadata is a clear exception, it's the only function that SCALE encodes its return value twice.

@tomaka
Copy link
Collaborator Author

tomaka commented Oct 3, 2022

Also, there are several other functions in the spec, such as BabeApi_submit_report_equivocation_unsigned_extrinsic or BabeApi_generate_key_ownership_proof, where the text says "A SCALE encoded Option ...", even though the function simply returns an Option (which is then SCALE-encoded according to C.3).
So to me there's a clear discrepancy here.

@bkchr
Copy link
Contributor

bkchr commented Oct 4, 2022

But what you're describing is a Substrate-specific problem, which is that Substrate can't make a runtime call without decoding the output. If Substrate didn't automatically decode the output (and there's no technical reason why it would always automatically decode the output) then this would be easily solved.

Yeah good point, I'm okay with changing that.

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

No branches or pull requests

3 participants