Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: re-export sp-core and sp-runtime #853

Merged
merged 1 commit into from
Mar 10, 2023

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Mar 9, 2023

When the substrate-compat is enabled let's re-export sp-core and sp-runtime again for compatibility reasons.

Further, it easy to pick the wrong version of sp-core and sp-runtime so let's make the usage a bit smoother.

Close #844

@deuszx
Copy link

deuszx commented Mar 9, 2023

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.

@niklasad1
Copy link
Member Author

@deuszx

Can you open another issue regarding that and explain why that is needed?
I want to keep it out this PR :)

@deuszx
Copy link

deuszx commented Mar 9, 2023

@niklasad1 But isn't it related? With substrate-impl we allow people to use AccountId32 as defined in sp_core (and other types) but when they try to use SubstrateConfig, the trait Config will give them Config::AccountId = subxt::utils::AccountId32 instead of sp_core::crypto::AccountId32. That's a compilation error and requires to manually map the types (from sp_core to subxt::utils and sometimes back).

If they choose to use sp_core::* and sp_runtime::* types, then that should be consistent across all of the API.

As for the PartialEq implementations: since the 0.26.0 release, all subxt-generated code uses subxt::utils::* types (like subxt::utils::AccountId32) even with substrate-impl feature enabled. This means that clients building higher-level API on top of subxt, will be using subxt::ext::sp_core::crypto::AccountId32 in their API (for example) but the low-level subxt-generated code will be running on subxt::utils::* types.

@niklasad1
Copy link
Member Author

It's is related but not what this PR is trying to tackle, thus please open another issue.

Personally, I don't like the suggested PartialEq/Eq impls we already gotfrom/into impls between subxt::AccoundId32/sp_runtime::AccountId32 so better to discuss it alone in another issue but I get your point roughly.

@niklasad1 niklasad1 merged commit d4545de into master Mar 10, 2023
@niklasad1 niklasad1 deleted the na-rexport-sp-core-sp-runtime branch March 10, 2023 09:25
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PairSigner fails to be created from sr25519::Pair
4 participants