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 subxt_signer crate for native & WASM compatible signing #1016

Merged
merged 19 commits into from
Jun 20, 2023

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Jun 14, 2023

The subxt-signer crate replaces the need to use sp_core::sr25519::Pair for sr25519 signing of transactions (which is something we allow via the substrate-compat feature flag.

Reasons to use subxt_signer over sp_core::sr25519::Pair

  • It can be compiled to WASM, so can be used to sign things in Subxt WASM programs.
  • Fewer dependencies; we only bring in what we need for singing and not a bunch of other sp_core deps.
  • A relatively small and simple API for creating and using keypairs.

This crate can be used independently of subxt by disabling the subxt feature flag, in case you'd like to use it to sign things independently of Subxt.

I also rejigged the feature flags a bit with this in mind. Subxt now has these main feature flags:

  • "native" and "web": pick exactly one of these to write native or web compatible subxt programs. (if you're writing a lib, best to re-expose these to allow upstream users to pick one).
  • "jsonrpsee": use to pull in jsonrpsee. Works with either of "native" or "web"
  • "substrate-compat": use to pull in key sp_* crates, sign things via them, and add compat impls to make using them a little nicer. Doesn't work with "web".

@jsdw jsdw marked this pull request as ready for review June 16, 2023 10:00
@jsdw jsdw requested review from a team as code owners June 16, 2023 10:00
Copy link
Contributor

@tadeohepperle tadeohepperle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, good work! :)

//! need additional signing algorithms that `subxt_signer` doesn't support, and don't care about WASM
//! compatibility.
//!
//! Let's see how to use each of these approaches:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good explanation, avoids confusion for users!

all(feature = "web", feature = "native"),
not(any(feature = "web", feature = "native"))
))]
compile_error!("subxt: exactly one of the 'web' and 'native' features should be used.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a smart way to handle mutually exclusive features! Haven't seen that yet.

Copy link
Member

@niklasad1 niklasad1 Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the smart way is not to have mutually exclusive features :)

it's a PITA to deal with but we have no better option here.

Copy link
Member

@niklasad1 niklasad1 Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't really have to be addressed in this PR but would be great if we could have APIs similar to this to avoid having to deal with "mutually exclusive features`

but ofc it won't work for if something doesn't work for WASM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the smart way is not to have mutually exclusive features :)

Yeah, ideally the features would be purely additive in general!

Probably I need to keep thinking about feature flags here, and also which flags are enabled by default (it's annoying re doc examples etc if you don't have feature flags enabled that you want to use but might be surmountable).

When I did this I figured that a non additive "native or web" feature and then everything else being additive (in terms of APIs exposed at least) felt like the nicest way to handle it (otherwise you end up with the non-additiveness being pushed to other features anyway like jsonrpsee-web and jsonrpsee-native, or lightclient-web and lightclient-native not playing well together).

Def open to more thoughts on all of this though!

use super::*;
use std::str::FromStr;

once_static_cloned! {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Amazing PR 👍

/// assert_eq!("0xe5be9a5092b81bca64be81d212e7f2f9eba183bb7a90954f7b76361f6edb5c0a", suri.phrase.expose_secret());
/// assert!(suri.password.is_none());
/// ```
pub struct SecretUri {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could maybe state that this code is extracted from substrate or something

I'm a little scared of it to be honest but looks identical to SecretUri in substrate...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah; in the mod.rs of the crypto folder I say that everything in the crypto folder comes from sp_core::crypto :) (though with tiny tweaks or doc comment updates).

I didn't like bringing it in particularly either but it is just a straight copy of the logic! Probably I could test this in isolation better too to prove it aligns.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a note at the top of each file too to say it was taken from sp_core, and tweaked one of the tests to check more paths


# Enable this to pull in extra Substrate dependencies which make it possible to
# use the `sp_core::crypto::Pair` Signer implementation, as well as adding some
# `From` impls for types like `AccountId32`. Cannot be used with "web".
substrate-compat = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, features = ["jsonrpsee", "native"] should make it work but not bring in sp-core and all these deps, correct?

This will be great

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup exactly :)

The only reason I left substrate_compat in the default deps iirc was so that doc examples (well, one doc example iirc, and maybe a couple of tests or something) would work. Probably need one more go-over on these features to sort out niggles like that because would be nicer not to have it by default at all!

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very excited about this one, but I have a few questions :)

@jsdw jsdw merged commit b4eb406 into master Jun 20, 2023
@jsdw jsdw deleted the jsdw-subxt-signer branch June 20, 2023 10:32
@jsdw jsdw mentioned this pull request Jul 24, 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

Successfully merging this pull request may close these issues.

5 participants