fix(lookupDomains): absorb per-resolver failures#448
Open
wa0x6e wants to merge 1 commit into
Open
Conversation
64f1b6c to
a4e4d1f
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates lookupDomains so per-resolver failures no longer reject the entire lookup_domains request, aligning the behavior with the existing fan-out pattern used by address resolvers.
Changes:
- Wraps each lookup domain resolver call with a fallback to
[]on rejection. - Adds unit coverage for partial resolver failure and all-resolvers-fail behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/lookupDomains/index.ts |
Absorbs individual resolver rejections during domain lookup fan-out. |
test/unit/lookupDomains.test.ts |
Adds mocked resolver tests for partial and total resolver failure scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lookupDomains fans out over multiple resolvers (ens, shibarium, unstoppableDomains) x chains via Promise.all with no per-resolver error handling. Each resolver captures/silences its own error and then throws a bare FetchError, which rejected the whole Promise.all and propagated to the api.ts catch-all, where it was re-captured as a context-free "Error". This double-reported real errors and defeated the resolvers' isSilencedError filtering (subgraph timeouts/504s got reported anyway). The single-resolver origin made propagation correct, but it was never revisited when the function became a fan-out; the sibling addressResolvers._call already swallows per-resolver failures for the same reason. Catch each resolver's rejection and return [] so one failing chain no longer rejects the lookup or escapes to api.ts, matching the addressResolvers pattern. Fixes STAMP-35
b9dfb64 to
9f1ab5c
Compare
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
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.
Summary
Fixes STAMP-35 — a context-free
Error(1589 occurrences) captured atapi.ts:38.lookupDomainsfans out over[ens, shibarium, unstoppableDomains]× chains viaPromise.allwith no per-resolver error handling. Each resolver captures (with context) or silences its own error and then throws a bareFetchError. That rejected the wholePromise.alland propagated to the api.ts catch-all, which re-captured it as a context-stripped"Error".Two consequences:
isSilencedError(subgraph timeouts / 504 / ECONNRESET) suppresses the resolver capture, but theFetchErrorwas still thrown and re-captured at api.ts. That's the bulk of the 1589.Why this is the right fix
Propagation was correct in the single-resolver origin (#231), where
lookupDomainswas one ENS call and api.ts was its only error boundary. It was never revisited when the function became a fan-out (#362, #394) — both pure feature additions that didn't touch error handling. The sibling fan-outaddressResolvers._callalready swallows per-resolver failures withtry { … } catch {}for exactly this reason.This change brings
lookupDomainsin line with that established pattern: catch each resolver's rejection and return[], so one failing chain no longer rejects the lookup or escapes to api.ts. The api.ts catch remains a genuine safety net for unexpected errors (e.g.getOwner); informative capture + silencing stay at the resolver layer.Behavior change
Previously, any single resolver/chain failure rejected the whole
lookup_domainsrequest → the client got a 500. Now the failing resolver contributes[]and the other chains' results are still returned → the client gets a partial 200.This path is not cached (no redis/cache layer on
lookup_domains, unlikelookup_addresses/resolve_names), so the partial result is per-request only and self-corrects on the next call — there is no risk of caching an incomplete result. The new behavior matches howaddressResolversalready treats per-resolver failures.Test plan
test/unit/lookupDomains.test.ts(first unit test — fully mocked resolvers; CI runs it viatest:coverage) — a resolver rejecting no longer propagates and the other resolvers' results are returned; all-fail returns[]. Fails before the change, passes after.yarn typecheckyarn lint