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

add bip39_entropy as alternative to mnemonic for signer-service #993

Merged
merged 6 commits into from
May 29, 2024

Conversation

holtzman
Copy link
Collaborator

Motivation

Offline signers have expressed a desire to store and manage the secret entropy of their account as a 32 byte value rather than as a 24 word mnemonic phrase for consistency with other blockchain wallets that their software manages this way.

Herein we introduce bip39_entropy, a 32 byte, hex-encoded string (64 characters) to the signer-service API to satisfy this need. When outputting account keys, signing txos, or syncing txos for a view-only wallet, users of the signer-service can now chose between providing a 24 word phrase, mnemonic, or a 64 character hex-encoded 32 byte secret, using bip39_entropy.

In this PR

  • json rpc request data structures updated to make mnemonic optional and to optionally allow bip39_entropy.
  • service calls updated to require exactly one of mnemonic or bip39_entropy and to use whichever is provided.

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2024

Codecov Report

Attention: Patch coverage is 0% with 85 lines in your changes are missing coverage. Please review.

Project coverage is 55.38%. Comparing base (ab2af32) to head (e7594fe).
Report is 196 commits behind head on main.

Files Patch % Lines
signer/src/service/api/mod.rs 0.00% 49 Missing ⚠️
signer/src/service/mod.rs 0.00% 30 Missing ⚠️
signer/src/service/api/request.rs 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #993      +/-   ##
==========================================
- Coverage   60.12%   55.38%   -4.75%     
==========================================
  Files          88      124      +36     
  Lines       12356    16354    +3998     
  Branches     2010     2815     +805     
==========================================
+ Hits         7429     9057    +1628     
- Misses       3238     5211    +1973     
- Partials     1689     2086     +397     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@nick-mobilecoin nick-mobilecoin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

main suggestion is to consider matching on a tuple.
I can understand if it feels too dense, but I would still encourage one to move that direction as I think it's more readable than the nested match statements

Copy link
Contributor

@sugargoat sugargoat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM & thank you @nick-mobilecoin for a great review too!

holtzman and others added 4 commits May 28, 2024 21:28
Co-authored-by: Nick Santana <nick@mobilecoin.com>
Co-authored-by: Nick Santana <nick@mobilecoin.com>
Co-authored-by: Nick Santana <nick@mobilecoin.com>
@holtzman holtzman merged commit 23f7d2e into main May 29, 2024
3 checks passed
@holtzman holtzman deleted the sign-with-b39-entropy branch May 29, 2024 05:21
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

Successfully merging this pull request may close these issues.

4 participants