Allows creating external keystore with the Worker #1569
Conversation
0xnullifier
force-pushed
the
feat/external-keystore-worker
branch
from
d77bcb7 to
3398498
Compare
0xnullifier
force-pushed
the
feat/external-keystore-worker
branch
from
3398498 to
65d9cee
Compare
WiktorStarczewski
left a comment
WiktorStarczewski
left a comment
There was a problem hiding this comment.
LGTM
Full disclosure, I did contribute a couple of snippets for this, so I'm biased though. (hence I shouldn't be the approver)
| insertKeyCb?: ( | ||
| pubKey: Uint8Array, | ||
| secretKey: Uint8Array |
There was a problem hiding this comment.
I'm aware this is already in the codebase. But It does make me raise an eyebrow the fact that we pass the secret key, I'm not saying this is wrong per se, but is this the proper way to handle it? Are there any chances of accidentaly revealing the SK given that it is a parameter?
There was a problem hiding this comment.
Anything can be revealed, but I think that the fact that its a parameter doesn't increase the exposure on its own. The only way this could be sniffed was if there was a console.log or a throw possible (generating stack trace), and that's down to the caller (callback implementation). So it is a risk in the sense that the caller uses a method exposing the secret key, therefore the secret is exposed, so care should be taken - but not because of the way we expose it. Does this make sense? Do you think it needs stronger documentation? Or would you advocate for a rethink whether not to pass it like this at all?
There was a problem hiding this comment.
Yes, it does make sense, and I think the callback explains itself (because of what you said about stronger docs).
That being said, I'm not really sure if there's really another way to do it though, plus the SK import should happen only once.
| this.signCb, | ||
| !!this.getKeyCb, | ||
| !!this.insertKeyCb, | ||
| !!this.signCb, |
There was a problem hiding this comment.
Why was this change needed?
There was a problem hiding this comment.
To signify that we are not actually going to call those callbacks, this is just a boolean condition on whether to use the callbacks defined at the point of client instantiation. Otherwise it felt misleading.
There was a problem hiding this comment.
Also adding to that we can't pass functions to the worker as they are not structurally cloneable thus we send boolean indicating if we have the functions on the main thread and use proxies instead
fkrause98
left a comment
fkrause98
left a comment
There was a problem hiding this comment.
Looks good, left some comments.
|
@0xnullifier can you resolve conflicts on this PR? I believe this should be ready to merge |
0xnullifier
force-pushed
the
feat/external-keystore-worker
branch
2 times, most recently
from
a61d0f6 to
ba647a5
Compare
0xnullifier
force-pushed
the
feat/external-keystore-worker
branch
from
ba647a5 to
df2a279
Compare
4 participants
No description provided.