[codex] Add verification relationship resolver#74
Conversation
Add opt-in `DidResolutionOptions.IncludeLog`; when set, `DidWebVhMethod` populates a new `DidResolutionResult.Artifacts` bag with the UTF-8 `did.jsonl` string and the parsed `IReadOnlyList<LogEntry>` chain. The log is already fetched, parsed, and validated on every resolve — this surfaces it instead of discarding. No new fetch, no new interface, no storage layer. Declare `DidMethodCapabilities.History` on did:webvh so callers can branch on capability rather than method name; did:key / did:peer ignore the flag and leave `Artifacts` null. `GetCacheDiscriminator()` mixes in `IncludeLog` so a no-log cached result cannot shadow a fresh request that wants the log. Tighten `LogEntry` to a `sealed record` with `init`-only `VersionId` and `Proof`. Five internal mutation sites refactored to `with` expressions across `DidWebVhMethod` (create/update/deactivate post-hash and post-sign) and `LogChainValidator` (the save-mutate-restore around hashing is gone). External callers receiving entries via `Artifacts["log.entries"]` cannot now tamper with them. Tests: 7 new cases covering `IncludeLog` on/off, raw vs parsed log shape, multi-entry chain after update, History capability advertisement, and cache-discriminator distinction; did:key gains an additivity test confirming `IncludeLog=true` is harmless on history-less methods. Updated 4 existing WebVh test sites that mutated `LogEntry` in place. Full suite: 782 passing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Add `NetDid.Method.WebVh.DidWebVhArtifacts` with `public const string` `DidJsonl`, `DidJson`, `DidWitnessJson`, `LogEntries`. Create / Update / Deactivate / Resolve and the WebVh + W3C-conformance test suites now bind to the symbols instead of duplicating the literals — a typo in an external consumer is a compile error rather than a runtime miss. - Harden `LogEntrySerializer.ParseJsonLines` to return `entries.AsReadOnly()` (`ReadOnlyCollection<LogEntry>`) so consumers receiving `Artifacts[DidWebVhArtifacts.LogEntries]` can no longer downcast to `List<LogEntry>` and mutate. Records made the entries immutable in the original PR; this extends the guarantee to the container. Internal callers remain source-compatible. - Drop the now-redundant `(IReadOnlyList<LogEntry>)entries` cast at the resolve-time artifact site. - CHANGELOG `[Unreleased]` updated with one `Added` (constants) and one `Changed` (defensive container) bullet. No version bump. - 782 tests still pass (Core 392, W3C 175, WebVh 130, Peer 40, Key 34, DI 11). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Fix #37: expose parsed did:webvh log on DidResolutionResult
There was a problem hiding this comment.
Review — PR #74: Add verification relationship resolver
Overall this is solid work. The design is sound, the refactor of DefaultDidUrlDereferencer is behaviorally identical, and the tri-state result is the right call for security-critical code that needs to distinguish "infrastructure unavailable" from "controller said no." A few items worth discussing before merge:
1. CHANGELOG entry is too long
The [Unreleased] ### Added bullet is several hundred words — more like a design doc than a changelog entry. Consider trimming to 3–4 sentences covering what it does, why the heuristic it replaces is wrong, and which downstream issue it unblocks. The full rationale already lives in PRD §10.5 and the test file docstrings.
2. Undocumented input behavior when verificationMethodDidUrl is fragment-relative
NormalizeVmId is applied to both the entry id and the query verificationMethodDidUrl. If a caller passes "#k1" as the VM URL, normalization silently converts it to "{controllerDid}#k1" before the comparison. That's probably useful behaviour, but:
- The interface signature says the parameter is a "verification method DID URL" — the name implies an absolute form.
ArgumentException.ThrowIfNullOrWhiteSpaceguards null/whitespace but not relative forms.- No test covers the case where the query (not the entry) is fragment-relative.
Recommend either: (a) add a test documenting this works, or (b) add a guard/note in the XML doc making the accepted forms explicit. Leaving it silent means a caller who accidentally passes "#k1" gets a result that may surprise them.
3. AuthorizationDecision, VerificationRelationshipAuthorizationResult, and IVerificationRelationshipResolver all in one file
Not wrong, but these are three distinct public API elements. Consumers browsing the public surface have to know to look in IVerificationRelationshipResolver.cs to find AuthorizationDecision. Splitting them into separate files (one type per file, matching the convention used everywhere else in NetDid.Core) would improve discoverability. Low priority if you'd rather do it at next touch.
What looks good
- Tri-state result —
ControllerNotResolvablewithResolutionErrorpropagated is the right fail-closed design. TryAddSingletonin DI — allows consumers to swap in their own implementation without fighting registration order.- Behavior-identical dereferencer refactor — the string overload of
GetRelationshipEntriesreturnsnullfor unknown names just like the old private switch, and theif (verificationRelationship is not null)guard is preserved. - Test coverage — all 10 acceptance cases from the plan have dedicated tests; the
did:keyregression suite locks the key-type/relationship contract end-to-end. - No internal caching on the resolver — the right call; caching belongs at the
IDidResolverlayer.
Item 1 is easy to address before marking ready for review. Items 2 and 3 are lower-priority polish. Overall the implementation is correct and production-ready.
Generated by Claude Code
…tps://github.com/moisesja/net-did into feat/issue-71-verification-relationship-resolver
…ypes - CHANGELOG: collapse the verification-relationship entry to 3-4 sentences (full rationale stays in PRD §10.5) and remove the duplicate `### Added` block that listed it twice. - IVerificationRelationshipResolver: document the three accepted forms of `verificationMethodDidUrl` (absolute DID URL, fragment-relative `#k1`, bare `k1`) and that ordinal comparison does not strip query/path. - Split AuthorizationDecision and VerificationRelationshipAuthorizationResult into their own files for discoverability, matching the one-type-per-file convention elsewhere in NetDid.Core. - Add two tests pinning down the documented normalization on the query side: fragment-only and bare-id `verificationMethodDidUrl` both match a controller-qualified entry. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Fixes #71
Summary
Validation