Make sp_core and sp_runtime dependencies optional, and bump to latest #760

jsdw · 2023-01-06T11:10:37Z

This PR makes sp_core and sp_runtime dependencies optional (opted-in by default). The reasons for doing this are:

Supporting compile-to-WASM is simpler without these dependencies (currently they have introduced a hard dependency on wasmtime (under the "std" feature) which cannot be compiled to WASM, but disabling the "std" feature hits a bunch of assumptions about no_std which breaks other things.
A smaller dependency graph: for the most part we only rely on certain types from these packages to describe the shape of some responses and such, so taking these types into Subxt allows us to not pull in a load of other stuff that we don't need.
Reducing the need to keep up to date with Substrate packlage versions: in the past, Substrate dependencies like parity-util-mem have prevented Subxt from being used in conjunction with Substrate packages unless the two used the same versions. We aren't there yet, but I'd like to get to the point where Subxt will happily co-exist with a wide range of Substrate versions, as long as the actual types sent back and forth via the RPC APIs remain compatible.

At the time of writing, the signer stuff still lives in Substrate and so we still depend on sp_core and sp_runtime to provide a PairSigner which allows transactions to be signed (which won't be available for now in the WASM builds as a result). This isn't ideal, but porting that logic over seemed more involved offhand and so I opted to avoid it in this first pass. Providing a simple set of signing primitives feels like a good next step though, and would allow us to completely shed our dependencies on sp_core and sp_runtime (and everything that they bring in).

…sed errors

…g examples

… (for decoding later)

jsdw · 2023-01-06T16:40:59Z

Ok so there is a feature flag, "substrate-compat", which when enabled will bring in sp_core and sp_runtime in order that we can provide a PairSigner to help sign transactions and be compatible with things like AccountKeyring and such from Substrate. The feature flag is enabled by default to try and minimise breakage (perhaps we'll want to make it opt-in in future).

The default SubstrateConfig and PolkadotConfig use our own versions of AccountId32 etc. The PairSigner will work so long as we can convert from the substrate AccountId32 etc into the ones we configure (this is true for our new built-in copies of AccountId32 etc).

Removing Substrate deps seems to shrink the total deps quite a lot:

Using a command like cargo tree --no-default-features -e no-dev,no-build | grep -v '*' | grep -v 'subxt v0.25.0' | wc -l:

Deps with no default features: ~237
Deps with sp_core/sp_runtime included: ~454

A future piece of work could be to provide our own Substrate/Polkadot compatible Signer implementation, so that people could more permanently opt-out of the substrate crates.

jsdw · 2023-01-06T16:48:33Z

subxt/src/tx/signer.rs

+    impl<T, Pair> PairSigner<T, Pair>
+    where
+        T: Config,
+        T::AccountId: From<SpAccountId32>,


You create a PairSigner by passing a Pair implementation whose signature type can provide an sp_runtime::MultiSignature, where that has a Signer type that can provide back an sp_runtime::AccountId32. You must be able to convert sp_runtime::AccountId32 into our configured account type, T::AccountId.

Below you can see that you also need to be able to convert a Pair::Signatureinto an sp_runtime::MultiSignature, and take that into our configured T::Signature type.

In other words, we use the substrate types as a go-between. I'm hoping that this won't limit the use cases for this type, but I'm not sure offhand so would love to hear any feedback or thoughts here!

I'll see how hard it is to impl the substrate signer stuff on MultiSignature etc and then maybe we can avoid any intermediate types.

I ended up simplifying them a bit and adding a couple of extra From impls to hopefully make the different signature types in Substrate "just work".

examples/Cargo.toml

niklasad1 · 2023-01-09T09:51:16Z

Reducing the need to keep up to date with Substrate packlage versions: in the past, Substrate dependencies like parity-util-mem have prevented Subxt from being used in conjunction with Substrate packages unless the two used the same versions. We aren't there yet, but I'd like to get to the point where Subxt will happily co-exist with a wide range of Substrate versions, as long as the actual types sent back and forth via the RPC APIs remain compatible

I don't understand this comment, you bumped the substrate dependencies so parity-util-mem isn't part of the dependency tree anymore. Are you referring to "old substrate dependencies" or what do you mean?

niklasad1 · 2023-01-09T09:53:08Z

subxt/src/config/mod.rs

@@ -34,7 +35,6 @@ pub trait Config: 'static {
        + Member
        + serde::de::DeserializeOwned
        + Default
-        + AtLeast32Bit


why removed? I guess that trait comes from substrate

Yeah; I basically removed various bits that we didn't actually make use of or care about on the Subxt side. Like, if somebody configures the type wrong (whether it's because it's not 32bit or simply just not what the chain is configured with) it won't work either way, so I didn't see the point of the extra check anyway

niklasad1 · 2023-01-09T09:56:26Z

subxt/src/rpc/rpc.rs

+#[derive(Clone, PartialEq, Eq, Debug)]
+pub enum DryRunError {
+    /// The extrinsic will not be included in the block
+    TransactionValidityError,


it's a bummer not to get back the reason why the transaction was rejected... but perhaps a PITA to get

Yeah I agree; what I wasn't sure about when I did this was whether anybody is currently using the full details. The reason I didn't include them all right here was just because it involved bringing a load more types over, including DispatchError, which we already have to handle when submitting transactions and we have special logic to decode it from the metadata, so having a "hard copy" felt wrong.

But maybe we do need more details to be returned here?

subxt/src/utils/multi_address.rs

niklasad1 · 2023-01-09T10:03:40Z

subxt/src/utils/multi_address.rs

+};
+
+/// A multi-format address wrapper for on-chain accounts. This is a simplified version ob Substrate's
+/// `sp_runtime::MultiAddress`. To obtain more functionality, convert this into that type.


perhaps explain than #[cfg(feature = "substrate-compat")] is required for converting it sp_runtime::MultiAddress

When I wrote this I was assuming that people could manually convert it if they wanted (because it has the same structure so is easy to see how), and then I added some From things behind substrate-compat becasue I needed them in the PairSigner impl to keep things simple!

Def worth mentioning substrate-compat here now though yup :)

niklasad1

nice work, I think this is a fair trade-off but a bit concerned that we have maintain these duplicated types but it should all right.

When it comes to WASM substrate deps is a no-go which is bummer but for WASM it might be possible to use a third-party signer or something.

Unless you are planning to support the signer stuff for WASM as well.

jsdw · 2023-01-09T10:37:49Z

Reducing the need to keep up to date with Substrate packlage versions: in the past, Substrate dependencies like parity-util-mem have prevented Subxt from being used in conjunction with Substrate packages unless the two used the same versions. We aren't there yet, but I'd like to get to the point where Subxt will happily co-exist with a wide range of Substrate versions, as long as the actual types sent back and forth via the RPC APIs remain compatible

I don't understand this comment, you bumped the substrate dependencies so parity-util-mem isn't part of the dependency tree anymore. Are you referring to "old substrate dependencies" or what do you mean?

Ah yeah; I just meant that in the past we had issues like parity-util-mem, and avoiding substrate deps avoids potential future issues like that from cropping up too :)

jsdw · 2023-01-09T11:02:07Z

Unless you are planning to support the signer stuff for WASM as well.

I think it would be nice to be able to provide some sort of built-in Substrate/Polkadot signing facility without needing the "substrate-compat" feature, if anything just so that all of our exampels and tests can work without said feature/deps, but also just to round things off so that those deps truly are optional and not necessary.

ascjones

LGTM!

Would be nice to have some basic regression tests for the duplicated types so we can catch any changes as early as possible.

ascjones · 2023-01-09T11:07:58Z

subxt/src/utils/mod.rs

@@ -4,7 +4,10 @@

 //! Miscellaneous utility helpers.

+pub mod account_id;


Not a big fan of these being grouped together in a module called utils, I think a more precise name would be better to indicate that these are primitive types from sp (substrate primitives) crates. Could just be primitives?

Yeah, I was considering where to put them too. Really, they are sortof just "optional types that you can use in Config if you like for easy Substrate/Polkadot compat", so they aren't explicitly a part of any API interface etc.

Perhaps they should be exposed in config::substrate for that reason?

For now I put them in utils I guess because codegen makes use of "utils" things in general, and they are just helpers to configure a substrate/polkkadot like config. I'm still a bit torn about the above idea also.

Yeah it's tricky, I think it's okay for misc stuff to go in helpers/utils namespace but in this case there is a clear category. Maybe config::types?

If it helps, they are actually already exposed as config::substrate::AccountId32 etc since they are used for substrate config, so they could also just be exposed from both places? And since they are used in codegen (in a way that I suppose isn't so directly related to the config?), maybe it's justifiable having them exposed in utils also?

But yeah if you prefer them living entirely in config::types or similar I'm happy with that too! I'm just sortof picturing people needing to import them sometimes for transactions, and importing form config for a type used in a transaction feels a bit weirder maybe

codegen/src/api/mod.rs

metadata/Cargo.toml

subxt/Cargo.toml

subxt/src/config/mod.rs

ascjones · 2023-01-09T11:27:29Z

subxt/src/config/substrate.rs

+    RuntimeEnvironmentUpdated = 8u32,
+}
+impl Encode for DigestItem {
+    fn encode(&self) -> Vec<u8> {


Maybe we could add some tests which perform encoding roundtrips of locally defined types with the substrate equivalents (as dev-dependencies). That way we will catch breaking changes to the custom encoding of these types.

I was actually just mulling over adding such tests now, so yup I'm def on board with this!

ascjones · 2023-01-09T11:32:46Z

subxt/src/utils/account_id.rs

+    // This isn't strictly needed, but to give our AccountId32 a little more usefulness, we also
+    // implement the logic needed to decode an AccountId32 from an SS58 encoded string. This is exposed
+    // via a `FromStr` impl.
+    fn from_ss58check(s: &str) -> Result<Self, FromSs58Error> {


This could do with at least one basic unit test to check it works.

lexnv · 2023-01-09T11:58:29Z

subxt/src/config/extrinsic_params.rs

@@ -2,19 +2,26 @@
 // This file is dual-licensed as Apache-2.0 or GPL-3.0.
 // see LICENSE for license details.

+//! This module contains a trait which controls the parameters that can must


Suggested change

//! This module contains a trait which controls the parameters that can must

//! This module contains a trait which controls the parameters that must

Xanewok

I agree with @ascjones that grouping sp-* related things under a dedicated module rather than utils would be great and it'd be also great to have regression tests using Substrate crates as part of dev-dependencies.

Otherwise, looks good - great job!

jsdw · 2023-01-10T11:28:11Z

I've added some tests to cover types (but haven't covered everything; there were various existing types that I was reluctant to pull in more substrate crates to write such tests for, though ultimately that may be exactly what we want to do here!

AccountId/MultiAddress/MultiSignature still live in utils, but are exported from config::substrate too since they are used there. I do agree that it'd be nice to organise the types a little better.

I originally thought of keepign all of the "sp" types together, but decided that it would be better to group them not according to whether they are defined in substrate crates, but according to how they are used in subxt (ie some types are duplicated because they are part of the RPC API, some types duplicated because they have some useful functionality that we use internally, some types because they are used in config etc. Ultimately we are aiming for "interface compatibility" anyway, and it just happens that duplicating some types helps us achieve that!

Anyway, I hope that for now this is a good place to start! The next step is to look into adding a "native" signer API so we don't need substrate crates at all for basic transaction submitting workflows. Even if such an API exists just as code to show others how to go about signing things, I'd be happy I think. I'll write an issue for this.

jsdw added 15 commits January 4, 2023 11:43

begin porting over traits; remove Config use of Hash

948f88d

port over the Header bits that we need

b8912be

sp_core_hashing where possible, move Verify to PairSigner, remove unu…

c1f0779

…sed errors

tidy up Config things and move related bits into one place

e7d23b2

fix codegen

5d4f168

copy Era over

801fbe6

move AccountId, Address, Signer to Signer trait and a pass over fixin…

1a0f4e8

…g examples

impl MultiAddress, MultiSignature, AccountId32 and add back to Config…

7df5ae1

… (for decoding later)

Copy over StorageKey, StorageData, StorageChangeSet

8eac9f2

subxt core compiling with no sp_core or sp_runtime

7b91522

Get examples compiling

22d2259

pass over fixing tests

7c283e7

cargo fmt

5776669

clippy tweaks and update polkadot.rs

cf7561e

fix codegen docs

7192cec

jsdw mentioned this pull request Jan 6, 2023

subxt depends on unmaintained component [RUSTSEC-2022-0061] #759

Closed

jsdw added 6 commits January 6, 2023 15:26

port over special DigestItem encoding/decoding

2c9838e

clippy and doc fixes

8d6685e

cargo fmt and example fix

ec53f2d

more cargo fmt-ing...

a6b451a

substrate-extra to substrate-compat

de1e7d7

cargo.toml comments

093f129

jsdw marked this pull request as ready for review January 6, 2023 16:33

jsdw changed the title ~~Make sp_core and sp_runtime dependencies optional~~ Make sp_core and sp_runtime dependencies optional, and bump to latest Jan 6, 2023

jsdw mentioned this pull request Jan 6, 2023

cargo: Update sp-* crates from v7.0.0 to v9.0.0 #748

Closed

jsdw commented Jan 6, 2023

View reviewed changes

simplify PairSigner trait bounds

84cf950

gilescope reviewed Jan 6, 2023

View reviewed changes

examples/Cargo.toml Outdated Show resolved Hide resolved

niklasad1 reviewed Jan 9, 2023

View reviewed changes

subxt/src/utils/multi_address.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Jan 9, 2023

View reviewed changes

niklasad1 approved these changes Jan 9, 2023

View reviewed changes

move RPC types to a separate file

6696003

fix docs

799abbd

ascjones approved these changes Jan 9, 2023

View reviewed changes

lexnv reviewed Jan 9, 2023

View reviewed changes

lexnv approved these changes Jan 9, 2023

View reviewed changes

Xanewok approved these changes Jan 9, 2023

View reviewed changes

Add some tests for things and other PR feedback

fa88846

lexnv mentioned this pull request Jan 9, 2023

Add standalone support for RPC Spec V2 chainHead #762

Closed

jsdw added 3 commits January 10, 2023 10:50

Merge branch 'master' into jsdw-remove-substrate-deps

7359b80

bump to latest sp deps

fa7913b

avoid needing substrate-compat feature in a test

df703ee

jsdw merged commit b316301 into master Jan 10, 2023

jsdw deleted the jsdw-remove-substrate-deps branch January 10, 2023 12:02

Xanewok mentioned this pull request Jan 11, 2023

Allow for remapping type parameters in type substitutions #735

Merged

This was referenced Jan 18, 2023

Upgrade to polkadot-v0.9.36 Phala-Network/phala-blockchain#1123

Closed

Upgrade to Polkadot v0.9.37 Phala-Network/phala-blockchain#1129

Merged

echevrier mentioned this pull request Apr 20, 2023

Clean up trait bounds scs/substrate-api-client#541

Merged

haerdib mentioned this pull request Apr 26, 2023

Investigate type usage of sp_runtime and sp_core scs/substrate-api-client#553

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sp_core and sp_runtime dependencies optional, and bump to latest #760

Make sp_core and sp_runtime dependencies optional, and bump to latest #760

jsdw commented Jan 6, 2023

jsdw commented Jan 6, 2023 •

edited

Loading

jsdw Jan 6, 2023

jsdw Jan 6, 2023

jsdw Jan 9, 2023

niklasad1 commented Jan 9, 2023

niklasad1 Jan 9, 2023

jsdw Jan 9, 2023

niklasad1 Jan 9, 2023

jsdw Jan 9, 2023

niklasad1 Jan 9, 2023

jsdw Jan 9, 2023

niklasad1 left a comment

jsdw commented Jan 9, 2023

jsdw commented Jan 9, 2023

ascjones left a comment

ascjones Jan 9, 2023

jsdw Jan 9, 2023 •

edited

Loading

ascjones Jan 9, 2023

jsdw Jan 9, 2023 •

edited

Loading

ascjones Jan 9, 2023

jsdw Jan 9, 2023 •

edited

Loading

ascjones Jan 9, 2023

lexnv Jan 9, 2023

Xanewok left a comment

jsdw commented Jan 10, 2023

		@@ -4,7 +4,10 @@

		//! Miscellaneous utility helpers.

		pub mod account_id;

	//! This module contains a trait which controls the parameters that can must
	//! This module contains a trait which controls the parameters that must

Make sp_core and sp_runtime dependencies optional, and bump to latest #760

Make sp_core and sp_runtime dependencies optional, and bump to latest #760

Conversation

jsdw commented Jan 6, 2023

jsdw commented Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 commented Jan 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

jsdw commented Jan 9, 2023

jsdw commented Jan 9, 2023

ascjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xanewok left a comment

Choose a reason for hiding this comment

jsdw commented Jan 10, 2023

jsdw commented Jan 6, 2023 •

edited

Loading

jsdw Jan 9, 2023 •

edited

Loading

jsdw Jan 9, 2023 •

edited

Loading

jsdw Jan 9, 2023 •

edited

Loading