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

Support loading keys from Polkadot-JS accounts. #1661

Merged
merged 31 commits into from
Aug 23, 2024
Merged

Conversation

ethernomad
Copy link
Contributor

A common way to manage keys for Substrate accounts is to use the Polkadot JS browser extension. Accounts in the extension can be exported to an encrypted JSON file.

It would be very useful for subxt to support loading keys from these files. Both long running services and interactive programs built with subxt would be able to load accounts exported from the browser extension.

The JSON schema is described here:
https://github.com/polkadot-js/common/blob/37fa211fdb141d4f6eb32e8f377a4651ed2d9068/packages/keyring/src/types.ts#L67

The password is hashed with Scrypt and then the ciphertext is decrypted with XSalsa20Poly1305.

The implementation in the pull request is very rigid, inline with the JS implementation.

Here is the original issue: #1631

@ethernomad ethernomad requested a review from a team as a code owner June 29, 2024 10:32
@cla-bot-2021
Copy link

cla-bot-2021 bot commented Jun 29, 2024

User @ethernomad, please sign the CLA here.

Cargo.toml Outdated Show resolved Hide resolved
Co-authored-by: Niklas Adolfsson <[email protected]>
@@ -35,6 +40,9 @@ sr25519 = ["schnorrkel"]
ecdsa = ["secp256k1"]
unstable-eth = ["keccak-hash", "ecdsa", "secp256k1", "bip32"]

# Enable support for loading key pairs from polkadot-js json.
json = ["std", "subxt", "sr25519", "base64", "scrypt", "crypto_secretbox", "serde", "serde_json"]
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need std enabled for this json feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is necessary for the following command to succeed:

cargo test --no-default-features -F polkadot-js-compat

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know why this is necessary for that command to succeed? I had another look over and it's not a huge deal but would be nice not to need the std feature :)

signer/src/json.rs Outdated Show resolved Hide resolved
signer/src/json.rs Outdated Show resolved Hide resolved
signer/src/json.rs Outdated Show resolved Hide resolved
signer/src/json.rs Outdated Show resolved Hide resolved
signer/src/json.rs Outdated Show resolved Hide resolved
signer/src/json.rs Outdated Show resolved Hide resolved
signer/src/json.rs Outdated Show resolved Hide resolved
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.

great stuff, I mostly want to get rid of the unwraps and rename a few things.

overall nice PR

signer/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

I'm happy with this in principle; just some suggesitons on the overall API etc to add to Niklas's ones! Nice one!

signer/src/json.rs Outdated Show resolved Hide resolved
signer/Cargo.toml Show resolved Hide resolved
signer/src/sr25519.rs Outdated Show resolved Hide resolved
@ethernomad
Copy link
Contributor Author

User @ethernomad, please sign the CLA here.

I get a 404 when I try to sign the CLA - anyone know where I can report this?

Screenshot 2024-07-18 at 10-19-20 Parity Contributor Signatures

@ethernomad
Copy link
Contributor Author

This pull request is ready to review again.

Copy link
Collaborator

@jsdw jsdw 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! Couple of tiny nits but happy to see this merge now :)

Ideally we'd avoid needing the std feature for the polkadot-js-compat, but it's not really a big deal!

@ethernomad
Copy link
Contributor Author

The cla bot still crashes when I try to sign it.

@jsdw
Copy link
Collaborator

jsdw commented Aug 23, 2024

The cla bot still crashes when I try to sign it.

It seems to have been accepted anyways by the looks of things :)

signer/Cargo.toml Outdated Show resolved Hide resolved
signer/Cargo.toml Outdated Show resolved Hide resolved
@jsdw
Copy link
Collaborator

jsdw commented Aug 23, 2024

Modulo a couple of tiny dependency related nits I'm happy with this! Thankyou @ethernomad!

@jsdw jsdw merged commit c7ccc58 into paritytech:master Aug 23, 2024
13 checks passed
@mordamax
Copy link

@ethernomad re. CLA bot - you've already signed it, so all good

image

it's just bad user-experience in its logic, when you try to sign once-again it gives 404 O_o which was unexpected for me to see too :D

@jsdw jsdw mentioned this pull request Oct 24, 2024
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.

4 participants