Redeploy 4 #27
Redeploy 4 #27
Conversation
📝 WalkthroughWalkthroughThis PR updates the Zylith contract deployment address across configuration files, modifies the private liquidity function ABI signatures in the backend to use felt252 types, refactors the frontend hook to include multi-step Merkle proof validation and ASP-backed proof generation, adds transaction confirmation logic, changes binary references from rapidsnark to prover, and includes new test cases for private liquidity operations. Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend as Frontend Hook<br/>(use-liquidity)
participant ASP as ASP Server<br/>(Proof Generation)
participant Contract as Starknet<br/>Contract
participant Network as Network
User->>Frontend: Initiate Mint Liquidity
activate Frontend
rect rgb(200, 220, 255)
Note over Frontend: Step 1: Fetch & Validate
Frontend->>Frontend: Load Merkle tree info
Frontend->>Frontend: Filter candidate notes<br/>(index, balance, token)
Frontend->>Frontend: Validate commitment<br/>via Merkle proof
end
rect rgb(220, 200, 255)
Note over Frontend: Step 2: Prepare Witness
Frontend->>Frontend: Generate change note
Frontend->>Frontend: Compute position commitment
Frontend->>Frontend: Assemble witness data
end
rect rgb(200, 255, 220)
Note over Frontend,ASP: Step 3: Generate Proof
Frontend->>ASP: POST witness + metadata
activate ASP
ASP->>ASP: Compute Groth16 proof
ASP-->>Frontend: Return proof + public inputs
deactivate ASP
end
rect rgb(255, 240, 200)
Note over Frontend: Step 4: Format & Validate
Frontend->>Frontend: Convert BN254 to felt252
Frontend->>Frontend: Perform range checks<br/>(tick bounds, balances)
Frontend->>Frontend: Format inputs for contract
end
rect rgb(255, 200, 200)
Note over Frontend,Contract: Step 5: Execute & Confirm
Frontend->>Contract: Call private_mint_liquidity<br/>(proof, public inputs, ticks)
activate Contract
Contract->>Contract: Verify proof
Contract->>Contract: Update positions & nullifiers
Contract-->>Network: Submit transaction
deactivate Contract
Network-->>Frontend: Poll for AcceptedOnL2
end
Frontend->>Frontend: Update portfolio & LP state
deactivate Frontend
Frontend-->>User: Liquidity minted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 (2)
zylith/src/zylith.cairo (2)
842-860: Suggest essential refactor: Use helper function for tick conversion.The
private_burn_liquidityfunction uses inline two's complement conversion logic fortick_lowerandtick_upper(lines 842-860), whileprivate_mint_liquidityuses thesafe_tick_conversionhelper function (line 636-638). For consistency and to avoid duplicating conversion logic across functions, use the helper function here as well.🔎 Proposed fix
let tick_lower_u256 = *_verified_inputs.at(2); - let verified_tick_lower: i32 = if tick_lower_u256.low >= 2147483648 { // 2^31 - let two_to_32: u256 = 4294967296; // 2^32 - let value_u256: u256 = tick_lower_u256.low.into(); - let complement: u256 = two_to_32 - value_u256; // 2^32 - value - let neg_val: i32 = complement.low.try_into().unwrap(); - -neg_val - } else { - tick_lower_u256.low.try_into().unwrap() - }; + let verified_tick_lower: i32 = InternalFunctionsImpl::safe_tick_conversion( + tick_lower_u256, + ); + let tick_upper_u256 = *_verified_inputs.at(3); - let verified_tick_upper: i32 = if tick_upper_u256.low >= 2147483648 { // 2^31 - let two_to_32: u256 = 4294967296; // 2^32 - let value_u256: u256 = tick_upper_u256.low.into(); - let complement: u256 = two_to_32 - value_u256; // 2^32 - value - let neg_val: i32 = complement.low.try_into().unwrap(); - -neg_val - } else { - tick_upper_u256.low.try_into().unwrap() - }; + let verified_tick_upper: i32 = InternalFunctionsImpl::safe_tick_conversion( + tick_upper_u256, + );
1551-1570: Suggest essential refactor: Use helper function for tick conversion.The
private_collectfunction also uses inline two's complement conversion logic for ticks (lines 1551-1570). For consistency with the approach being introduced in this PR, use thesafe_tick_conversionhelper function instead of duplicating the conversion logic.🔎 Proposed fix
let tick_lower_u256 = *_verified_inputs.at(2); - let verified_tick_lower: i32 = if tick_lower_u256.low >= 2147483648 { // 2^31 - let two_to_32: u256 = 4294967296; // 2^32 - let value_u256: u256 = tick_lower_u256.low.into(); - let complement: u256 = two_to_32 - value_u256; // 2^32 - value - let neg_val: i32 = complement.low.try_into().unwrap(); - -neg_val - } else { - tick_lower_u256.low.try_into().unwrap() - }; + let verified_tick_lower: i32 = InternalFunctionsImpl::safe_tick_conversion( + tick_lower_u256, + ); + let tick_upper_u256 = *_verified_inputs.at(3); - let verified_tick_upper: i32 = if tick_upper_u256.low >= 2147483648 { // 2^31 - let two_to_32: u256 = 4294967296; // 2^32 - let value_u256: u256 = tick_upper_u256.low.into(); - let complement: u256 = two_to_32 - value_u256; // 2^32 - value - let neg_val: i32 = complement.low.try_into().unwrap(); - -neg_val - } else { - tick_upper_u256.low.try_into().unwrap() - }; + let verified_tick_upper: i32 = InternalFunctionsImpl::safe_tick_conversion( + tick_upper_u256, + );
🤖 Fix all issues with AI agents
In @circuits/proof_script_1766978629003488834.js:
- Around line 1-25: This file circuits/proof_script_1766978629003488834.js is a
temporary proof-generation artifact with hardcoded absolute paths and should be
removed from source control; delete the file (git rm
circuits/proof_script_1766978629003488834.js), add an ignore pattern for these
temp proof/witness files (e.g., ignore swap_proof_*.json, swap_public_*.json,
swap_witness_*.wtns and any proof_script_*.js) to .gitignore, and ensure no
production code references the script (search for its filename or the string
'/tmp/swap_witness_' and remove/update any usages).
In @frontend/src/hooks/use-liquidity.ts:
- Around line 1134-1144: The burnLiquidity and collectFees callbacks have
incomplete dependency arrays causing stale closures; update the useCallback
dependency arrays for burnLiquidity to include notes, getPosition,
removePosition, and updatePosition, and for collectFees to include notes,
getPosition, and updatePosition (in addition to the existing account, provider,
aspClientInstance, removeNote, addNote, addTransaction, updateTransaction), and
make the same dependency corrections for the duplicate callbacks around the
1291-1300 region so the hooks capture the latest state and helper functions.
- Around line 911-923: The mintLiquidity useCallback closes over the notes
variable from usePortfolioStore but notes is missing from its dependency array;
update the dependency array for the mintLiquidity callback to include notes
(alongside account, provider, aspClientInstance, removeNote, addNote,
addTransaction, updateTransaction, addPosition, updatePosition, getPosition) so
the callback sees the latest notes value.
- Around line 1250-1255: The transaction logged in use-liquidity.ts uses
addTransaction with type: "mint" for a collectFees operation; change the
transaction.type to the correct value (e.g., "collect" or the project's
canonical "collectFees" string) so collect actions are recorded correctly,
updating the addTransaction call that builds the transaction object (hash:
tx.transaction_hash, type: ..., status: "pending", timestamp: Date.now()) to use
the correct type.
In @zylith/src/zylith.cairo:
- Line 592: The error string literal 'Failed_u128_into_felt252' contains a
trailing space before the closing quote; locate the occurrence of that literal
in zylith.cairo (the branch handling high == 0 / u256 conversion) and remove the
trailing space so the message is exactly 'Failed_u128_into_felt252', keeping
punctuation and casing unchanged.
- Around line 616-638: The inline two's-complement conversion for tick_lower
(using tick_lower_u256 → value → value_u32 → branching to produce
verified_tick_lower) is duplicated logic; replace that block with a single call
to the existing helper: set verified_tick_lower :=
InternalFunctionsImpl::safe_tick_conversion(tick_lower_u256) (keeping the prior
assert/TICK_OVERFLOW check only if the helper doesn't perform it), remove the
inline bit-twiddling and u32/i32 conversions, and keep the tick_upper_u256
conversion via InternalFunctionsImpl::safe_tick_conversion unchanged so both
tick_lower and tick_upper use the same helper for consistency.
In @zylith/tests/test_zylith.cairo:
- Around line 239-254: The test double-initializes the pool:
initialize_pool(@setup) is called, then the test calls
setup.zylith.initialize(...) between start_cheat_caller_address(...) and
stop_cheat_caller_address(...), causing a duplicate-initialization panic; remove
the redundant manual initialize block (the start_cheat_caller_address(...),
setup.zylith.initialize(...), stop_cheat_caller_address(...) lines) or instead
remove the initial initialize_pool(@setup) call so the pool is only initialized
once—use the existing helper initialize_pool/@setup_with_erc20 and keep only one
initialization path (references: initialize_pool, setup_with_erc20,
setup.zylith.initialize, start_cheat_caller_address, stop_cheat_caller_address).
- Around line 186-199: The test is initializing the pool twice: remove the
redundant call to initialize_pool(@setup) so the pool is only initialized once
via the explicit start_cheat_caller_address / setup.zylith.initialize(...) /
stop_cheat_caller_address sequence; specifically delete the
initialize_pool(@setup) invocation (or alternatively remove the manual
setup.zylith.initialize block) to avoid triggering the duplicate-initialization
assertion in initialize (and preserve test_initialize_twice_fails behavior).
🧹 Nitpick comments (7)
zylith/CONTRACT_ADDRESS.md (1)
25-26: Wrap bare URLs in markdown link syntax for linting compliance.Lines 25-26 contain bare URLs that violate markdown linting rule MD034. Wrap them in markdown link syntax or angle brackets for consistency with markdown standards.
🔎 Proposed markdown link formatting
- Class → https://sepolia.starkscan.co/class/0x42d20d69ec294308319c5cac2ec66052e54b67701de2f75b1bd48fe8d864daf - Contract → https://sepolia.starkscan.co/contract/0x03a2134229b1938316f0db062a15c79a426df51805f57e8efcf306ef1e916aa6 + Class → [https://sepolia.starkscan.co/class/0x42d20d69ec294308319c5cac2ec66052e54b67701de2f75b1bd48fe8d864daf](https://sepolia.starkscan.co/class/0x42d20d69ec294308319c5cac2ec66052e54b67701de2f75b1bd48fe8d864daf) + Contract → [https://sepolia.starkscan.co/contract/0x03a2134229b1938316f0db062a15c79a426df51805f57e8efcf306ef1e916aa6](https://sepolia.starkscan.co/contract/0x03a2134229b1938316f0db062a15c79a426df51805f57e8efcf306ef1e916aa6)Alternatively, wrap in angle brackets for a simpler format:
- Class → https://sepolia.starkscan.co/class/0x42d20d69ec294308319c5cac2ec66052e54b67701de2f75b1bd48fe8d864daf - Contract → https://sepolia.starkscan.co/contract/0x03a2134229b1938316f0db062a15c79a426df51805f57e8efcf306ef1e916aa6 + Class → <https://sepolia.starkscan.co/class/0x42d20d69ec294308319c5cac2ec66052e54b67701de2f75b1bd48fe8d864daf> + Contract → <https://sepolia.starkscan.co/contract/0x03a2134229b1938316f0db062a15c79a426df51805f57e8efcf306ef1e916aa6>asp/src/proof.rs (2)
39-40: Update variable names and comments to reflect binary rename.The binary path has been changed from
bin/rapidsnarktobin/prover, but the variable namerapidsnark_pathand related comments still reference "rapidsnark". This creates confusion about which binary is actually being used.🔎 Recommended refactoring
Consider updating variable names and comments for consistency:
- let rapidsnark_path = asp_dir.join("bin").join("prover"); - let use_rapidsnark = rapidsnark_path.exists(); + let prover_path = asp_dir.join("bin").join("prover"); + let use_prover = prover_path.exists();Also update:
- Line 9: Function doc comment "using rapidsnark" → "using prover"
- Line 14: Comment "Starting swap proof generation with rapidsnark"
- Line 108: Log message "rapidsnark (fast C++ prover)"
- Line 427-428: Conditional message "with rapidsnark"
- Similar changes needed at line 631 and throughout the
generate_lp_prooffunction
631-632: Update variable names and comments to reflect binary rename (LP proof function).The same binary path change from
bin/rapidsnarktobin/proveroccurs here, but variable names and comments still reference "rapidsnark" (lines 606, 700, 720, 1046-1047).This is a duplicate of the issue at line 39. Consider applying the same refactoring throughout the file for consistency.
note.md (1)
1-23: Consider moving deployment notes to a documentation folder.This file contains deployment details and appears to be internal documentation. Consider:
- Moving it to a dedicated
docs/ordeployment/folder for better organization- Using a more descriptive filename like
DEPLOYMENT_NOTES.mdorsepolia-deployment-2025-01.md- Removing or redacting sensitive information like account addresses if this repository is or will become public
The contract address (0x03a2134229b1938316f0db062a15c79a426df51805f57e8efcf306ef1e916aa6) correctly matches the updated address across all configuration files.
asp/src/bin/initialize_pool.rs (1)
135-150: Consider adding a timeout to the transaction confirmation loop.This polling loop could run indefinitely if the transaction gets stuck in a non-terminal state (e.g., network issues, RPC returning unexpected statuses). Consider adding a maximum retry count or timeout to prevent hanging.
🔎 Suggested improvement
// Wait for transaction (poll every 5 seconds) + let mut retries = 0; + const MAX_RETRIES: u32 = 60; // 5 minutes max wait loop { + if retries >= MAX_RETRIES { + return Err("Transaction confirmation timed out after 5 minutes".into()); + } let status = provider.get_transaction_status(transaction_hash).await?; match status { starknet::core::types::TransactionStatus::AcceptedOnL2(_) | starknet::core::types::TransactionStatus::AcceptedOnL1(_) => { println!("✅ Transaction confirmed!"); break; } starknet::core::types::TransactionStatus::Rejected => { return Err("Transaction was rejected".into()); } _ => { + retries += 1; tokio::time::sleep(tokio::time::Duration::from_secs(5)).await; } } }frontend/src/hooks/use-liquidity.ts (1)
360-367: Redundant negative check after conditional assignment.The check
if (changeAmount < 0n)on line 363 is unreachable. The assignment on line 360-361 ensureschangeAmountis eitherinputNote.amount - liquidity(wheninputNote.amount >= liquidity) or0n. Neither case can result in a negative value.🔎 Simplified code
- const changeAmount = - inputNote.amount >= liquidity ? inputNote.amount - liquidity : 0n; - - if (changeAmount < 0n) { - throw new Error( - `Insufficient balance: need ${liquidity.toString()}, have ${inputNote.amount.toString()}` - ); - } + if (inputNote.amount < liquidity) { + throw new Error( + `Insufficient balance: need ${liquidity.toString()}, have ${inputNote.amount.toString()}` + ); + } + const changeAmount = inputNote.amount - liquidity;zylith/src/zylith.cairo (1)
1687-1759: Optional refactor: Consider consolidating two's complement conversion logic.Both
_felt252_to_i32(lines 1687-1723) andsafe_tick_conversion(lines 1725-1759) contain similar two's complement conversion logic for handling negative i32 values. While they operate on different input types (felt252 vs u256), the core conversion logic is duplicated. Consider extracting the two's complement conversion (lines 1698-1722 and 1748-1758) into a shared helper function to improve maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
asp/README.mdasp/bin/proverasp/bin/rapidsnarkasp/bin/test_proverasp/bin/verifierasp/src/abis/zylith-abi.jsonasp/src/bin/initialize_pool.rsasp/src/main.rsasp/src/proof.rsasp/start.shcircuits/proof_script_1766978629003488834.jsfrontend/README.mdfrontend/src/components/portfolio/__tests__/NotesList.test.tsxfrontend/src/hooks/__tests__/use-private-deposit.test.tsfrontend/src/hooks/use-liquidity.tsfrontend/src/lib/abis/zylith-abi.jsonfrontend/src/lib/config.tsnote.mdzylith/CONTRACT_ADDRESS.mdzylith/src/interfaces/izylith.cairozylith/src/zylith.cairozylith/tests/test_zylith.cairo
🧰 Additional context used
🧬 Code graph analysis (2)
circuits/proof_script_1766978629003488834.js (1)
circuits/proof_script_1766710352513392000.js (1)
console(5-24)
asp/src/bin/initialize_pool.rs (2)
asp/src/calldata.rs (1)
u256_to_low_high(231-234)asp/src/blockchain.rs (1)
new(13-26)
🪛 Gitleaks (8.30.0)
asp/src/bin/initialize_pool.rs
[high] 37-37: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 markdownlint-cli2 (0.18.1)
zylith/CONTRACT_ADDRESS.md
25-25: Bare URL used
(MD034, no-bare-urls)
26-26: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (16)
frontend/README.md (1)
47-47: Environment variable example correctly updated.The NEXT_PUBLIC_ZYLITH_CONTRACT example reflects the new contract address as part of the redeploy. This keeps the README documentation synchronized with the actual deployment.
frontend/src/components/portfolio/__tests__/NotesList.test.tsx (1)
130-130: Test data correctly aligned with new contract address.Both instances of mockEvent.from_address are updated to match the new Zylith contract address. Test logic and assertions remain unchanged and correct.
Also applies to: 149-149
zylith/CONTRACT_ADDRESS.md (1)
19-20: Contract address documentation updated correctly.Both the Class Hash and Contract address have been updated to reflect the new deployment. Values are properly formatted and synchronized with other configuration files.
asp/README.md (1)
21-21: LGTM! Contract address updated consistently.The CONTRACT_ADDRESS environment variable has been updated to reflect the new deployment. This change is consistent with the PR-wide contract address update across all configuration files.
Also applies to: 45-45
asp/src/main.rs (1)
66-66: LGTM! Default contract address updated.The fallback contract address has been updated to match the new deployment. This ensures the ASP server uses the correct contract when the
CONTRACT_ADDRESSenvironment variable is not set.asp/src/proof.rs (1)
37-47: Prover binary is present and properly configured.The prover binary exists at
asp/bin/prover(358K, executable) and the code's fallback logic will correctly detect and use it. No action needed for deployment.asp/start.sh (1)
34-34: LGTM!Contract address default updated consistently with other files in the PR.
frontend/src/lib/config.ts (1)
3-5: LGTM!Contract address configuration updated consistently across the codebase.
frontend/src/hooks/__tests__/use-private-deposit.test.ts (1)
21-26: LGTM!Test fixtures properly updated to use the new contract address, maintaining consistency with the configuration changes.
Also applies to: 79-86, 97-106
asp/src/bin/initialize_pool.rs (2)
21-27: LGTM on contract address and env var handling.The contract address update is consistent with the rest of the PR. Using
expect()for required environment variables (PRIVATE_KEY,ACCOUNT_ADDRESS) is appropriate for a CLI tool that should fail early with a clear message if misconfigured.
37-38: False positive from static analysis - these are public contract addresses.The Gitleaks warning about "Generic API Key" is a false positive. These are well-known public Sepolia testnet token contract addresses (ETH and USDC), not API keys or secrets.
frontend/src/hooks/use-liquidity.ts (2)
40-47: LGTM on i32ToFelt252 implementation.The two's complement conversion using
2^32for negative i32 values is correct. This properly handles the conversion from signed integers to felt252 representation.
498-540: BN254 to felt252 conversion logic looks correct.The conversion properly handles the field modulus differences between BN254 and Starknet's felt252, including negative number representation. The fallback handling is reasonable for edge cases.
zylith/src/interfaces/izylith.cairo (1)
37-45: LGTM on interface parameter type changes.The change from
i32tofelt252for tick parameters in private liquidity functions (private_mint_liquidity,private_burn_liquidity,private_collect) is consistent with the ABI updates and frontend changes. The inline comments explaining the Starknet.js compatibility rationale are helpful.The public functions (
mint,burn,collect) correctly retain theiri32parameter types.Also applies to: 47-55, 57-64
asp/src/abis/zylith-abi.json (1)
163-170: LGTM on ABI updates.The ABI parameter changes for private liquidity functions are consistent with:
- The Cairo interface in
zylith/src/interfaces/izylith.cairo- The frontend's
i32ToFelt252conversion logic inuse-liquidity.tsThe distinction between private functions (now using
felt252) and public functions (retainingi32) is correctly maintained.Also applies to: 199-206, 235-242
frontend/src/lib/abis/zylith-abi.json (1)
260-265: LGTM! Public function parameter types updated consistently.The parameter name and type changes from
tick_lower_felt/tick_upper_felt(felt252) totick_lower/tick_upper(i32) for the publicmint,burn, andcollectfunctions are consistent with the contract implementation. The private function variants correctly retain felt252 types for Starknet.js compatibility.Also applies to: 308-313, 332-337
|
|
||
| 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_1766978629003488834.wtns' | ||
| ); | ||
|
|
||
| const elapsed = ((Date.now() - startTime) / 1000).toFixed(2); | ||
| console.log('Proof generated in', elapsed, 'seconds'); | ||
|
|
||
| fs.writeFileSync('/tmp/swap_proof_1766978629003488834.json', JSON.stringify(proof, null, 2)); | ||
| fs.writeFileSync('/tmp/swap_public_1766978629003488834.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.
Remove temporary proof generation script from version control.
This file appears to be a temporary proof generation script (indicated by the timestamp 1766978629003488834 in the filename). It contains hardcoded absolute paths specific to a local machine:
- Line 11:
/home/ryzen/Desktop/dev/starknet-bounty/circuits/build/zkeys/swap.zkey - Line 12:
/tmp/swap_witness_1766978629003488834.wtns - Lines 18-19:
/tmp/swap_proof_*.jsonand/tmp/swap_public_*.json
These paths won't work on other systems and the script appears to be a build artifact rather than source code.
🔎 Recommendation
Add this pattern to .gitignore to prevent similar files from being committed:
+# Temporary proof generation scripts
+circuits/proof_script_*.jsThen remove this file from the repository:
git rm circuits/proof_script_1766978629003488834.jsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @circuits/proof_script_1766978629003488834.js around lines 1 - 25, This file
circuits/proof_script_1766978629003488834.js is a temporary proof-generation
artifact with hardcoded absolute paths and should be removed from source
control; delete the file (git rm circuits/proof_script_1766978629003488834.js),
add an ignore pattern for these temp proof/witness files (e.g., ignore
swap_proof_*.json, swap_public_*.json, swap_witness_*.wtns and any
proof_script_*.js) to .gitignore, and ensure no production code references the
script (search for its filename or the string '/tmp/swap_witness_' and
remove/update any usages).
| [ | ||
| account, | ||
| provider, | ||
| aspClientInstance, | ||
| removeNote, | ||
| addNote, | ||
| addTransaction, | ||
| updateTransaction, | ||
| addPosition, | ||
| updatePosition, | ||
| getPosition, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Missing notes in useCallback dependency array.
The mintLiquidity callback references notes (line 125) from usePortfolioStore, but notes is not included in the dependency array. This can cause stale closure issues where the callback operates on outdated note data.
🔎 Proposed fix
[
account,
provider,
aspClientInstance,
+ notes,
removeNote,
addNote,
addTransaction,
updateTransaction,
addPosition,
updatePosition,
getPosition,
]📝 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.
| [ | |
| account, | |
| provider, | |
| aspClientInstance, | |
| removeNote, | |
| addNote, | |
| addTransaction, | |
| updateTransaction, | |
| addPosition, | |
| updatePosition, | |
| getPosition, | |
| ] | |
| ); | |
| [ | |
| account, | |
| provider, | |
| aspClientInstance, | |
| notes, | |
| removeNote, | |
| addNote, | |
| addTransaction, | |
| updateTransaction, | |
| addPosition, | |
| updatePosition, | |
| getPosition, | |
| ] |
🤖 Prompt for AI Agents
In @frontend/src/hooks/use-liquidity.ts around lines 911 - 923, The
mintLiquidity useCallback closes over the notes variable from usePortfolioStore
but notes is missing from its dependency array; update the dependency array for
the mintLiquidity callback to include notes (alongside account, provider,
aspClientInstance, removeNote, addNote, addTransaction, updateTransaction,
addPosition, updatePosition, getPosition) so the callback sees the latest notes
value.
| }, | ||
| [ | ||
| account, | ||
| provider, | ||
| aspClientInstance, | ||
| removeNote, | ||
| addNote, | ||
| addTransaction, | ||
| updateTransaction, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Missing dependencies in burnLiquidity and collectFees callbacks.
Similar to mintLiquidity, these callbacks are missing several dependencies:
burnLiquidityis missing:notes,getPosition,removePosition,updatePositioncollectFeesis missing:notes,getPosition,updatePosition
These missing dependencies can lead to stale closure issues.
🔎 Proposed fix for burnLiquidity
[
account,
provider,
aspClientInstance,
+ notes,
removeNote,
addNote,
addTransaction,
updateTransaction,
+ getPosition,
+ removePosition,
+ updatePosition,
]Also applies to: 1291-1300
🤖 Prompt for AI Agents
In @frontend/src/hooks/use-liquidity.ts around lines 1134 - 1144, The
burnLiquidity and collectFees callbacks have incomplete dependency arrays
causing stale closures; update the useCallback dependency arrays for
burnLiquidity to include notes, getPosition, removePosition, and updatePosition,
and for collectFees to include notes, getPosition, and updatePosition (in
addition to the existing account, provider, aspClientInstance, removeNote,
addNote, addTransaction, updateTransaction), and make the same dependency
corrections for the duplicate callbacks around the 1291-1300 region so the hooks
capture the latest state and helper functions.
| addTransaction({ | ||
| hash: tx.transaction_hash, | ||
| type: "mint", // Using 'mint' type for collect | ||
| status: "pending", | ||
| timestamp: Date.now(), | ||
| }); |
There was a problem hiding this comment.
Incorrect transaction type for collectFees.
The transaction is recorded with type: "mint" but this is a collect operation. This will cause confusion in transaction history and analytics.
🔎 Proposed fix
addTransaction({
hash: tx.transaction_hash,
- type: "mint", // Using 'mint' type for collect
+ type: "collect",
status: "pending",
timestamp: Date.now(),
});📝 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.
| addTransaction({ | |
| hash: tx.transaction_hash, | |
| type: "mint", // Using 'mint' type for collect | |
| status: "pending", | |
| timestamp: Date.now(), | |
| }); | |
| addTransaction({ | |
| hash: tx.transaction_hash, | |
| type: "collect", | |
| status: "pending", | |
| timestamp: Date.now(), | |
| }); |
🤖 Prompt for AI Agents
In @frontend/src/hooks/use-liquidity.ts around lines 1250 - 1255, The
transaction logged in use-liquidity.ts uses addTransaction with type: "mint" for
a collectFees operation; change the transaction.type to the correct value (e.g.,
"collect" or the project's canonical "collectFees" string) so collect actions
are recorded correctly, updating the addTransaction call that builds the
transaction object (hash: tx.transaction_hash, type: ..., status: "pending",
timestamp: Date.now()) to use the correct type.
| // Extract felt252 values from u256 | ||
| // The verifier returns u256, but felt252 values can be >= 2^128 | ||
| // If high == 0, use .low; otherwise use the full u256 | ||
| // If high == 0, use .l ow; otherwise use the full u256 |
There was a problem hiding this comment.
Minor: Trailing space in error message.
The error message 'Failed_u128_into_felt252' has a trailing space before the closing quote. This is inconsistent with error message formatting conventions and could cause confusion when debugging.
🔎 Proposed fix
- verified_nullifier_u256.low.try_into().expect('Failed_u128_into_felt252')
+ verified_nullifier_u256.low.try_into().expect('Failed_u128_into_felt252')Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @zylith/src/zylith.cairo at line 592, The error string literal
'Failed_u128_into_felt252' contains a trailing space before the closing quote;
locate the occurrence of that literal in zylith.cairo (the branch handling high
== 0 / u256 conversion) and remove the trailing space so the message is exactly
'Failed_u128_into_felt252', keeping punctuation and casing unchanged.
| let value = tick_lower_u256.low; // u128, must be < 2^32 | ||
|
|
||
| assert(value <= 4294967295, 'TICK_OVERFLOW'); | ||
|
|
||
| // Step 1: Convert u128 to u32 first (this is safe now) | ||
| let value_u32: u32 = value.try_into().expect('u32 conversion failed'); | ||
|
|
||
| // Step 2: Handle two's complement on u32 (avoiding u128 arithmetic) | ||
| let verified_tick_lower: i32 = if value_u32 >= 2147483648 { | ||
| // Negative number: use bitwise complement instead of subtraction | ||
| let complement = value_u32 ^ 0xFFFFFFFF_u32; // Flip all bits | ||
| let magnitude = complement + 1_u32; // Add 1 | ||
| let as_i32: i32 = magnitude.try_into().expect('i32 conversion failed'); | ||
| -as_i32 | ||
| } else { | ||
| tick_lower_u256.low.try_into().unwrap() | ||
| // Positive number: direct conversion | ||
| value_u32.try_into().expect('i32 conversion failed') | ||
| }; | ||
| let tick_upper_u256 = *_verified_inputs.at(3); | ||
| let verified_tick_upper: i32 = if tick_upper_u256.low >= 2147483648 { // 2^31 | ||
| let two_to_32: u256 = 4294967296; // 2^32 | ||
| let value_u256: u256 = tick_upper_u256.low.into(); | ||
| let complement: u256 = two_to_32 - value_u256; // 2^32 - value | ||
| let neg_val: i32 = complement.low.try_into().unwrap(); | ||
| -neg_val | ||
| } else { | ||
| tick_upper_u256.low.try_into().unwrap() | ||
| }; | ||
|
|
||
| let verified_tick_upper: i32 = InternalFunctionsImpl::safe_tick_conversion( | ||
| tick_upper_u256, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor: Inconsistent tick conversion approaches.
The code converts tick_lower using inline logic (lines 616-633) but converts tick_upper using the safe_tick_conversion helper function (lines 636-638). Both perform the same two's complement conversion from u256 to i32. For consistency and maintainability, use the helper function for both conversions.
🔎 Proposed fix
- // Convert u256 to i32 safely (handles two's complement for negative values)
let tick_lower_u256 = *_verified_inputs.at(2);
- let value = tick_lower_u256.low; // u128, must be < 2^32
-
- assert(value <= 4294967295, 'TICK_OVERFLOW');
-
- // Step 1: Convert u128 to u32 first (this is safe now)
- let value_u32: u32 = value.try_into().expect('u32 conversion failed');
-
- // Step 2: Handle two's complement on u32 (avoiding u128 arithmetic)
- let verified_tick_lower: i32 = if value_u32 >= 2147483648 {
- // Negative number: use bitwise complement instead of subtraction
- let complement = value_u32 ^ 0xFFFFFFFF_u32; // Flip all bits
- let magnitude = complement + 1_u32; // Add 1
- let as_i32: i32 = magnitude.try_into().expect('i32 conversion failed');
- -as_i32
- } else {
- // Positive number: direct conversion
- value_u32.try_into().expect('i32 conversion failed')
- };
+ let verified_tick_lower: i32 = InternalFunctionsImpl::safe_tick_conversion(
+ tick_lower_u256,
+ );
+
let tick_upper_u256 = *_verified_inputs.at(3);
-
let verified_tick_upper: i32 = InternalFunctionsImpl::safe_tick_conversion(
tick_upper_u256,
);
-Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @zylith/src/zylith.cairo around lines 616 - 638, The inline two's-complement
conversion for tick_lower (using tick_lower_u256 → value → value_u32 → branching
to produce verified_tick_lower) is duplicated logic; replace that block with a
single call to the existing helper: set verified_tick_lower :=
InternalFunctionsImpl::safe_tick_conversion(tick_lower_u256) (keeping the prior
assert/TICK_OVERFLOW check only if the helper doesn't perform it), remove the
inline bit-twiddling and u32/i32 conversions, and keep the tick_upper_u256
conversion via InternalFunctionsImpl::safe_tick_conversion unchanged so both
tick_lower and tick_upper use the same helper for consistency.
| let setup = setup_with_erc20(); | ||
| initialize_pool(@setup); | ||
|
|
||
| start_cheat_caller_address(setup.zylith.contract_address, caller()); | ||
| setup | ||
| .zylith | ||
| .initialize( | ||
| setup.token0.contract_address, | ||
| setup.token1.contract_address, | ||
| TEST_FEE, | ||
| TICK_SPACING, | ||
| sqrt_price, | ||
| ); | ||
| stop_cheat_caller_address(setup.zylith.contract_address); |
There was a problem hiding this comment.
Critical: Duplicate pool initialization will cause test to fail.
The test calls initialize_pool(@setup) at line 187, which already initializes the pool. Then lines 189-199 attempt to initialize the pool again, which will panic based on the assertion at line 112 (assert!(!self.initialized.read())). This duplicate initialization is tested and expected to fail in test_initialize_twice_fails (lines 148-177).
🔎 Proposed fix
fn test_private_mint_liquidity_with_valid_proof() {
let setup = setup_with_erc20();
- initialize_pool(@setup);
-
- start_cheat_caller_address(setup.zylith.contract_address, caller());
- setup
- .zylith
- .initialize(
- setup.token0.contract_address,
- setup.token1.contract_address,
- TEST_FEE,
- TICK_SPACING,
- sqrt_price,
- );
- stop_cheat_caller_address(setup.zylith.contract_address);
+ initialize_pool(@setup);
// Deposit tokens first
let commitment: felt252 = 0x111;📝 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.
| let setup = setup_with_erc20(); | |
| initialize_pool(@setup); | |
| start_cheat_caller_address(setup.zylith.contract_address, caller()); | |
| setup | |
| .zylith | |
| .initialize( | |
| setup.token0.contract_address, | |
| setup.token1.contract_address, | |
| TEST_FEE, | |
| TICK_SPACING, | |
| sqrt_price, | |
| ); | |
| stop_cheat_caller_address(setup.zylith.contract_address); | |
| let setup = setup_with_erc20(); | |
| initialize_pool(@setup); | |
| // Deposit tokens first | |
| let commitment: felt252 = 0x111; |
🤖 Prompt for AI Agents
In @zylith/tests/test_zylith.cairo around lines 186 - 199, The test is
initializing the pool twice: remove the redundant call to
initialize_pool(@setup) so the pool is only initialized once via the explicit
start_cheat_caller_address / setup.zylith.initialize(...) /
stop_cheat_caller_address sequence; specifically delete the
initialize_pool(@setup) invocation (or alternatively remove the manual
setup.zylith.initialize block) to avoid triggering the duplicate-initialization
assertion in initialize (and preserve test_initialize_twice_fails behavior).
| let setup = setup_with_erc20(); | ||
| initialize_pool(@setup); | ||
|
|
||
| // Initialize pool | ||
|
|
||
| start_cheat_caller_address(setup.zylith.contract_address, caller()); | ||
| setup | ||
| .zylith | ||
| .initialize( | ||
| setup.token0.contract_address, | ||
| setup.token1.contract_address, | ||
| TEST_FEE, | ||
| TICK_SPACING, | ||
| sqrt_price, | ||
| ); | ||
| stop_cheat_caller_address(setup.zylith.contract_address); |
There was a problem hiding this comment.
Critical: Duplicate pool initialization will cause test to fail.
Similar to the mint test, this test calls initialize_pool(@setup) at line 240, then immediately tries to initialize the pool again at lines 244-254. This will panic due to the duplicate initialization check.
🔎 Proposed fix
fn test_private_burn_liquidity_with_valid_proof() {
let setup = setup_with_erc20();
initialize_pool(@setup);
- // Initialize pool
-
- start_cheat_caller_address(setup.zylith.contract_address, caller());
- setup
- .zylith
- .initialize(
- setup.token0.contract_address,
- setup.token1.contract_address,
- TEST_FEE,
- TICK_SPACING,
- sqrt_price,
- );
- stop_cheat_caller_address(setup.zylith.contract_address);
-
// Private deposit🤖 Prompt for AI Agents
In @zylith/tests/test_zylith.cairo around lines 239 - 254, The test
double-initializes the pool: initialize_pool(@setup) is called, then the test
calls setup.zylith.initialize(...) between start_cheat_caller_address(...) and
stop_cheat_caller_address(...), causing a duplicate-initialization panic; remove
the redundant manual initialize block (the start_cheat_caller_address(...),
setup.zylith.initialize(...), stop_cheat_caller_address(...) lines) or instead
remove the initial initialize_pool(@setup) call so the pool is only initialized
once—use the existing helper initialize_pool/@setup_with_erc20 and keep only one
initialization path (references: initialize_pool, setup_with_erc20,
setup.zylith.initialize, start_cheat_caller_address, stop_cheat_caller_address).
2 participants
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.