identity: hack flag to skip handle resolution #854
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
note: now planning to not implement this method |
bnewbold
added a commit
that referenced
this pull request
Feb 18, 2025
… on `BaseDirectory` (#872) *NOTE: see comments below; the PR has been updated to be about a new `identity.Resolver` interface, instead of expanding `identity.Directory`* 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)
|
merged this functionality in a separate PR |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation here is to slash DNS lookups as part of cached DID resolution. As noted in the doc comment, we should loop back around and update the "Directory" interface/API to support this usecase better.
To actually deploy this in discover, we should:
This will only impact the "resolve identity by DID" path, not any "resolve identity by handle" paths.