[Feat]:: added tests for zylith contract #23
Conversation
📝 WalkthroughWalkthroughAdds debug print statements to the private_swap flow and expands test coverage with eight syscall-backed private-operation tests (deposits, withdrawals, mint/burn liquidity). Contract deployments in tests switched to syscall-backed variants. Changes
Sequence Diagram(s)(omitted — changes are debugging output and added tests; no new multi-component control flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 5
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
zylith/src/zylith.cairozylith/tests/test_zylith.cairo
🔇 Additional comments (2)
zylith/tests/test_zylith.cairo (2)
31-31: LGTM! Syscall-backed deployment is properly implemented.The switch from
.unwrap()to.unwrap_syscall()for contract deployment and instantiation is consistent throughout the test suite and aligns with the PR's objective of using syscall-backed variants.Also applies to: 37-37, 42-42, 45-49, 58-58
550-873: LGTM! The private withdraw, mint, and burn tests are well-structured.These tests properly:
- Initialize the pool without duplication
- Cover positive and negative test cases (valid proofs, invalid tokens, amount mismatches)
- Verify state changes (balance transfers, nullifier spending, root updates)
- Follow the CEI (Checks-Effects-Interactions) pattern
The test coverage for private operations is comprehensive.
| // for testing after all thing good then remove | ||
| println!("amount :{} expected amount0 : {}", amount0, expected_amount0_delta); | ||
| println!("amount :{} expected amount1 : {}", amount1, expected_amount1_delta); | ||
| println!( | ||
| "amount :{} expected amount1 : {}", actual_new_sqrt_price, expected_new_sqrt_price, | ||
| ); |
There was a problem hiding this comment.
Remove debugging output before merging to main.
These println! statements are temporary debugging hooks that should not be merged to the main branch. Additionally, line 255 has an incorrect label—it says "amount1" but prints actual_new_sqrt_price and expected_new_sqrt_price.
🔎 Proposed fix
- // for testing after all thing good then remove
- println!("amount :{} expected amount0 : {}", amount0, expected_amount0_delta);
- println!("amount :{} expected amount1 : {}", amount1, expected_amount1_delta);
- println!(
- "amount :{} expected amount1 : {}", actual_new_sqrt_price, expected_new_sqrt_price,
- );
-📝 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.
| // for testing after all thing good then remove | |
| println!("amount :{} expected amount0 : {}", amount0, expected_amount0_delta); | |
| println!("amount :{} expected amount1 : {}", amount1, expected_amount1_delta); | |
| println!( | |
| "amount :{} expected amount1 : {}", actual_new_sqrt_price, expected_new_sqrt_price, | |
| ); |
🤖 Prompt for AI Agents
In zylith/src/zylith.cairo around lines 251 to 256, remove the temporary
println! debugging statements; specifically delete the three println! lines and
ensure no stray debug output remains, and if logging is required later replace
with a proper debug log call — also correct the mislabeled third print (it
currently says "amount1" but prints
actual_new_sqrt_price/expected_new_sqrt_price) by removing it or fixing the
label if you choose to keep a formatted log.
| }; | ||
| use starknet::ContractAddress; | ||
| use starknet::{ContractAddress, SyscallResultTrait}; | ||
| use zylith::clmm::math; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the test file to understand its structure
cat -n zylith/tests/test_zylith.cairo | head -50Repository: SunsetLabs-Game/Zylith
Length of output: 2410
🏁 Script executed:
# Search for all math:: usage in the file
rg -n 'math::' zylith/tests/test_zylith.cairoRepository: SunsetLabs-Game/Zylith
Length of output: 285
🏁 Script executed:
# Check the total line count to understand the file size
wc -l zylith/tests/test_zylith.cairoRepository: SunsetLabs-Game/Zylith
Length of output: 100
Remove the unused math import on line 10.
The math module is imported but only referenced in commented-out code (lines 322, 324, 415, 497, 499). Since no active tests use it, remove this import to keep dependencies clean.
🤖 Prompt for AI Agents
In zylith/tests/test_zylith.cairo around line 10, the file imports
`zylith::clmm::math` which is unused (only referenced in commented-out lines
322, 324, 415, 497, 499); remove the `use zylith::clmm::math;` import from line
10 to eliminate the unused dependency and keep imports clean, and run
tests/linters to confirm no remaining references require restoring the import.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zylith/tests/test_zylith.cairo (1)
101-115: Critical: Helper function ignores its parameter.The
initialize_poolhelper accepts a@TestSetupparameter but immediately shadows it by creating a new setup withsetup_with_erc20()on line 102. This means the passed-in setup is completely ignored, wasting all the token minting and approvals done in the test.🔎 Proposed fix
fn initialize_pool(setup: @TestSetup) { - let setup = setup_with_erc20(); - - start_cheat_caller_address(setup.zylith.contract_address, caller()); - setup + start_cheat_caller_address((*setup.zylith).contract_address, caller()); + (*setup) .zylith .initialize( - setup.token0.contract_address, - setup.token1.contract_address, + (*setup.token0).contract_address, + (*setup.token1).contract_address, TEST_FEE, TICK_SPACING, sqrt_price, ); - stop_cheat_caller_address(setup.zylith.contract_address); + stop_cheat_caller_address((*setup.zylith).contract_address); }
♻️ Duplicate comments (6)
zylith/tests/test_zylith.cairo (6)
186-219: Remove duplicate initialization call.This test calls
initialize_pool(@setup)at line 188, then immediately callsinitialize()again at lines 190-200. This will trigger the double-initialization check and cause the test to fail.
223-237: Remove duplicate initialization call.Same issue as above:
initialize_pool(@setup)at line 225 followed by redundantinitialize()at lines 227-237.
249-262: Remove duplicate initialization call.Same issue:
initialize_pool(@setup)at line 250 followed by redundantinitialize()at lines 252-262.
286-377: Address the commented-out swap tests before merging.The commented-out
private_swaptest block indicates incomplete functionality. Per the PR description,_execute_swapreturns (0, 0) and throws "invalid sqrt_price" errors. Either fix the implementation or remove the commented code and track in a follow-up issue.
503-519: Remove duplicate initialization call.Same issue:
initialize_pool(@setup)at line 505 followed by redundantinitialize()at lines 509-519 will cause double-initialization failure.🔎 Proposed fix
#[test] #[should_panic(expected: ('AMOUNT_MISMATCH',))] fn test_private_withdraw_amount_mismatch_fails() { 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 first let commitment: felt252 = 0x111;
610-626: Remove duplicate initialization call.Same issue:
initialize_pool(@setup)at line 612 followed by redundantinitialize()at lines 616-626 will cause double-initialization failure.🔎 Proposed fix
#[test] 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 let deposit_commitment: felt252 = 0x111;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
zylith/tests/test_zylith.cairo
🔇 Additional comments (1)
zylith/tests/test_zylith.cairo (1)
9-9: LGTM: Syscall-backed deployment correctly implemented.The addition of
SyscallResultTraitand consistent switch fromunwrap()tounwrap_syscall()across all deployment calls properly implements syscall-backed contract deployment.Also applies to: 31-31, 37-37, 42-42, 45-49, 58-58
| fn test_private_withdraw_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); |
There was a problem hiding this comment.
Remove duplicate initialization call.
After calling initialize_pool(@setup) at line 386, this test redundantly calls initialize() again at lines 390-400. This will cause a double-initialization error and test failure.
🔎 Proposed fix
#[test]
fn test_private_withdraw_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 first
let commitment: felt252 = 0x111;🤖 Prompt for AI Agents
In zylith/tests/test_zylith.cairo around lines 384 to 400, the test calls
initialize_pool(@setup) at line ~386 and then redundantly calls
setup.zylith.initialize(...) inside a
start_cheat_caller_address/stop_cheat_caller_address block at lines ~390-400;
remove the duplicated explicit initialize call and its surrounding
start_cheat_caller_address/stop_cheat_caller_address wrapper so the pool is only
initialized once by initialize_pool(@setup).
| fn test_private_withdraw_invalid_token_fails() { | ||
| 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.
Remove duplicate initialization call.
Same issue: initialize_pool(@setup) at line 449 followed by redundant initialize() at lines 453-463 will cause double-initialization failure.
🔎 Proposed fix
#[test]
#[should_panic(expected: ('INVALID_TOKEN',))]
fn test_private_withdraw_invalid_token_fails() {
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 first
let commitment: felt252 = 0x111;🤖 Prompt for AI Agents
In zylith/tests/test_zylith.cairo around lines 447 to 463, the test calls
initialize_pool(@setup) and then immediately calls setup.zylith.initialize(...)
again causing duplicate initialization; remove the redundant explicit
setup.zylith.initialize(...) block (lines 453–463) so the pool is initialized
only once by initialize_pool(@setup), or alternatively remove initialize_pool
and keep the explicit initialize call—ensure only a single initialization call
remains.
| 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); |
There was a problem hiding this comment.
Remove duplicate initialization call.
After calling initialize_pool(@setup) at line 560, this test redundantly calls initialize() again at lines 562-572, causing a double-initialization error.
🔎 Proposed fix
#[test]
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);
-
// Deposit tokens first
let commitment: felt252 = 0x111;🤖 Prompt for AI Agents
In zylith/tests/test_zylith.cairo around lines 558 to 572, the test calls
initialize_pool(@setup) at line 560 and then redundantly calls
setup.zylith.initialize(...) between lines 562-572 causing
double-initialization; remove the second explicit initialize() call (lines
562-572) so the test uses the single initialization performed by
initialize_pool(@setup), ensuring any caller setup or cheat-caller handling from
the removed block is either moved into initialize_pool or omitted if
unnecessary.
1 participant
Note :
this above function not work as expected , it give amount0 , amount1 => (0 , 0) and also invalid sqrt_price etc
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.