Add ~90 cross-validation tests for alloy.rs parity#5
Merged
Conversation
📝 WalkthroughWalkthroughAdds extensive test cases across many modules (ABI encode/decode, EIP-712, Keccak, secp256k1, HD wallet, mnemonic, hex, primitives, RLP, transactions, uint256, units, ENS). No production code or public API signatures changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (5)
src/utils/units.zig (1)
57-59: Precision concern with large value test.The value
10_000_000_000_000_000_000_000(10^22) exceedsf64's exact integer representation range (~9×10^15). While this test may pass due to favorable rounding, it's testing in a range whereparseEtherinherently loses precision. Consider either:
- Adding a comment acknowledging this tests the precision boundary
- Using a value within f64's exact range (e.g.,
parseEther(9000.0)→ 9×10^21 is still borderline)The existing code documents this limitation for
formatEther/formatGweibut not forparseEther/parseGwei.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/units.zig` around lines 57 - 59, The test "parseEther large value" uses 10_000_000_000_000_000_000_000 which is beyond f64's exact integer range and can produce misleading precision behavior in parseEther; update the test to either (A) replace the input with a value within f64's exact integer range (e.g., use 9000.0 or another safe decimal) or (B) keep the large value but add a clear comment above the test and update the module docs to note that parseEther/parseGwei lose precision for very large values due to f64 limits; reference the test "parseEther large value" and the parseEther/parseGwei functions when making the change so reviewers can find and verify the fix.src/secp256k1.zig (1)
501-537: Reuse existing low-S helper to avoid duplicated comparison logic.This test currently duplicates half-order bytes and manual lexicographic comparison already represented by
HALF_ORDER_BYTES+isHighS.♻️ Suggested simplification
- // n/2 = 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0 - const half_n: [32]u8 = .{ - 0x7F, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, - 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, - 0x5D, 0x57, 0x6E, 0x73, 0x57, 0xA4, 0x50, 0x1D, - 0xDF, 0xE9, 0x2F, 0x46, 0x68, 0x1B, 0x20, 0xA0, - }; @@ - // Extract s as big-endian bytes and verify s <= n/2 - const s_bytes = sig.s; - var s_is_lte = false; - for (0..32) |i| { - if (s_bytes[i] < half_n[i]) { - s_is_lte = true; - break; - } else if (s_bytes[i] > half_n[i]) { - break; - } - } - // If we didn't break early, all bytes were equal (s == n/2), which is also valid - if (!s_is_lte) { - // Check if all bytes are equal (s == n/2) - s_is_lte = std.mem.eql(u8, &s_bytes, &half_n); - } - try std.testing.expect(s_is_lte); + const s_scalar = Scalar.fromBytes(sig.s, .big) catch unreachable; + try std.testing.expect(!isHighS(s_scalar));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/secp256k1.zig` around lines 501 - 537, The test "sign produces low-s (EIP-2) canonical signature" duplicates the half-order byte constant and manual lexicographic comparison; replace that logic by reusing the existing HALF_ORDER_BYTES constant and the isHighS function to check canonical low-S. Locate the test and remove the local half_n and byte-by-byte comparison, then call isHighS(sig.s) (or its inverse) alongside HALF_ORDER_BYTES to assert the signature is low-s, keeping the rest of the test (message hashing and sign call) unchanged.src/hd_wallet.zig (1)
230-258: Strengthen “known seed” test with exact expected key/chain-code assertions.For parity confidence, this test should pin exact BIP-32 vector outputs instead of only non-zero and deterministic checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hd_wallet.zig` around lines 230 - 258, The test "BIP-32 master key from known seed" currently only checks non-zero and deterministic outputs; replace or augment those checks with explicit expected BIP-32 vector values: call masterKeyFromSeed(seed) and assert master.key equals the known expected master private key bytes and master.chain_code equals the known expected chain code bytes (use the same fixed seed used in the test). Update the test to use try std.testing.expectEqualSlices(u8, &master.key, &expected_key) and try std.testing.expectEqualSlices(u8, &master.chain_code, &expected_chain_code) (keeping masterKeyFromSeed, master.key, master.chain_code and the test name intact) so the test pins exact BIP-32 vector outputs.src/eip712.zig (1)
1004-1066: Add canonical expected digest assertions for typed-data vectors.These tests currently validate determinism but not correctness against external vectors. For parity objectives, pin expected final hashes for the Permit and Permit2 cases.
Also applies to: 1134-1206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eip712.zig` around lines 1004 - 1066, The test "ERC20 Permit typed data hash" currently only checks determinism (hash1 == hash2); update it to assert the computed hash matches canonical expected EIP-712 digest vectors by adding fixed expected 32-byte digests and comparing hash1 against them; locate the test block that constructs DomainSeparator, permit_type (TypeDef), permit_msg (StructValue) and calls hashTypedData(allocator, domain, permit_msg, &type_defs) and add expectations like testing.expectEqualSlices(u8, &hash1, &expected_permit_hash) (and do the same for the Permit2 test referenced around lines 1134-1206) so the test verifies correctness against known vectors.src/transaction.zig (1)
845-870: Assert exact expected bytes for alloy vector tests, not only shape checks.In
legacy tx alloy.rs test vector, checks like determinism +payload.len > 40can still pass with subtly wrong encodings. Consider pinning full expected payload (or signing hash) for true byte-exact parity validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/eip712.zig`:
- Around line 1004-1066: The test "ERC20 Permit typed data hash" currently only
checks determinism (hash1 == hash2); update it to assert the computed hash
matches canonical expected EIP-712 digest vectors by adding fixed expected
32-byte digests and comparing hash1 against them; locate the test block that
constructs DomainSeparator, permit_type (TypeDef), permit_msg (StructValue) and
calls hashTypedData(allocator, domain, permit_msg, &type_defs) and add
expectations like testing.expectEqualSlices(u8, &hash1, &expected_permit_hash)
(and do the same for the Permit2 test referenced around lines 1134-1206) so the
test verifies correctness against known vectors.
In `@src/hd_wallet.zig`:
- Around line 230-258: The test "BIP-32 master key from known seed" currently
only checks non-zero and deterministic outputs; replace or augment those checks
with explicit expected BIP-32 vector values: call masterKeyFromSeed(seed) and
assert master.key equals the known expected master private key bytes and
master.chain_code equals the known expected chain code bytes (use the same fixed
seed used in the test). Update the test to use try
std.testing.expectEqualSlices(u8, &master.key, &expected_key) and try
std.testing.expectEqualSlices(u8, &master.chain_code, &expected_chain_code)
(keeping masterKeyFromSeed, master.key, master.chain_code and the test name
intact) so the test pins exact BIP-32 vector outputs.
In `@src/secp256k1.zig`:
- Around line 501-537: The test "sign produces low-s (EIP-2) canonical
signature" duplicates the half-order byte constant and manual lexicographic
comparison; replace that logic by reusing the existing HALF_ORDER_BYTES constant
and the isHighS function to check canonical low-S. Locate the test and remove
the local half_n and byte-by-byte comparison, then call isHighS(sig.s) (or its
inverse) alongside HALF_ORDER_BYTES to assert the signature is low-s, keeping
the rest of the test (message hashing and sign call) unchanged.
In `@src/utils/units.zig`:
- Around line 57-59: The test "parseEther large value" uses
10_000_000_000_000_000_000_000 which is beyond f64's exact integer range and can
produce misleading precision behavior in parseEther; update the test to either
(A) replace the input with a value within f64's exact integer range (e.g., use
9000.0 or another safe decimal) or (B) keep the large value but add a clear
comment above the test and update the module docs to note that
parseEther/parseGwei lose precision for very large values due to f64 limits;
reference the test "parseEther large value" and the parseEther/parseGwei
functions when making the change so reviewers can find and verify the fix.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/abi_decode.zigsrc/abi_encode.zigsrc/eip712.zigsrc/ens/namehash.zigsrc/hd_wallet.zigsrc/hex.zigsrc/keccak.zigsrc/mnemonic.zigsrc/primitives.zigsrc/rlp.zigsrc/secp256k1.zigsrc/signer.zigsrc/transaction.zigsrc/uint256.zigsrc/utils/units.zig
Adds comprehensive test vectors from Solidity ABI spec, EIP-55/712/137, BIP-32/39, Hardhat accounts, and ethereum/tests to validate byte-exact accuracy parity with alloy.rs across all core functionality. Coverage spans ABI encode/decode, RLP, keccak256, secp256k1 signing, transaction serialization, EIP-712 typed data, HD wallet derivation, mnemonic seed generation, ENS namehash, address checksums, hex codec, and unit conversions.
koko1123
force-pushed
the
test/alloy-parity-validation
branch
from
a6808c5 to
0ee6308
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hd_wallet.zig (1)
230-258: Strengthen this test with exact expected master key + chain code bytes.This currently checks non-zero and determinism only, which can still pass with an incorrect derivation. For parity-focused coverage, assert exact expected outputs from a canonical vector.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hd_wallet.zig` around lines 230 - 258, Replace the weak non-zero checks in the "BIP-32 master key from known seed" test by asserting the exact expected master private key and chain code bytes from a canonical BIP-32 test vector: compute the expected 32-byte key and 32-byte chain_code (use hex_mod.hexToBytesFixed to decode hex literals into arrays) and then call std.testing.expectEqualSlices(u8, &master.key, &expected_key) and std.testing.expectEqualSlices(u8, &master.chain_code, &expected_chain_code) after deriving master via masterKeyFromSeed to ensure exact-match validation; keep the existing determinism check if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/hd_wallet.zig`:
- Around line 230-258: Replace the weak non-zero checks in the "BIP-32 master
key from known seed" test by asserting the exact expected master private key and
chain code bytes from a canonical BIP-32 test vector: compute the expected
32-byte key and 32-byte chain_code (use hex_mod.hexToBytesFixed to decode hex
literals into arrays) and then call std.testing.expectEqualSlices(u8,
&master.key, &expected_key) and std.testing.expectEqualSlices(u8,
&master.chain_code, &expected_chain_code) after deriving master via
masterKeyFromSeed to ensure exact-match validation; keep the existing
determinism check if desired.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/abi_decode.zigsrc/abi_encode.zigsrc/eip712.zigsrc/ens/namehash.zigsrc/hd_wallet.zigsrc/hex.zigsrc/keccak.zigsrc/mnemonic.zigsrc/primitives.zigsrc/rlp.zigsrc/secp256k1.zigsrc/signer.zigsrc/transaction.zigsrc/uint256.zigsrc/utils/units.zig
🚧 Files skipped from review as they are similar to previous changes (8)
- src/primitives.zig
- src/eip712.zig
- src/secp256k1.zig
- src/abi_decode.zig
- src/rlp.zig
- src/mnemonic.zig
- src/ens/namehash.zig
- src/signer.zig
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
1 participant
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
Test plan
zig build testpasses with all ~1068 inline tests (0 failures)🤖 Generated with Claude Code
Summary by CodeRabbit