Skip to content

feat: noir data types #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Apr 8, 2025
Merged

feat: noir data types #2

merged 15 commits into from
Apr 8, 2025

Conversation

Envoy-VC
Copy link
Contributor

@Envoy-VC Envoy-VC commented Apr 8, 2025

Summary by CodeRabbit

  • New Features

    • Introduced enhanced data types for safer arithmetic, boolean, string, and bounded collection operations with robust conversion and serialization capabilities.
    • Added a new validation schema for field inputs using the Zod library.
    • Added new classes for fixed-size arrays, bounded vectors, field elements, and integer types, enhancing data manipulation capabilities.
  • Tests

    • Expanded automated test coverage to validate the new numeric and data handling features across a range of input scenarios and edge cases.
    • Introduced comprehensive test suites for the new Field and integer data types.
  • Chores

    • Updated dependency configurations and adjusted compiler options to improve build consistency and maintainability.
    • Enhanced public API by expanding exports from the data types module.

Copy link

coderabbitai bot commented Apr 8, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

This pull request introduces several new data type classes and validators, extending the module’s public API. New classes such as FixedSizeArray, Bool, BoundedVec, Field, Str, and an abstract AbstractInteger (with its signed and unsigned variants) have been added to support type-safe operations. In addition, new Zod-based schemas and helper types (including Bit and composite types within DataTypes) have been included along with comprehensive test suites for the Field and integer types. Minor configuration and export adjustments were also made in package.json, tsconfig.json, and index files.

Changes

