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 compatbility layer for using substrate types from sp-core and sp-runtime directly #854

Closed
niklasad1 opened this issue Mar 10, 2023 · 2 comments

Comments

@niklasad1
Copy link
Member

niklasad1 commented Mar 10, 2023

For context, after sp-runtime couldn't support WASM anymore subxt duplicates some types such as AccountId, Header, MultiAddress and similar which "may be annoying" to deal with for instance if one already has sp-runtime and sp-core in the dependency tree.

The intention behind these duplicated types are that the into/from impls exist when the #[cfg(feature = substrate-compat) is enabled

Adding @deuszx's suggestion in another PR:

It will be also useful if in substrate.rs we add this

#[cfg(feature = "substrate-compat")]
impl Config for SubstrateConfig {
    type Index = u32;
    type Hash = H256;
    type Hasher = BlakeTwo256;
    type AccountId = sp_runtime::AccountId32;
    type Address = sp_runtime::MultiAddress<Self::AccountId, u32>;
    type Header = SubstrateHeader<u32, Self::Hasher>;
    type Signature = sp_runtime::MultiSignature;
    type ExtrinsicParams = SubstrateExtrinsicParams<Self>;
}

and

impl PartialEq<sp_runtime::AccountId32> for AccountId32 {
      fn eq(&self, other: &sp_runtime::AccountId32) -> bool {
          AsRef::<[u8; 32]>::as_ref(other) == AsRef::<[u8; 32]>::as_ref(self)
      }
  }

impl PartialEq<AccountId32> for sp_runtime::AccountId32 {
    fn eq(&self, other: &AccountId32) -> bool {
        AsRef::<[u8; 32]>::as_ref(other) == AsRef::<[u8; 32]>::as_ref(self)
    }
}

in account_id.rs for better integration between sp_* and nonsp_* types.

Thus, I don't really get the why the SubstrateConfig and PartialEq is needed.

However, opening this issue such that @deuszx can elaborate with an example why ☝️ is needed for his usecase

@jsdw
Copy link
Collaborator

jsdw commented Mar 10, 2023

I'm generally keen to better understand why there is any requirement to use substrate types in subxt; in my mind, it's better to keep subxt entirely separate where possible, since it's typically used to communicate to substrate based nodes rather than being an actual part of a substrate node's code base.

Re SubstrateConfig, if you'd like a version that uses substrate types directly rather than the independent types, then that can I think be created by the user; do we need it in Subxt? How many would benefit from it? And if we do have a version in subxt, we should keep the existing version around too so that substrate-compat isn't required.

But going back to my original point, I'd love to understand better why this tight integration with Substrate crates is actually useful so that we can cater for those use cases better :)

@jsdw
Copy link
Collaborator

jsdw commented Mar 31, 2023

I think we should close this for now:

  • Keeping Substrate deps out of our SubstrateConfig enables WASM/lighter builds to use it.
  • Anybody can define a new Config implementation to use substrate types if they prefer (I don't think we need to also have this in Subxt itself at the moment).
  • The From impls on the subxt::utils::AccountId32 etc should make it trivial to convert between them and the Substrate types if the "substrate-compat" feature is enabled, so comparsing across types feels like extra code for no real win.

The upcoming Static type etc in #886 will also allow Substrate types to continue to be used in codegen if preferable, now that EncodeAsType and DecodeAsType are more widely used.

@jsdw jsdw closed this as completed Mar 31, 2023
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

2 participants