Extract signer crates #3087

kevinheavey · 2024-10-06T11:16:10Z

Problem

solana_sdk::signer needs to be made available outside solana_sdk

Summary of Changes

Rip out the following crates (not putting them in one crate because each one has quite significant dependencies): solana-presigner, solana-seed-derivable, solana-seed-phrase, solana-signer
Make the keypair module optional in solana-signer. This didn't go in its own crate because it seems it's not possible to do so without breaking the Signers trait
replace serde_json usage in keypair.rs with a simple inline parser
remove itertools from the unique_signers function

This branches off #2295 so that needs to be merged first

joncinque · 2024-11-04T14:33:25Z

Make the keypair module optional in solana-signer. This didn't go in its own crate because it seems it's not possible to do so without breaking the Signers trait

I took a look at the PR and managed to extract solana-keypair without any issues on the Signers trait. I think it would be much better to extract Keypair too if it's possible -- do you mind if I push my changes to your branch so you can check it out? I also removed the keypair feature from solana-seed-phrase and just implemented it in solana-keypair.

I'm still looking through the changes, but it all looks good so far

kevinheavey · 2024-11-04T14:34:42Z

Go for it!

joncinque

Ok I've pushed my changes -- let me know what you think! Also tagging @febo for another set of eyes

joncinque · 2024-11-04T20:07:04Z

sdk/seed-derivable/src/lib.rs

+impl SeedDerivable for Keypair {
+    fn from_seed(seed: &[u8]) -> Result<Self, Box<dyn error::Error>> {
+        keypair_from_seed(seed)
+    }
+
+    fn from_seed_and_derivation_path(
+        seed: &[u8],
+        derivation_path: Option<DerivationPath>,
+    ) -> Result<Self, Box<dyn error::Error>> {
+        keypair_from_seed_and_derivation_path(seed, derivation_path)
+    }
+
+    fn from_seed_phrase_and_passphrase(
+        seed_phrase: &str,
+        passphrase: &str,
+    ) -> Result<Self, Box<dyn error::Error>> {
+        keypair_from_seed_phrase_and_passphrase(seed_phrase, passphrase)
+    }
+}


note: I typically prefer to have the trait implementation in the crate with the concrete type, but I think it makes sense here since there isn't too much else here, and it's the only crate with the ed25519-dalek-bip32 dependency

Hmmm, I found this a bit unexpected to be here. Is there a scenario where you would want only the trait and using this crate will incur you getting a ed25519-dalek-bip32 dependency?

If it does not make sense to move it to the keypair crate, perhaps we can put this behind a feature flag?

Hm, this is a good point. The only other impls of SeedDerivable exist in the zk-sdk and zk-token-sdk crates, and they explicitly just return an error for from_seed_and_derivation_path, which means they don't use the bip32 dependency at all.

We could implement the trait and extra functions under a feature flag like seed-derivable in solana-keypair. What do you think @kevinheavey ?

yeah feature flag sounds good to me, you wanna take it?

Yep! Done with 1720552 -- let me know how it looks

kevinheavey · 2024-11-05T08:45:31Z

lgtm

kevinheavey · 2024-11-05T09:30:49Z

@yihau I've sent anza-team invites for the following crates:

solana-keypair
solana-presigner
solana-seed-derivable
solana-seed-phrase
solana-signer

yihau · 2024-11-05T10:55:50Z

✅

sdk/src/signer/keypair.rs

sdk/src/signer/mod.rs

febo

Just a couple of comments. Also, some of the crates have the "` ... `" formatting on the description field in the Cargo.toml file – wodering whether that is going to render properly or should be removed.

kevinheavey · 2024-11-05T14:39:11Z

Just a couple of comments. Also, some of the crate have the "..." formatting on the description field in the Cargo.toml file – wodering whether that is going to render properly or should be removed.

Confirming that crates.io does not give them proper rendering in the description field https://crates.io/crates/backtick-description-example

febo

Looks great!

mergify bot added the need:merge-assist label Oct 6, 2024

kevinheavey mentioned this pull request Oct 6, 2024

Bump ed25519-dalek to 2.1.1, ed25519-dalek-bip32 to 0.3.0 #3088

Draft

kevinheavey force-pushed the extract-signer-crates branch from 2d27cdb to d090fb0 Compare October 7, 2024 08:15

This was referenced Oct 7, 2024

Liberate the solana-client family from solana-sdk #3027

Closed

Extract quic-definitions crate #3119

Merged

kevinheavey force-pushed the extract-signer-crates branch 4 times, most recently from fc2e791 to eccfece Compare October 16, 2024 10:50

