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

Allow use of SEED when creating local wallet DID Issue-1682 #1705

Merged
merged 2 commits into from
Apr 5, 2022

Conversation

DaevMithran
Copy link
Contributor

@DaevMithran DaevMithran commented Apr 1, 2022

  • Added wallet startup parameter: --wallet-allow-insecure-seed
  • support seed in wallet/did/create route

Some questions around

  1. How do we support importing a key pair into the wallet as mentioned in the Issue
  2. Should I add seed in the schema of wallet/did/create ? . I don't think we can add conditional schemas according to startup parameter. I can have it as an optional field.

@codecov-commenter
Copy link

Codecov Report

Merging #1705 (8b05bce) into main (9a69a59) will decrease coverage by 0.00%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main    #1705      +/-   ##
==========================================
- Coverage   95.29%   95.29%   -0.01%     
==========================================
  Files         528      528              
  Lines       32933    32939       +6     
==========================================
+ Hits        31385    31389       +4     
- Misses       1548     1550       +2     

@swcurran swcurran merged commit bde6c85 into openwallet-foundation:main Apr 5, 2022
@ianco
Copy link
Contributor

ianco commented Apr 5, 2022

This looks good!

  1. How do we support importing a key pair into the wallet as mentioned in the Issue
    I'm not sure if we can support this. We need to store the private key as well, which I don't think we can get from the did/verkey pair. So we have to either use a seed to generate a random did.
  1. Should I add seed in the schema of wallet/did/create ? . I don't think we can add conditional schemas according to startup parameter. I can have it as an optional field.
    No I think let's leave it out of the schema. If someone is using this feature then they will know about it (after all they have to enable the feature using the new param), and if we include it always in the schema it will confuse people.

Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Sorry for being too late with my review, but added some nits.

Comment on lines +359 to +360
if context.settings.get("wallet.allow_insecure_seed"):
seed = body.get("seed") or None
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to throw an error if a seed is provided and wallet.allow_insecure_seed is not enable to make it more clear to the end user the seed is not used. This can give the false sense the seed is being used to create the did.

@@ -355,13 +355,20 @@ async def wallet_create_did(request: web.BaseRequest):
f" support key type {key_type.key_type}"
)
)
seed = None
Copy link
Contributor

Choose a reason for hiding this comment

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

The seed property is not added to the DIDCreateSchema making it harder to discover from the swagger ui and it won't be included when generating clients from the swagger file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should advertise the seed parameter unless the startup parameter is set.
So we need to either make the seed optional based on the parameter (i.e. doesn't show in swagger if not set), or else we need to break out a separate endpoint that is enabled with --wallet-allow-insecure-seed

@swcurran swcurran changed the title Fix Issue-1682 Allow use of SEED when creating local wallet DID Issue-1682 Apr 7, 2022
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.

5 participants