-
Notifications
You must be signed in to change notification settings - Fork 214
Add passphrase to ledger signing api #177
Add passphrase to ledger signing api #177
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good @Yasuke , maybe I could suggest a few potential refactorings/additional items:
- Perhaps introduce a new function,
signMessage
, signObservation'`, that use the default, and the un-ticked ones take the passphrase. This way we don't have random empty-strings in a few places, - Might not hurt to wrap up the PassPhrase in a newtype, so that it's a bit more clear it's purpose (and to allow later for nuanced handling, if we wish it),
- Maybe we could take this opportunity to have a few tests that verify this behaviour? (Not sure what tests we have already around verifying signatures and passphrases?)
Thanks for the work so far! :)
Any progress on this? |
Tried to build from PR branch - and it fails |
Hi @gege251 ; sorry to hear it is blocking you. As you can see, we are working on it, and it will be merged in due course :) |
8579a42
to
775f72a
Compare
Thank you for the feedback and review @silky! I have make Passphrase a distinct type, added prime functions that don't require a password, added some tests (and instances needed to make them work) for signing in both the ledger and oracle. |
I also generalized the ledger signing function to work with any ByteArrayAccess instead of just TxIds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work!
5f88734
to
286da68
Compare
Pending CI, required adjustments have been made, thanks @sjoerdvisscher for the review |
8a0cecf
to
c1c71dc
Compare
Rebased on current main |
This addresses IntersectMBO#168; Instead of having the passphrase for signing hardcoded to "Ledger.Crypto PassPhrase", allow the passphrase to be provided. This way we aren't prevented from using private keys created with tools like cardano-address and cardano-cli
c1c71dc
to
f975fa7
Compare
f975fa7
to
b5e64bd
Compare
hello, when this PR are planned to be merged? |
Thanks for the reminder! |
This addresses #168; Instead of having the passphrase for signing
hardcoded to "Ledger.Crypto PassPhrase", allow the passphrase to be
provided. This way we aren't prevented from using private keys
created with tools like cardano-address and cardano-cli