Add specific route to create did key #3168
Conversation
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
|
I like this idea in general but I have I don't think we should add (yet another) CLI Arg. For the default key type, I think we can have that be a value specific to a method or just require that it be explicitly given each time. I'd like to see a flatter structure. Rather than I think the DID Registrars should be structured as though they were plugins. I'd like to see the |
Signed-off-by: Patrick <[email protected]>
|
@dbluhm I removed, the cli argument, instead defining a default value for the web request model. There is already a did_key file at the root of the did folder and its a resolver class, I don't want to touch it for now. Instead I created a key directory and labeled the class Let me know if I didn't address something in your comment. |
Signed-off-by: Patrick <[email protected]>
|
Why return a DID Document rather than the DID? Also, why not combine the functions of this DID Key Manager with the existing DID Key class? |
|
The existing DIDKey class is expecting For returning the did doc...no real good answer here besides this will ensure the registered did is compliant. You can get the did value by getting the This being said we can also return just the did. |
PatStLouis
changed the title
👍
Generally speaking, working with DID Docs is complicated. When resolving, we don't have any choice but to work with the DID Doc representation of the keys and services of the resolved DID. Fortunately, though, we don't have to make this the controller's problem; ACA-Py handles extracting the necessary information and using it to accomplish whatever operation was requested. For creation of DIDs, we should continue to handle the details for the controller, potentially with the option for the controller to directly be involved in the DID Doc if absolutely necessary. But I don't think it's necessary in most cases. What we care about when creating a DID is making sure it is capable of doing the things we want it to (e.g. DIDComm v1, DIDComm v2, signing VCs with a particular crypto suite, etc.) I think returning DID Documents by default from the DID Creation endpoint is leading in the direction of making DID Document semantics and maintenance the controller's problem. I think this should be avoided. The controller can always call the resolve endpoint if they really want to get the full DID Document. This is why I proposed in the did:tdw implementation doc that we have "feature flags" for DID Creation. These flags would be used to indicate what features the DID should support (again e.g. DIDComm v1, DIDComm v2, signing VCs with a particular crypto suite, etc.). These feature flags would of course not really apply to a did:key or did:jwk, though. |
|
Thanks @dbluhm — that makes sense to me. Several times we have started with the approach of making ACA-Py “dumb”, and leaving it to the controller to figure out what to do. Usually we find that is the wrong approach, and forces the controller to have to do too much. It’s not easy to guess what the controller will want to do, but I think @dbluhm’s ideas and experience make sense. I’d rather we erred on making it easy and not giving enough power, vs. too much power and complexity. |
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
Signed-off-by: Patrick <[email protected]>
|
@dbluhm This is ready for a final review, I've merged the two classes together, added some unit tests. I return the did, verificationMethod and multikey instead of a did document. I also had a first attempt at binding a |
|
I think the Also, I think the manger could possibly become an abstract class as well but that could be tackled when additional methods get added. Also it looks like those 2 sonarcloud issues are valid to be fixed. |
|
@jamshale I had it in a sub-directory then moved it to the already existing did_key class, since that class was already used by other components in the code base and we wanted to avoid duplicates. I'm happy to move it back but let's make sure we are on the same page about the model we want here as I already spent time moving it back and forth. what I had was more of a I'm not sure if an abstract class is a great idea since we will be looking at maybe 2-3 did methods at top, (key, indy, web/tdw), and they are all fairly different. I'm not seeing any |
|
Can we have this conversation at the ACA-Pug meeting tomorrow? I’ll put it on the agenda. Please come ready to discuss. I don’t know enough of the technical details — I just know that I want us to be able to easily support registering other DID Methods — key, web, tdw, cheqd, hedera, etc. |
|
Ok. The only problem I have is we have did_key.py and manager.py in the root did directory. Now if we want to add did_tdw it will just be another file and the routes/manager will keep getting bigger. Thought it would be nice to have them seperated, but I didn't realize there was extra complexity. FYI. For sonarcloud sometimes it fails on issues and sometimes it doesn't. I'm not sure why.
For the abstract manager I was thinking each manager would have a create/resolve/update/delete method that would need to be implemented. If they are a lot different then it's not really needed. |
Signed-off-by: Patrick <[email protected]>
…-cloudagent-python into pstlouis/add-did-key-route
|
|
@swcurran sounds good. @jamshale I like this idea, as long as the abstract class is kept as minimal as possible. I fixed the issue with SonarCloud thanks for pointing it out. I see 3 current 'families' of dids. Self resolvable dids (peer, key, jwk), resolver dependent dids (sov, indy) and 'hosted' dids (web, tdw). Some reasons why this can create complexities is the relationship between the did and a keypair in aca-py. With self-resolvable dids, there is (most of the time) a one to one mapping between the did and a key pair (I think did peer can have multiple, not too sure). DIDs are used by aca-py as an issuer when signing a document, where it needs to find the associated signing key (in this scenario, it's fine to use a did in situations where there's a 1 to 1 mapping, but for others, you would want to use the |
There was a problem hiding this comment.
I'm really not a fan of having to pull up yet another file to find the expected inputs to the handlers. I'd strongly recommend keeping this in the same file as the handlers.
| @docs(tags=["did"], summary="Bind DID Key") | ||
| @request_schema(DIDKeyBindingRequest()) | ||
| @response_schema( | ||
| DIDKeyBindingResponse(), 201, description="Bind existing DID key to new KID" | ||
| ) | ||
| @tenant_authentication | ||
| async def bind_did_key(request): | ||
| """Request handler for binding a Key DID. | ||
|
|
||
| Args: | ||
| request: aiohttp request object | ||
|
|
||
| """ | ||
| try: | ||
| return web.json_response( | ||
| await DIDKey().bind( | ||
| profile=request["context"].profile, | ||
| did=request["data"]["did"], | ||
| kid=request["data"]["kid"], | ||
| ), | ||
| status=200, | ||
| ) | ||
| except (KeyError, ValidationError, DidOperationError) as err: | ||
| return web.json_response({"message": str(err)}, status=400) |
There was a problem hiding this comment.
This key "binding" concept feels like a hack. I understand the intent is to enable ACA-Py to sign things with a DID that it doesn't know how to use natively. I think there are better paths to accomplish this but even if we did have a mechanism for creating a key and associating it with a DID, why would we take the intermediate step of representing it as a did:key first?
| @@ -236,6 +236,7 @@ async def create_local_did( | |||
| seed: Optional seed to use for DID | |||
| did: The DID to use | |||
| metadata: Metadata to store with DID | |||
| kid: Optional key identifier | |||
There was a problem hiding this comment.
kid in docstring but missing from method parameters
| @@ -263,6 +263,7 @@ async def create_local_did( | |||
| key_type: The key type to use for the DID | |||
| seed: Optional seed to use for DID | |||
| did: The DID to use | |||
| kid: Optional kid to assign to the DID | |||
| self.profile.keys[verkey_enc] = { | ||
| "seed": seed, | ||
| "secret": secret, | ||
| "verkey": verkey_enc, | ||
| "metadata": metadata.copy() if metadata else {}, | ||
| "key_type": key_type, | ||
| } |
There was a problem hiding this comment.
Can you give some background to this addition?
There was a problem hiding this comment.
The askar wallet and in memory wallet both have the function create local did. In the askar wallet, it also inserts a key through that function:
https://github.com/hyperledger/aries-cloudagent-python/blob/17e9f1611c624c9e92d9abe233b233be8ce3addf/aries_cloudagent/wallet/askar.py#L264
If I had a wallet type of askar, I could assign a kid to this keypair, however with the in memory wallet it wouldn't be able to find the key_pair from the verkey when calling the assign_kid function because it only created the did. Both the askar wallet and in memory wallet are classes derived from the BaseWallet, but when creating a did through the AskarWallet, it also creates a keypair as with the in memory wallet it only creates the did.
|
closing this PR as the features have been implemented in #3246 |
In recent discussions, we approached the topic of having separate routes for registering respective did methods did in the wallet.
I would like to propose this first route using the current did key registration method, in order to align on the model to use. From then on, we can have a look for other did methods such as web, tdw, peer, in subsequent PRs
My proposal is to include did method specific operators to conduct operations related to specific did methods, such as registering, updating, removing, etc...
@aritroCoder @dbluhm @Jsyro @jamshale let's discuss this proposal