Skip to content
This repository has been archived by the owner on Dec 2, 2024. It is now read-only.

Add passphrase to ledger signing api #177

Merged
merged 5 commits into from
Dec 16, 2021

Conversation

Yasuke
Copy link
Contributor

@Yasuke Yasuke commented Dec 7, 2021

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

@Yasuke Yasuke requested a review from sjoerdvisscher December 7, 2021 21:07
Copy link
Contributor

@silky silky left a 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:

  1. 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,
  2. 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),
  3. 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! :)

@volodyad
Copy link

volodyad commented Dec 9, 2021

Any progress on this?

@volodyad
Copy link

volodyad commented Dec 9, 2021

Tried to build from PR branch - and it fails
src/Cardano/Wallet/Mock/Handlers.hs:128:23: error:
• Couldn't match expected type ‘Eff effs MockWallet’
with actual type ‘BS.ByteString -> Eff effs0 MockWallet’
• Probable cause: ‘newWallet’ is applied to too few arguments

@szg251
Copy link
Contributor

szg251 commented Dec 9, 2021

@silky @Yasuke sorry for urging you, but this issue is a blocker for other work, could we prioritize merging this once it builds?

@silky
Copy link
Contributor

silky commented Dec 9, 2021

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 :)

@Yasuke Yasuke force-pushed the so/sign-with-passphrase branch 2 times, most recently from 8579a42 to 775f72a Compare December 9, 2021 23:38
@Yasuke
Copy link
Contributor Author

Yasuke commented Dec 10, 2021

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.

@Yasuke
Copy link
Contributor Author

Yasuke commented Dec 10, 2021

I also generalized the ledger signing function to work with any ByteArrayAccess instead of just TxIds

Copy link
Contributor

@sjoerdvisscher sjoerdvisscher 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 work!

@Yasuke Yasuke force-pushed the so/sign-with-passphrase branch from 5f88734 to 286da68 Compare December 10, 2021 13:09
@Yasuke
Copy link
Contributor Author

Yasuke commented Dec 10, 2021

Pending CI, required adjustments have been made, thanks @sjoerdvisscher for the review

@Yasuke Yasuke force-pushed the so/sign-with-passphrase branch 4 times, most recently from 8a0cecf to c1c71dc Compare December 13, 2021 23:39
@Yasuke
Copy link
Contributor Author

Yasuke commented Dec 13, 2021

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
@Yasuke Yasuke force-pushed the so/sign-with-passphrase branch from c1c71dc to f975fa7 Compare December 13, 2021 23:47
@Yasuke Yasuke force-pushed the so/sign-with-passphrase branch from f975fa7 to b5e64bd Compare December 14, 2021 00:05
@volodyad
Copy link

hello, when this PR are planned to be merged?

@sjoerdvisscher sjoerdvisscher merged commit eba0a57 into IntersectMBO:main Dec 16, 2021
@sjoerdvisscher
Copy link
Contributor

Thanks for the reminder!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants