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

did:tdw resolver #3237

Merged
merged 3 commits into from
Nov 18, 2024
Merged

Conversation

jamshale
Copy link
Contributor

@jamshale jamshale commented Sep 16, 2024

  • Uses the trustdidweb-py library to implement the resolver.
  • Plugs the resolve endpoint and library into the base_resolver class for caching and future other resolution.

@jamshale
Copy link
Contributor Author

There's going to be an update to the spec so I'm not going to work on this until it gets approved and the library gets updated.

@jamshale jamshale closed this Sep 19, 2024
@swcurran
Copy link
Contributor

I don’t think the update to the spec will impact what is in this PR. AFAIK, it is making a call to a library. The call will not be impacted by the spec change, which is just some details around the DID Log format.

OK to leave if you think best — just noting that the spec change should not impact this.

@jamshale
Copy link
Contributor Author

OK. Sounds good. I can continue on this. I'm kind of blocked by bot getting it published. There's a pr in trustdidweb-py repo to do it but hasn't got completed yet.

@jamshale jamshale reopened this Sep 20, 2024
@jamshale jamshale marked this pull request as ready for review September 20, 2024 20:32
@jamshale jamshale changed the title [WIP] Did tdw resolver did:tdw resolver Sep 23, 2024
class DidTdwResolveResponse(OpenAPISchema):
"""Response schema for create DID endpoint."""

did_doc = fields.Dict(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking about just returning the diddoc dict here instead of inside this key.

@swcurran
Copy link
Contributor

Just as a matter of interest — if the DID to be resolved was a DID URL, could/should this code return the object that is referenced instead of the DIDDoc? For example if what was to be resolved was did:tdw:1234567:example.com/whois, the resulting object would be a Verifiable Presentation. Or maybe did:tdw:1234567:example.com/OCA/ocabundle.json?

@jamshale
Copy link
Contributor Author

Maybe that does make sense. After verifying and resolving the did it could get the response from the endpoint and return that. The way it is now, that responsibility would fall on the controller.

@swcurran
Copy link
Contributor

Perhaps not needed yet, but I think we want that in the future. It gives the controller the choice, without having to get its hands dirty with the rules of DIDs and DID Resolution. Either it gets a valid object back that it can use, or it gets an error that something is broken with its request.

@dbluhm
Copy link
Contributor

dbluhm commented Sep 23, 2024

Perhaps not needed yet, but I think we want that in the future. It gives the controller the choice, without having to get its hands dirty with the rules of DIDs and DID Resolution.

I am all for continuing to push for controller simplicity and keeping the controllers concerns focused on business logic and not DID and Key minutia (except in circumstances where business requirements dictate being picky about those things).

I think there's a place for a dereference endpoint alongside the resolve endpoint. Having it be separate would help the caller be certain they won't unintentionally get a did document when they expected a verification method or vice versa.

It is notable however that I don't think there's generally a reason for the controller to resolve/dereference DIDs and DID URLs directly. The only time I've done this from a controller was when working around bugs or features not yet supported in ACA-Py. Which is perhaps a fair reason to justify their existence alone but, all going well, I think it would generally be unnecessary to call them.

aries_cloudagent/did/tdw/routes.py Outdated Show resolved Hide resolved
aries_cloudagent/did/tdw/tdw_manager.py Outdated Show resolved Hide resolved
aries_cloudagent/did/tdw/tests/test_routes.py Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Sep 24, 2024

@PatStLouis
Copy link
Contributor

+1 for what @dbluhm is suggesting, we should have a resolve endpoint that will return a did document and use a de reference endpoint for any other resources represented by a did url.

@jamshale are you planning to do any verification as part of the resolving?

Did log processing is part of the resolving section in the spec.

@jamshale
Copy link
Contributor Author

jamshale commented Oct 8, 2024

@PatStLouis The library does the verification. I haven't gone over the spec with the code in detail, but basic testing seemed to be correct.

@PatStLouis
Copy link
Contributor

Sounds good, I wasn't aware how much of the process the library was covering. This is great!

@jamshale
Copy link
Contributor Author

jamshale commented Nov 16, 2024

Removed the did url usage in the resolver.

I think a did url dereference ticket should be created next as a separate task. Maybe with a similar pattern to the resolver.

@jamshale jamshale requested a review from dbluhm November 18, 2024 16:11
dbluhm
dbluhm previously approved these changes Nov 18, 2024
Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

LGTM. I agree with Jamie that we can worry about dereferencing next as a separate change.

@dbluhm
Copy link
Contributor

dbluhm commented Nov 18, 2024

@jamshale heads up on the conflict in the poetry.lock

Signed-off-by: jamshale <[email protected]>
Signed-off-by: jamshale <[email protected]>
Signed-off-by: jamshale <[email protected]>
Copy link

sonarcloud bot commented Nov 18, 2024

@jamshale jamshale requested a review from ianco November 18, 2024 19:30
@jamshale jamshale merged commit f5c49b0 into openwallet-foundation:main Nov 18, 2024
10 checks passed
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