-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several new data type classes and validators, extending the module’s public API. New classes such as Changes
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
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
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this 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 documentationThe 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 functionThe
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 implementationThe
Str
class is well-documented with clear JSDoc comments and provides proper encapsulation.For consistency with the
Bool
class, consider making theval
property readonly:- private val: string; + private readonly val: string;
46-64
: Complete implementation of basic string operationsThe 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 forU16
,U32
, andU64
(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 preferundefined
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 tomaxSize
with default values, then setslength
to 0. This design provides predictable allocations but can be memory-inefficient for largemaxSize
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 fortoBeBytes
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 methodsfromLeBytes
andfromBeBytes
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.
Thesub
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
⛔ 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
tofalse
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 validationThe 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 typesThe imports are well-organized and include all the necessary data type classes.
8-18
: Well-defined type definitions for data structuresThe
DataType
union type andStructMap
interface provide a solid foundation for type-safe operations across the library.
53-60
: Clean module exportsThe 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 typeThe 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 usesTextEncoder
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.jsLength 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
andMAX_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 fromMIN_VALUE
toMAX_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 fromMIN_VALUE
toMAX_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 theBoundedVec<T>
class.
83-88
: Bounds checking and error messages look good.
Theget
,at
, andset
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.
Bothpush
andpop
follow a similar pattern of first checking capacity limits (isFull
andisEmpty
), then adjustinglength
. This consistency helps keep the code maintainable and predictable.Also applies to: 137-144
152-169
: Efficient extension from arrays and otherBoundedVec
s.
TheextendFromArray
method checks capacity before appending elements, andextendFromVec
simply delegates toextendFromArray
. 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 globaltoJSON
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 fromFieldValidator
.
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.
There was a problem hiding this 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 storingMAX_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 oflog2
for computing bit length.
Becausethis.log2
safely returns at most 253, casting its result toNumber
is not a precision risk here. However, you could skiplog2
and directly useconst bitLength = this.value.toString(2).length;
for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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
, whereasMODULUS
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
andField.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.
There was a problem hiding this 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 suggestionRevisit 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 enforces2 <= 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
📒 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!
There was a problem hiding this 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 suggestionConfirm 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 tomaxSize
, including unused default entries. If you prefer to serialize only used elements, slice the array bythis.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
📒 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 ofother: Str
is sound.src/data-types/zod/index.ts (1)
25-26
: Confirm whether negative fields are indeed valid.
The currentFieldValidator
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 ifresult
is negative, but there's no test verifying that behavior. Consider adding a test case likenew Field(10n).sub(20n)
to confirm the exception is thrown as expected.
// 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; | ||
} |
There was a problem hiding this comment.
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.
constructor(input: FieldInput) { | ||
const parsed = FieldValidator.parse(input); | ||
this.value = parsed % Field.MODULUS; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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; | |
} |
Summary by CodeRabbit
New Features
Tests
Field
and integer data types.Chores