chore: upgrade to ENSv2 (Universal Resolver) - #49
Conversation
Route the package-level Resolve and ReverseResolve through the ENS Universal Resolver proxy at 0xeEeEEEeE14D718C2B47D9923Deab1335E144EeEe instead of walking the legacy registry. CCIP-Read (ERC-3668) is followed transparently via a new ccipread package, so offchain and L2 names now resolve correctly. Legacy Resolver/Registry/ReverseResolver/Name types keep their existing behaviour so write paths (SetAddress, SetText, etc.) are unaffected and no public function signatures change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| func TestResolveEthereum(t *testing.T) { | ||
| expected := "de0b295669a9fd93d5f28d9ec85e40f4cb697bae" | ||
| actual, err := Resolve(client, "ethereum.eth") | ||
| func TestResolveVitalik(t *testing.T) { |
There was a problem hiding this comment.
Do not test for vitalik.eth, he might change his address and this test will break than. Better to use any of the resolution-tests: https://github.com/ensdomains/resolution-tests/blob/main/test-cases.json
There was a problem hiding this comment.
Good point, agreed vitalik.eth is fragile. I've removed TestResolveVitalik entirely in e68d7b4 rather than adding yet another fixture: forward-resolution coverage already lives in universalresolver_test.go against the canonical ur.integration-tests.eth and test.offchaindemo.eth. The package-level Resolve is just a thin wrapper around UniversalResolver.ResolveAddress, so duplicating the assertion here didn't add much.
| name: "Invalid ENS domain", | ||
| expected: "", | ||
| input: "foo.bar", | ||
| input: "sirnotappearinginthisregistry.eth", |
There was a problem hiding this comment.
Fair concern. I switched to a longer descriptive label in e68d7b4 (this-name-is-not-registered-go-ens-test-fixture-do-not-register.eth) and added a comment flagging the registration-risk caveat. Worth noting: any .eth name is technically registerable, so this just makes a collision extremely less likely rather than impossible.
- Drop TestResolveVitalik from resolver_test.go: vitalik.eth's address could change, and the rename was unnecessary churn since broader Resolve coverage already lives in universalresolver_test.go. - Replace sirnotappearinginthisregistry.eth in tokenid_test.go with a longer, descriptive label that's much less likely to be registered; add a comment noting the registration-risk caveat. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AntiD2ta
left a comment
AntiD2ta
left a comment
There was a problem hiding this comment.
Hey Dhaiwat, thx for raising this PR. As far as I see, this PR's spec-compliance is excellent.
Regarding this:
No API changes. The legacy Resolver/Registry/ReverseResolver/Name types keep all their methods — write paths (SetAddress, SetText, SetContenthash, etc.) still target specific resolver contracts via the registry. Resolver.Address() / Name.Address(coinType) still walk the legacy path because they're paired with write methods on the same struct; happy to migrate those reads through UR too in a follow-up if you want full coverage.
It is probably a good idea to migrate the read paths on Resolver and Name to also route through the UR.
Downstream code that prefers the more idiomatic Name object (and there's plenty of it, the README in fact recommends it) gets a confusing split: ens.Resolve() works, but name.Address() doesn't.
Let's raise a follow-up issue before merge. While we decide whether to migrate now, could you please:
- Raise such an issue
- Add a doc comment on
Resolver.Address()/Name.Address()explicitly stating they do not support ENSIP-10 wildcard or CCIP-Read names, pointing readers atens.Resolve/UniversalResolver.ResolveAddressinstead.
Thanks a lot. Apart from this, I raised some minor concerns.
| return nil, 0, fmt.Errorf("ccipread: gateway %s: %w", url, err) | ||
| } | ||
| defer resp.Body.Close() | ||
| bodyBytes, err := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
I'm concerned about this unboundedio.ReadAll. A misbehaving or malicious gateway could stream an arbitrarily large body and exhaust the caller's heap.
Using something like io.LimitReader() could help mitigate the concern.
There was a problem hiding this comment.
Good catch — fixed in 0a6a086. Gateway bodies are now read through io.LimitReader(resp.Body, MaxGatewayBodyBytes) with a 16 MiB cap (MaxGatewayBodyBytes). Covered by unit tests in ccipread/ccipread_test.go.
| ur, err := NewUniversalResolver(backend) | ||
| if err != nil { | ||
| return UnknownAddress, err | ||
| } | ||
|
|
||
| // Resolve the domain. | ||
| address, err := resolver.Address() | ||
| if err != nil { | ||
| return UnknownAddress, err | ||
| } | ||
| if bytes.Equal(address.Bytes(), UnknownAddress.Bytes()) { | ||
| return UnknownAddress, errors.New("no address") | ||
| } | ||
|
|
||
| return address, nil | ||
| return ur.ResolveAddress(context.Background(), input) | ||
| } |
There was a problem hiding this comment.
I'm afraid that, with the current implementation relying on context.Background(), callers won't be able to cancel slow CCIP-Read hops.
The legacy code didn't take a context.Context, so this preserves source compatibility, but it means the new CCIP-Read path is effectively non-cancellable from the caller's side. A gateway can take several seconds, the per-call 30s default timeout is the only ceiling.
Concrete consequences for typical embedders I can think of:
- HTTP handlers that pass through a request context (with a 1s deadline, say) get no deadline propagation into the gateway call.
- Goroutine cancellation on shutdown won't interrupt an in-flight resolution.
- Server-side rate-limiters that rely on context cancellation are bypassed.
I suggest adding context-aware siblings without breaking the existing API.
// ResolveContext is identical to Resolve but honours ctx.
func ResolveContext(ctx context.Context, backend bind.ContractBackend, input string) (common.Address, error)
// Resolve is preserved as a thin wrapper around ResolveContext(context.Background(), …).
func Resolve(backend bind.ContractBackend, input string) (common.Address, error) {
return ResolveContext(context.Background(), backend, input)
}There was a problem hiding this comment.
Agreed, implemented exactly as you suggested in 22dfa44:
ResolveContext(ctx, backend, input)andReverseResolveContext(ctx, backend, address)honour caller cancellation/deadlines through the UR + CCIP-Read stack.Resolve/ReverseResolveremain thin wrappers aroundcontext.Background()for source compatibility.
resolveName now calls ResolveContext internally. Doc comments on the legacy entry points note the limitation and point at the *Context variants.
| return nil, resp.StatusCode, fmt.Errorf("ccipread: gateway %s: HTTP %d: %s", url, resp.StatusCode, truncate(string(bodyBytes), 256)) | ||
| } | ||
| var parsed gatewayResponse | ||
| if err := json.Unmarshal(bodyBytes, &parsed); err != nil { |
There was a problem hiding this comment.
Could we gate the unmarshal on resp.Header.Get("Content-Type") containing application/json, and return a typed error otherwise?
We could very much trust the gateway to always be returning 200s with JSON, but a defense-in-depth doesn't harm.
There was a problem hiding this comment.
Done in 0a6a086. We check that Content-Type contains application/json (tolerant of ; charset=…) before unmarshalling; otherwise we return a typed error instead of a misleading JSON decode failure. Tested in ccipread/ccipread_test.go.
| if client.Timeout == 0 { | ||
| // Best-effort timeout when the caller didn't supply one. Without this | ||
| // a wedged gateway would block resolution indefinitely. | ||
| ctxT, cancel := context.WithTimeout(req.Context(), 30*time.Second) |
There was a problem hiding this comment.
What do you think of using min(ctx.Deadline(), 30*time.Second) as the timeout?
We would have to gate the ctx by checking if its Deadline was set:
timeout := 30*time.Second
deadline, hasDeadline := ctx.Deadline()
if hasDeadline {
timeout = min(deadline, timeout)
}
...We shouldn't ignore the context timeout, and it is probably better not to let the 30s timeout compete with the caller's deadline (if set).
There was a problem hiding this comment.
Implemented in 22dfa44. When the HTTP client has no built-in timeout, queryGateway now uses min(time.Until(ctx.Deadline()), 30s) when the caller set a deadline, and falls back to 30s otherwise — so a 1s handler deadline is no longer silently extended.
| // be overridden with GO_ENS_TEST_RPC; without an override the test falls | ||
| // back to a public endpoint so that `go test` works out of the box. | ||
| func mainnetClient(t *testing.T) *ethclient.Client { | ||
| t.Helper() |
There was a problem hiding this comment.
We should support running offline tests (e.g you rely on the public endpoint and get rate-limited).
Letting run the codebase's tests by go test -short could be an option (using testing.Short() and skipping the tests)
We usually make these kinds of tests explicit/intentional instead (running them with a tag or similar), but it probably makes a lot of sense to run them by default in this case.
There was a problem hiding this comment.
Added in 7fcbe71: the mainnetClient(t) helper in universalresolver_test.go calls t.Skip() when testing.Short() is set, so the UR integration tests (ur.integration-tests.eth, test.offchaindemo.eth, etc.) can be skipped and go test -short ./... works offline for that package.
The pre-existing legacy live-network tests in resolver_test.go, name_test.go, reverseresolver_test.go, and tokenid_test.go still dial a hardcoded Infura endpoint and are not skipped under -short — that was pre-existing and is out of scope for this PR. Happy to tackle in a follow-up if you'd like the whole repo skippable under -short.
There was a problem hiding this comment.
If not addressed here, we prefer, at least, having an issue mentioning the missed support for -short for the pre-existing legacy live-network tests.
| // NewUniversalResolver returns a UniversalResolver bound to the canonical | ||
| // proxy address on whichever chain the backend is connected to. | ||
| func NewUniversalResolver(backend bind.ContractBackend) (*UniversalResolver, error) { | ||
| return NewUniversalResolverAt(backend, common.HexToAddress(UniversalResolverContractAddress)) |
There was a problem hiding this comment.
Shall we check the chain ID of whatever chain the backend points to as a guardrail? I'm thinking of go-ens being used in the context of a devnet, local fork, alt-L1, etc and the contract address being different.
The PR supports constructing a custom UR (`NewUniversalResolverAt) , but the following is probably a good idea:
- Checking
UniversalResolverContractAddressagainst a list of chains where the address is known to be valid and returning a proper error mentioning the usage ofNewUniversalResolverAtas an alternative. - Tighten the doc comment to reflect the above change.
I will be happy with another or a similar approach, as long as we still add a guardrail.
There was a problem hiding this comment.
Added in 3a12310; Holesky (17000) added in b42e4a8 per ENS deployments. NewUniversalResolver checks backend.ChainID() when available; if the chain isn't in the known list (mainnet 1, Sepolia 11155111, Holesky 17000), it returns a typed UnknownChainError pointing callers at NewUniversalResolverAt. Doc comment updated accordingly. Backends without ChainID() keep the previous behaviour. Unit tests in universalresolver_test.go (TestNewUniversalResolver_AcceptsMainnet, _AcceptsSepolia, _AcceptsHolesky, _RejectsUnknownChain).
|
|
||
| // ResolverErrorError corresponds to `ResolverError(bytes)`: the downstream | ||
| // resolver itself reverted; the inner revert bytes are preserved verbatim. | ||
| type ResolverErrorError struct { |
There was a problem hiding this comment.
The ErrorError suffix feels awkward. Could we please replace it with something like ResolverRevertError or ResolverInnerError? I'm happy with another option.
There was a problem hiding this comment.
Renamed to ResolverRevertError in 3f49037
There was a problem hiding this comment.
I'm seeing we are missing tests for the following typed-error decoders:
ResolverError(bytes)HttpError(uint16,string)ReverseAddressMismatch(string,bytes)
Is this intentional? Could we test-cover them?
There was a problem hiding this comment.
Not intentional - added in 3f49037. urerrors_test.go now covers:
ResolverError(bytes)→ResolverRevertErrorHttpError(uint16,string)→HTTPGatewayErrorReverseAddressMismatch(string,bytes)→ReverseAddressMismatchError
Each test packs an authentic ABI payload, runs it through translateURRevert, and checks the typed error + message.
There was a problem hiding this comment.
Could we please add unit tests for DNSEncode? I'm thinking we should particularly cover (apart from obvious and golden paths):
- normalization
- boundaries (label of exactly 255 bytes, label of 256 bytes)
- single trailing dot trim
- non-ASCII labels
- leading dot (
.eth)
There was a problem hiding this comment.
Added in 46e674e - 22 tests in dnsencode_test.go covering normalization, trailing-dot trim, empty labels, non-ASCII/IDN labels, and leading-dot input.
One note on the boundary case: per ENSIP-10, DNS labels are capped at 63 bytes (RFC 1035 §3.1), not 255, so the tests assert 63/64-byte label boundaries rather than 255/256.
DNSEncode previously allowed labels up to 255 bytes and had no guard on the total encoded name length, producing wire-format names the Universal Resolver's bytesToDNSName would reject on-chain. Tighten the label cap to 63 bytes (top two bits of the length octet are reserved for compression pointers) and reject encodings above 255 octets before appending the terminating null. Add dnsencode_test.go covering happy paths, normalization, trailing- dot trimming, empty-label rejection, both label-length boundaries (63/64) and total-name-length boundaries (255/256), and non-ASCII IDN labels (münchen, MÜNCHEN case-fold). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ENSIP-10 specifies the DNS encoding as RFC 1035 §3.1 "with the exception that there is no limit on the total length of the encoded name". The 255-octet total-name guard added in 46e674e violated that exception and would reject names a spec-compliant Universal Resolver client must accept. Remove the guard, keep the 63-byte per-label cap (which ENSIP-10 incorporates by reference), and update the doc comment to cite ENSIP-10. Adjust TestDNSEncode_TotalLength256 to assert success and add TestDNSEncode_TotalLengthFarAbove255 to exercise names well past the RFC 1035 cap. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three small but spec-relevant hardenings of the CCIP-Read client, plus a first set of unit tests for the package. - Hop counter: `for hop := 0; hop <= max; hop++` ran `max+1` iterations, so MaxRedirects=4 actually allowed five gateway queries before the loop's "exceeded 4 redirects" error fired. Tighten to `hop < max` so the bound matches the error message and viem/ethers. - Response body: replace the unbounded `io.ReadAll(resp.Body)` with `io.ReadAll(io.LimitReader(resp.Body, MaxGatewayBodyBytes))`. A misbehaving gateway streaming an arbitrarily large body can no longer exhaust the caller's heap. 16 MiB is far above any plausible CCIP-Read envelope. - Content-Type: gate the JSON decode on `Content-Type` containing `application/json` (tolerant of `; charset=…` parameters). A gateway returning HTML or text with a 200 status now surfaces as a clean typed error instead of a misleading "invalid JSON". Add ccipread/ccipread_test.go covering all three behaviours (loop bound, oversized body, content-type accept/reject), driven by a hand-rolled OffchainLookup revert helper and an `httptest.Server` gateway. This is the package's first set of unit tests and addresses audit item A12 in part. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Rename the awkwardly-doubled ResolverErrorError to ResolverRevertError. The old name read as "ErrorError"; the new one matches the typed-error vocabulary used elsewhere in the file (HTTPGatewayError, ReverseAddressMismatchError, etc.) and reads naturally at call sites. - Extend urerrors_test.go with decoder tests for the three custom errors that previously had none: ResolverError(bytes), HttpError(uint16,string), and ReverseAddressMismatch(string,bytes). Each test packs an authentic ABI payload, runs it through translateURRevert, asserts the typed Go error, and checks the user-facing message. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
mainnetClient() now t.Skip()s when testing.Short() is set, so contributors working offline (or behind a rate-limited public RPC) can still run `go test -short ./...` without spurious failures. The pre-existing legacy tests in resolver_test.go / name_test.go / reverseresolver_test.go / tokenid_test.go still hit the network via the hardcoded Infura endpoint — that's audit item A10 and ships separately. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Resolution previously ran on context.Background(), so callers could not cancel a slow CCIP-Read hop, propagate an HTTP-handler deadline, or interrupt resolution on shutdown. - Add ResolveContext(ctx, backend, input) and ReverseResolveContext( ctx, backend, address). The existing Resolve / ReverseResolve are preserved as background-context wrappers, so source compatibility is unchanged. - In ccipread.queryGateway, when the caller's HTTP client has no built-in timeout, use min(ctx.Deadline()-now, 30s) instead of a hard 30s. A handler-supplied 5s deadline is no longer silently extended to 30s; the same client can also still apply a sensible ceiling when no caller deadline is set. Docs on the legacy entry points note the limitation and point at the new Context variants. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
NewUniversalResolver previously bound to the canonical proxy address unconditionally. On a chain where the UR isn't deployed at that address (devnet, local fork, alt-L1, Base, etc.) every call would silently fail or hit an unrelated contract. When the backend exposes ChainID (ethclient.Client and most production backends do), NewUniversalResolver now verifies the chain has a known canonical deployment. Unknown chains return a typed UnknownChainError pointing the caller at NewUniversalResolverAt, which remains the escape hatch for non-standard deployments. Backends without a ChainID method keep the old behaviour. The known list covers Ethereum mainnet (1) and Sepolia (11155111), matching the addresses listed in the existing doc comment. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The ENSv2 upgrade routed ens.Resolve through the Universal Resolver, but Resolver.Address() and Name.Address() still walk the legacy registry directly. Downstream code that follows the README and reaches for the Name object gets confusingly different behaviour for wildcard and CCIP-Read names: ens.Resolve(client, name) returns the right address, name.Address(60) does not. Add doc comments on both methods making the limitation explicit and pointing callers at ens.Resolve / UniversalResolver.ResolveAddress. A full migration of these read paths through the UR is tracked separately. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Gateway URLs come from a smart-contract revert, i.e. untrusted input. Add two defense-in-depth controls and reshape the 4xx/5xx error path so callers can act on it programmatically. - Scheme allowlist: parse the gateway URL and reject anything other than http/https. Stops a malicious resolver from getting Go's net/http client (or, more realistically, a caller-supplied custom transport) to fetch file:// or gopher:// targets. - No-redirect policy: when the caller's http.Client has no CheckRedirect hook, install one that returns ErrUseLastResponse. ERC-3668 gateways are expected to return data inline, so a 3xx is suspicious; blocking it prevents Location: http://169.254.169.254/... or RFC1918 pivots. Defensive-copy the client so we don't mutate the caller's instance. - GatewayHTTPError: 4xx/5xx now returns a typed error with URL, Status, and Body fields. Callers can errors.As to recover them, which the parent ens package needs in order to translate gateway failures into the on-chain HttpError(uint16,string) revert path. Add tests covering file://, gopher://, redirect rejection, and 4xx typed-error recovery. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
go-ethereum's rpc.DataError.ErrorData() returns interface{}, and while
the JSON-RPC client surfaces revert data as a 0x-prefixed string,
mocked or custom backends may return the already-decoded bytes. The
previous string-only type assertion silently dropped those, causing
both translateURRevert (urerrors.go) and decodeOffchainLookup
(ccipread.go) to treat valid reverts as plain errors.
Centralise the type-switch in a normalizeErrorData helper in each
package, accepting string and []byte. Add a urerrors test that drives
the []byte path through a raw-data RPC error.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- universalresolver.go: stop swallowing failed type assertions on the ABI-unpacked results of Resolve and Reverse. Arity was checked but each element's `_ := values[i].(T)` returned a zero-value silently; if the UR ABI ever drifts, callers used to see a misleading "no record" response. Now each mis-typed element surfaces as an explicit decode error. - resolver.go: drop the dead `bytes.Equal(nameHash[:], zeroHash)` guard in resolveName. resolveName is reached only when input contains '.', and NameHash returns the zero hash only for the empty input, so the branch was unreachable. Remove the now-unused `zeroHash` variable along with it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Thanks for the thorough review, and glad the spec compliance looks good overall. On the
|
Per ENS deployments the canonical UR proxy at 0xeEeEEEeE… is also deployed on Holesky (chain ID 17000). Add it to knownURChains and cover with TestNewUniversalResolver_AcceptsHolesky.
| // byte are reserved for compression-pointer flags, so a 64+ byte label cannot | ||
| // be encoded validly. | ||
| // | ||
| // The current implementation uses a 255-byte cap (matching its doc comment). |
There was a problem hiding this comment.
Isn't this more accurate?
| // The current implementation uses a 255-byte cap (matching its doc comment). | |
| // The current implementation uses a 63-byte cap. |
There was a problem hiding this comment.
yes, updated the comments in 88946cd
| // deployed and returns *UnknownChainError otherwise. Backends that don't | ||
| // expose a chain ID skip the check and the canonical address is used as-is — | ||
| // callers on devnets / forks / alt-L1s should prefer NewUniversalResolverAt. | ||
| func NewUniversalResolver(backend bind.ContractBackend) (*UniversalResolver, error) { |
There was a problem hiding this comment.
Related to https://github.com/wealdtech/go-ens/pull/49/changes#r3298127263, we are relying on context.Background() here and ignoring any cancellation from the caller's context (such as resolveName own context).
|
Hey guys, any blockers to get this merged? We're looking for this too |
5 participants
Hey! I'm Dhaiwat from DevRel at ENS Labs, and I'm working with library maintainers across the ecosystem to get them ready for ENSv2. Closes #38 (Support CCIP Read and ENSIP-10).
This PR routes ENS resolution through the Universal Resolver (ENSv2) instead of the legacy ENS registry. ENSIP-10 wildcard resolution and EIP-3668 CCIP-Read both fall out of this routing.
ResolveandReverseResolvethrough the Universal Resolver at0xeEeEEEeE14D718C2B47D9923Deab1335E144EeEe. Without this, names with offchain (CCIP-Read) or L2 records — including the wildcard ENSv2 names rolling out now — silently return0x0or fail becausego-enswas walking the legacy registry directly instead of the UR proxy.ccipreadpackage implementing the ERC-3668OffchainLookupflow: decodes the revert viarpc.DataError, queries gateway URLs (GET when the URL contains{data}/{sender}, POST otherwise; 4xx aborts and 5xx falls through to the next URL), and re-invokes the callback selector. Bounded to 4 hops, matching viem/ethers.contracts/universalresolver/(ABI from ens-contractsstaging).UniversalResolverwrapper at the package root (Resolve,ResolveAddress,Reverse) plus aDNSEncodehelper for thebytes nameargument.ResolverNotFoundError(Error →"unregistered name", wrapsErrUnregistered),EmptyAddressError(wrapsErrNoAddress),ResolverNotContractError,UnsupportedResolverProfileError,ResolverErrorError,HTTPGatewayError,ReverseAddressMismatchError. Existing strings like"unregistered name"are preserved so most downstream error checks keep working.ur.integration-tests.eth(UR sentinel) andtest.offchaindemo.eth(CCIP-Read end-to-end), plus 6 unit tests for the error decoder. RPC defaults tohttps://ethereum.publicnode.comand is overridable viaGO_ENS_TEST_RPC. Three pre-existing tests had stale assertions updated.Resolver/Registry/ReverseResolver/Nametypes keep all their methods — write paths (SetAddress,SetText,SetContenthash, etc.) still target specific resolver contracts via the registry.Resolver.Address()/Name.Address(coinType)still walk the legacy path because they're paired with write methods on the same struct; happy to migrate those reads through UR too in a follow-up if you want full coverage.Verification
A/B test against the upstream
@latestversion vs this branch:ens.Resolve(client, "ur.integration-tests.eth")wealdtech/go-ens/v3@latest0x1111111111111111111111111111111111111111(legacy v1)0x2222222222222222222222222222222222222222(ENSv2 ✅)To verify the upgrade, resolve
ur.integration-tests.eth: it should return0x2222222222222222222222222222222222222222via the Universal Resolver. Legacy resolution returns0x1111111111111111111111111111111111111111, so seeing the latter means the upgrade did not take effect.You can read more about ENSv2 readiness here: https://docs.ens.domains/web/ensv2-readiness