feat(nip05)!: return Nip05ResolveResult from resolve()#635
Open
nogringo wants to merge 3 commits into
Open
Conversation
resolve() now returns a sealed Nip05ResolveResult (Nip05Found, Nip05NotFound, Nip05ResolveError) instead of Nip05?, so callers can distinguish "user not in the nostr.json file" from "server unreachable or invalid response". BREAKING CHANGE: resolve() return type changed from Future<Nip05?> to Future<Nip05ResolveResult>. Switch on the sealed result instead of null-checking the returned Nip05.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a sealed ChangesNIP-05 Resolution Result Type
Minor formatting
Sequence DiagramsequenceDiagram
participant Client
participant Nip05Usecase
participant Cache
participant Repository
Client->>Nip05Usecase: resolve(nip05)
Nip05Usecase->>Cache: lookup(nip05)
alt found in cache
Cache-->>Nip05Usecase: Nip05
Nip05Usecase-->>Client: Nip05Found(data)
else not in cache
Nip05Usecase->>Repository: fetch(nip05)
alt repository returns Nip05
Repository-->>Nip05Usecase: Nip05
Nip05Usecase->>Cache: save(result)
Nip05Usecase-->>Client: Nip05Found(data)
else repository returns null
Repository-->>Nip05Usecase: null
Nip05Usecase-->>Client: Nip05NotFound()
else repository throws
Repository-->>Nip05Usecase: exception
Nip05Usecase-->>Client: Nip05ResolveError(cause)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/ndk/test/usecases/nip05/nip05_network_test.dart (1)
405-407: ⚡ Quick winUse identity assertion instead of
hashCodefor dedup verification.At Line 406 and Line 407, comparing
hashCodeis an indirect check. Prefersame(results[1])/same(results[2])againstresults[0]for deterministic dedup validation.Suggested test change
- expect(results[0].hashCode, equals(results[1].hashCode)); - expect(results[1].hashCode, equals(results[2].hashCode)); + expect(results[0], same(results[1])); + expect(results[1], same(results[2]));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ndk/test/usecases/nip05/nip05_network_test.dart` around lines 405 - 407, The test currently verifies deduplication by comparing hashCode on the Nip05Found instances; instead replace those indirect checks with identity assertions so the test ensures the same instance is returned. Update the assertions that use results[0].hashCode, results[1].hashCode and results[2].hashCode to use expect(results[1], same(results[0])) and expect(results[2], same(results[0])) (keeping the first expect(results[0], isA<Nip05Found>()) intact) to assert object identity deterministically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ndk/lib/domain_layer/usecases/nip05/nip_05.dart`:
- Line 11: The `_inFlightResolves` map is declared static, causing
cross-instance deduping that can return results tied to another Nip05Usecase’s
`_database` and `_nip05Repository`; change `_inFlightResolves` to an instance
field on `Nip05Usecase` (remove `static`) and update all usages in `resolve()`
and the related block around lines 107-119 to reference the instance field
(`this._inFlightResolves`) so in-flight resolves are scoped per `Nip05Usecase`
instance and cannot mix request contexts across instances.
---
Nitpick comments:
In `@packages/ndk/test/usecases/nip05/nip05_network_test.dart`:
- Around line 405-407: The test currently verifies deduplication by comparing
hashCode on the Nip05Found instances; instead replace those indirect checks with
identity assertions so the test ensures the same instance is returned. Update
the assertions that use results[0].hashCode, results[1].hashCode and
results[2].hashCode to use expect(results[1], same(results[0])) and
expect(results[2], same(results[0])) (keeping the first expect(results[0],
isA<Nip05Found>()) intact) to assert object identity deterministically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 435d30fa-54a6-44f4-8d6d-7a6ac46c457e
📒 Files selected for processing (5)
packages/ndk/lib/data_layer/repositories/signers/nip46_event_signer.dartpackages/ndk/lib/domain_layer/entities/nip_05_resolve_result.dartpackages/ndk/lib/domain_layer/usecases/nip05/nip_05.dartpackages/ndk/lib/entities.dartpackages/ndk/test/usecases/nip05/nip05_network_test.dart
| // Static map to keep track of in-flight requests | ||
| static final Map<String, Future<Nip05>> _inFlightRequests = {}; | ||
| static final Map<String, Future<Nip05?>> _inFlightFetches = {}; | ||
| static final Map<String, Future<Nip05ResolveResult>> _inFlightResolves = {}; |
There was a problem hiding this comment.
Avoid static cross-instance dedup state for resolve().
Line 11 makes _inFlightResolves global across all Nip05Usecase instances, but resolve() depends on instance-specific _database and _nip05Repository. This can mix request contexts and return another instance’s in-flight result for the same identifier.
Suggested fix
- static final Map<String, Future<Nip05ResolveResult>> _inFlightResolves = {};
+ final Map<String, Future<Nip05ResolveResult>> _inFlightResolves = {};Also applies to: 107-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ndk/lib/domain_layer/usecases/nip05/nip_05.dart` at line 11, The
`_inFlightResolves` map is declared static, causing cross-instance deduping that
can return results tied to another Nip05Usecase’s `_database` and
`_nip05Repository`; change `_inFlightResolves` to an instance field on
`Nip05Usecase` (remove `static`) and update all usages in `resolve()` and the
related block around lines 107-119 to reference the instance field
(`this._inFlightResolves`) so in-flight resolves are scoped per `Nip05Usecase`
instance and cannot mix request contexts across instances.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #635 +/- ##
==========================================
- Coverage 72.03% 72.02% -0.01%
==========================================
Files 209 210 +1
Lines 10653 10672 +19
==========================================
+ Hits 7674 7687 +13
- Misses 2979 2985 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
frnandu
approved these changes
May 21, 2026
1-leo
requested changes
May 22, 2026
Contributor
1-leo
left a comment
1-leo
left a comment
There was a problem hiding this comment.
I like the code comments. Just the cause field needs a type
| /// (DNS, timeout, connection refused), non-2xx HTTP responses (e.g. 404 / 500), | ||
| /// malformed JSON bodies, and unexpected response schemas. | ||
| class Nip05ResolveError extends Nip05ResolveResult { | ||
| final Object cause; |
Nip05ResolveError is now sealed with two subtypes: Nip05ResolveNetworkError (fetch failed: DNS, timeout, non-2xx response) and Nip05ResolveInvalidResponse (body could not be parsed). Callers can switch on the subtype to tell a transient transport failure from a misconfigured server. BREAKING CHANGE: Nip05ResolveError can no longer be constructed directly; instantiate one of its sealed subtypes instead.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ndk/lib/domain_layer/usecases/nip05/nip_05.dart (1)
123-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCache write failures are being reported as resolve failures.
If
fetchNip05succeeds butsaveNip05throws,resolve()returnsNip05ResolveNetworkErroreven though the identifier was resolved. Scope the error mapping to fetch/parsing and handle cache persistence separately.Suggested fix
Future<Nip05ResolveResult> _performResolve(String nip05) async { + Nip05? result; try { - final result = await _nip05Repository.fetchNip05(nip05); - if (result == null) { - return const Nip05NotFound(); - } - await _database.saveNip05(result); - return Nip05Found(result); + result = await _nip05Repository.fetchNip05(nip05); } on FormatException catch (e) { return Nip05ResolveInvalidResponse(e); } on TypeError catch (e) { return Nip05ResolveInvalidResponse(e); } catch (e) { return Nip05ResolveNetworkError(e); } + + if (result == null) { + return const Nip05NotFound(); + } + + try { + await _database.saveNip05(result); + } catch (_) { + // Optional: log cache persistence failure. + } + return Nip05Found(result); }Also applies to: 134-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ndk/lib/domain_layer/usecases/nip05/nip_05.dart` around lines 123 - 129, The current try/catch wraps both network parsing (fetchNip05) and cache writes (saveNip05) so a cache write failure is incorrectly mapped to Nip05ResolveNetworkError; change resolve() to separate concerns: put _nip05Repository.fetchNip05(nip05) in its own try/catch and map only that catch to Nip05ResolveNetworkError (and return Nip05NotFound when result==null), then call _database.saveNip05(result) in a second try/catch so a save failure is handled separately (e.g., log the cache write failure and still return Nip05Found(result) or return a distinct cache error if one exists); apply the same separation for the similar block referenced around the 134-136 area.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/ndk/lib/domain_layer/usecases/nip05/nip_05.dart`:
- Around line 123-129: The current try/catch wraps both network parsing
(fetchNip05) and cache writes (saveNip05) so a cache write failure is
incorrectly mapped to Nip05ResolveNetworkError; change resolve() to separate
concerns: put _nip05Repository.fetchNip05(nip05) in its own try/catch and map
only that catch to Nip05ResolveNetworkError (and return Nip05NotFound when
result==null), then call _database.saveNip05(result) in a second try/catch so a
save failure is handled separately (e.g., log the cache write failure and still
return Nip05Found(result) or return a distinct cache error if one exists); apply
the same separation for the similar block referenced around the 134-136 area.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d94af01-d5f2-43e9-a505-fffa71e89179
📒 Files selected for processing (2)
packages/ndk/lib/domain_layer/entities/nip_05_resolve_result.dartpackages/ndk/lib/domain_layer/usecases/nip05/nip_05.dart
Contributor
|
@nogringo changed the returned errors to be exceptions |
1-leo
approved these changes
May 26, 2026
Collaborator
Author
|
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
resolve() now returns a sealed Nip05ResolveResult (Nip05Found, Nip05NotFound, Nip05ResolveError) instead of Nip05?, so callers can distinguish "user not in the nostr.json file" from "server unreachable or invalid response".
BREAKING CHANGE: resolve() return type changed from Future<Nip05?> to Future. Switch on the sealed result instead of null-checking the returned Nip05.
Summary by CodeRabbit
New Features
Breaking Changes
Tests