Optimize customHasher inputs and remove unnecessary allocations #1424
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
common/src/utils/hash.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
common/src/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
common/src/**/*.{ts,tsx,js,jsx}: Review shared utilities for:
- Reusability and modular design
- Type safety and error handling
- Side-effect management
- Documentation and naming clarity
Files:
common/src/utils/hash.ts
🧬 Code graph analysis (1)
common/src/utils/hash.ts (1)
common/index.ts (2)
customHasher(116-116)flexiblePoseidon(117-117)
🔇 Additional comments (1)
common/src/utils/hash.ts (1)
188-191: LGTM! Optimization eliminates redundant conversions.The change correctly removes the bigint → string → bigint round-trip by passing the
bigint[]frompackBytesArraydirectly tocustomHasher. This reduces allocations and achieves the PR's optimization goal while preserving behavior.
| export function customHasher(pubKeyFormatted: Array<string | bigint | number>) { | ||
| const asBigints: bigint[] = pubKeyFormatted.map((v) => | ||
| typeof v === 'bigint' ? v : BigInt(v) | ||
| ); |
There was a problem hiding this comment.
Add input validation and error handling for BigInt conversion.
The BigInt() conversion can throw TypeError for invalid inputs (non-numeric strings, undefined, null, etc.). Since customHasher is a public exported utility, unvalidated inputs could cause runtime crashes without helpful error messages.
Consider adding validation:
export function customHasher(pubKeyFormatted: Array<string | bigint | number>) {
+ if (!Array.isArray(pubKeyFormatted) || pubKeyFormatted.length === 0) {
+ throw new Error('customHasher requires a non-empty array');
+ }
+
+ try {
const asBigints: bigint[] = pubKeyFormatted.map((v) =>
typeof v === 'bigint' ? v : BigInt(v)
);
+ } catch (error) {
+ throw new Error(`Invalid input for customHasher: ${error instanceof Error ? error.message : 'Cannot convert to BigInt'}`);
+ }🤖 Prompt for AI Agents
In common/src/utils/hash.ts around lines 44 to 47, the direct BigInt conversion
can throw for invalid inputs; add input validation and error handling before
mapping: first verify pubKeyFormatted is an array and not null/undefined, then
for each element explicitly check for null/undefined and for allowed types
(bigint, number, numeric-string) and reject or coerce others; wrap the BigInt
conversion in a try/catch (or validate numeric-string with a regexp) and throw a
clear, descriptive TypeError that includes the offending value and its index so
callers get actionable errors instead of uncaught exceptions.
Forostovec
had a problem deploying
to
development
GitHub Actions
Failure
|
@seshanthS or @Nesopie could you review this pull request? |
2 participants
Refactor customHasher in common/src/utils/hash.ts to accept Array<string | bigint | number> and normalize inputs once to bigint[], eliminating repeated conversions. Replace the object-wrapped per-round input structure with a simple bigint[][] to reduce allocations and property lookups. Update packBytesAndPoseidon to pass the bigint[] from packBytesArray directly into customHasher, removing the bigint→string→bigint round-trip and the redundant .toString() call. These changes preserve the public API behavior, keep tests compatible with string inputs used by circom formatting, and reduce allocation pressure and runtime overhead in hashing hot paths.
Note
Refactors hashing utilities to accept mixed input types, normalize once to bigint[], use block-based Poseidon rounds, and remove redundant string conversions.
customHashernow acceptsArray<string | bigint | number>and normalizes inputs once tobigint[].bigint[][]blocks; computes Poseidon rounds viaposeidon16and aggregates withflexiblePoseidon.packBytesAndPoseidonto pass the packed array directly tocustomHasher, removing string conversions and extra.toString().Written by Cursor Bugbot for commit 9e25c95. This will update automatically on new commits. Configure here.
Summary by CodeRabbit