Add specific route to create did key

[PatStLouis]

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

[dbluhm]

I like this idea in general but I have ~two~ three objections:

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 /did/operators/did_key, I'd prefer just did/did_key. I'm not sold on the moniker of operator yet. I'd probably just call the class DidKey and be done with it.

I think the DID Registrars should be structured as though they were plugins. I'd like to see the DIDMethod definition, the routes, and all other relevant code inside of a package under did (e.g. did/did_key) and then have the package be loaded by the default context on startup as a (included by default) plugin.

[PatStLouis]

@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 DidKeyManager, please share thoughts on that.

Let me know if I didn't address something in your comment.

[dbluhm]

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?

[PatStLouis]

The existing DIDKey class is expecting key_bytes and key_type being passed as a requirement to initialize the class. It was designed to resolve an existing did. If I want to create my did, these values don't exist. I would be more inclined towards moving resolving feature to this new class and make these elements only applicable to that function. The reason I don't want to change this base class is because I do not know where else it's used in aca-py and don't want to introduce too much scope creep in this PR.

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 didDoc.id. Might be redundant with did key but I was thinking this could be a good pattern to have for all did registration as it ensures the did is conformant. The goal of a did is ultimately to resolve to a did doc.

This being said we can also return just the did.

[dbluhm]

The existing DIDKey class is expecting key_bytes and key_type being passed as a requirement to initialize the class. It was designed to resolve an existing did. If I want to create my did, these values don't exist. I would be more inclined towards moving resolving feature to this new class and make these elements only applicable to that function. The reason I don't want to change this base class is because I do not know where else it's used in aca-py and don't want to introduce too much scope creep in this PR.

:+1:

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 didDoc.id. Might be redundant with did key but I was thinking this could be a good pattern to have for all did registration as it ensures the did is conformant. The goal of a did is ultimately to resolve to a did doc.

This being said we can also return just the did.

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.

[swcurran]

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.

[PatStLouis]

@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 verificationMethod with a did. I think this is a perfect use case for did key. I notice the binding was actually coded for the key creation, not local did creation so I had to make a small modification to the in_memory wallet.

[jamshale]

I think the did directory should get restructured, because we are expecting more did methods to be added here. did_key and it's routes, objects, manager should get moved to a subdirectory. did --> key. Maybe the file could be renamed to key.py.

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.

[PatStLouis]

@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 /did/key/manager.py, which I also prefer. The issue is that the current directory is only used as a did_key resolver.

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 SonarCloud issues? all checks are passed for me.

[swcurran]

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.

[jamshale]

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.

[sonarcloud[bot]]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
83.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[PatStLouis]

@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 verificationMethod or kid to find the signing key, this is where the key binding comes at play). As a verifier, aca-py also needs to resolve the did document from the did and fetch the corresponding verificationMethod. This is easy for self resolvable dids and hosted dids, but for resolver dependent dids can create some complications.

[PatStLouis]

closing this PR as the features have been implemented in #3246