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

Implement config::Header and config::Hasher on the Substrate Header/Hasher types that we used to use in SubstrateConfig #835

Closed
yjhmelody opened this issue Feb 20, 2023 · 3 comments · Fixed by #934
Assignees
Labels
good first issue Small, well scoped, simple; good for newcomers

Comments

@yjhmelody
Copy link
Contributor

yjhmelody commented Feb 20, 2023

I found subxt define header and hasher by coping substrate types.
But it will be bad for our use case. We want to use the original header defined by substrate everywhere(for mock testings).

Sorry, I don't see any benefit in copying the code over.

@niklasad1
Copy link
Member

niklasad1 commented Feb 20, 2023

It's because these types comes from sp_runtime in substrate which doesn't compile for WASM and these dependencies are not part of the subxt repo anymore.

So that's the reason why we have duplicated the types.
Until paritytech/substrate#5547 is fixed this is the solution.

Perhaps, we could add some glue to use the types from substrate if that is possible. @jsdw ideas?

@jsdw
Copy link
Collaborator

jsdw commented Feb 20, 2023

The reasons for moving those traits in to Subxt are:

  1. sp_runtime stopped beingf WASM compatible; something we want from Subxt.
  2. Bringing in Substrate dependencies adds something like 200+ dependencies to the Subxt dependency graph.
  3. Having those traits locally has also allowed us to adapt and simplif ythem to be very specific to what Subxt actually needs to represent (which is only a subset of what Substrate cares about).

Two options here are:

  1. impl subxt's Header and Hash traits on key types in Substrate (if not too many), or
  2. Create your own SpRuntime<T>(T) and SpHeader<T>(T) types which impl subxt's Header and Hasher traits so long as their contents impl's substrates Header and Hasher traits. This would bridge the gap and allow you to use all valid substrate types as before within this small wrapper.

@jsdw
Copy link
Collaborator

jsdw commented Apr 6, 2023

Let's look at doing option 1 behind the substrate-compat feature flag, so that people can continue to use the substrate header and hasher types that we used to use. Hopefully we won't run into anything gnarly trying to do this :)

@jsdw jsdw added the good first issue Small, well scoped, simple; good for newcomers label Apr 6, 2023
@jsdw jsdw changed the title Should export Substrate Header and Hasher directly. Implement config::Header and config::Hasher on the Substrate Header/Hasher types that we used to use in SubstrateConfig Apr 6, 2023
@tadeohepperle tadeohepperle self-assigned this Apr 26, 2023
@jsdw jsdw closed this as completed in #934 Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Small, well scoped, simple; good for newcomers
Projects
None yet
4 participants