Feat/damm v2 compounding & single sided pool#49
Feat/damm v2 compounding & single sided pool#49andrewsource147 merged 7 commits intorelease_0.2.2from
Conversation
|
/bot review |
|
Bot Agent progress:
Progress:
|
|
Bot Agent progress:
Progress:
|
![metoeraag-bot[bot]](https://avatars.githubusercontent.com/u/126859799?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Compounding support is mostly wired through, but there are four substantive issues to address before this is safe: (1) the zap liquidity path reads DAMM v2 reserve fields directly even though migrated older-layout pools may not have valid stored token amounts yet, so it should use DAMM's own liquidity handler or gate on layout version; (2) the DAMM rev bump reintroduces git-sourced copies of workspace crates like jupiter, zap-sdk, and protocol-zap, which is already surfacing as duplicate builds and should be patched to local workspace crates; (3) validate_swap_result has reserve-accounting bugs where compounding_fee is credited to pool_amount_b in both branches even when the fee is actually denominated in token A for certain direction/fee-mode combinations, skewing subsequent iterations; and (4) safe_mul_div_cast_u128's rounding-up path uses div_ceil directly and can panic on zero denominator instead of returning the same kind of arithmetic error as the rounding-down path.
Inline comments
See inline comments on the diff.
Claude (API) review
Rounds: 3 (min=1, max=10).
First look: Codex + Claude (API).
Agreement: yes (no new findings/questions).
Debate transcript
First look comparison
Codex (first look):
Summary:
Compounding support is mostly wired through, but I found one correctness risk around how compounding reserves are sourced for migrated DAMM v2 pools, plus one dependency-management issue from the DAMM rev bump.
Inline comment highlights:
- programs/zap/src/utils/liquidity_handler/mod.rs:28: This compounding branch reads
pool.token_a_amount/pool.token_b_amountdirectly instead of delegating to DAMM's ownpool.get_liquidity_handler(). In the 0.2.0 layout those fields are only valid after the pool has been upgraded (`layo - programs/zap/Cargo.toml:28: Updating
cp-ammto this rev pullsprotocol-zap,zap-sdk, andjupiterback in from a git checkout of this same repo (you can see the duplicate crate entries inCargo.lock). That leaves the workspace building two copies of these cra
Claude (API) (first look):
Summary:
PR upgrades damm-v2 dependency to v0.2.0 and adds support for compounding fee mode pools. The main change introduces a LiquidityHandler trait to abstract between concentrated and compounding liquidity math. A few correctness issues were identified in the swap simulation and the compounding fee accounting for AtoB trades.
Inline comment highlights:
- programs/zap/src/utils/damm_v2_utils.rs:189: Bug: For AtoB trades, the compounding fee is denominated in the input token (token A) when
fees_on_input, or in the output token (token B) whenfees_on_output. However, you always addcompounding_feetopool_amount_bregardless - programs/zap/src/utils/liquidity_handler/compounding_liquidity.rs:48: Potential division-by-zero: If
token_a_amountis 0,get_liquidity_delta_from_amount_awill fail with a division-by-zero error. Same fortoken_b_amountinget_liquidity_delta_from_amount_b. While pools normally have non-zero reserves - programs/zap/src/math/utils_math.rs:34: The
Rounding::Upcase insafe_mul_div_cast_u128usesdiv_ceildirectly onU256without going throughsafe_div. Ifdenominatoris zero, this will panic rather than returning a controlled error. TheRounding::Downpath uses `saf - programs/zap/src/instructions/ix_zap_in_damm_v2.rs:274: The
drop(liquidity_handler)on line 274 is inside theif liquidity > 0branch, but ifliquidity == 0the handler is never dropped before the function continues. While Rust will drop it eventually at scope end, the explicit `drop(pool) - programs/zap/src/utils/liquidity_handler/mod.rs:23: Minor: Using
Box<dyn LiquidityHandler>incurs a heap allocation on every call. Since there are only two variants and this is called in a compute-constrained Solana program, consider using an enum dispatch instead to avoid the allocation o
Comparison notes:
Review synthesis
- No direct disagreements: both reviews agree the PR is close, but compounding support still has correctness risks.
- Review A emphasizes integration risk: migrated DAMM v2 pools may read stale/invalid reserve fields unless Zap delegates to DAMM’s own liquidity handler; it also flags the dependency duplication/regression from the
cp-ammrev bump. - Review B emphasizes execution-path correctness: swap simulation may apply compounding fees to the wrong token side on A→B trades, plus it calls out zero-reserve / zero-denominator safety and a couple of lower-priority perf/lifetime issues.
Most important follow-up action:
Fix and add tests for the compounding correctness path before merge—especially reserve sourcing for migrated pools and fee-token accounting in A→B swap simulation.
Round 1
Codex (primary):
Summary:
Compounding support is mostly wired through, but I found one correctness risk around how compounding reserves are sourced for migrated DAMM v2 pools, plus one dependency-management issue from the DAMM rev bump.
Inline comment highlights:
- programs/zap/src/utils/liquidity_handler/mod.rs:28: This compounding branch reads
pool.token_a_amount/pool.token_b_amountdirectly instead of delegating to DAMM's ownpool.get_liquidity_handler(). In the 0.2.0 layout those fields are only valid after the pool has been upgraded (`layo - programs/zap/Cargo.toml:28: Updating
cp-ammto this rev pullsprotocol-zap,zap-sdk, andjupiterback in from a git checkout of this same repo (you can see the duplicate crate entries inCargo.lock). That leaves the workspace building two copies of these cra
Claude (API):
(no additional notes)
Codex (response):
- Open questions kept for author: 1/1
Round 2
Codex (primary):
Summary:
Compounding support is mostly wired through, but I found one correctness risk around how compounding reserves are sourced for migrated DAMM v2 pools, plus one dependency-management issue from the DAMM rev bump.
Inline comment highlights:
- programs/zap/src/utils/liquidity_handler/mod.rs:28: This compounding branch reads
pool.token_a_amount/pool.token_b_amountdirectly instead of delegating to DAMM's ownpool.get_liquidity_handler(). In the 0.2.0 layout those fields are only valid after the pool has been upgraded (`layo - programs/zap/Cargo.toml:28: Updating
cp-ammto this rev pullsprotocol-zap,zap-sdk, andjupiterback in from a git checkout of this same repo (you can see the duplicate crate entries inCargo.lock). That leaves the workspace building two copies of these cra
Claude (API):
Critique of primary review
The primary review correctly identifies the dependency duplication issue from the cp-amm rev bump and the reserve sourcing risk for un-upgraded pools. Both are valid and important.
However, the primary review missed the most impactful correctness bug: the compounding fee is unconditionally added to pool_amount_b in both the AtoB and BtoA branches of validate_swap_result, regardless of which token the fee is actually denominated in. This affects the simulated reserve tracking across the iterative swap calculation loop and will produce incorrect zap amounts for compounding fee pools.
The primary review also missed the div_ceil panic-on-zero inconsistency in safe_mul_div_cast_u128.
The reserve sourcing concern from the primary review (line 28 of liquidity_handler/mod.rs) is valid and important — if layout_version == 0 pools can reach this code path, token_a_amount/token_b_amount will be zero/garbage, causing division-by-zero in CompoundingLiquidity. However, it's worth clarifying whether such pools can actually have collect_fee_mode == Compounding, since that mode may only exist for v1+ layouts.
Codex (response):
- New findings applied: 3/3
- Open questions kept for author: 2/2
Round 3
Codex (primary):
Summary:
Compounding support is mostly wired through, but I found four issues to address before this looks safe: a reserve-sourcing risk for migrated DAMM v2 pools that may not have valid stored token amounts yet, a dependency-management issue from the DAMM rev bump, a correctness bug in validate_swap_result where compounding fees are credited to the wrong reserve side for some direction/fee-mode combinations, and a panic-on-zero inconsistency in safe_mul_div_cast_u128's rounding-up path.
Inline comment highlights:
- programs/zap/src/utils/liquidity_handler/mod.rs:28: This compounding branch reads
pool.token_a_amount/pool.token_b_amountdirectly instead of delegating to DAMM's ownpool.get_liquidity_handler(). In the 0.2.0 layout those fields are only valid after the pool has been upgraded (`layo - programs/zap/Cargo.toml:28: Updating
cp-ammto this rev pullsprotocol-zap,zap-sdk, andjupiterback in from a git checkout of this same repo (you can see the duplicate crate entries inCargo.lock). That leaves the workspace building two copies of these cra - programs/zap/src/utils/damm_v2_utils.rs:189:
compounding_feegets added topool_amount_bunconditionally here, but forAtoBwithfees_on_inputthe fee is denominated in token A, not token B. That mis-attributes the fee to the wrong reserve and skews the simulated pool state fo - programs/zap/src/utils/damm_v2_utils.rs:204: Same reserve-accounting bug on the
BtoAbranch:compounding_feeis always added topool_amount_b, but when fees are charged on output the fee is denominated in token A forBtoA. In that case this should increasepool_amount_a, oth - programs/zap/src/math/utils_math.rs:34:
Rounding::Upusesdiv_ceildirectly here, which will panic ifdenominator == 0. TheRounding::Downpath usessafe_divand returns an error instead, so this introduces an inconsistent abort-on-zero path in code that otherwise handl
Claude (API):
The primary review correctly identified all four key issues: reserve-sourcing risk for migrated pools (layout_version == 0), dependency duplication from the cp-amm rev bump, compounding fee mis-attribution to wrong reserve side, and div_ceil panic-on-zero inconsistency. All three new findings from the previous critique round were accepted (3/3), and both open questions were kept for the author (2/2).
The independent first-look review converges on the same issues. The test additions for compounding fee (AtoB and BtoA) are present but won't catch the reserve-side bug if the compounding fee amounts are small enough relative to pool size — still, the primary review's inline comments clearly flag the root cause.
No additional issues or false positives to report. The primary review is comprehensive and well-targeted.
Codex (response):
Agreement reached. No further changes.
Synthesis
Review synthesis
- No direct disagreements: both reviews agree the PR is close, but compounding support still has correctness risks.
- Review A emphasizes integration risk: migrated DAMM v2 pools may read stale/invalid reserve fields unless Zap delegates to DAMM’s own liquidity handler; it also flags the dependency duplication/regression from the
cp-ammrev bump. - Review B emphasizes execution-path correctness: swap simulation may apply compounding fees to the wrong token side on A→B trades, plus it calls out zero-reserve / zero-denominator safety and a couple of lower-priority perf/lifetime issues.
Most important follow-up action:
Fix and add tests for the compounding correctness path before merge—especially reserve sourcing for migrated pools and fee-token accounting in A→B swap simulation.
Repo checks
Repo Checks
-
LLM checks planner: added package install step before running JS commands.
-
rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu rustfmt: ok
info: downloading component 'rustfmt'
info: installing component 'rustfmt'
- rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu clippy: ok
info: downloading component 'clippy'
info: installing component 'clippy'
- cargo fetch --locked: ok
Updating git repository `https://github.com/MeteoraAg/damm-v2`
From https://github.com/MeteoraAg/damm-v2
* [new ref] 6e9aee4e549bd792c8f5b82b88be04459e644f3c -> refs/commit/6e9aee4e549bd792c8f5b82b88be04459e644f3c
- pnpm install --frozen-lockfile: ok
Lockfile is up to date, resolution step is skipped
Already up to date
╭ Warning ─────────────────────────────────────────────────────────────────────╮
│ │
│ Ignored build scripts: bigint-buffer@1.1.5, bufferutil@4.0.9, │
│ utf-8-validate@5.0.10. │
│ Run "pnpm approve-builds" to pick which dependencies should be allowed │
│ to run scripts. │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
Done in 741ms using pnpm v10.28.2
! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-10.28.2.tgz
- cargo fmt --check: ok
- cargo clippy --workspace --all-targets -- -D warnings: failed
Checking jupiter v0.1.0 (https://github.com/MeteoraAg/zap-program?rev=064c58b317b9a85f212c0de72caea286fc72fdb4#064c58b3)
Checking zap-sdk v0.1.0 (/data/workdir/MeteoraAg/zap-program/zap-sdk)
Checking zap-sdk v0.1.0 (https://github.com/MeteoraAg/zap-program?rev=064c58b317b9a85f212c0de72caea286fc72fdb4#064c58b3)
Checking dlmm v0.10.1 (/data/workdir/MeteoraAg/zap-program/libs/dlmm)
Checking jupiter v0.1.0 (/data/workdir/MeteoraAg/zap-program/libs/jupiter)
Checking protocol-zap v0.1.0 (https://github.com/MeteoraAg/zap-program?rev=064c58b317b9a85f212c0de72caea286fc72fdb4#064c58b3)
error: this function has too many arguments (8/7)
--> libs/jupiter/src/lib.rs:3:1
|
3 | declare_program!(jupiter);
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
= note: `-D clippy::too-many-arguments` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::too_many_arguments)]`
= note: this error originates in the macro `declare_program` (in Nightly builds, run with -Z macro-backtrace for more info)
error: this function has too many arguments (8/7)
--> libs/jupiter/src/lib.rs:3:1
|
3 | declare_program!(jupiter);
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
= note: this error originates in the macro `declare_program` (in Nightly builds, run with -Z macro-backtrace for more info)
Checking cp-amm v0.2.0 (https://github.com/MeteoraAg/damm-v2?rev=6e9aee4e549bd792c8f5b82b88be04459e644f3c#6e9aee4e)
error: could not compile `jupiter` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `jupiter` (lib test) due to 2 previous errors
|
/bot review |
|
Bot Agent progress:
Progress:
|
There was a problem hiding this comment.
Summary
Compounding DAMM v2 support still has three substantiated issues: reserve-side accounting for compounding fees is wrong for some swap directions/fee modes, compounding liquidity math can divide by zero when pool reserves are empty, and compounding-pool reserve fields are being used without first validating layout_version.
Inline comments
See inline comments on the diff.
Claude (API) review
Rounds: 2 (min=1, max=10).
First look: Codex + Claude (API).
Agreement: yes (no new findings/questions).
Debate transcript
First look comparison
Codex (first look):
Summary:
Compounding DAMM v2 support looks mostly coherent, but the new local liquidity-handler shim has one compatibility/correctness risk around versioned pool layouts.
Inline comment highlights:
- programs/zap/src/utils/liquidity_handler/mod.rs:28: This reads
pool.token_a_amount/pool.token_b_amountfor everyCollectFeeMode::Compoundingpool, but the updated DAMM v2 IDL in this PR says those fields are only meaningful whenlayout_version == 1and even adds `fix_pool_layout_vers
Claude (API) (first look):
Summary:
PR updates damm-v2 dependency to v0.2.0 and adds support for compounding fee mode pools. A LiquidityHandler trait abstracts concentrated vs compounding liquidity calculations. The core logic looks correct, but there are a couple of notable issues: a potential division-by-zero in compounding liquidity handlers when pool reserves are zero, and the compounding fee is always added to token_b in the swap simulation regardless of trade direction, which may be incorrect for BtoA swaps.
Inline comment highlights:
- programs/zap/src/utils/liquidity_handler/compounding_liquidity.rs:52: Bug: if
self.token_a_amountis 0 (empty pool or edge case), this will divide by zero viasafe_div/safe_mul_div_cast_u128. The same issue exists inget_liquidity_delta_from_amount_bat line 65 withself.token_b_amount. Consider c - programs/zap/src/utils/damm_v2_utils.rs:189: Potential correctness issue: For AtoB swaps, the compounding fee is added back to
pool_amount_b, which makes sense (fee retained in output token). But at line 204, for BtoA swaps the compounding fee is also added topool_amount_b. In Bt - programs/zap/src/utils/liquidity_handler/compounding_liquidity.rs:25: Potential overflow:
self.token_a_amount.safe_add(amount_in)?is computed asu64, which could overflow if both values are large. The upstream damm-v2 reference code may use wider types for this calculation. Consider whether casting to u1 - programs/zap/src/math/utils_math.rs:14: The rounding-up formula
(prod + denominator - 1) / denominatorcan overflow onsafe_addifprodis nearu128::MAXanddenominatoris large. This is unlikely in practice for u64 inputs promoted to u128, but note the same pattern at - programs/zap/src/instructions/ix_zap_in_damm_v2.rs:176: Nit:
drop(liquidity_handler)is called explicitly here beforedrop(pool). Sinceliquidity_handleronly contains copied scalar values (not references topool), thisdropcall is unnecessary. It does no harm but adds confusion about
Comparison notes:
Review synthesis
Differences in emphasis
- No major disagreement: both reviews think the compounding support is broadly sound.
- Review A’s main concern is compatibility/correctness with DAMM v2 pool layouts: the shim reads
token_a_amount/token_b_amountfor all compounding pools, despite the IDL saying those are only valid forlayout_version == 1. - Review B focuses more on runtime correctness/edge cases in the new compounding math: possible division-by-zero on empty reserves, likely wrong fee-side accounting for B→A swaps, plus a few lower-priority overflow/nit comments.
Most important follow-up action
- Audit and align the compounding handler with upstream DAMM v2 semantics, then add targeted tests for:
layout_versionhandling,- zero-reserve pools,
- both swap directions’ fee accounting.
Round 1
Codex (primary):
Summary:
Compounding DAMM v2 support looks mostly coherent, but the new local liquidity-handler shim has one compatibility/correctness risk around versioned pool layouts.
Inline comment highlights:
- programs/zap/src/utils/liquidity_handler/mod.rs:28: This reads
pool.token_a_amount/pool.token_b_amountfor everyCollectFeeMode::Compoundingpool, but the updated DAMM v2 IDL in this PR says those fields are only meaningful whenlayout_version == 1and even adds `fix_pool_layout_vers
Claude (API):
Critique of primary review
The primary review correctly identified the layout_version gap — this is the single most important compatibility issue in the PR. However, it missed two significant correctness bugs that the round-0 independent review caught:
-
Compounding fee direction bug (line 204 in
damm_v2_utils.rs): The compounding fee is unconditionally added topool_amount_bfor both AtoB and BtoA swaps. For BtoA swaps with fees-on-output, the fee is deducted from token A, so it should be reflected inpool_amount_a. This will cause the swap simulation to mis-estimate pool reserves after a BtoA swap, leading to incorrect zap-in amounts. -
Division-by-zero in compounding liquidity (lines 52, 65): When pool reserves are zero,
get_liquidity_delta_from_amount_a/bwill divide by zero. This is an edge case but is reachable for newly-created pools.
The primary review's layout_version finding is valid and should be kept. I've re-raised it in new_findings with a concrete fix suggestion since it wasn't fully actionable in the primary review.
Overall the primary review caught one of the three main issues but missed the two runtime correctness bugs.
Codex (response):
- New findings applied: 3/3
- Open questions kept for author: 1/1
Round 2
Codex (primary):
Summary:
Compounding DAMM v2 support still has three important issues to address: missing layout_version validation before using compounding-pool reserve fields, incorrect reserve-side accounting for compounding fees in some swap simulations, and a possible divide-by-zero when compounding-pool reserves are zero.
Inline comment highlights:
- programs/zap/src/utils/damm_v2_utils.rs:204:
compounding_feeis always being added back topool_amount_b, but forBtoAswaps with fees-on-output the fee is deducted from token A, so the post-swap reserve adjustment should go topool_amount_ainstead. More generally, the reserv - programs/zap/src/utils/liquidity_handler/compounding_liquidity.rs:52: This path can divide by zero when a compounding pool has an empty reserve:
safe_mul_div_cast_u128usesself.token_a_amounthere, and the analogous calculation later usesself.token_b_amount. For a newly created / empty pool, add a zer - programs/zap/src/utils/liquidity_handler/mod.rs:28:
pool.token_a_amount/pool.token_b_amountare only meaningful for compounding pools whenlayout_version == 1. Forlayout_version == 0, those bytes are old padding, so constructingCompoundingLiquidityfrom them can produce incorre
Claude (API):
Critique of primary review (round 2)
The primary review now incorporates all three findings from the prior critique:
- Compounding fee direction bug at line 204 in
damm_v2_utils.rs - Division-by-zero in
compounding_liquidity.rsat lines 52/65 - Missing
layout_versionguard inliquidity_handler/mod.rsat line 28
All three are valid and correctly described. The primary review's summary accurately captures the remaining risk areas. No additional high-signal issues were found beyond what has already been flagged. The test additions (lines 412–478 in zapin_dammv2.test.ts) cover both AtoB and BtoA compounding scenarios, which is good, but they won't catch the fee-side accounting bug since the simulation still "succeeds" — it just produces slightly wrong reserve estimates. This is a test coverage gap but not a new code finding.
The clippy failure is environmental (generated code from declare_program! in libs/jupiter) and unrelated to this PR's changes.
Codex (response):
Agreement reached. No further changes.
Synthesis
Review synthesis
Differences in emphasis
- No major disagreement: both reviews think the compounding support is broadly sound.
- Review A’s main concern is compatibility/correctness with DAMM v2 pool layouts: the shim reads
token_a_amount/token_b_amountfor all compounding pools, despite the IDL saying those are only valid forlayout_version == 1. - Review B focuses more on runtime correctness/edge cases in the new compounding math: possible division-by-zero on empty reserves, likely wrong fee-side accounting for B→A swaps, plus a few lower-priority overflow/nit comments.
Most important follow-up action
- Audit and align the compounding handler with upstream DAMM v2 semantics, then add targeted tests for:
layout_versionhandling,- zero-reserve pools,
- both swap directions’ fee accounting.
Repo checks
Repo Checks
-
LLM checks planner: added package install step before running JS commands.
-
rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu clippy: ok
info: component 'clippy' for target 'x86_64-unknown-linux-gnu' is up to date
- cargo fetch --locked: ok
Updating git repository `https://github.com/MeteoraAg/damm-v2`
From https://github.com/MeteoraAg/damm-v2
* [new ref] 58a13fcf45516a9f27f2bd2a2056fb66673454e0 -> refs/commit/58a13fcf45516a9f27f2bd2a2056fb66673454e0
- pnpm install --frozen-lockfile: ok
Lockfile is up to date, resolution step is skipped
Already up to date
╭ Warning ─────────────────────────────────────────────────────────────────────╮
│ │
│ Ignored build scripts: bigint-buffer@1.1.5, bufferutil@4.0.9, │
│ utf-8-validate@5.0.10. │
│ Run "pnpm approve-builds" to pick which dependencies should be allowed │
│ to run scripts. │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
Done in 810ms using pnpm v10.28.2
- cargo check --workspace: ok
Checking protocol-zap v0.1.0 (/data/workdir/MeteoraAg/zap-program/protocol-zap)
Checking cp-amm v0.2.0 (https://github.com/MeteoraAg/damm-v2?rev=58a13fcf45516a9f27f2bd2a2056fb66673454e0#58a13fcf)
Checking zap v0.2.2 (/data/workdir/MeteoraAg/zap-program/programs/zap)
warning: use of deprecated method `anchor_lang::prelude::AccountInfo::<'a>::realloc`: Use AccountInfo::resize() instead
--> programs/zap/src/lib.rs:21:1
|
21 | #[program]
| ^^^^^^^^^^
|
= note: `#[warn(deprecated)]` on by default
= note: this warning originates in the attribute macro `program` (in Nightly builds, run with -Z macro-backtrace for more info)
warning: `zap` (lib) generated 1 warning
Finished `dev` profile [unoptimized + debuginfo] target(s) in 6.50s
- cargo clippy --workspace --all-targets -- -D warnings: failed
Checking jupiter v0.1.0 (/data/workdir/MeteoraAg/zap-program/libs/jupiter)
Checking zap-sdk v0.1.0 (/data/workdir/MeteoraAg/zap-program/zap-sdk)
Checking dlmm v0.10.1 (/data/workdir/MeteoraAg/zap-program/libs/dlmm)
error: this function has too many arguments (8/7)
--> libs/jupiter/src/lib.rs:3:1
|
3 | declare_program!(jupiter);
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
= note: `-D clippy::too-many-arguments` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::too_many_arguments)]`
= note: this error originates in the macro `declare_program` (in Nightly builds, run with -Z macro-backtrace for more info)
error: this function has too many arguments (8/7)
--> libs/jupiter/src/lib.rs:3:1
|
3 | declare_program!(jupiter);
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
= note: this error originates in the macro `declare_program` (in Nightly builds, run with -Z macro-backtrace for more info)
error: could not compile `jupiter` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `jupiter` (lib test) due to 2 previous errors
|
/bot review |
|
Bot Agent progress:
Progress:
|
bangyro
changed the title
There was a problem hiding this comment.
Summary
Core zap-side changes look reasonable, but the updated TS DAMM v2 pool fixture helper can still construct invalid test pools in two ways: it appears to keep using concentrated-liquidity formulas even when the Rust side now treats compounding pools as CompoundingLiquidity, and its new single-sided initialization paths do not assert the required boundary prices, so future tests could silently create impossible fixtures.
Inline comments
See inline comments on the diff.
Claude (API) review
Rounds: 10 (min=1, max=10).
First look: Codex + Claude (API).
Agreement: no (see notes below).
Debate transcript
First look comparison
Codex (first look):
Summary:
Core zap-side changes look reasonable, but the updated TS DAMM v2 pool fixture helper can still build invalid test pools in a couple of cases, which makes the new coverage easier to misread than it should be.
Inline comment highlights:
- tests/common/damm_v2.ts:225: Can we branch this
liquidityDeltacalculation oncollectFeeMode? The Rust side of this PR now switches compounding pools over toCompoundingLiquidity, but this helper still always derives liquidity with `getLiquidityDeltaFromAmountA/B - tests/common/damm_v2.ts:239: The new single-sided branches assume
amountAimpliesinitSqrtPrice == sqrtMinPriceandamountBimpliesinitSqrtPrice == sqrtMaxPrice, but there’s no guard for that. If a future test passes onlyamountAwith a mid-range `initSqrtPr
Claude (API) (first look):
Summary:
PR adds DAMM v2 compounding fee mode support and fixes single-sided pool zap-in. The Rust changes introduce a LiquidityHandler trait abstraction for concentrated vs compounding liquidity, update the swap simulation to track compounding fees, and handle edge cases at price boundaries. Key issues: a division-by-zero risk in the compounding liquidity handler when reserves are zero, and the compounding_fee accounting in the BtoA swap direction appears incorrect.
Inline comment highlights:
- programs/zap/src/utils/liquidity_handler/compounding_liquidity.rs:52: Bug: Division by zero when
token_a_amountis 0. If a compounding pool has zero token A reserves (e.g. after all token A has been swapped out), callingget_liquidity_delta_from_amount_awill attempt to divide by zero. The concentrated li - programs/zap/src/utils/liquidity_handler/compounding_liquidity.rs:65: Bug: Division by zero when
token_b_amountis 0. Same issue asget_liquidity_delta_from_amount_a– if the pool has zero token B reserves, this will panic or error. Add a zero-guard returningu128::MAXsimilar to the concentrated liquid - programs/zap/src/utils/damm_v2_utils.rs:204: Potential correctness issue: In the BtoA branch,
compounding_feeis added topool_amount_b. However, for a BtoA swap, the input token is B and the output token is A. If fees are on input, the compounding fee is deducted from the B input - programs/zap/src/utils/liquidity_handler/mod.rs:23: Heap allocation via
Box<dyn LiquidityHandler>in an on-chain Solana program. While this works, it's slightly unusual for Solana programs to use dynamic dispatch with heap allocation. This is called in a loop (up to 20 iterations in `calcu - programs/zap/src/math/utils_math.rs:14: The rounding-up formula
(x + denominator - 1) / denominatorcan overflow ifprodis close tou128::MAX(orU256::MAXfor the u128 variant). Specifically,prod.safe_add(denominator)?could overflow. While unlikely with real-world v - programs/zap/src/instructions/ix_zap_in_damm_v2.rs:176: The explicit
drop(liquidity_handler)beforedrop(pool)suggests there's a borrow ordering concern. Sinceget_liquidity_handlertakes&Pooland returns an ownedBox<dyn LiquidityHandler>(no borrow held), the drop ordering shouldn'
Comparison notes:
Review synthesis
- No direct disagreements — the reviews are complementary, but they emphasize different layers.
- Review A focuses on test validity: the updated TS DAMM v2 fixture helper can create impossible pools unless it branches on
collectFeeModeand guards single-sided setups against invalidinitSqrtPricevalues. - Review B focuses on production Rust correctness: it flags higher-severity runtime risks in compounding mode, especially zero-reserve division-by-zero cases and likely incorrect
compounding_feehandling in the B→A path. - Review B also raises secondary robustness/perf concerns (heap allocation via
Box<dyn>, rounding overflow risk, unnecessarydrops) that A does not mention.
Most important follow-up action:
Fix and validate the Rust compounding-liquidity path first — add zero-reserve guards and verify/correct B→A compounding_fee accounting — then lock it down with targeted tests.
Round 1
Codex (primary):
Summary:
Core zap-side changes look reasonable, but the updated TS DAMM v2 pool fixture helper can still build invalid test pools in a couple of cases, which makes the new coverage easier to misread than it should be.
Inline comment highlights:
- tests/common/damm_v2.ts:225: Can we branch this
liquidityDeltacalculation oncollectFeeMode? The Rust side of this PR now switches compounding pools over toCompoundingLiquidity, but this helper still always derives liquidity with `getLiquidityDeltaFromAmountA/B - tests/common/damm_v2.ts:239: The new single-sided branches assume
amountAimpliesinitSqrtPrice == sqrtMinPriceandamountBimpliesinitSqrtPrice == sqrtMaxPrice, but there’s no guard for that. If a future test passes onlyamountAwith a mid-range `initSqrtPr
Claude (API):
(no additional notes)
Codex (response):
- Open questions kept for author: 1/1
Round 2
Codex (primary):
Summary:
Core zap-side changes look reasonable, but the updated TS DAMM v2 pool fixture helper can still build invalid test pools in a couple of cases, which makes the new coverage easier to misread than it should be.
Inline comment highlights:
- tests/common/damm_v2.ts:225: Can we branch this
liquidityDeltacalculation oncollectFeeMode? The Rust side of this PR now switches compounding pools over toCompoundingLiquidity, but this helper still always derives liquidity with `getLiquidityDeltaFromAmountA/B - tests/common/damm_v2.ts:239: The new single-sided branches assume
amountAimpliesinitSqrtPrice == sqrtMinPriceandamountBimpliesinitSqrtPrice == sqrtMaxPrice, but there’s no guard for that. If a future test passes onlyamountAwith a mid-range `initSqrtPr
Claude (API):
(no additional notes)
Codex (response):
- Open questions kept for author: 1/1
Round 3
Codex (primary):
Summary:
Core zap-side changes look reasonable, but the updated TS DAMM v2 pool fixture helper can still build invalid test pools in a couple of cases, which makes the new coverage easier to misread than it should be.
Inline comment highlights:
- tests/common/damm_v2.ts:225: Can we branch this
liquidityDeltacalculation oncollectFeeMode? The Rust side of this PR now switches compounding pools over toCompoundingLiquidity, but this helper still always derives liquidity with `getLiquidityDeltaFromAmountA/B - tests/common/damm_v2.ts:239: The new single-sided branches assume
amountAimpliesinitSqrtPrice == sqrtMinPriceandamountBimpliesinitSqrtPrice == sqrtMaxPrice, but there’s no guard for that. If a future test passes onlyamountAwith a mid-range `initSqrtPr
Claude (API):
(no additional notes)
Codex (response):
- Open questions kept for author: 1/1
Round 4
Codex (primary):
Summary:
Core zap-side changes look reasonable, but the updated TS DAMM v2 pool fixture helper can still build invalid test pools in a couple of cases, which makes the new coverage easier to misread than it should be.
Inline comment highlights:
- tests/common/damm_v2.ts:225: Can we branch this
liquidityDeltacalculation oncollectFeeMode? The Rust side of this PR now switches compounding pools over toCompoundingLiquidity, but this helper still always derives liquidity with `getLiquidityDeltaFromAmountA/B - tests/common/damm_v2.ts:239: The new single-sided branches assume
amountAimpliesinitSqrtPrice == sqrtMinPriceandamountBimpliesinitSqrtPrice == sqrtMaxPrice, but there’s no guard for that. If a future test passes onlyamountAwith a mid-range `initSqrtPr
Claude (API):
(no additional notes)
Codex (response):
- Open questions kept for author: 1/1
Round 5
Codex (primary):
Summary:
Core zap-side changes look reasonable, but the updated TS DAMM v2 pool fixture helper can still build invalid test pools in a couple of cases, which makes the new coverage easier to misread than it should be.
Inline comment highlights:
- tests/common/damm_v2.ts:225: Can we branch this
liquidityDeltacalculation oncollectFeeMode? The Rust side of this PR now switches compounding pools over toCompoundingLiquidity, but this helper still always derives liquidity with `getLiquidityDeltaFromAmountA/B - tests/common/damm_v2.ts:239: The new single-sided branches assume
amountAimpliesinitSqrtPrice == sqrtMinPriceandamountBimpliesinitSqrtPrice == sqrtMaxPrice, but there’s no guard for that. If a future test passes onlyamountAwith a mid-range `initSqrtPr
Claude (API):
(no additional notes)
Codex (response):
- Open questions kept for author: 1/1
Round 6
Codex (primary):
Summary:
Core zap-side changes look reasonable, but the updated TS DAMM v2 pool fixture helper can still build invalid test pools in a couple of cases, which makes the new coverage easier to misread than it should be.
Inline comment highlights:
- tests/common/damm_v2.ts:225: Can we branch this
liquidityDeltacalculation oncollectFeeMode? The Rust side of this PR now switches compounding pools over toCompoundingLiquidity, but this helper still always derives liquidity with `getLiquidityDeltaFromAmountA/B - tests/common/damm_v2.ts:239: The new single-sided branches assume
amountAimpliesinitSqrtPrice == sqrtMinPriceandamountBimpliesinitSqrtPrice == sqrtMaxPrice, but there’s no guard for that. If a future test passes onlyamountAwith a mid-range `initSqrtPr
Claude (API):
(no additional notes)
Codex (response):
- Open questions kept for author: 1/1
Round 7
Codex (primary):
Summary:
Core zap-side changes look reasonable, but the updated TS DAMM v2 pool fixture helper can still build invalid test pools in a couple of cases, which makes the new coverage easier to misread than it should be.
Inline comment highlights:
- tests/common/damm_v2.ts:225: Can we branch this
liquidityDeltacalculation oncollectFeeMode? The Rust side of this PR now switches compounding pools over toCompoundingLiquidity, but this helper still always derives liquidity with `getLiquidityDeltaFromAmountA/B - tests/common/damm_v2.ts:239: The new single-sided branches assume
amountAimpliesinitSqrtPrice == sqrtMinPriceandamountBimpliesinitSqrtPrice == sqrtMaxPrice, but there’s no guard for that. If a future test passes onlyamountAwith a mid-range `initSqrtPr
Claude (API):
(no additional notes)
Codex (response):
- Open questions kept for author: 1/1
Round 8
Codex (primary):
Summary:
Core zap-side changes look reasonable, but the updated TS DAMM v2 pool fixture helper can still build invalid test pools in a couple of cases, which makes the new coverage easier to misread than it should be.
Inline comment highlights:
- tests/common/damm_v2.ts:225: Can we branch this
liquidityDeltacalculation oncollectFeeMode? The Rust side of this PR now switches compounding pools over toCompoundingLiquidity, but this helper still always derives liquidity with `getLiquidityDeltaFromAmountA/B - tests/common/damm_v2.ts:239: The new single-sided branches assume
amountAimpliesinitSqrtPrice == sqrtMinPriceandamountBimpliesinitSqrtPrice == sqrtMaxPrice, but there’s no guard for that. If a future test passes onlyamountAwith a mid-range `initSqrtPr
Claude (API):
(no additional notes)
Codex (response):
- Open questions kept for author: 1/1
Round 9
Codex (primary):
Summary:
Core zap-side changes look reasonable, but the updated TS DAMM v2 pool fixture helper can still build invalid test pools in a couple of cases, which makes the new coverage easier to misread than it should be.
Inline comment highlights:
- tests/common/damm_v2.ts:225: Can we branch this
liquidityDeltacalculation oncollectFeeMode? The Rust side of this PR now switches compounding pools over toCompoundingLiquidity, but this helper still always derives liquidity with `getLiquidityDeltaFromAmountA/B - tests/common/damm_v2.ts:239: The new single-sided branches assume
amountAimpliesinitSqrtPrice == sqrtMinPriceandamountBimpliesinitSqrtPrice == sqrtMaxPrice, but there’s no guard for that. If a future test passes onlyamountAwith a mid-range `initSqrtPr
Claude (API):
(no additional notes)
Codex (response):
- Open questions kept for author: 1/1
Round 10
Codex (primary):
Summary:
Core zap-side changes look reasonable, but the updated TS DAMM v2 pool fixture helper can still build invalid test pools in a couple of cases, which makes the new coverage easier to misread than it should be.
Inline comment highlights:
- tests/common/damm_v2.ts:225: Can we branch this
liquidityDeltacalculation oncollectFeeMode? The Rust side of this PR now switches compounding pools over toCompoundingLiquidity, but this helper still always derives liquidity with `getLiquidityDeltaFromAmountA/B - tests/common/damm_v2.ts:239: The new single-sided branches assume
amountAimpliesinitSqrtPrice == sqrtMinPriceandamountBimpliesinitSqrtPrice == sqrtMaxPrice, but there’s no guard for that. If a future test passes onlyamountAwith a mid-range `initSqrtPr
Claude (API):
(no additional notes)
Codex (response):
- Open questions kept for author: 1/1
Synthesis
Review synthesis
- No direct disagreements — the reviews are complementary, but they emphasize different layers.
- Review A focuses on test validity: the updated TS DAMM v2 fixture helper can create impossible pools unless it branches on
collectFeeModeand guards single-sided setups against invalidinitSqrtPricevalues. - Review B focuses on production Rust correctness: it flags higher-severity runtime risks in compounding mode, especially zero-reserve division-by-zero cases and likely incorrect
compounding_feehandling in the B→A path. - Review B also raises secondary robustness/perf concerns (heap allocation via
Box<dyn>, rounding overflow risk, unnecessarydrops) that A does not mention.
Most important follow-up action:
Fix and validate the Rust compounding-liquidity path first — add zero-reserve guards and verify/correct B→A compounding_fee accounting — then lock it down with targeted tests.
Repo checks
Repo Checks
- rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu rustfmt: ok
info: downloading component 'rustfmt'
info: installing component 'rustfmt'
- rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu clippy: ok
info: downloading component 'clippy'
info: installing component 'clippy'
- cargo fetch --locked: ok
Updating git repository `https://github.com/MeteoraAg/damm-v2`
From https://github.com/MeteoraAg/damm-v2
* [new ref] 58a13fcf45516a9f27f2bd2a2056fb66673454e0 -> refs/commit/58a13fcf45516a9f27f2bd2a2056fb66673454e0
Downloading crates ...
Downloaded solana-sysvar v2.2.2
Downloaded ruint v1.16.0
Downloaded anyhow v1.0.98
Downloaded redox_syscall v0.5.14
Downloaded bytemuck_derive v1.10.0
Downloaded cc v1.2.30
Downloaded pest v2.8.3
Downloaded serde_json v1.0.141
Downloaded winnow v0.7.12
Downloaded indexmap v2.10.0
Downloaded solana-zk-sdk v2.3.5
Downloaded solana-curve25519 v2.3.5
- cargo check --workspace: ok
hor-attribute-program v0.31.1
Compiling anchor-attribute-event v0.31.1
Compiling anchor-attribute-account v0.31.1
Compiling anchor-attribute-constant v0.31.1
Compiling anchor-attribute-error v0.31.1
Compiling anchor-attribute-access-control v0.31.1
Checking spl-program-error v0.6.0
Compiling anchor-derive-accounts v0.31.1
Checking spl-pod v0.5.1
Compiling anchor-derive-serde v0.31.1
Checking spl-type-length-value v0.7.0
Checking spl-tlv-account-resolution v0.9.0
Checking spl-token-confidential-transfer-proof-extraction v0.2.1
Checking spl-token-metadata-interface v0.6.0
Checking spl-token-group-interface v0.5.0
Checking spl-elgamal-registry v0.1.1
Checking spl-token-confidential-transfer-proof-generation v0.2.0
Checking spl-transfer-hook-interface v0.9.0
Checking spl-token-confidential-transfer-ciphertext-arithmetic v0.2.1
Checking spl-token v7.0.0
Checking spl-token-2022 v6.0.0
Checking anchor-lang v0.31.1
Checking jupiter v0.1.0 (/data/workdir/MeteoraAg/zap-program/libs/jupiter)
Checking dlmm v0.10.1 (/data/workdir/MeteoraAg/zap-program/libs/dlmm)
Checking protocol-zap v0.1.0 (/data/workdir/MeteoraAg/zap-program/protocol-zap)
Checking spl-associated-token-account v6.0.0
Checking anchor-spl v0.31.1
Checking cp-amm v0.2.0 (https://github.com/MeteoraAg/damm-v2?rev=58a13fcf45516a9f27f2bd2a2056fb66673454e0#58a13fcf)
Checking zap v0.2.2 (/data/workdir/MeteoraAg/zap-program/programs/zap)
warning: use of deprecated method `anchor_lang::prelude::AccountInfo::<'a>::realloc`: Use AccountInfo::resize() instead
--> programs/zap/src/lib.rs:21:1
|
21 | #[program]
| ^^^^^^^^^^
|
= note: `#[warn(deprecated)]` on by default
= note: this warning originates in the attribute macro `program` (in Nightly builds, run with -Z macro-backtrace for more info)
warning: `zap` (lib) generated 1 warning
Finished `dev` profile [unoptimized + debuginfo] target(s) in 26.99s
- cargo fmt --check: ok
- cargo clippy --workspace --all-targets -- -D warnings: failed
Checking jupiter v0.1.0 (/data/workdir/MeteoraAg/zap-program/libs/jupiter)
Checking zap-sdk v0.1.0 (/data/workdir/MeteoraAg/zap-program/zap-sdk)
Checking dlmm v0.10.1 (/data/workdir/MeteoraAg/zap-program/libs/dlmm)
error: this function has too many arguments (8/7)
--> libs/jupiter/src/lib.rs:3:1
|
3 | declare_program!(jupiter);
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
= note: `-D clippy::too-many-arguments` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::too_many_arguments)]`
= note: this error originates in the macro `declare_program` (in Nightly builds, run with -Z macro-backtrace for more info)
error: this function has too many arguments (8/7)
--> libs/jupiter/src/lib.rs:3:1
|
3 | declare_program!(jupiter);
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
= note: this error originates in the macro `declare_program` (in Nightly builds, run with -Z macro-backtrace for more info)
error: could not compile `jupiter` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `jupiter` (lib test) due to 2 previous errors
| @@ -205,17 +225,31 @@ export async function createDammV2Pool( | |||
| if (amountA && amountB) { | |||
There was a problem hiding this comment.
Can we branch this liquidityDelta calculation on collectFeeMode? The Rust side of this PR now switches compounding pools over to CompoundingLiquidity, but this helper still always derives liquidity with getLiquidityDeltaFromAmountA/B (the concentrated-liquidity formulas). That means compounding fixtures with a custom price/range or non-symmetric seed amounts can be initialized with the wrong liquidity and make the new tests give false confidence.
|
/bot review |
|
Bot Agent progress:
Progress:
|
There was a problem hiding this comment.
Summary
The DAMM v2 compounding/single-sided support still has three well-supported correctness issues: the compounding handler is constructed from pool.token_a_amount/pool.token_b_amount even though those fields are only valid for layout v1 pools, the compounding liquidity delta helpers can divide by zero for single-sided or empty pools, and the swap-amount binary search reuses a compounding reserve snapshot instead of updating reserves across simulated swaps.
Inline comments
See inline comments on the diff.
Claude (API) review
Rounds: 3 (min=1, max=10).
First look: Codex + Claude (Claude first-look failed; fell back to Codex).
Agreement: yes (no new findings/questions).
Debate transcript
First look comparison
Codex (first look):
Summary:
The DAMM v2 compounding/single-sided support is mostly coherent, but the new compounding handler currently assumes layout-v1 tracked reserves and can miscompute or fail on unmigrated pools.
Inline comment highlights:
- programs/zap/src/utils/liquidity_handler/mod.rs:28:
CompoundingLiquidityis being initialized frompool.token_a_amount/pool.token_b_amountunconditionally, but the updated DAMM v2 layout explicitly documents those fields as only valid oncelayout_version == 1(older pools need `fix
Codex (first look fallback):
Summary:
Mostly solid DAMM v2 support update; the main risk I found is around using compounding reserve fields without considering the new pool layout version.
Inline comment highlights:
- programs/zap/src/utils/liquidity_handler/mod.rs:27: This branch assumes
pool.token_a_amount/pool.token_b_amountare always valid for compounding pools, but the upgraded DAMM v2 layout now explicitly introduceslayout_versionbecause v0 pools did not track those fields. If zap eve
Comparison notes:
Review synthesis
Differences in emphasis
- No substantive disagreement: both reviews identify the same core bug in
liquidity_handleraround compounding pools. - Codex emphasizes failure modes: the handler may miscompute reserves or fail on unmigrated/legacy pools.
- Buddy emphasizes compatibility/layout semantics:
layout_versionexists because v0 pools never trackedtoken_a_amount/token_b_amount, so blindly reading them is unsafe.
Most important follow-up
- Fix
CompoundingLiquidityinitialization to respectpool.layout_version.
Only usetoken_a_amount/token_b_amountforlayout_version == 1; for legacy/v0 pools, either derive reserves via the proper fallback/migration path or reject clearly. Add tests covering both v0 and v1 compounding pools.
Round 1
Codex (primary):
Summary:
The DAMM v2 compounding/single-sided support is mostly coherent, but the new compounding handler currently assumes layout-v1 tracked reserves and can miscompute or fail on unmigrated pools.
Inline comment highlights:
- programs/zap/src/utils/liquidity_handler/mod.rs:28:
CompoundingLiquidityis being initialized frompool.token_a_amount/pool.token_b_amountunconditionally, but the updated DAMM v2 layout explicitly documents those fields as only valid oncelayout_version == 1(older pools need `fix
Claude (API):
(no additional notes)
Codex (response):
- Open questions kept for author: 1/1
Round 2
Codex (primary):
Summary:
The DAMM v2 compounding/single-sided support is mostly coherent, but the new compounding handler currently assumes layout-v1 tracked reserves and can miscompute or fail on unmigrated pools.
Inline comment highlights:
- programs/zap/src/utils/liquidity_handler/mod.rs:28:
CompoundingLiquidityis being initialized frompool.token_a_amount/pool.token_b_amountunconditionally, but the updated DAMM v2 layout explicitly documents those fields as only valid oncelayout_version == 1(older pools need `fix
Claude (API):
Critique
The primary review correctly identifies the layout_version concern in get_liquidity_handler at mod.rs:28. That finding is valid and should stay.
However, two additional issues were missed:
-
Division-by-zero in compounding single-sided pools.
CompoundingLiquidity::get_liquidity_delta_from_amount_adivides byself.token_a_amount. If a compounding pool is single-sided (one reserve is 0), this will fail. The concentrated path already handles this by returningu128::MAX. The compounding path needs the same guard. -
Stale reserves in the binary-search swap simulation.
calculate_swap_amountcaptures the compounding handler (with its reserve snapshot) once before the loop, but each simulated swap changes the effective reserves. For a constant-product pool, the simulation will be inaccurate becausex*y=kshifts after each virtual swap. The concentrated handler doesn't have this issue because it trackssqrt_price/liquiditywhich the simulation doesn't feed back. For compounding mode, the reserves must be updated between iterations.
Codex (response):
- New findings applied: 2/2
- Open questions kept for author: 1/1
Round 3
Codex (primary):
Summary:
The DAMM v2 compounding/single-sided support still has three correctness gaps: handler construction assumes layout-v1 tracked reserves, compounding liquidity math can divide by zero on single-sided or empty pools, and the swap-amount binary search reuses stale compounding reserves across simulated swaps.
Inline comment highlights:
- programs/zap/src/utils/liquidity_handler/mod.rs:28:
CompoundingLiquidityis being initialized frompool.token_a_amount/pool.token_b_amountunconditionally, but the updated DAMM v2 layout explicitly documents those fields as only valid oncelayout_version == 1(older pools need `fix - programs/zap/src/utils/liquidity_handler/compounding_liquidity.rs:48: When
token_a_amountis 0 (single-sided B compounding pool), this divides by zero. The concentrated handler returnsu128::MAXas a sentinel for the no-token side. Add the same guard here, and symmetrically in `get_liquidity_delta_from_am - programs/zap/src/utils/damm_v2_utils.rs:84: The compounding
handlercaptures a reserve snapshot once before the binary-search loop, but each simulated swap changes the effective reserves. For a constant-product pool, subsequent iterations should see the post-swap reserves; otherwis
Claude (API):
Critique
The primary review now covers all three significant issues:
layout_versioncheck inget_liquidity_handleratmod.rs:28— valid and important.- Division-by-zero guard in
compounding_liquidity.rs:48— valid, matches theu128::MAXsentinel pattern from the concentrated handler. - Stale reserves in binary-search loop at
damm_v2_utils.rs:84— valid, the compounding handler's constant-product invariant shifts after each simulated swap.
All three findings from prior rounds were accepted by the primary reviewer (2/2 new findings applied, 1/1 question kept). The test coverage added in this PR covers single-sided concentrated pools and compounding fee pools, but does not exercise the edge cases flagged (zero-reserve compounding, unmigrated v0 layout). Those gaps are adequately captured by the existing review comments.
No additional issues or false positives identified. The primary review is comprehensive for the changes in this PR.
Codex (response):
Agreement reached. No further changes.
Synthesis
Review synthesis
Differences in emphasis
- No substantive disagreement: both reviews identify the same core bug in
liquidity_handleraround compounding pools. - Codex emphasizes failure modes: the handler may miscompute reserves or fail on unmigrated/legacy pools.
- Buddy emphasizes compatibility/layout semantics:
layout_versionexists because v0 pools never trackedtoken_a_amount/token_b_amount, so blindly reading them is unsafe.
Most important follow-up
- Fix
CompoundingLiquidityinitialization to respectpool.layout_version.
Only usetoken_a_amount/token_b_amountforlayout_version == 1; for legacy/v0 pools, either derive reserves via the proper fallback/migration path or reject clearly. Add tests covering both v0 and v1 compounding pools.
Repo checks
Repo Checks
-
LLM checks planner: added package install step before running JS commands.
-
rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu clippy: ok
info: component 'clippy' for target 'x86_64-unknown-linux-gnu' is up to date
- cargo fetch --locked: ok
- pnpm install --frozen-lockfile: ok
Lockfile is up to date, resolution step is skipped
Progress: resolved 1, reused 0, downloaded 0, added 0
Packages: +198
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Progress: resolved 198, reused 93, downloaded 0, added 0
Progress: resolved 198, reused 196, downloaded 2, added 198, done
dependencies:
+ @coral-xyz/anchor 0.31.1
+ @solana/spl-token 0.4.13
+ @solana/web3.js 1.98.2
+ babar 0.2.3
+ borsh 2.0.0
+ litesvm 0.1.0
devDependencies:
+ @meteora-ag/cp-amm-sdk 1.3.7
+ @types/bn.js 5.2.0
+ @types/chai 4.3.20
+ @types/mocha 9.1.1
+ chai 4.5.0
+ mocha 9.2.2
+ prettier 2.8.8
+ ts-mocha 10.1.0
+ typescript 5.8.3
╭ Warning ─────────────────────────────────────────────────────────────────────╮
│ │
│ Ignored build scripts: bigint-buffer@1.1.5, bufferutil@4.0.9, │
│ utf-8-validate@5.0.10. │
│ Run "pnpm approve-builds" to pick which dependencies should be allowed │
│ to run scripts. │
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
Done in 2.2s using pnpm v10.28.2
! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-10.28.2.tgz
- cargo check --workspace: ok
warning: use of deprecated method `anchor_lang::prelude::AccountInfo::<'a>::realloc`: Use AccountInfo::resize() instead
--> programs/zap/src/lib.rs:21:1
|
21 | #[program]
| ^^^^^^^^^^
|
= note: `#[warn(deprecated)]` on by default
= note: this warning originates in the attribute macro `program` (in Nightly builds, run with -Z macro-backtrace for more info)
warning: `zap` (lib) generated 1 warning
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.21s
- cargo clippy --workspace --all-targets -- -D warnings: failed
Checking jupiter v0.1.0 (/data/workdir/MeteoraAg/zap-program/libs/jupiter)
error: this function has too many arguments (8/7)
--> libs/jupiter/src/lib.rs:3:1
|
3 | declare_program!(jupiter);
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
= note: `-D clippy::too-many-arguments` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::too_many_arguments)]`
= note: this error originates in the macro `declare_program` (in Nightly builds, run with -Z macro-backtrace for more info)
error: this function has too many arguments (8/7)
--> libs/jupiter/src/lib.rs:3:1
|
3 | declare_program!(jupiter);
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_many_arguments
= note: this error originates in the macro `declare_program` (in Nightly builds, run with -Z macro-backtrace for more info)
error: could not compile `jupiter` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `jupiter` (lib test) due to 2 previous errors
2 participants
Uh oh!
There was an error while loading. Please reload this page.