File(s) Change Summary
package.json Consolidated the "files" field into a single-line format; added dependency "zod": "^3.24.2"; modified "json2toml" to include a trailing comma.
src/data-types/array.ts, src/data-types/bool.ts, src/data-types/bounded-vec.ts, src/data-types/field.ts, src/data-types/integer.ts, src/data-types/string.ts Introduced new classes: FixedSizeArray, Bool, BoundedVec, Field, AbstractInteger (with U8, U16, U32, U64, I8, I16, I32, I64), and Str, each with various methods for type safety, arithmetic, serialization, and conversion.
src/data-types/index.ts, src/data-types/zod/index.ts Added new types (DataType, StructMap), a toJSON conversion function, and Zod-based schemas/validators (including FieldValidator, IntegerValidator, and types FieldInput/IntegerInput).
src/index.ts, src/types/data-types.ts, src/types/index.ts Expanded module exports by adding new re-export statements and introduced a new type Bit (defined as `0
tests/data-types/field.test.ts, tests/data-types/integer.test.ts Added comprehensive test suites for the Field and integer data types covering instantiation, conversion, arithmetic operations, error handling, and cloning.
tsconfig.json Added a new compiler option "noUncheckedIndexedAccess": false to adjust TypeScript’s type-checking behavior for indexed access.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Field
    Client->>Field: new Field(input)
    Client->>Field: add(value)
    Field-->>Client: Returns new Field instance (sum)
    Client->>Field: toJSON()
    Field-->>Client: Returns JSON representation
Loading
sequenceDiagram
    participant Client
    participant BoundedVec
    Client->>BoundedVec: new BoundedVec(maxSize, defaultValueFactory)
    Client->>BoundedVec: push(item)
    BoundedVec-->>Client: Vector updated
    Client->>BoundedVec: pop()
    BoundedVec-->>Client: Returns last item
Loading

Poem

I’m a rabbit with a joyful leap,
Hop into new code that’s fresh and deep.
Arrays fixed and booleans shining bright,
Fields and vectors dancing in delight.
With bits and bytes, we bound through the night,
Celebrating every change with pure rabbit might!
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6be7867 and d91cfde.

📒 Files selected for processing (1)
  • src/data-types/field.ts (1 hunks)
✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (14)
src/data-types/bool.ts (1)

1-57: Well-designed Bool class with good documentation

The implementation provides type-safety by wrapping primitive boolean values with good documentation and appropriate methods. The class effectively encapsulates boolean operations with a clean API.

Consider adding more boolean algebra operations for completeness:

 export class Bool {
   /** The underlying boolean value */
   private readonly val: boolean;

   // Existing methods...

+  /**
+   * Performs a logical AND operation with another boolean.
+   * Returns a new boolean instance with the result.
+   *
+   * @param other - The boolean to AND with
+   * @returns A new boolean instance with the result of the AND operation
+   */
+  and(other: Bool): Bool {
+    return new Bool(this.val && other.val);
+  }
+
+  /**
+   * Performs a logical OR operation with another boolean.
+   * Returns a new boolean instance with the result.
+   *
+   * @param other - The boolean to OR with
+   * @returns A new boolean instance with the result of the OR operation
+   */
+  or(other: Bool): Bool {
+    return new Bool(this.val || other.val);
+  }
 }
src/data-types/index.ts (1)

20-51: Enhance error message in toJSON function

The toJSON function handles all data types correctly, but the error message for invalid types could be more descriptive to aid debugging.

- throw new Error('Invalid value type');
+ throw new Error(`Invalid value type: ${value === null ? 'null' : typeof value}`);
src/data-types/string.ts (2)

3-28: Well-documented string class implementation

The Str class is well-documented with clear JSDoc comments and provides proper encapsulation.

For consistency with the Bool class, consider making the val property readonly:

- private val: string;
+ private readonly val: string;

46-64: Complete implementation of basic string operations

The equality and serialization methods are correctly implemented.

Consider adding a length method for completeness:

 export class Str {
   // Existing methods...

+  /**
+   * Returns the length of the string.
+   *
+   * @returns The number of characters in the string
+   */
+  length(): number {
+    return this.val.length;
+  }
 }
tests/data-types/integer.test.ts (2)

5-5: Refine the test description to maintain consistency.

"should create a Unsigned Numbers" is slightly off grammatically. Consider updating to something like "should create unsigned numbers" (plural) or "should create an unsigned number" (singular) to improve clarity.


69-74: Extend wrapping arithmetic tests to other integer types.

Currently, wrapping tests exist only for U8. Consider adding similar tests for U16, U32, and U64 (and their signed counterparts, if relevant) to achieve more comprehensive coverage.

Also applies to: 75-80, 81-86

src/data-types/array.ts (2)

25-29: Consider using a custom error type for length mismatch.

Throwing a generic Error is fine, but a distinct error type (e.g., LengthMismatchError) or standardized message codes could improve debugging clarity if array construction fails.


67-78: Allow optional user-defined behavior for negative indices out of range.

at(index) throws an error if the adjusted index is out of bounds. This is correct for strict array usage, but some users might prefer undefined or a fallback value instead of throwing. Offering an optional mode or method variant could improve extensibility.

src/data-types/zod/index.ts (1)

25-30: Improve user-facing error messages with dynamic bounds.

While the current message is functional (Value must be in range [min, max]), you could consider including the user-provided input to speed up debugging or clarifying whether the range is inclusive/exclusive.

src/data-types/integer.ts (1)

46-57: Use a specialized error type for integer validation.

Relying solely on Zod errors is sufficient in many cases, but if the domain requires granular handling of out-of-range or type mismatch errors, adopting or extending custom error types might enhance maintainability.

src/data-types/bounded-vec.ts (1)

27-31: Consider offering a dynamic initialization option.
Currently, the constructor pre-fills an internal array up to maxSize with default values, then sets length to 0. This design provides predictable allocations but can be memory-inefficient for large maxSize when few elements are pushed. You could add an option to defer initialization until items are actually added, saving memory.

 constructor(maxSize: number, defaultValueFactory: () => T) {
-  this.maxSize = maxSize;
-  this.items = Array.from({ length: maxSize }, defaultValueFactory);
-  this.length = 0;
+  if (maxSize < 0) {
+    throw new Error('maxSize cannot be negative');
+  }
+  this.maxSize = maxSize;
+  // Initialize a smaller internal array; lazily fill if needed
+  this.items = [];
+  for (let i = 0; i < maxSize; i++) {
+    this.items.push(defaultValueFactory());
+  }
+  this.length = 0;
 }
src/data-types/field.ts (3)

127-128: Minor JSDoc mismatch ("0-length").
The documentation for toBeBytes says it returns an "array of bytes (0-length) in big-endian order," which may be confusing. Consider clarifying "0-based indexing" or removing the “0-length” phrase if unintended.


420-442: Remove or finalize commented-out code.
The static methods fromLeBytes and fromBeBytes are commented out. If they are no longer needed, removing them reduces clutter. Otherwise, finalizing them adds clarity to the codebase.

-// static fromLeBytes(bytes: number[]): Field {
-//   ...
-// }
+// Option 1: Remove entirely if not used
 // OR
+// Option 2: Uncomment and maintain if needed

356-362: Negative results are not reduced modulo the field.
The sub method throws an error if the result is negative. This is valid if you want to forbid negative intermediate values, but consider offering modular subtraction if you plan to remain within the 2^254 field arithmetic context.

- if (this.value < otherField.value) {
-   throw new Error('Result would be negative');
- }
- return new Field(this.value - otherField.value);
+ // Alternatively, do a mod-based approach:
+ let diff = (this.value - otherField.value) % Field.MODULUS;
+ if (diff < 0n) diff += Field.MODULUS;
+ return new Field(diff);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c65dd67 and 87d7a93.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (15)
  • package.json (2 hunks)
  • src/data-types/array.ts (1 hunks)
  • src/data-types/bool.ts (1 hunks)
  • src/data-types/bounded-vec.ts (1 hunks)
  • src/data-types/field.ts (1 hunks)
  • src/data-types/index.ts (1 hunks)
  • src/data-types/integer.ts (1 hunks)
  • src/data-types/string.ts (1 hunks)
  • src/data-types/zod/index.ts (1 hunks)
  • src/index.ts (1 hunks)
  • src/types/data-types.ts (1 hunks)
  • src/types/index.ts (1 hunks)
  • tests/data-types/field.test.ts (1 hunks)
  • tests/data-types/integer.test.ts (1 hunks)
  • tsconfig.json (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/data-types/string.ts (1)
src/data-types/integer.ts (1)
  • U8 (174-177)
src/data-types/zod/index.ts (1)
src/data-types/integer.ts (2)
  • min (21-23)
  • max (29-31)
tests/data-types/integer.test.ts (1)
src/data-types/integer.ts (8)
  • U8 (174-177)
  • U16 (183-186)
  • U32 (192-195)
  • U64 (201-204)
  • I8 (210-213)
  • I16 (219-222)
  • I32 (228-231)
  • I64 (237-240)
src/data-types/integer.ts (2)
src/data-types/field.ts (1)
  • Field (25-493)
src/data-types/zod/index.ts (2)
  • IntegerInput (33-33)
  • IntegerValidator (25-30)
src/data-types/bounded-vec.ts (1)
src/data-types/index.ts (2)
  • DataType (8-14)
  • toJSON (20-51)
src/data-types/field.ts (2)
src/data-types/zod/index.ts (2)
  • FieldInput (32-32)
  • FieldValidator (16-23)
src/types/data-types.ts (1)
  • Bit (1-1)
🔇 Additional comments (21)
src/types/data-types.ts (1)

1-1: Clear and concise type definition for binary values.

Using a union type of literal values (0 and 1) is an excellent choice for type safety when working with binary values.

src/types/index.ts (1)

57-58: Appropriate re-export of data types.

This export statement makes all the entities from the data-types module accessible through the types namespace, which is consistent with the module's organization pattern.

tsconfig.json (1)

8-8: Consider documenting the reasoning behind disabling noUncheckedIndexedAccess.

Setting noUncheckedIndexedAccess to false explicitly (which is actually the default behavior) suggests this was a deliberate decision. This setting affects type safety for indexed access, potentially allowing access to undefined properties without type warnings.

Could you clarify why this compiler option is being explicitly set to false? If this is intentional, consider adding a comment in the tsconfig file or documentation explaining the decision, especially since the new data types might benefit from stricter type checking.

src/index.ts (1)

3-3: Appropriate export of new data types to the public API.

This export statement makes all the new data types (Field, Bool, Str, BoundedVec, FixedSizeArray, etc.) part of the public API, which aligns with the PR objective of introducing new noir data types.

package.json (1)

32-32: Added Zod dependency for schema validation

The addition of the Zod library is appropriate for implementing type-safe validation schemas for the new data types being introduced. The version constraint ^3.24.2 follows best practices by allowing compatible updates.

Also applies to: 55-56

src/data-types/index.ts (3)

1-7: Comprehensive imports for all data types

The imports are well-organized and include all the necessary data type classes.


8-18: Well-defined type definitions for data structures

The DataType union type and StructMap interface provide a solid foundation for type-safe operations across the library.


53-60: Clean module exports

The exports are well-organized, making all data types available from a single entry point.

src/data-types/string.ts (2)

1-2: Appropriate import of U8 type

The import of U8 from the integer module is correctly used for byte representation.


30-44:

✅ Verification successful

Verify TextEncoder compatibility in non-browser environments

The asBytes() method uses TextEncoder which is standard in browsers but might need a polyfill in some Node.js environments.


🏁 Script executed:

#!/bin/bash
# Check if TextEncoder is available in the current Node.js environment

cat << EOF > check-text-encoder.js
try {
  const encoder = new TextEncoder();
  console.log('TextEncoder is available:', encoder !== undefined);
} catch (e) {
  console.error('TextEncoder is not available:', e.message);
}
EOF

node check-text-encoder.js

Length of output: 65


Action Required: No Additional Changes Needed

The verification confirms that the current Node.js environment supports TextEncoder (output: "TextEncoder is available: true"). This means that, for now, a polyfill is not required for the asBytes() method. If you plan to support older Node.js versions where TextEncoder might not be available, please consider adding a polyfill or documenting the version requirements.

tests/data-types/integer.test.ts (1)

30-54: Solid out-of-range coverage.

These tests thoroughly verify that values exceeding each integer type’s boundary are rejected. No concerns here.

src/data-types/zod/index.ts (1)

3-3:

✅ Verification successful

Confirm correctness of the field limit definition.

2n ** 254n - 1n is a strict maximum for certain field operations. If there's any scenario requiring different bounds or modular checks, ensure they're handled upstream or documented clearly.

Would you like to verify this value’s usage across the codebase to confirm it’s appropriately recognized?


🏁 Script executed:

#!/bin/bash
# Checking references to MAX_FIELD in the code
rg -A 3 "MAX_FIELD"

Length of output: 532


Approved: MAX_FIELD Definition and Usage are Correct

The constant definition in src/data-types/zod/index.ts:

const MAX_FIELD = 2n ** 254n - 1n;

is consistently used to validate that the field value does not exceed this bound. The schema refinement checking n <= MAX_FIELD and the corresponding error message ("Field value must be between 0 and 2^254 - 1") confirm that the usage aligns with the intended strict maximum for the field.

Overall, the definition and its integration into the Zod schema look correct.

src/data-types/integer.ts (2)

170-240: All derived integer classes are clearly defined with appropriate bounds.

The overridden MIN_VALUE and MAX_VALUE constants look correct and align with standard integer definitions. No issues found.


115-127:

✅ Verification successful

Ensure wrapping arithmetic handles negative ranges gracefully.

The wrapping implementations (wrappingAdd, wrappingSub, wrappingMul) assume the numeric range is contiguous from MIN_VALUE to MAX_VALUE. This works well. However, if future changes introduce specialized integer or partially restricted ranges that are not purely sequential, these methods may need re-evaluation.

Please confirm that all derived classes (e.g., I8, I16) maintain contiguous ranges, making this logic valid.

Also applies to: 135-147, 155-167


🏁 Script executed:

#!/bin/bash
# Searching for derived classes outside of this file to confirm consistent usage.
fd --type f --exec rg "extends AbstractInteger" {}

Length of output: 396


Wrapping Arithmetic Assumption Verified

  • Confirmed that derived classes (U8, U16, U32, U64, I8, I16, I32, I64) extend AbstractInteger, which implies they follow the standardized contiguous range pattern.
  • The current wrapping implementations (in wrappingAdd, as well as in the similar methods at lines 135–147 and 155–167) correctly assume contiguous ranges from MIN_VALUE to MAX_VALUE and are valid given the current design.
  • Moving forward, please ensure that any new or modified derived classes continue to enforce contiguous numeric ranges. Any deviation could require a re-evaluation of the wrapping arithmetic logic.
src/data-types/bounded-vec.ts (5)

1-10: Well-documented purpose and usage.
The introductory docstring clearly states the class's purpose and gives a concise overview of its functionality. This helps future developers understand how and when to use the BoundedVec<T> class.


83-88: Bounds checking and error messages look good.
The get, at, and set methods each correctly perform index validation and throw informative errors. This prevents undefined behavior and makes debugging easier.

Also applies to: 98-101, 124-129


109-115: Methods for modifying vector size are consistent.
Both push and pop follow a similar pattern of first checking capacity limits (isFull and isEmpty), then adjusting length. This consistency helps keep the code maintainable and predictable.

Also applies to: 137-144


152-169: Efficient extension from arrays and other BoundedVecs.
The extendFromArray method checks capacity before appending elements, and extendFromVec simply delegates to extendFromArray. This avoids duplicating logic and maintains clarity. The overall approach looks good.


188-193: Serialization approach is intuitive.
Converting each stored item to JSON via the global toJSON function nicely leverages polymorphism while preserving the core structure. This keeps the type system flexible.

tests/data-types/field.test.ts (2)

20-22: Check consistency of error messages.
The test expects 'Field input must be an integer', but the actual constructor message from the Zod schema is 'Field value must be between 0 and 2^254 - 1'. Ensure the test aligns with any custom messages from FieldValidator.


1-265: Comprehensive coverage.
The suite thoroughly exercises arithmetic, serialization, bit/byte/radix conversions, and edge cases. This significantly reduces the likelihood of undiscovered regressions.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (2)
src/data-types/field.ts (2)

30-31: Consider storing MAX_BIT_SIZE as a plain number.
Since 254 is small enough to stay within typical numeric bounds without BigInt, using a regular number (e.g. static readonly MAX_BIT_SIZE = 254;) may simplify comparisons and type usage, unless you specifically require a BigInt for uniformity.


80-88: Reevaluate the use of log2 for computing bit length.
Because this.log2 safely returns at most 253, casting its result to Number is not a precision risk here. However, you could skip log2 and directly use const bitLength = this.value.toString(2).length; for clarity.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 87d7a93 and 9fd0384.

📒 Files selected for processing (1)
  • src/data-types/field.ts (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/data-types/field.ts (2)
src/data-types/zod/index.ts (2)
  • FieldInput (32-32)
  • FieldValidator (16-23)
src/types/data-types.ts (1)
  • Bit (1-1)
🔇 Additional comments (3)
src/data-types/field.ts (3)

33-34: Verify the validator’s upper bound versus the modulus.
Currently, FieldValidator checks the parsed value <= 2^254 - 1, whereas MODULUS is ~2^254. This mismatch might allow values ≥ MODULUS, violating the notion of a strictly prime field in BN254. Confirm the correct upper bound in the validator.


127-169: Confirm byte array size references.
Field.modLeBytes().length and Field.modBeBytes().length both return 33 elements, which is one more than the typical 32 bytes needed to hold a 254-bit number. Validate that consuming code indeed requires 33 bytes rather than 32.


468-512: Check for off-by-one in static modulus arrays.
All four static arrays (modBeBits, modBeBytes, modLeBits, modLeBytes) appear to represent a 254-bit prime but each has 33 entries in the byte arrays. Validate that this extra byte is neither an unintended leading/trailing zero nor a mistake in the prime representation.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
src/data-types/field.ts (1)

380-387: 🛠️ Refactor suggestion

Revisit throwing on negative subtraction in a finite field context.

Currently, the sub method rejects results that would go negative before modular reduction. In typical prime-field arithmetic, negative intermediate results are reduced modulo the prime instead of raising an error. If you intend partial integer semantics, consider clarifying it in the docs; otherwise, remove the error throw to ensure consistent field behavior:

sub(input: Field | number | string | bigint): Field {
  const otherField = input instanceof Field ? input : new Field(input);
- if (this.value < otherField.value) {
-   throw new Error('Result would be negative');
- }
  const res = (this.value - otherField.value + Field.MODULUS) % Field.MODULUS;
  return new Field(res);
}
🧹 Nitpick comments (2)
src/data-types/field.ts (2)

150-151: Clarify the JSDoc for big-endian byte extraction.

The return description states "an array of bytes (0-length)" which could be confusing. Consider updating the JSDoc to indicate it returns an array of the specified length in big-endian order.

To fix:

- * @returns An array of bytes (0-length) in big-endian order
+ * @returns An array of bytes in big-endian order, sized by the length parameter

199-203: Align radix range checking with documentation.

The documentation mentions (must be a power of 2 between 2 and 256), but the code currently enforces 2 <= radix <= 36. This misalignment can confuse contributors and consumers of your API. Consider either updating the code to support up to 256 or revising the docs to reflect the current radix range:

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9fd0384 and 27ada7f.

📒 Files selected for processing (1)
  • src/data-types/field.ts (1 hunks)
🔇 Additional comments (2)
src/data-types/field.ts (2)

262-264: Enforce or clarify exponent safety checks.

The doc states that negative or non-safe integer exponents are disallowed, yet the code only checks for negative exponents at runtime. For extremely large exponents beyond JavaScript’s safe integer range, the loop will still run. If enforcing a safe integer exponent is intentional, add a check to ensure the exponent does not exceed Number.MAX_SAFE_INTEGER.


1-1: Overall Good Implementation!

Aside from the minor doc clarifications and optional field-semantics adjustments, the rest of your code effectively implements BN254 field operations with modular arithmetic, bit/byte array conversions, and validations. Nice work!

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/data-types/field.ts (1)

408-415: 🛠️ Refactor suggestion

Confirm intended behavior for negative results in subtraction.

Throwing an error for negative results is atypical in finite field arithmetic, which usually wraps values modulo Field.MODULUS. If partial integer semantics are intentional, please add docs clarifying that negative results are disallowed. Otherwise, consider unconditional modular subtraction:

- if (this.value < otherField.value) {
-   throw new Error('Result would be negative');
- }
- const res = (this.value - otherField.value + Field.MODULUS) % Field.MODULUS;
+ const res = (this.value - otherField.value + Field.MODULUS) % Field.MODULUS;
🧹 Nitpick comments (2)
src/data-types/string.ts (1)

45-53: Consider environment compatibility for TextEncoder usage.
TextEncoder is broadly supported in modern environments, but older Node releases and some browsers may require polyfills. If you need broader compatibility, ensure a suitable runtime or fallback logic is in place.

src/data-types/bounded-vec.ts (1)

192-195: Avoid including default elements in JSON if you only need the used portion.
Currently, storage() returns all items up to maxSize, including unused default entries. If you prefer to serialize only used elements, slice the array by this.length:

-      storage: this.storage().map((item) => toJSON(item)),
+      storage: this.toArray().map((item) => toJSON(item)),
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 27ada7f and 6be7867.

📒 Files selected for processing (8)
  • index.ts (1 hunks)
  • src/data-types/bounded-vec.ts (1 hunks)
  • src/data-types/field.ts (1 hunks)
  • src/data-types/index.ts (1 hunks)
  • src/data-types/string.ts (1 hunks)
  • src/data-types/zod/index.ts (1 hunks)
  • tests/data-types/field.test.ts (1 hunks)
  • tests/data-types/integer.test.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/data-types/integer.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/data-types/string.ts (1)
src/data-types/integer.ts (1)
  • U8 (174-177)
src/data-types/zod/index.ts (1)
src/data-types/integer.ts (2)
  • min (21-23)
  • max (29-31)
src/data-types/field.ts (2)
src/data-types/zod/index.ts (2)
  • FieldInput (36-36)
  • FieldValidator (17-27)
src/types/data-types.ts (1)
  • Bit (1-1)
🔇 Additional comments (4)
src/data-types/string.ts (1)

61-63: eq method looks good.
The equality check via strict string comparison is straightforward, and strongly typed usage of other: Str is sound.

src/data-types/zod/index.ts (1)

25-26: Confirm whether negative fields are indeed valid.
The current FieldValidator allows values down to -MAX_FIELD_SIZE, implying negative fields are acceptable. Verify if that's intentional or if fields are meant to be unsigned.

tests/data-types/field.test.ts (2)

21-21: Verify actual error message for non-integer inputs.

The test expects an error 'Field input must be an integer', but the validator message might differ (e.g. 'Field value must be between -MAX_FIELD_SIZE and MAX_FIELD_SIZE'). Ensure that the thrown error matches the tested string or update the test to match the real validator message.


408-415: Add coverage for subtracting a larger value from a smaller one.

The Field#sub method throws if result is negative, but there's no test verifying that behavior. Consider adding a test case like new Field(10n).sub(20n) to confirm the exception is thrown as expected.

Comment on lines +41 to +49
// Recursively process the properties of the object
if (typeof value === 'object' && value !== null) {
// If the value is a StructMap (an object), recursively process its properties
const result: { [key: string]: string | object | boolean } = {};
for (const [key, val] of Object.entries(value)) {
result[key] = toJSON(val);
}
return result;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Watch out for potential cyclical references.
Recursively converting objects can lead to infinite loops if any data structure references itself. If self-references are possible, consider adding a cycle detection mechanism.

Comment on lines +42 to +45
constructor(input: FieldInput) {
const parsed = FieldValidator.parse(input);
this.value = parsed % Field.MODULUS;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure proper normalization of negative inputs.

Currently, parsed % Field.MODULUS may remain negative in JavaScript when input is negative (e.g., -1n % prime === -1n). This can lead to errors in subsequent methods (like toLeBits, which throws for <= 0). If you intend to represent all values in [0, Field.MODULUS - 1], consider normalizing with an additional + Field.MODULUS before taking % Field.MODULUS.

Apply this diff to enforce non-negative values:

- this.value = parsed % Field.MODULUS;
+ this.value = (parsed % Field.MODULUS + Field.MODULUS) % Field.MODULUS;
📝 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.

Suggested change
constructor(input: FieldInput) {
const parsed = FieldValidator.parse(input);
this.value = parsed % Field.MODULUS;
}
constructor(input: FieldInput) {
const parsed = FieldValidator.parse(input);
- this.value = parsed % Field.MODULUS;
+ this.value = (parsed % Field.MODULUS + Field.MODULUS) % Field.MODULUS;
}

@Envoy-VC Envoy-VC merged commit 4a07998 into main Apr 8, 2025
1 check was pending
@Envoy-VC Envoy-VC deleted the feat/data-types branch April 9, 2025 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant