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

Save multiple keys in one call (with single final call to user save f… #9

Merged
merged 4 commits into from Apr 13, 2019

Conversation

Projects
None yet
2 participants
@PaulBernier
Copy link
Contributor

commented Mar 31, 2019

…unction)

Hello, I propose the introduction of a saveKeys function that allow to save multiple keys while triggering a single final call to the user defined save function. I was needing that new function because I have many cases when I want to update multiple keys at once, and my save function is writing to a file, which can occasionally cause problems:

  1. Performance: writing on disk can be expensive, and I only really need eventual consistency so it is fine to bulk save all those changes
  2. Inconsistency due to concurrent writing on file

Let me know what you think!

@@ -64,6 +65,20 @@ export function createStore<PrivateKeyData, PublicKeyData = {}> (
const { iterations = 10000 } = options
let keysData = initialKeys

function _saveKey (keyID: string, password: string, privateData: PrivateKeyData, publicData: PublicKeyData | {} = {}): void {

This comment has been minimized.

Copy link
@andywer

andywer Apr 1, 2019

Owner

Let's drop the leading _. Will be called the same as the store method then, but that's fine, since it also does the very same thing :)

@andywer

This comment has been minimized.

Copy link
Owner

commented Apr 1, 2019

Hey Paul!
Thanks for your contribution :) Yeah, the change makes sense. I commented one thing in the changed files; would be great if you can do that.

Will merge it and update the dev dependencies tomorrow.

@PaulBernier

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

For sure, code updated.

@andywer

This comment has been minimized.

Copy link
Owner

commented Apr 2, 2019

Ahh, just realized one thing that slipped my attention yesterday: The password should also be set per key for that new method.

The reasoning is this: If you have a key store that supports a dedicated password per key, you are free to give those keys different passwords or you can also just use the same password for all of them. It works for both use cases (one password for all + one per key).

That's why I want to keep the API strictly per-key-password, so the library user can decide for themself if they want a global password or different ones.

Feel free to challenge that mindset, though :)

@PaulBernier

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Agreed. Once again the initial design stemmed from my particular use case where the whole store is encrypted with a single password.
I changed the interface.

@PaulBernier

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Ah good catch I missed re-generating the type file. Is there anything missing at this point to merge this PR?

@andywer

This comment has been minimized.

Copy link
Owner

commented Apr 11, 2019

Sorry for the slow response times right now, Paul, but I'm so super busy right now 😐
Will have a final look at it and release those days!

Btw, did you look into node-tar? Can't remember if you are working on a web-based wallet or app, but for the latter case that looks pretty cool :)

@PaulBernier

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Great thank you.
And thanks for sharing that relevant link, I am also doing an Electron app so it's definitively a nice feature I could add!

@andywer andywer merged commit e225026 into andywer:master Apr 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@andywer

This comment has been minimized.

Copy link
Owner

commented Apr 13, 2019

Published as v1.1.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.