refactor: Extract verification method ID generation to a separate class #2235
Conversation
Signed-off-by: Sacha Kozma <[email protected]>
| verkey_id_strategy = self.profile.context.inject(DefaultVerificationKeyStrategy) | ||
| verification_method = verkey_id_strategy.get_verkey_id_for_did(issuer_id) | ||
|
|
||
| if verification_method is None: | ||
| raise V20CredFormatError( | ||
| f"Unable to get retrieve verification method for did {issuer_id}" | ||
| ) |
There was a problem hiding this comment.
I think making this pluggable could work, however maybe it'd be simpler to just provide the verification_method as part of the issue credential options?
So issuer = did:example:123, verification_method is did:example:123#123.
Otherwise, I think the get_verkey_id_for_did should at least take a proof_type as input, so it can determine the correct key based on the proof type (different proof types need different keys)
I would also suggest renaming it to get_verification_method_id_for_did.
There was a problem hiding this comment.
Thinking about it a bit more, If we want to make this a generic / pluggable interface, I think the method should have more input so you can correctly define the verification method.
It should contain:
- proof_type (or something like
allowed_verification_method_types) - optional proof_purpose (so we only return a verification method that is allowed to be used for the intended purpose (assertionMethod, authentication, keyAgreement, etc..)
- did
Also, maybe it should return an array of verification method instances itself, so the caller can determine based on the whole verification method object + can choose which one to take if there's multiple candidates (currnet callees could just take the first one)
There was a problem hiding this comment.
Hi @TimoGlastra, thanks for your review.
I think making this pluggable could work, however maybe it'd be simpler to just provide the
verification_methodas part of the issue credential options?
Indeed, I inadvertently removed this part. I added this back again, so now during issuing it'll try to use the given verification_method (if provided) and will otherwise default to generating it dynamically. However, for other protocols such as credential presentation, I think we still need a way to dynamically produce the verification_method ID, as the ACA-Py instance of the presenter could be set to automatically accept presentation requests, in which cases the ID cannot be given in the options.
I would also suggest renaming it to
get_verification_method_id_for_did.
Done
Also, maybe it should return an array of verification method instances itself, so the caller can determine based on the whole verification method object + can choose which one to take if there's multiple candidates (currnet callees could just take the first one)
I'm not sure I understand this: from my limited understanding of how ACA-Py works, only a single verification key is used at a time and is stored in the verkey field of a DIDInfo record returned by using get_local_did, or others. The pluggable method this PR introduce is a way to produce the verification method ID associated with this verkey, as ACA-Py doesn't store it in this DIDInfo record. Are you suggesting that we remove this verkey field, and instead use this newly added class when querying for a verification method? And then it would return an array of verification method instances with each verification method instance containing a verkey plus its ID?
There was a problem hiding this comment.
On your last point. Yeah what I was saying doesn't fully make sense, as we already have the public key (verkey) and the did, we just want to know the verification method id (so did + # part). I was thinking more of determining the best
However, I think we should provide more than just the issuer_id/did to determine the verification_method_id. In ACA-Py's current model dids only have a single key, but as we already know the public key, the proof type, etc.. I think it would be good to pass it, so once ACA-Py starts supporitng multiple keys for a did, this interface won't have to change.
There was a problem hiding this comment.
However, I think we should provide more than just the issuer_id/did to determine the verification_method_id. In ACA-Py's current model dids only have a single key, but as we already know the public key, the proof type, etc.. I think it would be good to pass it, so once ACA-Py starts supporitng multiple keys for a did, this interface won't have to change.
Done
Signed-off-by: Sacha Kozma <[email protected]>
Signed-off-by: Sacha Kozma <[email protected]>
There was a problem hiding this comment.
Interested to see @TimoGlastra's response to @yvgny's questions in the comments but I'm happy with these changes overall
| @@ -270,10 +272,17 @@ async def _get_suite_for_detail( | |||
| ) | |||
|
|
|||
| did_info = await self._did_info_for_did(issuer_id) | |||
| verification_method = verification_method or self._get_verification_method( | |||
| issuer_id | |||
| verkey_id_strategy = self.profile.context.inject(DefaultVerificationKeyStrategy) | |||
There was a problem hiding this comment.
Shouldn't this be Base instead of Default, as you can override the default with a custom one? It would't make sense to make the injection token the default implementation IMO.
There was a problem hiding this comment.
@TimoGlastra So the injection token should be renamed to BaseVerificationKeyStrategy? This would mean the base (abstract) class would be named BaseVerificationKeyStrategy and the default implementation DefaultVerificationKeyStrategy, is this correct?
| verkey_id_strategy = self.profile.context.inject(DefaultVerificationKeyStrategy) | ||
| verification_method = verkey_id_strategy.get_verkey_id_for_did(issuer_id) | ||
|
|
||
| if verification_method is None: | ||
| raise V20CredFormatError( | ||
| f"Unable to get retrieve verification method for did {issuer_id}" | ||
| ) |
There was a problem hiding this comment.
On your last point. Yeah what I was saying doesn't fully make sense, as we already have the public key (verkey) and the did, we just want to know the verification method id (so did + # part). I was thinking more of determining the best
However, I think we should provide more than just the issuer_id/did to determine the verification_method_id. In ACA-Py's current model dids only have a single key, but as we already know the public key, the proof type, etc.. I think it would be good to pass it, so once ACA-Py starts supporitng multiple keys for a did, this interface won't have to change.
|
Please up date the branch to include changes to main. |
|
Ready to merge, but @dkulic or @dbluhm — could you answer @TimoGlastra ’s last question:
|
Signed-off-by: Sacha Kozma <[email protected]>
Signed-off-by: Sacha Kozma <[email protected]>
|
Please update to the base branch and we're ready to merge this one. It will be included in 0.8.2. |
|
Kudos, SonarCloud Quality Gate passed!
|
Currently, the strategy to generate the verification key IDs only supports an arbitrary set of DID methods, and the IDs are derived from hardcoded strings, which can be too restrictive for some use cases.
For example, if one adds DID methods (such as did:web) using third-party plugins, the current method will always raise an error. Also, one might want to have more advanced ways of deriving the verification key ID, for example when multiple verification methods are used for a given DID and are rotated regularly.
This PR allows for more flexibility by extracting the verification key ID generation to a separate class so that plugins can switch implementations depending on the needs using injection. For example, for the key rotation use-case cited above, the implementation could be switched to one that dynamically returns the correct verkey ID, even if different did methods are used. I believe this solution avoids breaking changes, and keeping the current strategy as the default one allows keeping backward compatibility.
Let me know if you have any comments/suggestions.