paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.com/
1.88k stars 689 forks source link

Splitting code inside impl_runtime_apis! macro into separate files #3067

Open amiyatulu opened 9 months ago

amiyatulu commented 9 months ago

How can I separate custum runtime api into separate file, instead of putting in all in one file.

impl_runtime_apis! {

impl profile_validation_runtime_api::ProfileValidationApi<Block, AccountId> for Runtime {

        fn get_challengers_evidence(profile_user_account: AccountId, offset: u64, limit: u16) -> Vec<ChallengePostId> {
            ProfileValidation::get_challengers_evidence(profile_user_account, offset, limit)
        }
 }
}
bkchr commented 9 months ago

That is currently not possible.

gupnik commented 9 months ago

Yeah, I wanted to explore this after Construct Runtime v2. Current impl_runtime_apis seems too restrictive and probably deserves a v2. @kianenigma Thoughts?

bkchr commented 9 months ago

If it would be supported, I would have implemented it years ago. If we remove support for the native runtime entirely, we can improve the situation and switch to an attribute macro.

kianenigma commented 9 months ago

@gupnik you should follow the work regarding the deprecation of the native runtime and see how it can simplify CRV macros.

  1. In the short term, we can still have a new CRV macro that is easier on the eye.
  2. Once complexities of the native runtime or gone, if we can have it be split, even better

But it is important that step 1 and 2 are compatible, so it is good to keep an eye on this.

@bkchr are you sure the existing macro is impossible to break up? It might be a good reason for @gupnik to already start exploring.

bkchr commented 9 months ago

I just realized that if we use a static constructor that registers the different runtime api in the native runtime it could work.

pkhry commented 1 week ago

I just realized that if we use a static constructor that registers the different runtime api in the native runtime it could work.

I'm not quite following this, can you elaborate on your suggestion?

kianenigma commented 1 week ago

fwiw, the end goal of this feature is to

  1. enable teams to upgrade to latest "default/standard" set of runtime apis, without needing to manually alter this macro
  2. Split it into smaller files for better organization.

It might be worth looking into what happens out of the box when you just have two impl_runtime_apis! {} in a file. Which duplicate symbols are generated, if any?

pkhry commented 1 week ago

fwiw, the end goal of this feature is to

  1. enable teams to upgrade to latest "default/standard" set of runtime apis, without needing to manually alter this macro
  2. Split it into smaller files for better organization.

It might be worth looking into what happens out of the box when you just have two impl_runtime_apis! {} in a file. Which duplicate symbols are generated, if any?

ok, some notes:

If we call impl_runtime_apis multiple times within context of a single file we will see that those symbols will get duplicated:

possible solutions to having two macro invocations:

kianenigma commented 1 week ago

Thanks for the great exploration here!

I am on the edge of my knowledge here, so take what I say with a grain of salt + trust your opinion from the code and or a comment from someone who knows the code better like Basti more.

Introducing new macros that can be more granular in things that will get generated per macro block Can work, but would require loading generated code in memory and parsing trait impls to generate wasm shims.

I like this either this, or moving towards making these ident names configurable so that you can call the macro multiple times. It seems simple, and no antipatterns are in it.

Also it might be worth noting that if anything is generated by this macro as #[cfg(feature = "std") is only needed for the legacy native runtime, which we can already count on removing.

This gives me hope that we can easily rename/dedupe these Rust idents, and have zero impact on the final wasm.

pkhry commented 5 days ago

i think the issue can be postponed until later/closed for now. as we need to have cross macro communication in impl_runtime_apis! due to generating code for native runtime and the need to define RuntimeApi struct to be used for all subsequent macro invocations.

native runtime shims need to know about all of the apis and methods for its codegen to function as it generates a large switch statement with all the <ApiTraitName>_<method_name> match statement