fix(contract): redeploy Zylith with Argent wallet compatibility #28
Conversation
## Changes ### Contract Updates - Move proof and public_inputs arrays to end of function parameters for Argent wallet compatibility - Change tick_lower/tick_upper from i32 to felt252 for Starknet.js compatibility - Add get_version() function to contract interface - Update Scarb.toml to use starknet 2.13.1 ### Redeployment - New contract address: 0x00c692a0a7b34ffe8c5484e6db9488dc881ceae9c9b05d67de21387ea9f3edd6 - Same verifier contracts maintained ### Frontend/ASP Updates - Update contract address in all configuration files - Update ABIs to match new contract interface - Update tests with new contract address ### New Scripts - Add circuit verification scripts - Add position commitment calculation helpers Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughUpdates contract addresses, reorders proof/public_inputs parameters across on-chain ABI, Cairo contract, and client callsites for wallet compatibility; adds CONTRACT_VERSION/get_version; introduces SNARK tooling scripts and position-commitment utilities; adjusts ABIs and frontend calldata; replaces/adds Cairo tests and deployment metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend as Frontend<br/>(hooks)
participant ABI as ABI<br/>(calldata)
participant ASP as ASP<br/>(server)
participant Contract as Zylith<br/>Contract
Frontend->>ABI: build calldata (params, new_commitment, proof, public_inputs)
ABI-->>Frontend: populated calldata
Frontend->>ASP: check ASP availability / query fallback
alt ASP available
Frontend->>ASP: send calldata for execution
ASP-->>Frontend: execution result / tx receipt
else ASP not available
Frontend->>Contract: submit transaction (account.execute) with calldata
Contract-->>Frontend: tx result / events
end
sequenceDiagram
participant Scripts as Circuits<br/>Scripts
participant Groth16 as Groth16<br/>Setup
participant Garaga as Garaga<br/>Verifier Gen
participant Repo as Repo<br/>(zylith)
Scripts->>Groth16: generate keys/proofs per circuit
Groth16-->>Scripts: zkey & vk files
Scripts->>Garaga: finalize_verifiers.sh (use vk)
Garaga-->>Repo: produce groth16_verifier.cairo artifacts
Repo-->>Garaga: copy verifier artifacts into zylith/src/privacy/verifiers/*
Garaga-->>Scripts: completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/src/hooks/use-private-swap.ts (1)
894-894: MissingisConnectedandisConnectingin dependency array.The
executeSwapcallback usesisConnectedandisConnectingfrom theuseStarknethook but they are not included in the dependency array. This could lead to stale closure issues.Proposed fix
- }, [account, provider, aspClientInstance, removeNote, addNote, updateNote, addTransaction, updateTransaction]) + }, [account, provider, aspClientInstance, removeNote, addNote, updateNote, addTransaction, updateTransaction, isConnected, isConnecting])zylith/src/zylith.cairo (1)
1529-1532: Inconsistent u256 to felt252 conversion in private_collect.For
verified_nullifierandverified_root(lines 1529-1532), the code only uses.low.try_into().unwrap()without checking ifhigh != 0. This differs from other functions likeprivate_swap,private_withdraw, andprivate_mint_liquiditywhich properly reconstruct the full value whenhigh != 0.The comment on lines 1524-1528 says "always use .low" but this is incorrect for felt252 values >= 2^128, which would have
high != 0.Proposed fix
let verified_nullifier_u256 = *_verified_inputs.at(0); - let verified_nullifier: felt252 = verified_nullifier_u256.low.try_into().unwrap(); + let verified_nullifier: felt252 = if verified_nullifier_u256.high == 0 { + verified_nullifier_u256.low.try_into().unwrap() + } else { + let q128: u256 = 340282366920938463463374607431768211456; // 2^128 + let high_u256: u256 = verified_nullifier_u256.high.into(); + let low_u256: u256 = verified_nullifier_u256.low.into(); + let reconstructed: u256 = high_u256 * q128 + low_u256; + reconstructed.try_into().unwrap() + }; let verified_root_u256 = *_verified_inputs.at(1); - let verified_root: felt252 = verified_root_u256.low.try_into().unwrap(); + let verified_root: felt252 = if verified_root_u256.high == 0 { + verified_root_u256.low.try_into().unwrap() + } else { + let q128: u256 = 340282366920938463463374607431768211456; // 2^128 + let high_u256: u256 = verified_root_u256.high.into(); + let low_u256: u256 = verified_root_u256.low.into(); + let reconstructed: u256 = high_u256 * q128 + low_u256; + reconstructed.try_into().unwrap() + };frontend/src/lib/contracts/zylith-contract.ts (1)
188-204: Update remaining private_ method calls to match the new ABI parameter order (scalars first, arrays at end).*The
private_swap,private_mint_liquidity,private_burn_liquidity, andprivate_collectmethods still pass proof and publicInputs arrays first, but the updated ABI expects all scalar parameters before arrays. This mismatch will cause calldata encoding errors and transaction failures.🛠️ Proposed fix (reorder calldata + calls)
- return await contract.private_swap( - proof, - publicInputs, - zeroForOne, - amountSpecified, - sqrtPriceLimitX128, - newCommitment - ); + return await contract.private_swap( + zeroForOne, + amountSpecified, + sqrtPriceLimitX128, + newCommitment, + proof, + publicInputs + ); ... - return await contract.private_mint_liquidity( - proof, - publicInputs, - tickLower, - tickUpper, - liquidity, - newCommitment - ); + return await contract.private_mint_liquidity( + tickLower, + tickUpper, + liquidity, + newCommitment, + proof, + publicInputs + ); ... - return await contract.private_burn_liquidity( - proof, - publicInputs, - tickLower, - tickUpper, - liquidity, - newCommitment - ); + return await contract.private_burn_liquidity( + tickLower, + tickUpper, + liquidity, + newCommitment, + proof, + publicInputs + ); ... - return await contract.private_collect( - proof, - publicInputs, - tickLower, - tickUpper, - newCommitment - ); + return await contract.private_collect( + tickLower, + tickUpper, + newCommitment, + proof, + publicInputs + );Also update the manual calldata logging in
privateSwap(lines 127–136) to reflect the new order.asp/src/abis/zylith-abi.json (1)
88-247: Calldata builders in asp/src/calldata.rs have inverted parameter ordering that no longer matches the ABI.The interface signatures now place
proofandpublic_inputsarrays at the end of parameter lists (for Argent wallet compatibility), but the Rust calldata builders construct them at the beginning:
build_swap_calldata(lines 65–87): emits[proof_len, ...proof, pub_inputs_len, ...pub_inputs, zero_for_one, amount_specified, ...]but should emit[zero_for_one, amount_specified, ..., proof_len, ...proof, pub_inputs_len, ...pub_inputs]build_withdraw_calldata(lines 111–131): emits[proof_len, ...proof, pub_inputs_len, ...pub_inputs, token, recipient, amount]but should emit[token, recipient, amount, proof_len, ...proof, pub_inputs_len, ...pub_inputs]build_mint_liquidity_calldata(lines 166–212): emits[proof_len, ...proof, pub_inputs_len, ...pub_inputs, tick_lower, tick_upper, liquidity, commitment]but should emit[tick_lower, tick_upper, liquidity, commitment, proof_len, ...proof, pub_inputs_len, ...pub_inputs]The inline comments in these builders (lines 53–60, 100–106, 146–153) also describe the wrong parameter order. Update the builders to match the new interface ordering or remove them if deprecated.
🤖 Fix all issues with AI agents
In `@circuits/proof_script_1766861888677913151.js`:
- Around line 1-25: This file (circuits/proof_script_1766861888677913151.js)
contains hardcoded absolute paths in the call to
snarkjs.groth16.prove('/home/ryzen/.../swap.zkey',
'/tmp/swap_witness_1766861888677913151.wtns') and in fs.writeFileSync('/tmp/…')
— either delete this generated/temp script if it was accidentally committed, or
refactor: replace the hardcoded zkey and witness paths with configurable values
(e.g., process.env.ZKEY_PATH and process.env.WITNESS_PATH or relative paths
using path.join(__dirname,...)), and write outputs using a safe temp or
project-relative location (e.g., os.tmpdir() or a configurable OUTPUT_DIR) when
calling fs.writeFileSync for the files containing proof and publicSignals;
ensure you update the snarkjs.groth16.prove call and the fs.writeFileSync
targets (the proof and publicSignals file writes) accordingly so the script
works on other machines.
In `@circuits/scripts/finalize_verifiers.sh`:
- Line 4: The top-of-file comment in finalize_verifiers.sh incorrectly
references "pot16_prepared.ptau" while the script actually uses
"pot15_prepared.ptau"; update the comment to mention "pot15_prepared.ptau" (or
make it generic) so it matches the actual file referenced in the script and
avoid confusion when running or reviewing the file.
In `@circuits/scripts/test_position_commitment_direct.js`:
- Around line 17-19: The script assigns tickLower and tickUpper via parseInt
without validation (const tickLower, const tickUpper), which can yield NaN for
non-numeric inputs; add explicit checks after parsing (e.g., verify
!Number.isNaN(tickLower) && Number.isInteger(tickLower) and same for tickUpper)
and throw a clear error or exit when validation fails, mirroring the validation
in calculate_position_commitment.js so the script rejects non-numeric tick args
and prevents downstream NaN behavior.
- Line 6: The require path for generatePositionCommitment is wrong and will fail
module resolution; update the import in test_position_commitment_direct.js to
point to the local utils module (use the correct relative path to utils.js from
the current directory) so that generatePositionCommitment is required from the
actual utils.js file in the same scripts folder; modify the require(...) for
generatePositionCommitment accordingly.
In `@circuits/scripts/test_position_commitment_fixture.js`:
- Line 6: The require path is incorrect and causes module resolution failure;
update the import in test_position_commitment_fixture.js so
generatePositionCommitment is required from the local scripts utils module (use
'./utils.js' or './utils') instead of '../circuits/scripts/utils.js'; locate the
require statement that references generatePositionCommitment and change the path
to the correct relative path to utils.js.
In `@zylith/deployment_sepolia_20260117_134421.json`:
- Around line 1-26: The deployment JSON currently contains placeholder
class_hashes ("0x0") and empty addresses for contracts like "zylith",
"membership_verifier", "swap_verifier", "withdraw_verifier", and "lp_verifier";
update the "zylith" object to set its "address" to the redeployed address
0x00c692a0a7b34ffe8c5484e6db9488dc881ceae9c9b05d67de21387ea9f3edd6 and replace
its "class_hash" with the actual deployed class hash from the deployment
artifacts, and if available populate the other verifiers' "address" and
"class_hash" fields similarly; if this file is not intended to represent the
Sepolia deployment, add a short in-file comment or PR note clarifying its
purpose instead of leaving placeholders.
In `@zylith/Scarb.toml`:
- Line 9: zylith pins starknet and assert_macros to 2.13.1 which conflicts with
the garaga verifier manifests that require 2.14.0; update zylith/Scarb.toml to
use the same versions as the garaga modules (set starknet and assert_macros to
2.14.0) or otherwise ensure compatibility by documenting and validating that
2.13.1 is compatible with the garaga build; modify the dependency entries in
zylith/Scarb.toml (starknet, assert_macros) to match the garaga
circuits/build/garaga/*/Scarb.toml versions or run compatibility tests if you
choose to keep 2.13.1.
In `@zylith/src/interfaces/izylith.cairo`:
- Around line 18-26: The wrapper execute_private_swap has parameters in the old
order and the wrong sqrt_price_limit type, causing a mismatch with the
izylith::private_swap interface; update execute_private_swap to use the same
signature as private_swap: place zero_for_one, amount_specified,
sqrt_price_limit_x128 (u256), new_commitment, then proof: Array<felt252>,
public_inputs: Array<felt252>, and return (i128, i128), so the wrapper's
parameter order and sqrt_price_limit type exactly match the private_swap
declaration and implementation.
🧹 Nitpick comments (14)
.claude/settings.json (1)
3-5: Tighten Bash allowlist to the minimal commands needed.
The wildcard pattern is broader than necessary and could permit unintended subcommands/args. Prefer explicit entries for the exact build commands you want to allow.♻️ Suggested tightening
"permissions": { "allow": [ - "Bash(scarb build:*)" + "Bash(scarb build)" ] }scripts/calculate_position_commitment.js (1)
41-44: Redundant conditional: both branches are identical.The ternary on lines 42-44 performs the same operation regardless of the condition. The
0xprefix check has no effect sinceBigInt()already handles hex strings with the0xprefix.Suggested simplification
- // Convert secret to BigInt (handle hex strings) - const secretBigInt = secret.startsWith('0x') - ? BigInt(secret) - : BigInt(secret); + // Convert secret to BigInt (BigInt() handles both decimal and 0x-prefixed hex) + const secretBigInt = BigInt(secret);circuits/scripts/test_position_commitment_fixture.js (2)
22-22: Minor inconsistency: tick_sum computed differently.Line 22 uses regular number arithmetic (
tickLower + tickUpper) while line 28 uses BigInt arithmetic. For typical tick values this is fine, but for consistency with the actual result, consider using the same approach.Optional fix for consistency
- console.log(`Expected tick_sum: ${tickLower + tickUpper}`); + console.log(`Expected tick_sum: ${BigInt(tickLower) + BigInt(tickUpper)}`);
51-51: Missing promise rejection handler.Unlike
scripts/calculate_position_commitment.js, this script doesn't handle unhandled rejections frommain(). While the try/catch inside handles most errors, adding a top-level catch is defensive and consistent with the other scripts.Proposed fix
-main(); +main().catch((error) => { + console.error('Fatal error:', error); + process.exit(1); +});circuits/scripts/test_position_commitment_direct.js (1)
57-57: Missing promise rejection handler.For consistency with
scripts/calculate_position_commitment.jsand defensive error handling, add a top-level catch.Proposed fix
-main(); +main().catch((error) => { + console.error('Fatal error:', error); + process.exit(1); +});circuits/scripts/generate_proof_keys.sh (3)
45-46: Document that dummy entropy is for development only.The zkey contribution uses predictable entropy (
"random entropy $CIRCUIT"), which is acceptable for development/testing but would compromise security in a production trusted setup ceremony.Consider adding a comment to make this explicit:
Suggested documentation
- # Contribute (dummy randomness) + # Contribute (dummy randomness - FOR DEVELOPMENT ONLY) + # Production deployments require a proper multi-party trusted setup ceremony ./node_modules/.bin/snarkjs zkey contribute out/${CIRCUIT}_0000.zkey out/${CIRCUIT}_final.zkey --name="Zylith" -e="random entropy $CIRCUIT"
16-20: Add error handling for POT file download.The
curlcommand doesn't detect HTTP errors (e.g., 404 or 500 responses). Adding--failensures the script fails if the download fails.Suggested fix
if [ ! -f "$PTAU_FINAL" ]; then echo "Downloading Powers of Tau (POT15)..." - curl -L -o "$PTAU_FINAL" "$PTAU_URL" + curl -L --fail -o "$PTAU_FINAL" "$PTAU_URL" echo "POT15 downloaded successfully!" fi
24-24: Script assumes execution from a specific working directory.The script uses relative paths (
./node_modules/.bin/snarkjs,out/) which require running from thecircuits/directory. Consider adding a directory change at the start or documenting the required working directory.Suggested improvement
#!/bin/bash set -e +# Ensure we're in the circuits directory +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +cd "$SCRIPT_DIR/.." + # Powers of Tau fileAlso applies to: 43-43, 46-46, 49-49
zylith/tests/test_u256_to_felt252_conversion.cairo (1)
167-195: Consider extracting repeated reconstruction logic into a helper function.The reconstruction logic for converting u256 to felt252 is duplicated for nullifier, root, and commitment. While acceptable in test code, extracting this into a helper would improve readability and reduce maintenance burden.
Example helper function
fn reconstruct_felt252(value: u256) -> felt252 { let q128: u256 = 340282366920938463463374607431768211456; // 2^128 if value.high == 0 { value.low.try_into().unwrap() } else { let high_u256: u256 = value.high.into(); let low_u256: u256 = value.low.into(); let reconstructed: u256 = high_u256 * q128 + low_u256; reconstructed.try_into().unwrap() } }zylith/tests/test_swap_with_frontend_values.cairo (1)
8-50: Test duplicatestest_u256_to_felt252_conversion_with_large_values.The
test_conversion_with_exact_frontend_valuestest is nearly identical totest_u256_to_felt252_conversion_with_large_valuesintest_u256_to_felt252_conversion.cairo. Consider removing this duplicate or consolidating the test files.circuits/scripts/finalize_verifiers.sh (1)
22-22: Declare and assign separately to avoid masking return values.Per ShellCheck SC2155, if
$(pwd)fails, the exit code would be masked by the assignment.Proposed fix
- export PATH="$(pwd)/mock_bin:$PATH" + local cwd + cwd="$(pwd)" + export PATH="$cwd/mock_bin:$PATH"Note: Since this is in a loop without a function context, you may alternatively use:
SCRIPT_DIR="$(pwd)" # ... at the top of the script, then use $SCRIPT_DIR in the loopzylith/src/zylith.cairo (1)
236-251: Consider extracting repeated u256 to felt252 conversion into a helper.The reconstruction logic (
if high == 0 { low } else { high * q128 + low }) is repeated many times throughout the contract. Extracting this into a helper function inInternalFunctionsImplwould reduce code duplication and make maintenance easier.Example helper function
/// Convert u256 (from verifier) back to felt252 /// Handles values >= 2^128 which have high != 0 fn _u256_to_felt252(ref self: ContractState, value: u256) -> felt252 { if value.high == 0 { value.low.try_into().unwrap() } else { let q128: u256 = 340282366920938463463374607431768211456; // 2^128 let high_u256: u256 = value.high.into(); let low_u256: u256 = value.low.into(); let reconstructed: u256 = high_u256 * q128 + low_u256; reconstructed.try_into().unwrap() } }Then replace all occurrences:
let verified_nullifier = InternalFunctionsImpl::_u256_to_felt252(ref self, *_verified_inputs.at(0));frontend/src/hooks/use-liquidity.ts (1)
631-634: Gate verbose proof/public-input logs in production.These arrays can be large and noisy; consider guarding behind a debug flag.
♻️ Suggested guard
- console.log(`[Frontend] 🔍 Debug - Array values:`) - console.log(` - normalizedProof:`, normalizedProof) - console.log(` - normalizedPublicInputs:`, normalizedPublicInputs) + if (process.env.NODE_ENV !== "production") { + console.log(`[Frontend] 🔍 Debug - Array values:`) + console.log(` - normalizedProof:`, normalizedProof) + console.log(` - normalizedPublicInputs:`, normalizedPublicInputs) + }frontend/src/lib/abis/zylith-abi.json (1)
1-6: Consider single-source ABI to avoid drift.This file mirrors asp/src/abis/zylith-abi.json; generating both from a single artifact would reduce mismatch risk.
|
|
||
| const snarkjs = require('snarkjs'); | ||
| const fs = require('fs'); | ||
|
|
||
| (async () => { | ||
| try { | ||
| console.log('Generating proof...'); | ||
| const startTime = Date.now(); | ||
|
|
||
| const { proof, publicSignals } = await snarkjs.groth16.prove( | ||
| '/home/ryzen/Desktop/dev/starknet-bounty/circuits/build/zkeys/swap.zkey', | ||
| '/tmp/swap_witness_1766861888677913151.wtns' | ||
| ); | ||
|
|
||
| const elapsed = ((Date.now() - startTime) / 1000).toFixed(2); | ||
| console.log('Proof generated in', elapsed, 'seconds'); | ||
|
|
||
| fs.writeFileSync('/tmp/swap_proof_1766861888677913151.json', JSON.stringify(proof, null, 2)); | ||
| fs.writeFileSync('/tmp/swap_public_1766861888677913151.json', JSON.stringify(publicSignals, null, 2)); | ||
| } catch (error) { | ||
| console.error('Error:', error.message); | ||
| process.exit(1); | ||
| } | ||
| })(); | ||
|
No newline at end of file |
There was a problem hiding this comment.
Hardcoded absolute path will break on other machines; likely an accidental commit.
This script contains a hardcoded user-specific path (/home/ryzen/Desktop/dev/starknet-bounty/) that will fail on any other machine. The filename with a unique ID (1766861888677913151) and corresponding temp file paths suggest this is an auto-generated one-off script rather than reusable tooling.
Consider:
- Removing this file if it was accidentally committed (appears to be a generated debug/temp script).
- If intentional, refactor to use relative paths or environment variables:
Suggested refactor if the script should be kept
- const snarkjs = require('snarkjs');
- const fs = require('fs');
-
- (async () => {
- try {
- console.log('Generating proof...');
- const startTime = Date.now();
-
- const { proof, publicSignals } = await snarkjs.groth16.prove(
- '/home/ryzen/Desktop/dev/starknet-bounty/circuits/build/zkeys/swap.zkey',
- '/tmp/swap_witness_1766861888677913151.wtns'
- );
+const snarkjs = require('snarkjs');
+const fs = require('fs');
+const path = require('path');
+
+(async () => {
+ try {
+ const zkeyPath = process.argv[2] || path.join(__dirname, '../build/zkeys/swap.zkey');
+ const witnessPath = process.argv[3];
+
+ if (!witnessPath) {
+ console.error('Usage: node generate_proof.js [zkey_path] <witness_path>');
+ process.exit(1);
+ }
+
+ console.log('Generating proof...');
+ const startTime = Date.now();
+
+ const { proof, publicSignals } = await snarkjs.groth16.prove(zkeyPath, witnessPath);🤖 Prompt for AI Agents
In `@circuits/proof_script_1766861888677913151.js` around lines 1 - 25, This file
(circuits/proof_script_1766861888677913151.js) contains hardcoded absolute paths
in the call to snarkjs.groth16.prove('/home/ryzen/.../swap.zkey',
'/tmp/swap_witness_1766861888677913151.wtns') and in fs.writeFileSync('/tmp/…')
— either delete this generated/temp script if it was accidentally committed, or
refactor: replace the hardcoded zkey and witness paths with configurable values
(e.g., process.env.ZKEY_PATH and process.env.WITNESS_PATH or relative paths
using path.join(__dirname,...)), and write outputs using a safe temp or
project-relative location (e.g., os.tmpdir() or a configurable OUTPUT_DIR) when
calling fs.writeFileSync for the files containing proof and publicSignals;
ensure you update the snarkjs.groth16.prove call and the fs.writeFileSync
targets (the proof and publicSignals file writes) accordingly so the script
works on other machines.
| #!/bin/bash | ||
| set -e | ||
|
|
||
| # This script assumes pot16_prepared.ptau exists |
There was a problem hiding this comment.
Comment mentions wrong pot file.
The comment says "pot16_prepared.ptau" but line 12 uses "pot15_prepared.ptau". Update the comment to match the actual file used.
Proposed fix
-# This script assumes pot16_prepared.ptau exists
+# This script assumes pot15_prepared.ptau exists📝 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.
| # This script assumes pot16_prepared.ptau exists | |
| # This script assumes pot15_prepared.ptau exists |
🤖 Prompt for AI Agents
In `@circuits/scripts/finalize_verifiers.sh` at line 4, The top-of-file comment in
finalize_verifiers.sh incorrectly references "pot16_prepared.ptau" while the
script actually uses "pot15_prepared.ptau"; update the comment to mention
"pot15_prepared.ptau" (or make it generic) so it matches the actual file
referenced in the script and avoid confusion when running or reviewing the file.
| * Test directo para calcular position_commitment | ||
| * Usa el mismo código que generate_test_fixtures.js | ||
| */ | ||
| const { generatePositionCommitment } = require('../circuits/scripts/utils.js'); |
There was a problem hiding this comment.
Incorrect import path will cause module resolution failure.
Same issue as in test_position_commitment_fixture.js. From circuits/scripts/, the path '../circuits/scripts/utils.js' resolves incorrectly.
Proposed fix
-const { generatePositionCommitment } = require('../circuits/scripts/utils.js');
+const { generatePositionCommitment } = require('./utils.js');📝 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.
| const { generatePositionCommitment } = require('../circuits/scripts/utils.js'); | |
| const { generatePositionCommitment } = require('./utils.js'); |
🤖 Prompt for AI Agents
In `@circuits/scripts/test_position_commitment_direct.js` at line 6, The require
path for generatePositionCommitment is wrong and will fail module resolution;
update the import in test_position_commitment_direct.js to point to the local
utils module (use the correct relative path to utils.js from the current
directory) so that generatePositionCommitment is required from the actual
utils.js file in the same scripts folder; modify the require(...) for
generatePositionCommitment accordingly.
| const secret = BigInt(args[0]); | ||
| const tickLower = parseInt(args[1]); | ||
| const tickUpper = parseInt(args[2]); |
There was a problem hiding this comment.
Missing validation for non-numeric tick arguments.
Unlike scripts/calculate_position_commitment.js (lines 35-38), this script doesn't validate that tickLower and tickUpper are valid integers. Passing non-numeric values will result in NaN, which could cause unexpected behavior downstream.
Proposed fix
const secret = BigInt(args[0]);
const tickLower = parseInt(args[1]);
const tickUpper = parseInt(args[2]);
+
+ if (isNaN(tickLower) || isNaN(tickUpper)) {
+ console.error('Error: tick_lower and tick_upper must be valid integers');
+ process.exit(1);
+ }🤖 Prompt for AI Agents
In `@circuits/scripts/test_position_commitment_direct.js` around lines 17 - 19,
The script assigns tickLower and tickUpper via parseInt without validation
(const tickLower, const tickUpper), which can yield NaN for non-numeric inputs;
add explicit checks after parsing (e.g., verify !Number.isNaN(tickLower) &&
Number.isInteger(tickLower) and same for tickUpper) and throw a clear error or
exit when validation fails, mirroring the validation in
calculate_position_commitment.js so the script rejects non-numeric tick args and
prevents downstream NaN behavior.
| * Test usando generate_test_fixtures.js que ya funciona | ||
| * Calcula position_commitment con los mismos valores que se usan en los tests | ||
| */ | ||
| const { generatePositionCommitment } = require('../circuits/scripts/utils.js'); |
There was a problem hiding this comment.
Incorrect import path will cause module resolution failure.
This file is located at circuits/scripts/test_position_commitment_fixture.js. The path '../circuits/scripts/utils.js' resolves to circuits/circuits/scripts/utils.js (going up one level from circuits/scripts/ to circuits/, then into circuits/scripts/), which doesn't exist.
Proposed fix
-const { generatePositionCommitment } = require('../circuits/scripts/utils.js');
+const { generatePositionCommitment } = require('./utils.js');📝 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.
| const { generatePositionCommitment } = require('../circuits/scripts/utils.js'); | |
| const { generatePositionCommitment } = require('./utils.js'); |
🤖 Prompt for AI Agents
In `@circuits/scripts/test_position_commitment_fixture.js` at line 6, The require
path is incorrect and causes module resolution failure; update the import in
test_position_commitment_fixture.js so generatePositionCommitment is required
from the local scripts utils module (use './utils.js' or './utils') instead of
'../circuits/scripts/utils.js'; locate the require statement that references
generatePositionCommitment and change the path to the correct relative path to
utils.js.
| { | ||
| "profile": "sepolia", | ||
| "timestamp": "2026-01-17T13:44:30-06:00", | ||
| "contracts": { | ||
| "zylith": { | ||
| "class_hash": "0x0", | ||
| "address": "" | ||
| }, | ||
| "membership_verifier": { | ||
| "class_hash": "0x0", | ||
| "address": "" | ||
| }, | ||
| "swap_verifier": { | ||
| "class_hash": "0x0", | ||
| "address": "" | ||
| }, | ||
| "withdraw_verifier": { | ||
| "class_hash": "0x0", | ||
| "address": "" | ||
| }, | ||
| "lp_verifier": { | ||
| "class_hash": "0x0", | ||
| "address": "" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Deployment file has empty addresses despite PR stating contract was redeployed.
The PR objectives indicate the contract was redeployed to 0x00c692a0a7b34ffe8c5484e6db9488dc881ceae9c9b05d67de21387ea9f3edd6, but this deployment file contains empty addresses and placeholder class_hash values for all contracts.
If this file is meant to track the Sepolia deployment, populate the zylith contract address and class hash with the actual deployed values. Otherwise, clarify the purpose of this file.
🤖 Prompt for AI Agents
In `@zylith/deployment_sepolia_20260117_134421.json` around lines 1 - 26, The
deployment JSON currently contains placeholder class_hashes ("0x0") and empty
addresses for contracts like "zylith", "membership_verifier", "swap_verifier",
"withdraw_verifier", and "lp_verifier"; update the "zylith" object to set its
"address" to the redeployed address
0x00c692a0a7b34ffe8c5484e6db9488dc881ceae9c9b05d67de21387ea9f3edd6 and replace
its "class_hash" with the actual deployed class hash from the deployment
artifacts, and if available populate the other verifiers' "address" and
"class_hash" fields similarly; if this file is not intended to represent the
Sepolia deployment, add a short in-file comment or PR note clarifying its
purpose instead of leaving placeholders.
|
|
||
| [dependencies] | ||
| starknet = "2.14.0" | ||
| starknet = "2.13.1" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate manifests and lockfiles
echo "=== Scarb.toml files ==="
fd Scarb.toml -t f
echo ""
echo "=== Scarb.lock files ==="
fd Scarb.lock -t f
echo ""
echo "=== Starknet and assert_macros version pins in all manifests ==="
fd Scarb.toml -t f -x bash -c 'echo "File: $1"; cat "$1" | grep -E "^\s*(starknet|assert_macros)\s*=" || echo " (no matches)"' _ {}
echo ""
echo "=== Content of zylith/Scarb.toml ==="
cat -n zylith/Scarb.toml
echo ""
echo "=== Lockfile content (if exists) ==="
if [ -f "Scarb.lock" ]; then
cat -n Scarb.lock | head -100
fi
if [ -f "zylith/Scarb.lock" ]; then
cat -n zylith/Scarb.lock | head -100
fiRepository: SunsetLabs-Game/Zylith
Length of output: 6318
Resolve version conflict: zylith pins starknet 2.13.1 while garaga verifiers pin 2.14.0.
The workspace has conflicting dependency versions. The zylith/Scarb.toml pins starknet and assert_macros to 2.13.1, but all four garaga verifier modules (circuits/build/garaga/*/Scarb.toml) pin them to 2.14.0. Since the garaga dependency in zylith is pulled from the main branch, this version mismatch may cause build or compatibility failures. Align the versions across all manifests, or verify that 2.13.1 is explicitly compatible with the garaga build.
🤖 Prompt for AI Agents
In `@zylith/Scarb.toml` at line 9, zylith pins starknet and assert_macros to
2.13.1 which conflicts with the garaga verifier manifests that require 2.14.0;
update zylith/Scarb.toml to use the same versions as the garaga modules (set
starknet and assert_macros to 2.14.0) or otherwise ensure compatibility by
documenting and validating that 2.13.1 is compatible with the garaga build;
modify the dependency entries in zylith/Scarb.toml (starknet, assert_macros) to
match the garaga circuits/build/garaga/*/Scarb.toml versions or run
compatibility tests if you choose to keep 2.13.1.
| fn private_swap( | ||
| ref self: TContractState, | ||
| proof: Array<felt252>, | ||
| public_inputs: Array<felt252>, | ||
| zero_for_one: bool, | ||
| amount_specified: u128, | ||
| sqrt_price_limit_x128: u256, | ||
| new_commitment: felt252, | ||
| proof: Array<felt252>, // ← Moved to end for Argent wallet compatibility | ||
| public_inputs: Array<felt252>, // ← Moved to end for Argent wallet compatibility | ||
| ) -> (i128, i128); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'private_swap\s*\(' --type cairoRepository: SunsetLabs-Game/Zylith
Length of output: 95
🏁 Script executed:
rg -n 'private_swap\s*\(' --glob='*.cairo'Repository: SunsetLabs-Game/Zylith
Length of output: 244
🏁 Script executed:
cat -n zylith/src/zylith.cairo | sed -n '170,210p'Repository: SunsetLabs-Game/Zylith
Length of output: 2238
🏁 Script executed:
cat -n zylith/src/integration/swap.cairo | head -50Repository: SunsetLabs-Game/Zylith
Length of output: 1869
🏁 Script executed:
rg -n 'private_swap\(' --glob='*.cairo' -A2 -B2 | grep -v '^\s*fn\|^\s*pub fn'Repository: SunsetLabs-Game/Zylith
Length of output: 1112
🏁 Script executed:
rg -n '\.private_swap\(' --glob='*.cairo'Repository: SunsetLabs-Game/Zylith
Length of output: 48
🏁 Script executed:
rg -n 'execute_private_swap\(' --glob='*.cairo'Repository: SunsetLabs-Game/Zylith
Length of output: 131
The main interface and implementation signatures are correctly aligned with the new parameter order, but the wrapper function in swap.cairo has misaligned parameters.
The private_swap interface (izylith.cairo:18) and implementation (zylith.cairo:176) both correctly place proof and public_inputs at the end. However, execute_private_swap in swap.cairo:18 defines them at the start—the old order. Additionally, it uses sqrt_price_limit_x96 instead of sqrt_price_limit_x128. When this wrapper is implemented, it will be incompatible with the interface contract call. Update the wrapper signature to match:
pub fn execute_private_swap(
zero_for_one: bool,
amount_specified: u128,
sqrt_price_limit_x128: u256,
new_commitment: felt252,
proof: Array<felt252>,
public_inputs: Array<felt252>,
) -> (i128, i128)
🤖 Prompt for AI Agents
In `@zylith/src/interfaces/izylith.cairo` around lines 18 - 26, The wrapper
execute_private_swap has parameters in the old order and the wrong
sqrt_price_limit type, causing a mismatch with the izylith::private_swap
interface; update execute_private_swap to use the same signature as
private_swap: place zero_for_one, amount_specified, sqrt_price_limit_x128
(u256), new_commitment, then proof: Array<felt252>, public_inputs:
Array<felt252>, and return (i128, i128), so the wrapper's parameter order and
sqrt_price_limit type exactly match the private_swap declaration and
implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/hooks/use-liquidity.ts (2)
1072-1084: Fixprivate_burn_liquidityargument order in manual fallback.The manual fallback still sends
proof/public_inputsfirst, which no longer matches the contract signature, so calldata will be rejected when ASP isn’t used.Proposed fix
- tx = await contract.private_burn_liquidity( - proof, - publicInputs, - tickLowerFelt, // felt252 (contract converts to i32 internally) - tickUpperFelt, // felt252 (contract converts to i32 internally) - liquidity, - outputNote.commitment - ); + tx = await contract.private_burn_liquidity( + tickLowerFelt, // felt252 (contract converts to i32 internally) + tickUpperFelt, // felt252 (contract converts to i32 internally) + liquidity, + outputNote.commitment, + proof, + publicInputs + );
1232-1248: Fixprivate_collectargument order in manual call.This still uses the old order, so the call will not match the updated ABI.
Proposed fix
- const tx = await contract.private_collect( - proof, - publicInputs, - tickLowerFelt, // felt252 (contract converts to i32 internally) - tickUpperFelt, // felt252 (contract converts to i32 internally) - outputNote.commitment - ); + const tx = await contract.private_collect( + tickLowerFelt, // felt252 (contract converts to i32 internally) + tickUpperFelt, // felt252 (contract converts to i32 internally) + outputNote.commitment, + proof, + publicInputs + );
Changes
Contract Updates
Redeployment
Frontend/ASP Updates
New Scripts
Summary by CodeRabbit
New Features
New Tools
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.