feat: add integration test for zap-protocol validation (do not merge)#197
feat: add integration test for zap-protocol validation (do not merge)#197
Conversation
bangyro
commented
Mar 25, 2026
bangyro
commented
- should reject when first swap input token account does not match claiming account
- should reject when first swap input token account does not match claiming account
|
/bot review |
|
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
The test has a real correctness issue: it reads state from the outer-scope pool and passes pool into zapProtocolFee, so the negative case is not actually exercising the freshly created dammV2PoolAddress. Even after correcting that, the test should ensure the selected pool has accrued non-zero protocol fees so the claim path is not trivially zero. Separately, the test is also brittle because it mutates zapOutIx.keys[14] based on a hard-coded Jupiter/protocol-zap account index, which can silently break if account ordering changes.
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:
The dependency pin update and negative integration test mostly match the PR intent. The main issue I found is a maintainability risk in the new test: it hard-codes a Jupiter/protocol-zap account index without verifying it still points to the first swap source account.
Inline comment highlights:
- tests/zapProtocolFees.test.ts:462: This assertion path is brittle because it assumes
zapOutIx.keys[14]will always be the first DAMM v2 swap source account. That index is derived from Jupiter/protocol-zap account ordering, so a future rev bump can silently make this mutate
Claude (API) (first look):
Summary:
Integration test for zap-protocol validation. The test has a bug where it reads pool state from the wrong pool variable, and the dependency bump introduces clippy regressions (pre-existing). The test logic for account substitution is otherwise sound.
Inline comment highlights:
- tests/zapProtocolFees.test.ts:432: Bug:
getPool(svm, pool)reads the state of the outer-scopepool, not the freshly createddammV2PoolAddress. This meansclaimAmount,tokenVault,tokenMint, andreceiverTokenall refer to a different pool than the one used to bu - tests/zapProtocolFees.test.ts:435: If
pool(the outer-scope pool) hasn't accumulated any protocol fees by the time this test runs,claimAmount(totalProtocolAFee) could be zero, which may make the claim trivially succeed or fail for an unrelated reason, masking the int - tests/zapProtocolFees.test.ts:462: The hard-coded index
14is fragile and relies on a specific account layout of the Jupiter route + DAMM v2 swap instruction. A comment explains the derivation, which is good, but ifbuildZapOutJupV6UsingDammV2RouteInstructionever change
Comparison notes:
Review synthesis
Top differences in emphasis
- Correctness vs. maintainability: Buddy flags a likely test bug (
getPool(svm, pool)uses the wrong pool) and a possible zero-fee false positive; Codex does not mention these. - Shared issue, different framing: Both call out the hard-coded
zapOutIx.keys[14], but Codex emphasizes future silent breakage/maintainability, while Buddy says the substitution logic is otherwise sound and mainly fragile. - Dependency update impact: Buddy notes clippy regressions from the dependency bump (pre-existing); Codex says the pin update mostly matches intent and focuses only on the test.
Most important follow-up
- Fix the new integration test’s pool-state bug first: use
dammV2PoolAddressconsistently when reading pool state, so the test validates the intended pool before addressing the brittle hard-coded account index.
Round 1
Codex (primary):
Summary:
The dependency pin update and negative integration test mostly match the PR intent. The main issue I found is a maintainability risk in the new test: it hard-codes a Jupiter/protocol-zap account index without verifying it still points to the first swap source account.
Inline comment highlights:
- tests/zapProtocolFees.test.ts:462: This assertion path is brittle because it assumes
zapOutIx.keys[14]will always be the first DAMM v2 swap source account. That index is derived from Jupiter/protocol-zap account ordering, so a future rev bump can silently make this mutate
Claude (API):
The primary review correctly identified the hard-coded zapOutIx.keys[14] brittleness, which is a real maintainability concern. However, it missed the higher-severity correctness bug: the test uses the wrong pool variable in two places (line 432 and line 470), meaning the test does not actually validate what it claims to. This is the most important finding in this diff — the test will either spuriously pass or fail for unrelated reasons, providing zero signal about the zap-protocol-fee validation logic.
The zero-fee risk (line 435) is a secondary consequence: the freshly created dammV2PoolAddress has had no swaps, so totalProtocolAFee on it would be zero even if the pool reference were corrected. Both bugs together mean this test is essentially dead code.
I agree with the primary review's hard-coded-index finding but would elevate the pool-variable bug as the blocking issue.
Codex (response):
- New findings applied: 2/2
- Open questions kept for author: 1/1
Round 2
Codex (primary):
Summary:
The hard-coded Jupiter/protocol-zap account index is still a maintainability risk, but the more serious issue in this test is correctness: it reads state from the outer-scope pool and also passes pool into zapProtocolFee, so the negative case is not actually exercising the freshly created dammV2PoolAddress. After fixing that, the test should also ensure the selected pool has accrued non-zero protocol fees so the claim path cannot pass or fail trivially with a zero amount.
Inline comment highlights:
- tests/zapProtocolFees.test.ts:432: Bug: this test is pulling pool state from the outer-scope
poolinstead of the freshly createddammV2PoolAddress. That meansclaimAmount,tokenVault,tokenMint, andreceiverTokenall come from a different pool, and `zapProtocolFe - tests/zapProtocolFees.test.ts:435: Even after fixing the pool reference, this test can still be non-deterministic if the target pool has not accrued any protocol fees yet:
claimAmountmay be zero, and claiming zero can succeed or fail for reasons unrelated to the mismatche - tests/zapProtocolFees.test.ts:462: This assertion path is brittle because it assumes
zapOutIx.keys[14]will always be the first DAMM v2 swap source account. That index is derived from Jupiter/protocol-zap account ordering, so a future rev bump can silently make this mutate
Claude (API):
The primary review now covers all three key issues: the wrong-pool-variable bug (line 432 and 470), the zero-fee risk (line 435), and the brittle hard-coded index (line 462). The prior critique's findings were accepted (2/2) and the open question was kept for the author. I have no additional high-signal findings to add and no false positives to retract.
Codex (response):
Agreement reached. No further changes.
Synthesis
Review synthesis
Top differences in emphasis
- Correctness vs. maintainability: Buddy flags a likely test bug (
getPool(svm, pool)uses the wrong pool) and a possible zero-fee false positive; Codex does not mention these. - Shared issue, different framing: Both call out the hard-coded
zapOutIx.keys[14], but Codex emphasizes future silent breakage/maintainability, while Buddy says the substitution logic is otherwise sound and mainly fragile. - Dependency update impact: Buddy notes clippy regressions from the dependency bump (pre-existing); Codex says the pin update mostly matches intent and focuses only on the test.
Most important follow-up
- Fix the new integration test’s pool-state bug first: use
dammV2PoolAddressconsistently when reading pool state, so the test validates the intended pool before addressing the brittle hard-coded account index.
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
- rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu clippy: ok
info: component clippy is up to date
- cargo fetch --locked: ok
Updating git repository `https://github.com/MeteoraAg/zap-program`
From https://github.com/MeteoraAg/zap-program
* [new ref] e44c59b5b6cf0324bcc4d3bc182811a8762beeb3 -> refs/commit/e44c59b5b6cf0324bcc4d3bc182811a8762beeb3
- pnpm install --frozen-lockfile: ok
Lockfile is up to date, resolution step is skipped
Progress: resolved 1, reused 0, downloaded 0, added 0
Packages: +192
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Progress: resolved 192, reused 192, downloaded 0, added 192, done
dependencies:
+ @coral-xyz/anchor 0.31.1
+ @coral-xyz/borsh 0.31.1
+ @solana/spl-token 0.4.14
+ @solana/web3.js 1.98.4
+ borsh 2.0.0
+ decimal.js 10.6.0
+ litesvm 0.1.0
devDependencies:
+ @solana/spl-token-metadata 0.1.6
+ @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.9.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 1s using pnpm v10.33.0
! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-10.33.0.tgz
- cargo fmt --check: ok
- cargo clippy --workspace --all-targets -- -D warnings: failed
ee_mode.fees_on_input);
|
error: used `assert_eq!` with a literal bool
--> programs/cp-amm/src/tests/test_collect_fee_mode.rs:60:5
|
60 | assert_eq!(fee_mode.fees_on_token_a, false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
help: replace it with `assert!(..)`
|
60 - assert_eq!(fee_mode.fees_on_token_a, false);
60 + assert!(!fee_mode.fees_on_token_a);
|
error: importing legacy numeric constants
--> programs/cp-amm/src/tests/test_liquidity_compounding.rs:18:5
|
18 | use std::u64;
| ^^^^^^^^
|
= help: remove this import
= note: then `u64::<CONST>` will resolve to the respective associated constant
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#legacy_numeric_constants
error: manual `RangeInclusive::contains` implementation
--> programs/cp-amm/src/tests/test_liquidity_compounding.rs:34:9
|
34 | sqrt_price >= MIN_SQRT_PRICE && sqrt_price <= MAX_SQRT_PRICE,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `(MIN_SQRT_PRICE..=MAX_SQRT_PRICE).contains(&sqrt_price)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
error: could not compile `cp-amm` (lib) due to 117 previous errors
warning: build failed, waiting for other jobs to finish...
error: casting to the same type is unnecessary (`u16` -> `u16`)
--> programs/cp-amm/src/params/fee_parameters.rs:167:38
|
167 | self.reduction_factor <= MAX_BASIS_POINT as u16,
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `MAX_BASIS_POINT`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
error: could not compile `cp-amm` (lib test) due to 182 previous errors
| const poolState = getPool(svm, pool); | ||
| const operatorAddress = whitelistedAccount.publicKey; | ||
|
|
||
| const claimAmount = poolState.metrics.totalProtocolAFee; |
There was a problem hiding this comment.
Even after fixing the pool reference, this test can still be non-deterministic if the target pool has not accrued any protocol fees yet: claimAmount may be zero, and claiming zero can succeed or fail for reasons unrelated to the mismatched-account validation. Consider generating fees on dammV2PoolAddress before the assertion, or at minimum assert claimAmount.gtn(0) so the test cannot silently pass with a zero claim.
| // base offset is 12 (2 (ix_zap_out) + 9 (jup route accounts) + 1 (jup route swap_program)). | ||
| // the damm v2 input_token_account is the 3rd account (index 2). so (12 + 3 - 1 = 14) | ||
| // https://github.com/jup-ag/jupiter-aggregator-program/blob/e583ab6619f4646b4d7a0e2514aec62ae9fb62ec/dex_interfaces/src/lib.rs#L6150 | ||
| zapOutIx.keys[14] = { |
There was a problem hiding this comment.
This assertion path is brittle because it assumes zapOutIx.keys[14] will always be the first DAMM v2 swap source account. That index is derived from Jupiter/protocol-zap account ordering, so a future rev bump can silently make this mutate the wrong key and still produce a misleading pass/fail. Please add a guard before overwriting (for example, assert that keys[14].pubkey is the expected receiverToken) or derive the position from the generated route accounts so layout changes fail loudly.
|
/bot review |
|
Bot Agent progress:
Progress:
|
|
Bot Agent progress:
Progress:
|
|
Bot Agent progress:
Progress:
|
There was a problem hiding this comment.
Verdict
Request changes
Summary
There's a merge-blocking correctness issue in the new negative test: it reads pool state from and submits zapProtocolFee against the outer-scope pool instead of the freshly created dammV2PoolAddress, so the assertion can pass for the wrong reason. I also still see a non-blocking brittleness issue from hardcoding the Jupiter account index (keys[14]), which makes the test fragile to upstream route/account-order changes.
Findings
- [P0]
tests/zapProtocolFees.test.ts:432Bug:poolhere refers to the outer-scope pool, notdammV2PoolAddresscreated above. BothgetPool(svm, pool)and the laterzapProtocolFee({ pool, ... })call should usedammV2PoolAddress. As written, the test mixes pool state and instruction inputs from different pools, so the negative assertion may pass vacuously for a pool mismatch instead of the intended invalid source-account case. - [P3]
tests/zapProtocolFees.test.ts:462This test is tightly coupled to the current Jupiter/DAMM v2 account ordering by hardcodingkeys[14]. Since this PR already bumps the upstream zap/Jupiter dependency, a future route-shape change can make the test mutate the wrong account and either fail for the wrong reason or stop exercisingInvalidZapAccountsat all. If possible, havebuildZapOutJupV6UsingDammV2RouteInstructionexpose the source-account position (or locate it by pubkey/role) instead of relying on a magic absolute index.
Claude (API) review
Rounds: 2 (min=1, max=10).
First look: Codex + Claude (API).
Agreement: yes (no new findings/questions).
Synthesis
Review synthesis
Top differences in emphasis
- Severity: Codex sees the PR as largely correct with one P3 brittleness issue; Buddy treats it as request changes because of a P0 correctness bug.
- Primary risk: Codex focuses on the fragile hardcoded Jupiter account index (
keys[14]); Buddy agrees on that, but says the bigger problem is the test may be reading the wrong pool state entirely. - Additional scope issue: Buddy also flags
tokenBMintcoming from an outer scope as another source of false-positive behavior; Codex does not mention this.
Most important follow-up action
- Fix the test to use the newly created pool/address consistently (
dammV2PoolAddress/ correct mint variables) before addressing the hardcoded account index. If the test is reading stale state, its assertions may be invalid regardless of the account-index brittleness.
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: downloading component clippy
- cargo fetch --locked: ok
Updating git repository `https://github.com/MeteoraAg/zap-program`
From https://github.com/MeteoraAg/zap-program
* [new ref] e44c59b5b6cf0324bcc4d3bc182811a8762beeb3 -> refs/commit/e44c59b5b6cf0324bcc4d3bc182811a8762beeb3
- pnpm install --frozen-lockfile: ok
Lockfile is up to date, resolution step is skipped
Progress: resolved 1, reused 0, downloaded 0, added 0
Packages: +192
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Progress: resolved 192, reused 91, downloaded 0, added 0
Progress: resolved 192, reused 192, downloaded 0, added 192, done
dependencies:
+ @coral-xyz/anchor 0.31.1
+ @coral-xyz/borsh 0.31.1
+ @solana/spl-token 0.4.14
+ @solana/web3.js 1.98.4
+ borsh 2.0.0
+ decimal.js 10.6.0
+ litesvm 0.1.0
devDependencies:
+ @solana/spl-token-metadata 0.1.6
+ @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.9.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.5s using pnpm v10.33.0
! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-10.33.0.tgz
- cargo check --workspace: ok
Checking solana-loader-v3-interface v3.0.0
Checking solana-loader-v4-interface v2.2.1
Checking solana-program-pack v2.2.1
Checking solana-loader-v2-interface v2.2.1
Checking solana-stable-layout v2.2.1
Checking spl-discriminator v0.4.1
Checking spl-memo v6.0.0
Checking spl-associated-token-account-client v2.0.0
Checking solana-sysvar v2.2.1
Compiling anchor-attribute-error v0.31.1
Compiling anchor-attribute-access-control v0.31.1
Compiling anchor-attribute-event v0.31.1
Compiling anchor-attribute-program v0.31.1
Compiling anchor-derive-accounts v0.31.1
Compiling anchor-attribute-constant v0.31.1
Checking solana-program v2.2.1
Checking spl-program-error v0.6.0
Compiling anchor-derive-serde v0.31.1
Compiling anchor-attribute-account v0.31.1
Checking spl-token v7.0.0
Checking spl-pod v0.5.1
Checking spl-token-confidential-transfer-ciphertext-arithmetic v0.2.1
Checking spl-token-confidential-transfer-proof-generation v0.2.0
Checking spl-type-length-value v0.7.0
Checking spl-token-confidential-transfer-proof-extraction v0.2.1
Checking spl-token-group-interface v0.5.0
Checking spl-tlv-account-resolution v0.9.0
Checking spl-token-metadata-interface v0.6.0
Checking spl-elgamal-registry v0.1.1
Checking spl-transfer-hook-interface v0.9.0
Checking spl-token-2022 v6.0.0
Checking anchor-lang v0.31.1
Checking jupiter v0.1.0 (https://github.com/MeteoraAg/zap-program?rev=e44c59b5b6cf0324bcc4d3bc182811a8762beeb3#e44c59b5)
Checking spl-associated-token-account v6.0.0
Checking protocol-zap v0.1.0 (https://github.com/MeteoraAg/zap-program?rev=e44c59b5b6cf0324bcc4d3bc182811a8762beeb3#e44c59b5)
Checking anchor-spl v0.31.1
Checking cp-amm v0.2.0 (/data/workdir/2962945/MeteoraAg/damm-v2/programs/cp-amm)
Checking rust-sdk v0.2.0 (/data/workdir/2962945/MeteoraAg/damm-v2/rust-sdk)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 50.94s
- cargo clippy --workspace --all-targets -- -D warnings: failed
ee_mode.fees_on_input);
|
error: used `assert_eq!` with a literal bool
--> programs/cp-amm/src/tests/test_collect_fee_mode.rs:60:5
|
60 | assert_eq!(fee_mode.fees_on_token_a, false);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_assert_comparison
help: replace it with `assert!(..)`
|
60 - assert_eq!(fee_mode.fees_on_token_a, false);
60 + assert!(!fee_mode.fees_on_token_a);
|
error: importing legacy numeric constants
--> programs/cp-amm/src/tests/test_liquidity_compounding.rs:18:5
|
18 | use std::u64;
| ^^^^^^^^
|
= help: remove this import
= note: then `u64::<CONST>` will resolve to the respective associated constant
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#legacy_numeric_constants
error: manual `RangeInclusive::contains` implementation
--> programs/cp-amm/src/tests/test_liquidity_compounding.rs:34:9
|
34 | sqrt_price >= MIN_SQRT_PRICE && sqrt_price <= MAX_SQRT_PRICE,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `(MIN_SQRT_PRICE..=MAX_SQRT_PRICE).contains(&sqrt_price)`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
error: casting to the same type is unnecessary (`u16` -> `u16`)
--> programs/cp-amm/src/params/fee_parameters.rs:167:38
|
167 | self.reduction_factor <= MAX_BASIS_POINT as u16,
| ^^^^^^^^^^^^^^^^^^^^^^ help: try: `MAX_BASIS_POINT`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
error: could not compile `cp-amm` (lib) due to 117 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `cp-amm` (lib test) due to 182 previous errors
1 participant
