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

add Resolve* methods to Directory interface #872

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bnewbold
Copy link
Collaborator

@bnewbold bnewbold commented Dec 6, 2024

This is a step in the right direction for this package, I think:

  • Lookup* for full bi-di verification, and gives a compact/ergonomic struct for atproto use-cases (and good for caching)
  • Resolve* does more bare-bones resolution: you get specifically what you ask for. Things like Relay, or pure service auth validation, which don't care about handles, can use ResolveDID

Downsides and open questions:

  • ResolveDID doesn't return the raw DID doc verbatim, it is a struct, and might be discarding info (like fields in DID core specification that we aren't using). I think this is good enough, maybe BaseDirectory can expose more "Raw" variants which return json.RawMessage or map[string]any or something.
  • the struct returned by ResolveDID isn't the same Identity type used in possible function signatures. They can be converted, but it uses some allocs etc. I think adding a helper to quickly extract the DID signing key and/or PDS endpoint is probably sufficient? But feels a bit redundant with Identity struct
  • caching: do we cache one of the Identity struct (compact) or the DIDDocument struct, and convert between? Or both, would be less conversions but more RAM? I think what we should do is cache what is actually requested. Lookup* caches Identity, ResolveDID caches DIDDocument, and these caches aren't shared. Services will be using one or the other anyways. What about an identity caching layer? That could cache the raw DID document and handle resolution, and convert to Identity on demand.

Alternative would be breaking changes to existing structs (which are heavily cached), or maybe a flag or variant like LookupDIDWithoutHandle?

Another alternative is to set a flag to skip handle resolution; I don't like this: #854

An upcoming change here is to add ResolveNSID, which will be used for Lexicon resolution.

(have not pushed implementation of exiting interfaces until design questions are resolved)

@bnewbold bnewbold requested a review from ericvolp12 December 6, 2024 03:48
Base automatically changed from bnewbold/identity-reorg to main December 6, 2024 03:52
@ericvolp12
Copy link
Collaborator

I don't mind the idea of two caches here, I think your assumption that a caller will likely be using mostly one type of method (Resolve vs Lookup) is reasonable. The naming is a bit ambiguous imo but I think it makes sense to maintain compatibility with the existing APIs and it's fine.

I do think the "Resolve" method should get something that contains the entire DID doc as it's a "low level" API here compared to the "Lookup" API.

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.

2 participants