feat(contracts): optimize Uint64/Uint128 to 0.28.0, introduce Vector8, part 1/3 - #290
Conversation
- Upgrade Uint64 and Uint128 compact modules to 0.28.0 API - Add new Vector8 and Bytes8 compact modules with full test suites - Extract shared witness infrastructure (types, sqrt, div, conversion helpers) - Include mock contracts, simulators, and witnesses for all modules
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces new byte and vector conversion modules (Bytes8, Vector8) for 8-byte little-endian operations, refactors witness naming convention across Uint128 and Uint64 from local suffixes to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
Consolidate scattered "Theoretical Description:" and "Mathematical Steps:" sections into unified @remarks tags for cleaner documentation structure in Uint64 and Uint128 modules.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@contracts/src/math/Bytes8.compact`:
- Line 21: The import { toUint64 } from "./Vector8" is unused in Bytes8.compact;
either remove that unused import line or change the Bytes8.compact circuit named
toUint64 to call the Vector8-provided conversion (e.g., use Vector8.toUint64 /
Vector8_toUint64 on the bytes cast to a Vector8) so the implementation matches
the documentation; update the import usage or delete the import accordingly and
ensure the toUint64 circuit in this file references the Vector8 symbol if you
choose the second option.
🧹 Nitpick comments (6)
contracts/src/math/witnesses/types.ts (2)
46-49: Consider masking high bits for defensive consistency.If
valueexceeds 128 bits, thehighfield will contain more than 64 bits. While witnesses should receive valid inputs from circuits, masking both components would ensure well-formed U128 values.♻️ Suggested defensive masking
export const toU128 = (value: bigint): U128 => ({ low: value & UINT64_MASK, - high: value >> 64n, + high: (value >> 64n) & UINT64_MASK, });
51-52: Minor: RedundantBigInt()calls.Since
value.highandvalue.loware already typed asbigint, theBigInt()wrappers are unnecessary.♻️ Simplified version
export const toBigint = (value: U128): bigint => - (BigInt(value.high) << 64n) + BigInt(value.low); + (value.high << 64n) + value.low;contracts/src/math/witnesses/wit_divUint64.ts (1)
9-16: Division by zero will throw an unhandledRangeError.If
divisoris0n, the native bigint division throwsRangeError: Division by zero. While the circuit should validate the divisor before calling this witness, an explicit check would provide a clearer error message for debugging.🛡️ Suggested defensive check
export const wit_divUint64 = ( dividend: bigint, divisor: bigint, ): DivResultU64 => { + if (divisor === 0n) { + throw new Error('wit_divUint64: division by zero'); + } const quotient = dividend / divisor; const remainder = dividend % divisor; return { quotient, remainder }; };contracts/src/math/witnesses/wit_divU128.ts (1)
9-18: Division by zero will throw ifbis zero.Same concern as
wit_divUint64: iftoBigint(b)returns0n, division throwsRangeError. Consider adding an explicit check for clearer error messages.🛡️ Suggested defensive check
export const wit_divU128 = (a: U128, b: U128): DivResultU128 => { const aValue = toBigint(a); const bValue = toBigint(b); + if (bValue === 0n) { + throw new Error('wit_divU128: division by zero'); + } const quotient = aValue / bValue; const remainder = aValue - quotient * bValue; return { quotient: toU128(quotient), remainder: toU128(remainder), }; };contracts/src/math/witnesses/wit_divUint128.ts (1)
12-19: Division by zero will throw ifbis0n.Consistent with the other division witnesses, consider adding an explicit zero check for clearer debugging.
🛡️ Suggested defensive check
export const wit_divUint128 = (a: bigint, b: bigint): DivResultU128 => { + if (b === 0n) { + throw new Error('wit_divUint128: division by zero'); + } const quotient = a / b; const remainder = a - quotient * b; return { quotient: toU128(quotient), remainder: toU128(remainder), }; };contracts/src/math/test/Uint64.test.ts (1)
333-341: Remove duplicate divRem remainder test.Line 333-341 repeats the remainder ≥ divisor case already covered at Line 313-321; consider dropping to reduce redundancy.
🧹 Suggested cleanup
- test('should fail when remainder >= divisor (duplicate)', () => { - uint64Simulator.overrideWitness('wit_divUint64', (context) => [ - context.privateState, - { quotient: 1n, remainder: 10n }, - ]); - expect(() => uint64Simulator.divRem(10n, 5n)).toThrow( - 'failed assert: Math: remainder error', - ); - });
Update @circuitInfo annotations across all Part 1 math modules with actual k and rows values from compiled mock contracts: - Uint64: 18 circuits updated (MAX constants, arithmetic, division, sqrt, etc.) - Uint128: 42 circuits updated (comparisons, arithmetic, division, sqrt, etc.) - Vector8: 2 circuits updated (toUint64, toBytes) - Bytes8: 2 circuits updated (toUint64, toVector) All values verified by compiling corresponding .mock.compact test contracts.
Bytes8 is a leaf node with no downstream consumers — its two circuits are trivial casts (bytes as Uint<64>, bytes as Vector<8, Uint<8>>) that any consumer can do inline. Removing to keep the PR focused.
0xisk
changed the title
0xisk
changed the title
That's funny actually 😅 |
0xisk
changed the title
0xisk
changed the title
1
0xisk
changed the title
11/3
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
Looking good, @0xisk! Left some comments
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com> Signed-off-by: 0xisk <0xisk@proton.me>
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com> Signed-off-by: 0xisk <0xisk@proton.me>
Remove 4 unnecessary assertions from Uint128 module that add no value: - Removed 3 mathematically unreachable defensive checks in _mul and _sqrt circuits that can never fail with valid inputs - Removed 1 redundant division-by-zero check in _isMultiple already performed by _div Updated corresponding @throws documentation and circuit info annotations.
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com> Signed-off-by: 0xisk <0xisk@proton.me>
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
Nice work on the improvements 🚀 LGTM!
| /** | ||
| * @description Factory function creating witness implementations for Uint128 module operations. | ||
| */ | ||
| export const Uint128Witnesses = (): Witnesses<Uint128PrivateState> => ({ | ||
| wit_sqrtU128(_context, radicand) { | ||
| return [{}, wit_sqrtU128(radicand)]; | ||
| }, | ||
|
|
||
| wit_divU128(_context, a, b) { | ||
| return [{}, wit_divU128(a, b)]; | ||
| }, | ||
|
|
||
| wit_divUint128(_context, a, b) { | ||
| return [{}, wit_divUint128(a, b)]; | ||
| }, | ||
| }); |
3 participants
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyPart of #279 (Part 1 of 2 of that main draft PR: #289 )
Note: This PR is part 1 of a larger upgrade that was split into two stacked PRs for easier review. Part 2 (coming soon) will add the 256-bit modules (Uint256, Bytes32, Vector32, Field255) that build on this foundation.
Upgrades Uint64/Uint128 to 0.28.0 API and introduces Vector8 as a foundational building block for efficient integer ↔ Vector conversions. Vector8 provides the core primitive (
Vector<8, Uint<8>>↔Uint<64>) using pure arithmetic instead of expensive byte slicing, which will enable higher-level modules (Uint256, Field255) to split into 4x Uint64 limbs for efficient conversions in part 2.Implements witness-optimized verification pattern: compute conversions off-chain, verify in circuit/on-chain (e.g.,
toVector(toUint64(input)) == input).Changes:
Dependency Tree (Part 1 Scope)
graph TD subgraph "<b>Part 1: 8 / 64 / 128-bit Foundation</b>" Vector8 Uint64 Uint128 end subgraph "<b>Part 2: Coming Soon</b>" Vector32[Vector32] Uint256[Uint256] Bytes32[Bytes32] Field255[Field255] end Vector8 --> Uint64 Uint64 --> Uint128 Uint64 -.-> Uint256 Uint128 -.-> Uint256 Uint128 -.-> Field255 Uint256 -.-> Bytes32 Uint256 -.-> Field255 Bytes32 -.-> Field255 style Vector32 fill:#f0f0f0,stroke:#999,stroke-dasharray: 5 5 style Uint256 fill:#f0f0f0,stroke:#999,stroke-dasharray: 5 5 style Bytes32 fill:#f0f0f0,stroke:#999,stroke-dasharray: 5 5 style Field255 fill:#f0f0f0,stroke:#999,stroke-dasharray: 5 5PR Checklist