-
Notifications
You must be signed in to change notification settings - Fork 619
Handle ENS resolution errors gracefully #8045
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
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughError handling in ENS resolution was changed to return nulls instead of throwing. A related ERC721 test snapshot was updated to reflect a null owner value while keeping the structure intact. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant ENS as resolveEns
participant Prov as Provider
Caller->>ENS: resolveEns(input)
alt input is ENS name
ENS->>Prov: resolveName(input)
Note over ENS,Prov: Errors caught → null
Prov-->>ENS: address or null
ENS->>Prov: resolveAddress(address)
Note over ENS,Prov: Errors caught → null
Prov-->>ENS: ensName or null
ENS-->>Caller: { address, ensName }
else input is address
ENS->>Prov: resolveAddress(input)
Note over ENS,Prov: Errors caught → null
Prov-->>ENS: ensName or null
ENS-->>Caller: { address: input or null, ensName }
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
Comment |
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
838ce8a to
4d05df2
Compare
1f9ab75 to
6e6b8f4
Compare
6e6b8f4 to
fbf1fa3
Compare
size-limit report 📦
|
fbf1fa3 to
f745905
Compare
f745905 to
67a701c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8045 +/- ##
==========================================
- Coverage 56.64% 56.63% -0.01%
==========================================
Files 904 904
Lines 58755 58755
Branches 4170 4166 -4
==========================================
- Hits 33280 33275 -5
- Misses 25369 25374 +5
Partials 106 106
🚀 New features to boost your workflow:
|

TL;DR
Added error handling to ENS resolution functions to prevent unhandled promise rejections.
What changed?
Modified the
resolveEnsfunction to catch errors when resolving ENS names and addresses. Now, bothresolveNameandresolveAddressfunction calls include.catch(() => null)to gracefully handle failures by returning null instead of throwing exceptions.How to test?
Why make this change?
This change improves error handling in the ENS resolution process, preventing unhandled promise rejections that could crash the application. By gracefully handling resolution failures, the application can continue to function even when ENS lookups fail, providing a more robust user experience.
PR-Codex overview
This PR focuses on modifying the handling of NFT ownership and improving error handling in ENS resolution.
Detailed summary
getNFT.test.ts, theownerproperty of an NFT is changed from a specific address tonull.ens.ts, error handling is updated:addressandensNameset tonull.resolveAddressis now caught and returnsnullon failure.Summary by CodeRabbit