feat(contracts): add Uint256 and Vector32 modules, remove archived Uint256, part 2/3 - #291
Conversation
|
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 removes archived Uint256 utilities and introduces new, active implementations of Uint256 and Vector32 modules with comparison and conversion circuits. It updates the format script configuration, adds comprehensive test suites and mock simulators, and applies various formatting refinements across the codebase. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
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)
Comment |
0xisk
changed the title
2/3
01485c0 to
97bf030
Compare
![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/test/Uint64.test.ts`:
- Around line 139-170: The test file contains a duplicate describe('AddChecked',
...) block that repeats the same six tests against uint64Simulator.addChecked
and MAX_UINT64; remove the second (duplicate) describe('AddChecked', ...) block
so only one AddChecked suite remains to avoid redundant tests and duplicate test
names.
🧹 Nitpick comments (4)
contracts/src/math/Uint256.compact (2)
47-62: Inconsistentpuremodifier betweenZERO_U256andMAX_U256.
ZERO_U256(line 47) is not marked aspure, whileMAX_U256(line 60) is marked aspure. Both circuits only perform struct construction with constant values and have no side effects. Consider adding thepuremodifier toZERO_U256for consistency and potential optimization.♻️ Proposed fix
- export circuit ZERO_U256(): U256 { + export pure circuit ZERO_U256(): U256 { return U256 { low: U128 { low: 0, high: 0 }, high: U128 { low: 0, high: 0 } }; }
213-215: Consider optimizinglteandgteto avoid redundant comparisons.Both
lteandgtecall their respectivelt/gtpluseq. Sinceltandgtalready perform partial equality checks internally (viaUint128_eqU128for high parts), there may be circuit-level redundancy. However, the current implementation is straightforward and correct.If circuit row count becomes a concern, you could implement
lteas!gt(a, b)andgteas!lt(a, b), which avoids callingeqseparately. This is a minor optimization suggestion.Also applies to: 261-263
contracts/src/math/test/mocks/contracts/Vector32.mock.compact (1)
17-25: Inconsistent use ofdisclose()betweentoU256andtoBytes.
toU256(line 19) wraps the result withdisclose(), buttoBytes(line 24) does not. In the sourceVector32.compact,toU256already usesdisclose()internally (line 119), so the mock is adding a redundantdisclose(). Meanwhile,toBytesin the source does not usedisclose().Consider either:
- Removing the redundant
disclose()fromtoU256in the mock, or- Adding
disclose()totoBytesif the intent is to expose all outputs for circuit metadata reporting.contracts/src/math/test/mocks/contracts/Uint256.mock.compact (1)
4-4: Inconsistent pragma version:0.18.0vs0.20.0in other files.This file uses
pragma language_version >= 0.18.0(line 4), while the production modules (Uint256.compact,Vector32.compact) and the other mock (Vector32.mock.compact) usepragma language_version >= 0.20.0. Consider updating to0.20.0for consistency.♻️ Proposed fix
-pragma language_version >= 0.18.0; +pragma language_version >= 0.20.0;
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
@0xisk looks really good! I left a few minor comments and suggestions :)
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>
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
Almost there! Left a suggestion and a suggestion/question
| export pure circuit toUint64( | ||
| b0: Uint<8>, | ||
| b1: Uint<8>, | ||
| b2: Uint<8>, | ||
| b3: Uint<8>, | ||
| b4: Uint<8>, | ||
| b5: Uint<8>, | ||
| b6: Uint<8>, | ||
| b7: Uint<8> | ||
| ): Uint<64> { |
There was a problem hiding this comment.
Good call with abstracting this out! I'm wondering if it's better to keep this separate or if we can just add this to Vector8 because there's this similar circuit
export pure circuit toUint64(vec: Vector<8, Uint<8>>): Uint<64> {
const [b0, b1, b2, b3, b4, b5, b6, b7] = vec;
return b0 +
b1 * 0x100 +
b2 * 0x10000 +
b3 * 0x1000000 +
b4 * 0x100000000 +
b5 * 0x10000000000 +
b6 * 0x1000000000000 +
b7 * 0x100000000000000;
}
What do you think about overloading the toUint64 circuit name so we can have a single module that offers both? They're doing the same thing, they just have different sigs
There was a problem hiding this comment.
If you like the idea, I'd probably change the module name to something more generic though...Pack64 or something. lmk what you think
There was a problem hiding this comment.
That's a great point! I did those changes following on that:
-
Module Consolidation
- Removed Vector8.compact and Vector32.compact modules
- Renamed Vector32 → Bytes32 with expanded functionality
- Enhanced Bytes8 with pack/unpack operations (previously only had toUint64)
-
Naming Convention Standardization
- BytesN modules: Use
pack(vec)andunpack(bytes)for Vector ↔ Bytes conversions - UintN modules: Use
toBytes(value)for packed bytes andtoUnpackedBytes(value)for vector representation - Renamed
Uint64_toVector→Uint64_toUnpackedBytesfor clarity - Renamed
Uint256_toVector→Uint256_.toUnpackedBytesfor consistency
- BytesN modules: Use
-
Circuit Additions
- Bytes8: Added
pack(),unpack(), and overloadedtoUint64()circuits - Bytes32: Added
pack(),unpack(), and overloadedtoU256()circuits - Both modules now support conversions from both Vector and Bytes inputs
- Bytes8: Added
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.
Tiny tidy up details and we're good to go! The main thing is the bad witness tests
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>
Co-authored-by: Andrew Fleming <fleming.andrew@protonmail.com> Signed-off-by: 0xisk <0xisk@proton.me>
|
@andrew-fleming thank you for the review. I covered all the missing cases for tests. Also took the chance to refactor Uint128 module circuits with overloading. Let me know what do you think? and I was thinking to rename circuits |
andrew-fleming
left a comment
andrew-fleming
left a comment
There was a problem hiding this comment.
LGTM @0xisk! I left a copy tiny suggestions, but we can address them in another PR to not stall this any further if you'd like. Happy to re-approve if not
| test('bytesToUint64 should fail when witness returns pack(vec) != bytes', () => { | ||
| bytes8Simulator.overrideWitness('wit_unpackBytes8', (context, _bytes) => [ | ||
| context.privateState, | ||
| bytes(0, 0, 0, 0, 0, 0, 0, 0), | ||
| ]); | ||
| const packed = new Uint8Array(8); | ||
| packed[0] = 1; | ||
| expect(() => bytes8Simulator.bytesToUint64(packed)).toThrow( | ||
| 'failed assert: Bytes8: unpack verification failed', | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Nit: I'd move this to the bytesToUint64 block
| test('bytesToU256 should fail when witness returns pack(vec) != bytes', () => { | ||
| bytes32Simulator.overrideWitness( | ||
| 'wit_unpackBytes32', | ||
| (context, _bytes) => [ | ||
| context.privateState, | ||
| new Array<bigint>(32).fill(0n), | ||
| ], | ||
| ); | ||
| const bytes = new Uint8Array(32); | ||
| bytes[0] = 1; | ||
| expect(() => bytes32Simulator.bytesToU256(bytes)).toThrow( | ||
| 'failed assert: Bytes32: unpack verification failed', | ||
| ); |
There was a problem hiding this comment.
I'd move this to the bytesToU256 block
| test('should fail when witness returns remainder >= divisor', () => { | ||
| uint128Simulator.overrideWitness('wit_divU128', (context) => [ | ||
| context.privateState, | ||
| { quotient: { low: 1n, high: 0n }, remainder: { low: 5n, high: 0n } }, | ||
| ]); | ||
| expect(() => uint128Simulator.divRemU128(u128(10n), u128(5n))).toThrow( | ||
| 'failed assert: Uint128: remainder error', | ||
| ); | ||
| }); | ||
|
|
||
| test('should fail when witness returns quotient * b + remainder != a', () => { | ||
| uint128Simulator.overrideWitness('wit_divU128', (context) => [ | ||
| context.privateState, | ||
| { quotient: { low: 2n, high: 0n }, remainder: { low: 0n, high: 0n } }, | ||
| ]); |
Thank you @andrew-fleming! I will address this in the 3rd part.
Also @andrew-fleming let me know wdyt here? |
2 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
Introduce the U256 struct foundation and 32-element vector conversions:
PR Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...
Summary by CodeRabbit
Release Notes
New Features
Chores