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

Add scaffolding to declare Facade APIs #6396

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jsdw
Copy link
Contributor

@jsdw jsdw commented Nov 6, 2024

This PR adds two new crates:

  • sp-facade-apis: A crate to define Facade Runtime APIs. This has two purposes:
    1. Help enforce certain rules to promote the stability/documentation/naming of these APIs.
    2. Provide metadata about the declared APIs so that we can eg check the compatibility of a given runtime with current facade APIs.
  • sp-facade-apis-macro: A supporting macro to enable the above.

The Facade APIs are not actually populated yet, except with an example showing the syntax (same as decl_runtime_apis basically).

I wonder whether I should also "implement" the example APi in the kitchensink runtime just to show it working (my current plan is to have an example runtime in a separate crate along with a compatibility tool etc which shows the usage and allows trying out things).

\cc @kianenigma

@jsdw jsdw added I5-enhancement An additional feature request. T4-runtime_API This PR/Issue is related to runtime APIs. labels Nov 6, 2024
@@ -576,6 +577,7 @@ runtime = [
"sp-crypto-hashing-proc-macro",
"sp-debug-derive",
"sp-externalities",
"sp-facade-apis/decl-runtime-apis",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder whether this should be in the "runtime" feature or else in "runtime_full" or kept entirely separate.

@jsdw
Copy link
Contributor Author

jsdw commented Nov 6, 2024

/cmd prdoc --audience runtime_dev

@jsdw
Copy link
Contributor Author

jsdw commented Nov 6, 2024

Looking at the "Check publish" error, I assume I need to manually publish these crates to crates.io before I can merge this, since they are new.

In that case, I'll wait for reviews and if we are happy with things, I can go and manually publish etc so that it's all good to go.

@jsdw jsdw marked this pull request as ready for review November 6, 2024 17:30
@bkchr
Copy link
Member

bkchr commented Nov 6, 2024

I don't get why we need this. I can come up with millions of ways to still change the trait without the macro being able to detect this. The only thing it would detect is that the thing it is named FascadeSomething.

IMO it would be much better to just have these interfaces declared with proper documentation around them and how versioning works. Or even create a separate document explaining the versioning etc. Anyone writing software should be aware on "what a stable interface" is and that you can not just change it etc. You want a social consensus and not this macro.

@jsdw
Copy link
Contributor Author

jsdw commented Nov 7, 2024

We could definitely just have a facade crate which calls decl_runtime_apis directly instead, and has documentation explaining about the stability and so on that we want (I agree that this is a social thing as much as anything). We should have this documentation/agreement here too.

I considered this, but ultimately I saw enough benefits to writing a small wrapper around the decl_runtime_apis macro:

  • We generate metadata about the facade APIs, which allows for:
    • Tooling to check which versions of facade APIs a node implements (this could be used in CI to help ensure we don't break compatibility, but also in higher level tooling to check what versions of facade APIs different runtimes implement, so we know what we can call and how).
    • Generating example code/docs for the facade runtime APIs.
  • We add simple protections against using decl_runtime_api features that break compat and a couple of convention enforcements, like ensuring each method is documented (#[deny(missing_docs)] doesn't play nice with decl_runtime_apis when I checked) and ensuing we prefix Facade etc.
    • I agree that we should document the rules anyway, and of course it's easy to still write something wrong, but having some basic "linting" I thought would be helpful.
  • I also had in mind that we might want to add things in the future, and this makes that easy to do:
    • One thing I had in mind was to append some documentation for each trait saying what the hash/version for it was, so people would know what to look for in the system.version constant too to check for support in that way.

@bkchr
Copy link
Member

bkchr commented Nov 8, 2024

  • Tooling to check which versions of facade APIs a node implements (this could be used in CI to help ensure we don't break compatibility, but also in higher level tooling to check what versions of facade APIs different runtimes implement, so we know what we can call and how).

A node doesn't implement any facade API. A runtime, but not a node. Also to check the version you just need to check the version of the runtime api.

  • Generating example code/docs for the facade runtime APIs.

Use external tooling. You really want to shove all this stuff into some macro? How should that work? Should that generate python, js and whatever examples? Doesn't sound reasonable to me.

  • I agree that we should document the rules anyway, and of course it's easy to still write something wrong, but having some basic "linting" I thought would be helpful.

Could be a custom linter that also has much more context etc.

  • One thing I had in mind was to append some documentation for each trait saying what the hash/version for it was, so people would know what to look for in the system.version constant too to check for support in that way.

Please just no. So you want people to lookup this hash somewhere and then fetch the system version manually to find out if this hash is part of it? Provide a tool Name => is_it_part_of_runtime? The hash can also just be derived from the name, so it could be done everywhere in the external tooling.

@jsdw
Copy link
Contributor Author

jsdw commented Nov 8, 2024

A node doesn't implement any facade API. A runtime, but not a node. Also to check the version you just need to check the version of the runtime api.

Yup fair; I was thinking only about connecting to a node via RPC and using its metadata to check the compatibility of the runtime the node is using.

And yeah, checking the version of the runtime API can be done just with system.version overall. Using the facade metadata you get more precise details about what is missing/needs implementing from the latest facade APIs (and can check even if somebody manually declares matching APIs themselves or whatever), so it's more robust but yeah maybe not needed.

Use external tooling. You really want to shove all this stuff into some macro? How should that work? Should that generate python, js and whatever examples? Doesn't sound reasonable to me.

I indeed would use external tooling for that and not shove it in a macro; because the macro generates some metadata it's easy to write some external lib which takes that metadata and uses it to hand back docs or whatever.

Please just no. So you want people to lookup this hash somewhere and then fetch the system version manually to find out if this hash is part of it? Provide a tool Name => is_it_part_of_runtime? The hash can also just be derived from the name, so it could be done everywhere in the external tooling.

Having the hash would make it a little easier for devs to check things in the case of issues, but I agree that having a proper tool or way to check it would be nicer. That's why the main thing of this facade macro is handing back metadata: we can write this sort of tool without manually redefining somewhere else what the facade APIs should look like and having to keep that in sync with reality.

If you're generally against the facade macro and metadata it creates then we can strip it out and just use decl_runtime_apis, but I think it would be useful to write these external tools, and eg Subxt could import the facade metadata to offer exactly these sorts of compat checks or whatever really easily :)

@bkchr
Copy link
Member

bkchr commented Nov 8, 2024

But what is this extra metadata that the macro could generate? The macro has exactly access to the same information as the decl_runtime_apis macro. How can it generate more metadata? Isn't all the metadata about the runtime api not already part of the metadata and you can generate exactly the same with your offchain tooling?

@jsdw
Copy link
Contributor Author

jsdw commented Nov 8, 2024

Ah yeah I'm being dumb; I did this stuff before digging into decl_runtime_apis for the other PR and forgot here that it also provides metadata!

The metadata I generate directly in decl_runtime_apis has two small advantages still:

  • It has the api_version of each method.
  • It's very light on dependencies. For facade-api, cargo tree --no-default-features --features metadata -e normal | wc -l is 37, whereas for sp-api, cargo tree --no-default-features --features frame-metadata -e normal | wc -l is 412.

For using in eg Subxt, I'd rather avoid bringing that many dependencies in, but I could look into adding some feature flag so that I can obtain the metadata without pulling in all of these other crates (sp-runtime and sp-core are the main offenders and aren't needed for metadata unless I'm missing something)? For the api_version of each method I could add that to the RuntimeApiMetadataIR easily enough; I almost did in that other PR anyways so could revert to that approach :D

What do you reckon?

@jsdw
Copy link
Contributor Author

jsdw commented Nov 8, 2024

A final thought is that if I rely on the RuntimeApiMetadataIR, it feels sensible to me to wrap this type anyway and avoid exposing it to external tools, since it's meant as an internal representation and it's good to be able to add whatever data we want to it without thinking about breaking an interface external tools are relying on.

@bkchr
Copy link
Member

bkchr commented Nov 8, 2024

  • It's very light on dependencies. For facade-api, cargo tree --no-default-features --features metadata -e normal | wc -l is 37, whereas for sp-api, cargo tree --no-default-features --features frame-metadata -e normal | wc -l is 412.

sp-api is a dependency of facade-api, but yeah it is optional. Also I don't get why you need to pull in sp-api? Why don't you only pull in frame-metadata?

api_version could be added to the frame metadata as well.

@jsdw
Copy link
Contributor Author

jsdw commented Nov 11, 2024

sp-api is a dependency of facade-api, but yeah it is optional. Also I don't get why you need to pull in sp-api?

My thoughts on the features of this facade crate were:

  • If you want to implement the facade APIs in a runtime, you enable only the decl_runtime_apis features which declare them, and you don't care about the metadata being generated. This only brings in sp-api.
  • If you're writing some external tool or want to know about facade APIs, you enable only the metadata feature to generate the metadata. This avoids bringing in sp-api as we don't need to decl_runtime_apis at all now. We only need scale-info.
  • I also added a frame-metadata feature flag which enables the same name feature in sp-api. I'm not sure whether this feature is useful/needed at all here actually with the current approach.

Why don't you only pull in frame-metadata?

Sorry; I don't really follow this bit? 😅

if I generate the metadata locally as I am now then I only pull in scale-info and nothing else.

If I make use of the RuntimeMetadataIR then I think I still need sp-api because declare_runtime_apis (with frame-metadata enabled) is the thing which generates this metadata. sp-api pulls in sp-core and sp-runtime regardless.

api_version could be added to the frame metadata as well.

Yup that's an interesting thought! I'm unsure how much apps care about this, but having eg Compact<u32> would only add a byte per call/trait (depending on which level we added it).

This would make checking compat with facade APIs easy too (if we trust that the facade APIs have indeed remained stable), so potentially the facade metadata we spit out here could be simplified to just method names and versions and no scale-info stuff (trusting that the types are stable and line up). I'm easy either way on this bit though; some part of me likes checking the full type signatures to be 100% confident things match up!

@jsdw jsdw mentioned this pull request Nov 11, 2024
@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12034188784
Failed job name: fmt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. T4-runtime_API This PR/Issue is related to runtime APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants