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

JS keystore support #8096

Merged
merged 25 commits into from
Jun 28, 2021
Merged

JS keystore support #8096

merged 25 commits into from
Jun 28, 2021

Conversation

eelanagaraj
Copy link
Contributor

@eelanagaraj eelanagaraj commented Jun 14, 2021

Description

  • Implements most basic keystore functionality (encrypt, decrypt, change password) base class (+ file system for data persistence), based on ethereumjs-wallet
  • Keystore abstraction should make it fairly straightforward to add additional IO formats (browser storage, DB, however valora wants to do this...)
  • KeystoreWalletWrapper is a bare bones wrapper of a file keystore + the LocalWallet -- I expect this to change (+ maybe move into its own directory), but would wait until implementing attestation service/CLI-relevant changes before adding more here, in order to see what's necessary/makes sense

Tested

  • Unit tests
  • Tested instantiating, signing, sending txs via the KeystoreWalletWrapper + keystore files
  • Tested that keystore files created by geth can be copied into this keystore directory and decrypted with the same passphrase; same with the reverse (i.e. keystore files created + encrypted with the FileKeystore can be copied to the geth keystore directory, decrypted, and used to send txs)

Related issues

Backwards compatibility

New module, no changes to existing wallets or tools.

Remaining TODOs

  • [later PR] modify KeystoreWalletWrapper as makes sense with future use cases

Copy link
Contributor

@AlexBHarley AlexBHarley left a comment

Choose a reason for hiding this comment

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

Really nice progress!

@eelanagaraj eelanagaraj force-pushed the eelanagaraj/js-keystore branch 3 times, most recently from dde7727 to bb40112 Compare June 18, 2021 10:23
Copy link
Contributor

@AlexBHarley AlexBHarley left a comment

Choose a reason for hiding this comment

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

This looks great. I think the most powerful thing here is the fact that we can plug our existing wallet abstraction into this, meaning now files, databases, what have you can back our wallets.

I'm very curious to see how this ends up being used with our other projects like the CLI and attestation service.

@AlexBHarley AlexBHarley added the automerge Have PR merge automatically when checks pass label Jun 28, 2021
@eelanagaraj eelanagaraj added automerge Have PR merge automatically when checks pass and removed automerge Have PR merge automatically when checks pass labels Jun 28, 2021
@mergify mergify bot merged commit 28ee22d into master Jun 28, 2021
@eelanagaraj eelanagaraj deleted the eelanagaraj/js-keystore branch June 29, 2021 13:48
tkporter pushed a commit that referenced this pull request Jul 8, 2021
### Description
- Implements most basic keystore functionality (encrypt, decrypt, change password) base class (+ file system for data persistence), based on `ethereumjs-wallet`
- `Keystore` abstraction should make it fairly straightforward to add additional IO formats (browser storage, DB, however valora wants to do this...)
- `KeystoreWalletWrapper` is a bare bones wrapper of a file keystore + the `LocalWallet` -- I expect this to change (+ maybe move into its own directory), but would wait until implementing attestation service/CLI-relevant changes before adding more here, in order to see what's necessary/makes sense

### Tested

- Unit tests
- Tested instantiating, signing, sending txs via the `KeystoreWalletWrapper` + keystore files
- Tested that keystore files created by geth can be copied into this keystore directory and decrypted with the same passphrase; same with the reverse (i.e. keystore files created + encrypted with the `FileKeystore` can be copied to the geth keystore directory, decrypted, and used to send txs)

### Related issues

- #7063

- related autogenerated docs (split PR so as to not clutter this one) [old file structure PR](#8160), [docs PR new file structure](#8185)

### Backwards compatibility

New module, no changes to existing wallets or tools.

### Remaining TODOs
- [later PR] modify KeystoreWalletWrapper as makes sense with future use cases
mergify bot pushed a commit that referenced this pull request Jul 8, 2021
(Up to date with new file structure)

- Added autogenerated docs for keystore
- Added a walkthrough with code/usage examples in `using-js-keystores.md` (note: will not work until first version is published); [this is what it will look like on gitbook](https://eela.gitbook.io/forked-monorepo-docs-test/~/revisions/-MczCo4GM6pOBxl1YTZq/v/docs-test%2Feelanagaraj%2Fjs-keystore-docs-new/developer-guide/start/using-js-keystores)

Split so as to not clutter the first PR (#8096)

(apologies for the messy commits here -- wanted to stack them to make the differences cleaner initially but didn't want to wait to land docs with the rest of the PR!)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants