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

Breaking cleanup upgrade: 3.0.0 #134

Closed
coder5876 opened this issue Mar 3, 2017 · 7 comments
Closed

Breaking cleanup upgrade: 3.0.0 #134

coder5876 opened this issue Mar 3, 2017 · 7 comments

Comments

@coder5876
Copy link
Contributor

Returning from a bit of an absence from Lightwallet I'm going to do a long overdue cleanup/refactoring release. Things that I intend to address are:

Cruft/unused stuff

These things I don't think are used very much, makes the code bloated and complex and I intend on removing them:

  • Encryption keys in the keystore
  • Support for multiple HD paths (makes the function signatures and logic unnecessarily complex)
  • Deprecated constructor (will use exclusively the new createVault())
  • Default salt, HD Path & duplicated key derivation functions

Updated hex formats

When lightwallet was created it was unclear if addresses should have a 0x prefix or not, so we opted to use without 0x. Now it's clear that most applications use 0x and this is even required in some places so we'll be switching to using 0x exclusively.

Feedback

If you have any other changes that you would like to see, please let us know here 😃

@cpetty-Novetta
Copy link

cpetty-Novetta commented Mar 7, 2017

Big plus for the hex format issue.

@jaycarey
Copy link

Does 'Encryption keys in the keystore' include removing the multiEncryptString / multiDecryptString functions? We use those a fair bit in our app.

@slothbag
Copy link

This is great, we use this library and think it is fantastic. Glad to see it is still being supported.

We rely on serialize/deserialize the keystore to and from localStorage, and also the client side tx signing is super useful.

@coder5876
Copy link
Contributor Author

@jaycarey Good to know, thanks! The plan was to remove the explicit handling of encryption keys in the keystore itself. What I'll probably end up doing is to modify the encryption module to use a raw private key. That way you can still use it by exporting private keys from the keystore.

@tzapu
Copy link

tzapu commented Aug 16, 2017

just wanted to add that we rely on serializing/deserializing the keystore to and from a db, seems like a pretty big breaking change if that were to be removed

@coder5876
Copy link
Contributor Author

I have found the time to finish this up, will try to put up a pull request and merge in this today. Note that you'll need to update the following:

  • Encryption keys are no longer indexed by public keys. If you wish to use encryption keys you will need to refer to them by the ethereum address of the corresponding private key
  • The createVault function is the only way to create new vaults right now, the old constructor is deprecated
  • Addresses and raw transaction hex values are now prepended by 0x, so you'll need to make sure your app can handle that
  • There is only one hdPath in the keystore. If you need more than one you'll need to create more than one vault each with its own hdPath.
  • You need to specifically choose an HD path, there is no default anymore.

@tzapu Serialization and deserialization is not removed.

@coder5876
Copy link
Contributor Author

This is now merged: #166

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

No branches or pull requests

5 participants