kevinheavey force-pushed the extract-signer-crates branch 2 times, most recently from c1dd543 to 04c1682 Compare October 22, 2024 13:36

kevinheavey marked this pull request as ready for review October 22, 2024 18:36

kevinheavey force-pushed the extract-signer-crates branch 3 times, most recently from 647adc5 to 1e002af Compare October 29, 2024 09:05

kevinheavey requested a review from joncinque October 29, 2024 09:50

kevinheavey force-pushed the extract-signer-crates branch 2 times, most recently from 52c3b5a to e616a0f Compare November 1, 2024 08:29

kevinheavey added 7 commits November 2, 2024 19:57

extract signer crate

cfa9391

fmt

9c65ee0

fix bs58 std usage

c8fbe04

extract seed-derivable crate

eefbb87

extract seed-phrase crate

50224e5

fix copy-pasted doc

d63eb33

extract presigner crate

a8b6c38

kevinheavey added 8 commits November 2, 2024 19:57

remove itertools from solana-signer

fd8a47a

move wasm impl to signer crate

144ec3c

add doc_auto_cfg like in anza-xyz#3121

4252bb7

post-rebase fix

017783b

another post-rebase fix

bc8fe70

fmt

5f3b787

make keypair functionality of solana-seed-phrase optional

c5cbeb6

fix feature-gated imports

f531541

kevinheavey force-pushed the extract-signer-crates branch from a96f16f to f531541 Compare November 2, 2024 15:57

joncinque added 4 commits November 4, 2024 15:36

Extract solana-keypair, move other things around

952c997

Cleanup things that I missed

5d19c05

Remove space

d6c275f

Add deprecated notice

a8ef164

joncinque reviewed Nov 4, 2024

View reviewed changes

joncinque requested a review from febo November 4, 2024 22:52

febo reviewed Nov 5, 2024

View reviewed changes

sdk/src/signer/keypair.rs Outdated Show resolved Hide resolved

febo reviewed Nov 5, 2024

View reviewed changes

sdk/src/signer/mod.rs Outdated Show resolved Hide resolved

febo reviewed Nov 5, 2024

View reviewed changes

joncinque added 2 commits November 6, 2024 02:28

Add deprecation notices

06426cc

Refactor SeedDerivable trait implementation

1720552

joncinque requested a review from febo November 6, 2024 15:46

febo approved these changes Nov 6, 2024

View reviewed changes

kevinheavey added the automerge automerge Merge this Pull Request automatically once CI passes label Nov 6, 2024

mergify bot merged commit e752b38 into anza-xyz:master Nov 6, 2024
53 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract signer crates #3087

Extract signer crates #3087

kevinheavey commented Oct 6, 2024 •

edited

Loading

joncinque commented Nov 4, 2024

kevinheavey commented Nov 4, 2024

joncinque left a comment

joncinque Nov 4, 2024

febo Nov 5, 2024

febo Nov 5, 2024

joncinque Nov 6, 2024

kevinheavey Nov 6, 2024

joncinque Nov 6, 2024

kevinheavey Nov 6, 2024

kevinheavey commented Nov 5, 2024

kevinheavey commented Nov 5, 2024

yihau commented Nov 5, 2024

febo left a comment •

edited

Loading

kevinheavey commented Nov 5, 2024

febo left a comment

Extract signer crates #3087

Extract signer crates #3087

Conversation

kevinheavey commented Oct 6, 2024 • edited Loading

Problem

Summary of Changes

joncinque commented Nov 4, 2024

kevinheavey commented Nov 4, 2024

joncinque left a comment

Choose a reason for hiding this comment

joncinque Nov 4, 2024

Choose a reason for hiding this comment

febo Nov 5, 2024

Choose a reason for hiding this comment

febo Nov 5, 2024

Choose a reason for hiding this comment

joncinque Nov 6, 2024

Choose a reason for hiding this comment

kevinheavey Nov 6, 2024

Choose a reason for hiding this comment

joncinque Nov 6, 2024

Choose a reason for hiding this comment

kevinheavey Nov 6, 2024

Choose a reason for hiding this comment

kevinheavey commented Nov 5, 2024

kevinheavey commented Nov 5, 2024

yihau commented Nov 5, 2024

febo left a comment • edited Loading

Choose a reason for hiding this comment

kevinheavey commented Nov 5, 2024

febo left a comment

Choose a reason for hiding this comment

kevinheavey commented Oct 6, 2024 •

edited

Loading

febo left a comment •

edited

Loading