feat: Universal Resolver migration with full ENSIP-15 normalization - #113
feat: Universal Resolver migration with full ENSIP-15 normalization#113koko1123 wants to merge 12 commits into
Conversation
From adraffy/go-ens-normalize @ 165fa80 (MIT). spec.bin/nf.bin are the compressed ENSIP-15 and Unicode NF tables (36 KB total, shipped in the package); the JSON vectors live under tests/ and are not packaged.
Port of go-ens-normalize util/ (decoder.go, runeset.go) at 165fa80. Both hand-computed test buffers in the brief (0x09, 0x83) were verified bit-by-bit against decoder.go's LSB-first readBit/readUnary/readBinary and are correct as written; no test corrections were needed. Fixed one Zig-specific memory-safety issue not present in the Go source: a literal translation of readUnique would wrap the errdefer-tracked sorted-ascending prefix in an ArrayList via fromOwnedSlice and grow it in place, so an allocation failure mid-append could free that buffer twice (once via the list's own errdefer, once via the outer errdefer on the original slice). Reworked to build the appended ranges in a separate buffer and copy once at the end; added a regression test that exercises the append path end to end.
Port of go-ens-normalize nf/nf.go with embedded nf.bin tables.
Port of go-ens-normalize ensip15/ at 165fa80: tokenization, NFC, script groups, whole-script confusables, emoji sequences, and the label validation rules. Public API: ens_normalize.normalize.
zig build vector-test runs the full upstream suites (ens-normalize 1.11.1, Unicode 17) in CI.
An unconditional stderr summary made the Zig build runner print a "failed command" trailer on successful vector-test runs. Print diagnostics only when failures occur; the expectEqual assertion still fails the build on any mismatch.
lastError() now exposes the error.data hex payload so callers can decode contract custom errors (needed for Universal Resolver).
resolve/getText/getContentHash now normalize per ENSIP-15, then build namehash and ENSIP-10 dnsEncode from the same normalized string and call resolve(bytes,bytes) on the UR (same address on mainnet and Sepolia). UR custom-error reverts map to null / OffchainLookupRequired. Adds EIP-1577 contenthash decoding (ipfs/ipns/swarm).
reverse(bytes,uint256) with on-chain forward verification replaces the namehash-of-addr.reverse registry flow. Removes the now-unused ens_registry chain metadata.
Integration tests are gated on ETH_RPC_URL and skip when unset.
Final-review cleanup: NoResolver was declared but never produced (the no-resolver case returns null) and its docs contradicted the null contract; parseSelector existed verbatim in resolver.zig and reverse.zig; provider revert-data buffer comment and the ens docs normalization-timing claim were imprecise.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis change adds ENSIP-15 normalization, ENSIP-10 Universal Resolver support, DNS encoding, EIP-1577 contenthash decoding, reverse lookup verification, provider revert-data capture, conformance tests, integration tests, and updated ENS documentation. ChangesENS resolution support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (5)
tests/ens_vectors_test.zig (1)
71-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the case-array length before indexing.
v[0],v[1], andv[2]assume every case holds at least three elements. A refreshednf-tests.jsonwith a shorter row causes an index-out-of-bounds panic, and the panic does not identify the offending group or row. A length check turns that into a readable test failure.♻️ Proposed guard
for (entry.value_ptr.array.items) |case| { const v = case.array.items; + if (v.len < 3) { + fail += 1; + std.debug.print("malformed NF case in {s}: {d} fields\n", .{ entry.key_ptr.*, v.len }); + continue; + } const input = try utf8ToCps(allocator, v[0].string);🤖 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 `@tests/ens_vectors_test.zig` around lines 71 - 76, In the test loop over cases, validate that case.array.items contains at least three elements before assigning v[0], v[1], or v[2]. Turn insufficient rows into a readable test failure that identifies the current group and row, then continue indexing only after the check passes.src/ens/normalize/nf.zig (1)
159-195: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFree the pending row when the map insertion fails.
oneat Line 165 andpairat Line 185 are allocated beforedecomps.put. Ifputreturnserror.OutOfMemory, the slice is not yet a map value, sofreeDecompsin theerrdefercannot free it. The result is a leaked row on the allocation-failure path. The same allocator is used for both, so anerrdeferinside each loop body closes the gap.🛡️ Proposed fix for the OOM path
for (decomp1, 0..) |cp_raw, i| { const one = try allocator.alloc(u21, 1); + errdefer allocator.free(one); one[0] = `@intCast`(decomp1a[i]); try decomps.put(allocator, `@intCast`(cp_raw), one); }const pair = try allocator.alloc(u21, 2); + errdefer allocator.free(pair); pair[0] = cp_b; pair[1] = cp_a; try decomps.put(allocator, cp, pair);🤖 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 `@src/ens/normalize/nf.zig` around lines 159 - 195, Add an errdefer immediately after allocating each pending decomposition row in the single-codepoint loop and the two-codepoint loop, covering one and pair respectively, so failed decomps.put calls free the allocation while successful insertion transfers ownership to decomps.src/ens/normalize/ensip15.zig (2)
886-897: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a negative-path unit test in this module.
The tests in
src/ens/normalize.zigcover rejection cases, andtests/ens_vectors_test.zigcovers the official vectors. This file has only one positive test. A single confusable or illegal-mixture case here keeps the core module self-checking when the public wrapper changes, and it exercisescheckWholeanddetermineGroup, which no test in this module reaches today.🤖 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 `@src/ens/normalize/ensip15.zig` around lines 886 - 897, Add a negative-path unit test alongside the existing normalize test in this module, using a confusable or illegal mixed-script name that normalize rejects. Assert the call returns the expected NormalizeError, exercising the internal checkWhole and determineGroup validation path while leaving the existing positive test unchanged.
860-884: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the busy spin with
std.Io.Mutex.
Ensip15.initcan take a long time. Use a module-levelstd.Io.Mutexwith an atomicreadyfast path. Re-checkreadyafterlockUncancelable(runtime.blockingIo()), initialize under the lock, publishtables, and then release the lock.std.Thread.Futexis not required; a futex implementation would also need a 32-bit wait word instead of the currentu8atomic.🤖 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 `@src/ens/normalize/ensip15.zig` around lines 860 - 884, Replace the initializing-state CAS and spin loop in getTables with a module-level std.Io.Mutex and an atomic ready fast path. When not ready, acquire the mutex via lockUncancelable(runtime.blockingIo()), re-check readiness after locking, initialize Ensip15 only if still needed, publish tables, mark ready with release ordering, and unlock; preserve the existing page_allocator initialization and catch behavior.src/ens/normalize.zig (1)
23-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider limiting the public surface for the NF re-export.
testing_nfexposes the internal NF implementation through the publishedeth.ens_normalizeAPI. Only the vector suite needs it. The name marks the intent, but consumers can still depend on it, and later table refactors then become breaking changes. An alternative is to importsrc/ens/normalize/nf.zigdirectly fromtests/ens_vectors_test.zigthrough a dedicated build module, and keepnormalize.ziglimited toNormalizeErrorandnormalize.🤖 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 `@src/ens/normalize.zig` around lines 23 - 25, Remove the public testing_nf re-export from normalize.zig and update the vector suite to import normalize/nf.zig directly through its dedicated build module. Keep normalize.zig’s public surface limited to NormalizeError and normalize, while preserving the vector tests’ direct access to the NF implementation.
🤖 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 `@src/ens/contenthash.zig`:
- Around line 125-133: Update decodeIpns to validate the payload as a CID before
calling baseNEncode, matching the validation performed by the IPFS and Swarm
decoding paths; reject empty or otherwise invalid payloads instead of
constructing an unresolved ipns://k URI.
In `@src/ens/normalize/data/NOTICE`:
- Around line 1-7: Update src/ens/normalize/data/NOTICE to attribute
adraffy/go-ens-normalize as Copyright (c) 2024 Andrew Raffensperger, and add the
complete upstream MIT license text. Place the license text in an appropriate
repository license file since no aggregate third-party license file exists,
while preserving the existing provenance details.
In `@src/ens/normalize/decoder.zig`:
- Around line 109-140: Update the documentation for readUnsortedDeltas to remove
the claim that intermediate sums may be negative and state that decoded
accumulated values must remain non-negative for valid input. Keep the existing
i64 accumulator implementation in readArray unchanged.
In `@src/ens/resolver.zig`:
- Around line 170-172: Update the catch block around provider.call in the
resolver flow to inspect the caught error and call mapRevert(provider) only when
it is error.RpcError; propagate all other errors directly so allocation failures
cannot consume stale provider.lastError() data.
In `@src/ens/reverse.zig`:
- Around line 156-159: Update the encodeReverseTuple documentation and parameter
names to match the UR tuple order: primary name, forward resolver address, and
reverseResolver address. Rename the resolved parameter and its corresponding
AbiValue usage without changing tuple encoding behavior or lookupAddress.
- Around line 89-92: Update the primary-name handling after decoding in the
reverse-resolution function: normalize the decoded name only for comparison,
return null when the normalized form differs from the stored name, and return
the original name when they match. Update the test covering “Vitalik.ETH” to
expect null instead of the rewritten lowercase name.
In `@tests/integration_tests.zig`:
- Line 196: Update the private-key wallet initialization calls in README.md,
docs/content/docs/, and examples/04_send_transaction.zig to use
eth.wallet.Wallet.initLocal rather than Wallet.init; preserve Wallet.init for
signer_mod.Signer inputs, and leave the already-correct
tests/integration_tests.zig call unchanged.
---
Nitpick comments:
In `@src/ens/normalize.zig`:
- Around line 23-25: Remove the public testing_nf re-export from normalize.zig
and update the vector suite to import normalize/nf.zig directly through its
dedicated build module. Keep normalize.zig’s public surface limited to
NormalizeError and normalize, while preserving the vector tests’ direct access
to the NF implementation.
In `@src/ens/normalize/ensip15.zig`:
- Around line 886-897: Add a negative-path unit test alongside the existing
normalize test in this module, using a confusable or illegal mixed-script name
that normalize rejects. Assert the call returns the expected NormalizeError,
exercising the internal checkWhole and determineGroup validation path while
leaving the existing positive test unchanged.
- Around line 860-884: Replace the initializing-state CAS and spin loop in
getTables with a module-level std.Io.Mutex and an atomic ready fast path. When
not ready, acquire the mutex via lockUncancelable(runtime.blockingIo()),
re-check readiness after locking, initialize Ensip15 only if still needed,
publish tables, mark ready with release ordering, and unlock; preserve the
existing page_allocator initialization and catch behavior.
In `@src/ens/normalize/nf.zig`:
- Around line 159-195: Add an errdefer immediately after allocating each pending
decomposition row in the single-codepoint loop and the two-codepoint loop,
covering one and pair respectively, so failed decomps.put calls free the
allocation while successful insertion transfers ownership to decomps.
In `@tests/ens_vectors_test.zig`:
- Around line 71-76: In the test loop over cases, validate that case.array.items
contains at least three elements before assigning v[0], v[1], or v[2]. Turn
insufficient rows into a readable test failure that identifies the current group
and row, then continue indexing only after the check passes.
🪄 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 Plus
Run ID: 7cf36218-7bb2-4cb5-bda4-8458dd738df7
⛔ Files ignored due to path filters (2)
src/ens/normalize/data/nf.binis excluded by!**/*.binsrc/ens/normalize/data/spec.binis excluded by!**/*.bin
📒 Files selected for processing (28)
.github/workflows/ci.ymlMakefileREADME.mdbuild.zigdocs/content/docs/ens.mdxdocs/content/docs/modules.mdxsrc/chains/arbitrum.zigsrc/chains/base.zigsrc/chains/chain.zigsrc/chains/ethereum.zigsrc/chains/optimism.zigsrc/chains/polygon.zigsrc/ens/contenthash.zigsrc/ens/namehash.zigsrc/ens/normalize.zigsrc/ens/normalize/data/NOTICEsrc/ens/normalize/decoder.zigsrc/ens/normalize/ensip15.zigsrc/ens/normalize/nf.zigsrc/ens/normalize/rune_set.zigsrc/ens/resolver.zigsrc/ens/reverse.zigsrc/provider.zigsrc/root.zigtests/data/ens-normalize/nf-tests.jsontests/data/ens-normalize/tests.jsontests/ens_vectors_test.zigtests/integration_tests.zig
💤 Files with no reviewable changes (6)
- src/chains/base.zig
- src/chains/optimism.zig
- src/chains/polygon.zig
- src/chains/arbitrum.zig
- src/chains/ethereum.zig
- src/chains/chain.zig
| fn decodeIpns(allocator: std.mem.Allocator, payload: []const u8) DecodeError!?ContentHash { | ||
| const b36 = try baseNEncode(allocator, base36_alphabet, payload); | ||
| defer allocator.free(b36); | ||
| // multibase prefix 'k' + base36(cid bytes). | ||
| const uri = try allocator.alloc(u8, "ipns://k".len + b36.len); | ||
| @memcpy(uri[0.."ipns://k".len], "ipns://k"); | ||
| @memcpy(uri["ipns://k".len..], b36); | ||
| return .{ .protocol = .ipns, .uri = uri }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the ipns payload before encoding.
decodeIpns encodes any payload, including an empty one. A record of only the 0xe5 0x01 prefix produces the URI ipns://k, which no client can resolve. The ipfs and swarm paths both validate the CID first. Add the same check here.
🛡️ Proposed fix
fn decodeIpns(allocator: std.mem.Allocator, payload: []const u8) DecodeError!?ContentHash {
+ // ipns records carry a CIDv1 (libp2p-key); reject anything else.
+ const version = readVarint(payload) orelse return error.InvalidContentHash;
+ if (version.value != 1 or payload.len <= version.len) return error.InvalidContentHash;
const b36 = try baseNEncode(allocator, base36_alphabet, payload);🤖 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 `@src/ens/contenthash.zig` around lines 125 - 133, Update decodeIpns to
validate the payload as a CID before calling baseNEncode, matching the
validation performed by the IPFS and Swarm decoding paths; reject empty or
otherwise invalid payloads instead of constructing an unresolved ipns://k URI.
| spec.bin and nf.bin are vendored unmodified from adraffy/go-ens-normalize | ||
| https://github.com/adraffy/go-ens-normalize | ||
| commit 165fa80d967c431535e77f04a1ee017c941746cd (2025-09-20) | ||
| License: MIT (Copyright (c) 2023 Andrew Raffensperger) | ||
| The Zig sources in src/ens/normalize/ are a port of the same project. | ||
| tests/data/ens-normalize/*.json are the upstream validation vectors | ||
| (ens-normalize version 1.11.1, Unicode 17.0.0). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find existing license/attribution files and any third-party license aggregation.
set -euo pipefail
fd --hidden --type f --ignore-case 'licen[cs]e|notice|copying|third.party|vendor' .
echo '--- references to ens-normalize / adraffy outside the data dir ---'
rg -n --iglob '!src/ens/normalize/data/**' 'ens-normalize|adraffy'Repository: StrobeLabs/eth.zig
Length of output: 378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- NOTICE ---'
cat -n src/ens/normalize/data/NOTICE
echo '--- root LICENSE ---'
sed -n '1,120p' LICENSE
echo '--- related tracked files ---'
git ls-files src/ens/normalize | sed -n '1,120p'
echo '--- repository license references ---'
rg -n --hidden -g '!src/ens/normalize/data/*' -i 'MIT License|permission notice|third[- ]party|copyright' README.md .github src build.zig build.zig.zon 2>/dev/null || true
echo '--- upstream license at the cited commit ---'
curl -fsSL https://raw.githubusercontent.com/adraffy/go-ens-normalize/165fa80d967c431535e77f04a1ee017c941746cd/LICENSERepository: StrobeLabs/eth.zig
Length of output: 20113
Add the upstream MIT license text and correct its attribution.
The repository has no aggregate third-party license file. Add the upstream license text for adraffy/go-ens-normalize and update NOTICE to Copyright (c) 2024 Andrew Raffensperger.
🤖 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 `@src/ens/normalize/data/NOTICE` around lines 1 - 7, Update
src/ens/normalize/data/NOTICE to attribute adraffy/go-ens-normalize as Copyright
(c) 2024 Andrew Raffensperger, and add the complete upstream MIT license text.
Place the license text in an appropriate repository license file since no
aggregate third-party license file exists, while preserving the existing
provenance details.
| fn readArray(self: *Decoder, allocator: std.mem.Allocator, n: usize, comptime combine: fn (prev: i64, x: u32) i64) ![]u32 { | ||
| const v = try allocator.alloc(u32, n); | ||
| errdefer allocator.free(v); | ||
| var prev: i64 = -1; | ||
| for (v) |*slot| { | ||
| const x = self.readUnsigned(); | ||
| prev = combine(prev, x); | ||
| slot.* = @intCast(prev); | ||
| } | ||
| return v; | ||
| } | ||
|
|
||
| /// Reads `n` values as ascending deltas: `v[i] = v[i-1] + 1 + x`, with | ||
| /// `v[-1]` treated as `-1`. | ||
| pub fn readSortedAscending(self: *Decoder, allocator: std.mem.Allocator, n: usize) ![]u32 { | ||
| return self.readArray(allocator, n, struct { | ||
| fn f(prev: i64, x: u32) i64 { | ||
| return prev + 1 + @as(i64, x); | ||
| } | ||
| }.f); | ||
| } | ||
|
|
||
| /// Reads `n` values as zigzag-signed deltas: `v[i] = v[i-1] + asSigned(x)`, | ||
| /// with `v[-1]` treated as `-1`. Intermediate sums may go negative | ||
| /// mid-stream even though the final accumulated values are codepoints. | ||
| pub fn readUnsortedDeltas(self: *Decoder, allocator: std.mem.Allocator, n: usize) ![]u32 { | ||
| return self.readArray(allocator, n, struct { | ||
| fn f(prev: i64, x: u32) i64 { | ||
| return prev + asSigned(@as(i64, x)); | ||
| } | ||
| }.f); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
adraffy go-ens-normalize decoder.go ReadUnsortedDeltas negative delta values
💡 Result:
There is no function named ReadUnsortedDeltas that handles negative delta values within the adraffy/go-ens-normalize library [1][2]. The primary purpose of this library is to implement ENSIP-15 normalization in Go [1][3]. It appears the user may be conflating this library with the standard library's encoding/gob package, which does contain logic that explicitly checks for and errors on negative delta values in its decoder [4]. Specifically, in src/encoding/gob/decode.go, both decodeStruct and ignoreStruct check if a delta value is negative and trigger a "corrupted data: negative delta" error [4].
Citations:
- 1: https://github.com/adraffy/go-ens-normalize
- 2: https://github.com/adraffy/ENSNormalize.go
- 3: Adopt ENSIP-15 normalization (replace IDNA-based
Normalize) wealdtech/go-ens#51 - 4: https://github.com/golang/go/blob/master/src/encoding/gob/decode.go
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(decoder\.zig|.*normalize.*|.*encoder.*|.*spec\.bin|.*nf\.bin)$' | head -200
printf '%s\n' '--- symbols and references ---'
rg -n --hidden -g '!node_modules' 'readUnsortedDeltas|readSortedAscending|asSigned|readArray|negative|delta' .
printf '%s\n' '--- remotes ---'
git remote -vRepository: StrobeLabs/eth.zig
Length of output: 12606
🌐 Web query:
github adraffy ENS normalize encoder readUnsortedDeltas asSigned decoder
💡 Result:
The library @adraffy/ens-normalize is the standard, 0-dependency implementation of the ENS Name Normalization Standard (ENSIP-15) [1][2]. While it contains a decoder component for processing internal data [3], the specific functions readUnsortedDeltas and asSigned do not appear to be part of the public API or internal architecture of the adraffy/ens-normalize.js library. The terms readUnsortedDeltas and asSigned are more commonly associated with other technical domains, such as data processing libraries for Apache Delta Lake or specific cryptographic and codec implementations [4][5][6][7]. Based on the repository content, the adraffy/ens-normalize library focuses on Unicode-based normalization for Ethereum Name Service names [1][8][9].
Citations:
- 1: https://github.com/adraffy/ens-normalize.js/
- 2: https://www.npmjs.com/package/@adraffy/ens-normalize
- 3: Please polyfill atob function adraffy/ens-normalize.js#4
- 4: https://github.com/iwasa-kosui/delta-ts
- 5: https://github.com/shudv/deltasort
- 6: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/f7c16c10/contracts/utils/cryptography/signers/MultiSignerERC7913.sol
- 7: https://github.com/ethereum-optimism/contracts/blob/0ad4dcfdef11ef87e278a8159de8414c8e329ba1/contracts/optimistic-ethereum/libraries/codec/Lib_OVMCodec.sol
- 8: https://github.com/adraffy/ensip-norm
- 9: https://raffy.antistupid.com/eth/ens-slides/ENSWG_ENSNormalize-20230601.pdf
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- decoder implementation and tests ---'
cat -n src/ens/normalize/decoder.zig | sed -n '1,270p'
printf '%s\n' '--- consumers ---'
cat -n src/ens/normalize/nf.zig | sed -n '130,205p'
cat -n src/ens/normalize/ensip15.zig | sed -n '400,450p'
printf '%s\n' '--- provenance and data documentation ---'
rg -n --hidden -g '!src/crypto/**' -g '!docs/pnpm-lock.yaml' 'spec\.bin|nf\.bin|ENSIP-15|ens-normalize|normalize.*data|delta|zigzag|asSigned' README.md build.zig build.zig.zon src tests docs .github 2>/dev/null | head -300
printf '%s\n' '--- recent file history ---'
git log --oneline -12 -- src/ens/normalize/decoder.zig src/ens/normalize/nf.zig src/ens/normalize/data/NOTICERepository: StrobeLabs/eth.zig
Length of output: 26599
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import json, urllib.request
urls = [
"https://api.github.com/repos/adraffy/ens-normalize.js/contents/src",
"https://api.github.com/repos/adraffy/ens-normalize.js/contents",
"https://api.github.com/repos/adraffy/ensip-norm/contents",
"https://api.github.com/repos/adraffy/ens-normalize/contents",
]
for url in urls:
print("\n---", url, "---")
try:
req = urllib.request.Request(url, headers={"Accept":"application/vnd.github+json","User-Agent":"review-check"})
with urllib.request.urlopen(req, timeout=10) as r:
data = json.load(r)
if isinstance(data, list):
print("\n".join(x.get("name","") for x in data))
else:
print(json.dumps(data)[:1000])
except Exception as e:
print(type(e).__name__, e)
PYRepository: StrobeLabs/eth.zig
Length of output: 1015
🏁 Script executed:
#!/bin/bash
set -o pipefail
base='https://raw.githubusercontent.com/adraffy/go-ens-normalize/master'
for path in util/decoder.go util/encoder.go util/runeset.go nf/nf.go ensip15/ensip15.go; do
echo "--- $path ---"
curl -k -L --fail --silent --show-error "$base/$path" | sed -n '1,260p'
done
echo '--- repository tree names ---'
curl -k -L --fail --silent --show-error \
-H 'Accept: application/vnd.github+json' \
'https://api.github.com/repos/adraffy/go-ens-normalize/git/trees/master?recursive=1' |
jq -r '.tree[]?.path' | rg '(^|/)(encoder|decoder|nf|spec|README|go\.mod)' | head -200Repository: StrobeLabs/eth.zig
Length of output: 14604
🏁 Script executed:
#!/bin/bash
set -o pipefail
for ref in 165fa80 master main; do
echo "--- go-ens-normalize $ref util/decoder.go ---"
curl -k -L --fail --silent --show-error \
"https://raw.githubusercontent.com/adraffy/go-ens-normalize/$ref/util/decoder.go" |
rg -n -C 8 'asSigned|ReadUnsorted|ReadArray|int64|uint32|negative|delta'
doneRepository: StrobeLabs/eth.zig
Length of output: 3812
🏁 Script executed:
#!/bin/bash
set -o pipefail
for url in \
'https://api.github.com/repos/adraffy/go-ens-normalize/contents' \
'https://api.github.com/repos/adraffy/go-ens-normalize/git/trees/165fa80?recursive=1' \
'https://api.github.com/repos/adraffy/ens-normalize.js/git/trees/main?recursive=1'; do
echo "--- $url ---"
curl -k -L --silent --show-error \
-H 'Accept: application/vnd.github+json' \
-H 'User-Agent: review-check' "$url" |
jq -r 'if type=="array" then .[] | (.path // .name) else (.message // (.tree[]?.path)) end' |
rg '(^|/)(encode|encoder|decode|decoder|pack|spec|nf|data|README|package.json|go.mod)' | head -250
doneRepository: StrobeLabs/eth.zig
Length of output: 9711
🏁 Script executed:
#!/bin/bash
set -o pipefail
for path in README.md util/README.md nf/README.md ensip15/README.md; do
echo "--- go-ens-normalize/$path ---"
curl -k -L --silent --show-error \
"https://raw.githubusercontent.com/adraffy/go-ens-normalize/165fa80/$path" |
rg -n -C 5 'bin|generat|pack|encod|delta|Unicode|source|data'
done
for path in README.md package.json src; do
echo "--- ens-normalize.js/$path ---"
curl -k -L --silent --show-error \
"https://raw.githubusercontent.com/adraffy/ens-normalize.js/main/$path" |
rg -n -C 5 'bin|generat|pack|encod|delta|Unicode|source|data' | head -200
doneRepository: StrobeLabs/eth.zig
Length of output: 4378
🏁 Script executed:
#!/bin/bash
set -o pipefail
tree_url='https://api.github.com/repos/adraffy/go-ens-normalize/git/trees/165fa80?recursive=1'
curl -k -L --silent --show-error "$tree_url" |
jq -r '.tree[]?.path' | rg '^compress/'
echo '--- compress source files ---'
for path in $(curl -k -L --silent --show-error "$tree_url" | jq -r '.tree[]?.path' | rg '^compress/.*\.(js|ts|go|md)$'); do
echo "--- $path ---"
curl -k -L --silent --show-error "https://raw.githubusercontent.com/adraffy/go-ens-normalize/165fa80/$path" |
sed -n '1,300p'
doneRepository: StrobeLabs/eth.zig
Length of output: 13730
🏁 Script executed:
#!/bin/bash
set -o pipefail
tree_url='https://api.github.com/repos/adraffy/ens-normalize.js/git/trees/main?recursive=1'
curl -k -L --silent --show-error "$tree_url" |
jq -r '.tree[]?.path' | rg '(^|/)(compress|include|pack|encode|decoder|encoder|spec\.json|nf\.json)(/|\.|$)' | head -300Repository: StrobeLabs/eth.zig
Length of output: 404
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
import json, subprocess
def fetch(path):
url = f"https://raw.githubusercontent.com/adraffy/go-ens-normalize/165fa80/{path}"
return subprocess.check_output(["curl", "-k", "-L", "--fail", "--silent", url])
def numbers(x, path="$"):
if isinstance(x, bool):
return
if isinstance(x, (int, float)):
yield path, x
elif isinstance(x, list):
for i, v in enumerate(x):
yield from numbers(v, f"{path}[{i}]")
elif isinstance(x, dict):
for k, v in x.items():
yield from numbers(v, f"{path}.{k}")
for path in ("compress/data/nf.json", "compress/data/spec.json"):
data = json.loads(fetch(path))
vals = list(numbers(data))
negatives = [(p, v) for p, v in vals if v < 0]
print(path, "numeric_values=", len(vals), "minimum=", min(v for _, v in vals))
print("negative_values=", negatives[:10], "negative_count=", len(negatives))
assert not negatives
print("All numeric values used by the upstream compression inputs are non-negative.")
PYRepository: StrobeLabs/eth.zig
Length of output: 422
Document the non-negative accumulator precondition.
Encoder.unsorted_deltas reconstructs the original non-negative codepoints and run lengths. Valid upstream blobs therefore do not produce a negative prev. Remove the claim that intermediate sums may go negative and document that decoded values must be non-negative. No i64 storage change is needed.
🤖 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 `@src/ens/normalize/decoder.zig` around lines 109 - 140, Update the
documentation for readUnsortedDeltas to remove the claim that intermediate sums
may be negative and state that decoded accumulated values must remain
non-negative for valid input. Keep the existing i64 accumulator implementation
in readArray unchanged.
| const response = provider.call(UNIVERSAL_RESOLVER, outer) catch { | ||
| return mapRevert(provider); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Consult lastError() only for revert failures.
This catch block discards the error value and always reads provider.lastError(). Provider.call allocates its params before rpcCall resets last_error (see src/provider.zig lines 200-210 and 351-357). If that allocation fails, lastError() still holds the previous call's revert data. mapRevert can then map a stale ResolverNotFound to null, so resolve reports "no record" for a call that never reached the chain.
Gate the mapping on error.RpcError. The test double already returns error.RpcError, so the current tests stay green.
🐛 Proposed fix
- const response = provider.call(UNIVERSAL_RESOLVER, outer) catch {
- return mapRevert(provider);
- };
+ const response = provider.call(UNIVERSAL_RESOLVER, outer) catch |e| {
+ // Only a revert carries diagnostics; any other failure would read a
+ // stale `lastError()` from an earlier call.
+ if (e != error.RpcError) return error.ProviderError;
+ return mapRevert(provider);
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = provider.call(UNIVERSAL_RESOLVER, outer) catch { | |
| return mapRevert(provider); | |
| }; | |
| const response = provider.call(UNIVERSAL_RESOLVER, outer) catch |e| { | |
| // Only a revert carries diagnostics; any other failure would read a | |
| // stale `lastError()` from an earlier call. | |
| if (e != error.RpcError) return error.ProviderError; | |
| return mapRevert(provider); | |
| }; |
🤖 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 `@src/ens/resolver.zig` around lines 170 - 172, Update the catch block around
provider.call in the resolver flow to inspect the caught error and call
mapRevert(provider) only when it is error.RpcError; propagate all other errors
directly so allocation failures cannot consume stale provider.lastError() data.
| const name = decoded[0].string; | ||
| if (name.len == 0) return null; | ||
|
|
||
| if (name_response.len < 64) return ReverseResolveError.InvalidResponse; | ||
| return try ens_normalize.normalize(allocator, name); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject an unnormalized primary name instead of rewriting it.
The Universal Resolver verifies the forward record of the primary name exactly as stored. ENS documents that if the primary name is unnormalized, for example normalize("Nick.eth") != "nick.eth", then name and resolver are invalid.
Line 92 normalizes the returned name unconditionally. For a stored primary name of "Nick.eth", the UR verified the namehash of "Nick.eth", but this function returns "nick.eth" — a different name whose forward record was never verified. A caller then displays a verified-looking name for an unverified record.
Compare the normalized form against the returned name and return null on mismatch.
🔒️ Proposed fix
- return try ens_normalize.normalize(allocator, name);
+ // ENSIP-23: the UR verifies the forward record for the name exactly as
+ // stored. An unnormalized primary name is invalid, so do not rewrite it.
+ const normalized = try ens_normalize.normalize(allocator, name);
+ if (!std.mem.eql(u8, normalized, name)) {
+ allocator.free(normalized);
+ return null;
+ }
+ return normalized;Note that the test at lines 267-281 asserts the current rewriting behavior. Update it to expect null for "Vitalik.ETH".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const name = decoded[0].string; | |
| if (name.len == 0) return null; | |
| if (name_response.len < 64) return ReverseResolveError.InvalidResponse; | |
| return try ens_normalize.normalize(allocator, name); | |
| const name = decoded[0].string; | |
| if (name.len == 0) return null; | |
| // ENSIP-23: the UR verifies the forward record for the name exactly as | |
| // stored. An unnormalized primary name is invalid, so do not rewrite it. | |
| const normalized = try ens_normalize.normalize(allocator, name); | |
| if (!std.mem.eql(u8, normalized, name)) { | |
| allocator.free(normalized); | |
| return null; | |
| } | |
| return normalized; |
🤖 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 `@src/ens/reverse.zig` around lines 89 - 92, Update the primary-name handling
after decoding in the reverse-resolution function: normalize the decoded name
only for comparison, return null when the normalized form differs from the
stored name, and return the original name when they match. Update the test
covering “Vitalik.ETH” to expect null instead of the rewritten lowercase name.
| /// Encode `(string name, address resolvedAddress, address resolver)` as the | ||
| /// UR's `reverse(bytes,uint256)` would return it. | ||
| fn encodeReverseTuple(allocator: std.mem.Allocator, name: []const u8, resolved: Address, resolver_addr: Address) ![]u8 { | ||
| const values = [_]AbiValue{ .{ .string = name }, .{ .address = resolved }, .{ .address = resolver_addr } }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the reverse tuple field names.
The UR returns (string primary, address resolver, address reverseResolver). The second value is the forward resolver address, not the resolved address. The doc comment and the resolved parameter name state otherwise. lookupAddress ignores both addresses today, so behavior is unaffected, but the wrong name invites a future change that reads decoded[1] as the resolved address for verification.
♻️ Proposed fix
-/// Encode `(string name, address resolvedAddress, address resolver)` as the
-/// UR's `reverse(bytes,uint256)` would return it.
-fn encodeReverseTuple(allocator: std.mem.Allocator, name: []const u8, resolved: Address, resolver_addr: Address) ![]u8 {
- const values = [_]AbiValue{ .{ .string = name }, .{ .address = resolved }, .{ .address = resolver_addr } };
+/// Encode `(string primary, address resolver, address reverseResolver)` as the
+/// UR's `reverse(bytes,uint256)` returns it.
+fn encodeReverseTuple(allocator: std.mem.Allocator, name: []const u8, resolver_addr: Address, reverse_resolver_addr: Address) ![]u8 {
+ const values = [_]AbiValue{ .{ .string = name }, .{ .address = resolver_addr }, .{ .address = reverse_resolver_addr } };
return abi_encode.encodeValues(allocator, &values);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Encode `(string name, address resolvedAddress, address resolver)` as the | |
| /// UR's `reverse(bytes,uint256)` would return it. | |
| fn encodeReverseTuple(allocator: std.mem.Allocator, name: []const u8, resolved: Address, resolver_addr: Address) ![]u8 { | |
| const values = [_]AbiValue{ .{ .string = name }, .{ .address = resolved }, .{ .address = resolver_addr } }; | |
| /// Encode `(string primary, address resolver, address reverseResolver)` as the | |
| /// UR's `reverse(bytes,uint256)` returns it. | |
| fn encodeReverseTuple(allocator: std.mem.Allocator, name: []const u8, resolver_addr: Address, reverse_resolver_addr: Address) ![]u8 { | |
| const values = [_]AbiValue{ .{ .string = name }, .{ .address = resolver_addr }, .{ .address = reverse_resolver_addr } }; | |
| return abi_encode.encodeValues(allocator, &values); |
🤖 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 `@src/ens/reverse.zig` around lines 156 - 159, Update the encodeReverseTuple
documentation and parameter names to match the UR tuple order: primary name,
forward resolver address, and reverseResolver address. Rename the resolved
parameter and its corresponding AbiValue usage without changing tuple encoding
behavior or lookupAddress.
|
|
||
| const private_key = try eth.hex.hexToBytesFixed(32, ACCOUNT_0_KEY_HEX); | ||
| const wallet = eth.wallet.Wallet.init(allocator, private_key, &provider); | ||
| const wallet = eth.wallet.Wallet.initLocal(allocator, private_key, &provider); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify Wallet.initLocal exists and find any remaining Wallet.init callers.
set -uo pipefail
echo "=== Wallet constructors ==="
fd -t f 'wallet.zig' src | while IFS= read -r f; do
echo "--- $f ---"
rg -nP '\bpub fn init[A-Za-z]*\s*\(' "$f"
done
echo "=== callers ==="
rg -nP '\bWallet\.init[A-Za-z]*\s*\(' -g '!**/zig-cache/**' .
echo "=== docs/examples references ==="
rg -nP 'Wallet\.init\b' docs README.md 2>/dev/nullRepository: StrobeLabs/eth.zig
Length of output: 2898
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== constructor definitions ==="
cat -n src/wallet.zig | sed -n '35,75p'
echo "=== remaining Wallet.init call sites with context ==="
rg -n -C 2 'Wallet\.init\b' README.md docs examples tests srcRepository: StrobeLabs/eth.zig
Length of output: 5245
Use Wallet.initLocal for raw private keys.
Wallet.init still accepts a signer_mod.Signer; it was not renamed. Update the private-key calls in README.md, docs/content/docs/, and examples/04_send_transaction.zig. The tests/integration_tests.zig call is correct.
🤖 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 `@tests/integration_tests.zig` at line 196, Update the private-key wallet
initialization calls in README.md, docs/content/docs/, and
examples/04_send_transaction.zig to use eth.wallet.Wallet.initLocal rather than
Wallet.init; preserve Wallet.init for signer_mod.Signer inputs, and leave the
already-correct tests/integration_tests.zig call unchanged.
1 participant
Summary
zig build vector-test.resolve,getText, newgetContentHash) migrated to the Universal Resolver (0xeEeEEEeE14D718C2B47D9923Deab1335E144EeEe, mainnet + Sepolia) viaresolve(bytes,bytes), with UR custom-error reverts mapped tonull/ typed errors (selectors verified withcast sig).reverse(bytes,uint256)with on-chain forward verification; returned primary names are normalized before being handed to callers.Qm...) per EIP-1577's canonical example; see the docs for the cross-library string-compat caveat.lastError()now captures JSON-RPC revert data so contract custom errors can be decoded.ens_registrychain metadata; zero new external dependencies (spec tables are 36 KB of vendored MIT-licensed data).Testing
zig build vector-test).ETH_RPC_URL(skip when unset); fixtures independently verified against mainnet viacast.Not included (follow-ups)
error.OffchainLookupRequired. Follow-up issue to be filed on merge.beautify(), comptime table decode,tokenize()introspection.Closes #110. Supersedes #109 -- thanks @yashgo0018 for the report and the initial PR.
Summary by CodeRabbit
New Features
Documentation
Tests