Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces Lunarswap, a new decentralized exchange protocol with liquidity routing, pair factory, reserve management, and fee handling. It refactors math libraries (upgrading to compiler v0.21.0, adding Field255 and Pack modules, removing archived predecessors), renames token APIs (tokenType → color), expands test infrastructure with simulators and witnesses, and includes a comprehensive SDK. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router as LunarswapRouter
participant Factory as LunarswapFactory
participant Pair as LunarswapPair
participant Liquidity as LunarswapLiquidity
participant Reserves as Factory.reserves
User->>Router: addLiquidity(tokenA, tokenB, amountA, amountB, minA, minB, to)
Router->>Factory: getPairId(tokenA, tokenB)
Factory-->>Router: pairId
Router->>Factory: isPairExists(pairId)
Factory-->>Router: exists (true/false)
alt Pair doesn't exist
Router->>Pair: initializePair(tokenA, tokenB)
Pair->>Liquidity: initialize(...)
Liquidity->>Liquidity: initializePairLpToken(pairId)
else Pair exists
Router->>Factory: getPair(pairId)
Factory-->>Router: pair
end
Router->>Router: _addLiquidity(compute optimal amounts)
Router->>Factory: getReserves(pairId, reserve0Id, reserve1Id)
Factory-->>Router: [reserve0, reserve1]
Router->>Pair: mint(pairId, pair, amount0, amount1, reserves..., to)
Pair->>Liquidity: mint(lpTokens)
Liquidity-->>Pair: lpToken
Pair-->>Router: [updatedPair, liquidityMinted]
Router->>Factory: updatePair(pairId, pair)
Router->>Factory: swapReserves(update reserves)
Router-->>User: lpTokens
sequenceDiagram
participant Compiler as Compact Compiler v0.21.0
participant Bytes32 as Bytes32.compact
participant Pack as Pack<N>.compact
participant Field255 as Field255.compact
participant Uint256 as Uint256.compact
Compiler->>Bytes32: compile with language_version 0.21.0
Bytes32->>Pack: import pack, unpack
Pack-->>Bytes32: Pack32_pack, Pack32_unpack
Bytes32->>Uint256: use toU256 conversions
Compiler->>Field255: compile with language_version 0.21.0
Field255->>Bytes32: import for comparisons (lt, eq)
Bytes32-->>Field255: Bytes32_lt, Bytes32_eq
Compiler->>Uint256: compile with updated imports
Uint256->>Pack: use Pack32_pack for conversions
Pack-->>Uint256: Pack32_pack result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes The changes introduce a substantial new protocol (Lunarswap) with 8 core modules, comprehensive test coverage, and refactoring of math libraries. The diff spans 70+ files with ~15,000+ lines added/modified, including intricate financial logic (liquidity calculations, swap invariants, reserve management), new witness implementations, API renamings affecting multiple modules, and migration to a new compiler version. The heterogeneous nature—combining protocol logic, math library updates, test infrastructure, SDK tooling, and configuration changes—demands careful review across multiple domains. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
0xisk
changed the title
0.28.00.29.0
0xisk
force-pushed
the
refactor/upgrade-math-0.28.0-part3
branch
from
1da3b72 to
44dda7e
Compare
0xisk
force-pushed
the
feat/lunarswap-0.28.0
branch
2 times, most recently
from
6be66d6 to
3949c16
Compare
0xisk
force-pushed
the
feat/lunarswap-0.28.0
branch
from
475004f to
3d681ca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
.gitignore-158-166 (1)
158-166:⚠️ Potential issue | 🟡 MinorRemove duplicate
.pnpm-store/entry.
.pnpm-store/is listed twice (lines 160 and 166). Remove the duplicate to keep the file clean.🧹 Proposed fix
node-compile-cache/ .pnpm-store/ midnight-level-db/ .vite/ keys/ shielded-tokens/ zkir/ -.pnpm-store/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 158 - 166, Remove the duplicate .pnpm-store/ entry from the .gitignore by keeping a single occurrence of the pattern ".pnpm-store/" and deleting the redundant line; ensure other ignore entries (e.g., node-compile-cache/, .vite/, keys/, shielded-tokens/, zkir/, midnight-level-db/) remain unchanged so the file stays clean and deduplicated.packages/lunarswap-sdk/package.json-7-13 (1)
7-13:⚠️ Potential issue | 🟡 MinorESM output may not work with
requireexport.The
tsconfig.jsoncompiles to ESNext modules (ESM), but the exports map includes a"require"entry pointing to the same ESM file. CommonJS consumers usingrequire()won't be able to load ESM directly.Options:
- Remove the
"require"entry if CJS support isn't needed- Add a separate CJS build (e.g., using a bundler or dual compilation)
- Change tsconfig to output CJS (
"module": "CommonJS")Proposed fix if CJS support is not needed
"exports": { ".": { "types": "./dist/index.d.ts", - "import": "./dist/index.js", - "require": "./dist/index.js" + "import": "./dist/index.js" } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lunarswap-sdk/package.json` around lines 7 - 13, The exports map currently exposes a "require" entry that points at the ESM output, which will break CommonJS consumers; update the package exports so CJS consumers either aren't advertised or receive a proper CJS build: either remove the "require" key from the "exports" map if you don't need CJS, or produce a separate CJS bundle (and point "require" to that file), or change the TypeScript compile target in tsconfig.json (the "module" option) to "CommonJS" and rebuild so the "require" entry points to valid CJS output; locate the "exports" block and the "require" entry in package.json and adjust according to the chosen option.contracts/package.json-24-24 (1)
24-24:⚠️ Potential issue | 🟡 MinorVersion mismatch: README badge shows 0.28.0 while package.json uses 0.29.0.
The
compactVersionhere is set to+0.29.0, but the README.md badge at line 3 still references0.28.0. This inconsistency could confuse contributors about which compiler version is actually in use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/package.json` at line 24, The package's compactVersion value (compactVersion: "+0.29.0") and the README badge are out of sync; update the README badge version to match compactVersion (0.29.0) or change compactVersion to the badge value so both reflect the same compiler version—locate the compactVersion setting and the README badge string and make them identical.README.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorUpdate the Compact Compiler badge version to 0.29.0.
The badge shows version
0.28.0, but the PR upgrades the compiler to0.29.0(seecontracts/package.jsonline 24). Update the badge to reflect the correct version.Proposed fix
-[](https://docs.midnight.network/relnotes/compact/) +[](https://docs.midnight.network/relnotes/compact/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 3, Update the Compact Compiler badge in README.md to match the new compiler version by replacing "0.28.0" with "0.29.0" in the badge URL/label (the badge markdown line starting with []); ensure the visible label and the URL both show 0.29.0 so the badge reflects contracts/package.json's updated version.contracts/src/math/types/TU128.compact-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorConsolidate type imports: Lunarswap still imports from deprecated
TU128while other modules useTypes.Lunarswap.compact imports
U128fromTU128, but other modules like Uint128.compact and Bytes32.compact have migrated to the newTypesmodule. Update Lunarswap.compact to import fromTypesto complete the consolidation and allow deprecatingTU128.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/math/types/TU128.compact` at line 4, Lunarswap.compact currently imports U128 from the deprecated TU128 module; update its import to pull U128 from the consolidated Types module instead (replace the TU128 import with an import from Types), ensuring any references to U128 in Lunarswap.compact remain unchanged and compile against the new Types module.packages/lunarswap-sdk/README.md-58-61 (1)
58-61:⚠️ Potential issue | 🟡 MinorClarify or correct the "~95%" statement for new pairs.
The comment states that
amountAMinandamountBMinwill be "~95% of desired amounts" when usingSLIPPAGE_TOLERANCE.LOW(0.5%). However, 0.5% slippage should yield ~99.5% of desired amounts, not ~95%.If new pairs have special handling that applies a larger buffer (e.g., ~5% reduction), this should be explicitly documented. Otherwise, update the percentage to match the actual slippage tolerance.
📝 Suggested clarification
-// amountAMin and amountBMin will be ~95% of desired amounts +// amountAMin and amountBMin will be ~99.5% of desired amounts (0.5% slippage)Or if 95% is intentional for new pairs:
-// amountAMin and amountBMin will be ~95% of desired amounts +// For new pairs, a 5% safety buffer is applied, so amountAMin and amountBMin will be ~95% of desired amounts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lunarswap-sdk/README.md` around lines 58 - 61, The README's statement that amountAMin and amountBMin will be "~95% of desired amounts" is incorrect for SLIPPAGE_TOLERANCE.LOW (0.5%); update the text to state the correct approximate value (~99.5%) or, if there is special handling for new pairs that intentionally applies a ~5% buffer, explicitly document that behavior and where it happens; reference the symbols amountAMin, amountBMin and SLIPPAGE_TOLERANCE.LOW and either correct the percentage to ~99.5% or add a short note explaining the additional reduction for new pairs and link to the code path that implements that special-case handling.contracts/src/lunarswap/tests/Lunarswap.test.ts-425-431 (1)
425-431:⚠️ Potential issue | 🟡 MinorTest does not verify order-independence as described.
The test is named "should generate same hash regardless of token order" but both calls to
getPairIduse the same token order(usdcCoin, nightCoin). To verify order-independence, the second call should reverse the token order.🐛 Proposed fix
it('should generate same hash regardless of token order', () => { const usdcCoin = usdc.mint(createEitherFromHex(LP_USER), 100n); const nightCoin = night.mint(createEitherFromHex(LP_USER), 100n); const hash1 = lunarswap.getPairId(usdcCoin, nightCoin); - const hash2 = lunarswap.getPairId(usdcCoin, nightCoin); + const hash2 = lunarswap.getPairId(nightCoin, usdcCoin); expect(hash1).toEqual(hash2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/tests/Lunarswap.test.ts` around lines 425 - 431, The test "should generate same hash regardless of token order" incorrectly calls lunarswap.getPairId with the same argument order twice; update the second call to lunarswap.getPairId(nightCoin, usdcCoin) (i.e., reverse the token arguments) so the test actually verifies order-independence of getPairId for the usdcCoin and nightCoin pair.contracts/src/lunarswap/LunarswapLibrary.compact-233-268 (1)
233-268:⚠️ Potential issue | 🟡 MinorDocumentation formula doesn't match implementation.
The
@remarkssection describes:amountOutWithFee = amountOut * 997 numerator = amountOutWithFee * reserveIn * 1000 denominator = reserveOut * amountOutWithFee - 997But the implementation (Lines 265-267) computes:
numerator = reserveIn * amountOut * 1000 denominator = (reserveOut - amountOut) * 997The standard AMM formula for
getAmountInshould be(reserveIn * amountOut * 1000) / ((reserveOut - amountOut) * 997) + 1, which the implementation correctly follows. Update the documentation to match.Fix documentation
* `@remarks` * This circuit implements the constant product formula to calculate the expected * input amount when swapping tokens. The calculation is as follows: - * amountOutWithFee = amountOut * 997 - * numerator = amountOutWithFee * reserveIn * 1000 - * denominator = reserveOut * amountOutWithFee - 997 - * amountIn = numerator / denominator + 1 + * numerator = reserveIn * amountOut * 1000 + * denominator = (reserveOut - amountOut) * 997 + * amountIn = (numerator / denominator) + 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/LunarswapLibrary.compact` around lines 233 - 268, The docblock for getAmountIn is inconsistent with the implementation: update the `@remarks` to reflect the actual formula used in the code (numerator = reserveIn * amountOut * 1000; denominator = (reserveOut - amountOut) * 997; amountIn = numerator / denominator + 1). Edit the comment above the getAmountIn circuit (function getAmountIn) to remove the incorrect amountOutWithFee steps and replace them with the correct expression matching the implementation and AMM standard so the documentation and the code (Uint128_mulChecked/reserve subtraction logic) align.contracts/src/lunarswap/LunarswapFee.compact-130-140 (1)
130-140:⚠️ Potential issue | 🟡 MinorDocumentation error message mismatch.
Same issue as
setFeeTo: the@throwsdocumentation states"LunarswapFactory: forbidden"but the actual assertion throws"LunarswapFee: setFeeToSetter() - forbidden".Suggested fix
- * `@throws` {Error} "LunarswapFactory: forbidden" if the caller is not the fee setter. + * `@throws` {Error} "LunarswapFee: setFeeToSetter() - forbidden" if the caller is not the fee setter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/LunarswapFee.compact` around lines 130 - 140, The JSDoc `@throws` message for setFeeToSetter is incorrect: update the documentation or the assertion so they match; specifically, either change the `@throws` text from "LunarswapFactory: forbidden" to "LunarswapFee: setFeeToSetter() - forbidden" or change the assert message in the setFeeToSetter function (which uses Utils_callerZPK() and compares to feeToSetter) to match the documented "LunarswapFactory: forbidden"; ensure the unique symbols referenced (setFeeToSetter, feeToSetter_, Utils_callerZPK, feeToSetter) are consistent between doc and runtime error.contracts/src/lunarswap/LunarswapFee.compact-103-115 (1)
103-115:⚠️ Potential issue | 🟡 MinorDocumentation error message mismatch.
The
@throwsdocumentation states"LunarswapFactory: forbidden"but the actual assertion at Line 112 throws"LunarswapFee: setFeeTo() - forbidden". Update the documentation to match the implementation.Suggested fix
- * `@throws` {Error} "LunarswapFactory: forbidden" if the caller is not the fee setter. + * `@throws` {Error} "LunarswapFee: setFeeTo() - forbidden" if the caller is not the fee setter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/LunarswapFee.compact` around lines 103 - 115, Update the `@throws` docstring for the setFeeTo circuit to match the actual assertion text: change the documented error from "LunarswapFactory: forbidden" to "LunarswapFee: setFeeTo() - forbidden" so the documentation reflects the actual runtime message thrown in setFeeTo (the assertion using Utils_callerZPK() == feeToSetter).contracts/src/lunarswap/Lunarswap.compact-98-108 (1)
98-108:⚠️ Potential issue | 🟡 MinorDocumentation mismatch:
feeToSetterparameter documented but not implemented.The
@paramdocumentation mentionsfeeToSetteras a constructor parameter, but the actual constructor signature only acceptsname,symbol,nonce, anddecimals. The docstring and implementation are inconsistent.Either update the documentation to remove the
feeToSetterreference, or add it as a parameter if fee setter configuration is intended:📝 Option 1: Fix documentation
* `@param` {Opaque<"string">} lpTokenName - The name of the LP token. * `@param` {Opaque<"string">} lpTokenSymbol - The symbol of the LP token. * `@param` {Bytes<32>} lpTokenNonce - The nonce for LP token generation. * `@param` {Uint<8>} lpTokenDecimals - The number of decimals for LP tokens. - * `@param` {ZswapCoinPublicKey} feeToSetter - The address that can set protocol fees.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/Lunarswap.compact` around lines 98 - 108, The constructor docstring lists a feeToSetter parameter that isn't in the actual constructor signature; update consistency by either removing the `@param {ZswapCoinPublicKey} feeToSetter` line from the docblock or add `feeToSetter` to the constructor signature and pass it into `LunarswapRouter_initialize` (or into whatever initializer expects it); locate the constructor and the call to `LunarswapRouter_initialize(nonce, name, symbol, decimals, ownPublicKey())` and either remove the unused doc entry or extend the signature to `constructor(..., feeToSetter)` and forward `feeToSetter` into `LunarswapRouter_initialize` as required.contracts/src/lunarswap/tests/mocks/contracts/LunarswapFactory.mock.compact-1-94 (1)
1-94:⚠️ Potential issue | 🟡 MinorAdd missing
getLpTokenTotalSupplyexport circuit.The main
LunarswapFactory.compactexportsgetLpTokenTotalSupply(pairId: PairId): Uint<128>at line 478, but this is missing from the mock file. Add:Add this export circuit to the mock file
export circuit getLpTokenTotalSupply(pairId: PairId): Uint<128> { toImpure = true; return LunarswapFactory_getLpTokenTotalSupply(pairId); }This maintains consistency with the factory's public API and the mock's pattern of wrapping all exported circuits.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/tests/mocks/contracts/LunarswapFactory.mock.compact` around lines 1 - 94, The mock is missing an exported wrapper for getLpTokenTotalSupply, so add an export circuit named getLpTokenTotalSupply that sets toImpure = true and returns LunarswapFactory_getLpTokenTotalSupply(pairId) with the correct signature Uint<128>; locate where other wrappers are defined in this mock (e.g., getPairId, getAllPairLength) and add the new export following the same pattern using the unique symbols getLpTokenTotalSupply and LunarswapFactory_getLpTokenTotalSupply.contracts/src/math/test/mocks/contracts/Bytes32.mock.compact-72-75 (1)
72-75:⚠️ Potential issue | 🟡 Minor
isZerodoesn't usedisclose()unlike other circuits in this mock.All other comparison circuits (
eq,lt,lte,gt,gte) wrap their return values withdisclose(), butisZeroreturns the result directly. For consistency with the other circuits and proper disclosure of the impure circuit result, adddisclose():Suggested change
export circuit isZero(a: Bytes<32>): Boolean { toImpure = true; - return Bytes32_isZero(a); + return disclose(Bytes32_isZero(a)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/math/test/mocks/contracts/Bytes32.mock.compact` around lines 72 - 75, The isZero circuit returns the raw result of Bytes32_isZero without using disclose(), unlike the other comparison circuits; update the isZero export (function/circuit name: isZero) to wrap the returned value with disclose() so the impure result from Bytes32_isZero is properly disclosed (i.e., call disclose(...) on the result before returning).
🧹 Nitpick comments (32)
contracts/src/math/test/Bytes32.test.ts (2)
253-255: Redundant test case.This test duplicates the assertion already covered in
should return false for different values(lines 246-251), which tests the exact same values (0nand1n).♻️ Consider removing the redundant test
- test('should return false for zero and one', () => { - expect(bytes32Simulator.eq(toBytes(0n), toBytes(1n))).toBe(false); - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/math/test/Bytes32.test.ts` around lines 253 - 255, Remove the redundant test "should return false for zero and one" that duplicates the assertion in "should return false for different values"; locate the test block that calls bytes32Simulator.eq(toBytes(0n), toBytes(1n)) and delete it (or consolidate its expectation into the existing "should return false for different values" test) so the same assertion isn't repeated.
37-38: Acknowledged: TODO for reusable test fixture.This is tracked via issue
#297. A shared fixture could reduce duplication across the math module tests (Uint64, Uint128, Field255, etc.).Would you like me to help draft a reusable test fixture implementation that can be shared across the math module tests?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/math/test/Bytes32.test.ts` around lines 37 - 38, Create a reusable test fixture for math modules to remove duplicate setup in Bytes32.test.ts and other math tests; implement a helper (e.g., export function createMathFixture or createMathTests) in a new test/helpers/mathFixture.ts that accepts parameters like moduleName/typeName, bitWidth/bounds, and provides common setup, sample values, and assertion helpers, then update Bytes32.test.ts to import and call createMathFixture("Bytes32", { bitWidth: 32, sampleValues: [...] }) (and similarly use it in Uint64/Uint128/Field255 tests) so all common logic lives in the fixture and individual tests only supply module-specific params and assertions.packages/lunarswap-sdk/vitest.config.ts (1)
19-23: Path alias@may need to be added totsconfig.json.The
@alias is configured here for Vitest, but the correspondingtsconfig.jsondoesn't define a matchingpathsentry. If source files use@/...imports, TypeScript won't resolve them correctly during compilation.Consider adding to
tsconfig.json:"baseUrl": ".", "paths": { "@/*": ["src/*"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lunarswap-sdk/vitest.config.ts` around lines 19 - 23, The Vitest config defines an alias "@" in resolve.alias but tsconfig.json lacks a matching paths mapping, causing TypeScript to fail resolving "@/..." imports; open tsconfig.json and add a baseUrl and a paths entry mapping "@/*" to "src/*" so the TypeScript compiler and editors resolve the same "@" alias used in vitest.config.ts and throughout the codebase.README.md (1)
55-58: Consider using consistent indentation for the bash command.The
cdcommand on line 57 lacks consistent indentation compared to other code blocks in the file.Proposed fix
2. **Build the compact-tools Submodule**: ```bash -cd compact-tools && yarn install && yarn build && cd - + cd compact-tools && yarn install && yarn build && cd -</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 55 - 58, The bash snippet in the README code block
for "Build the compact-tools Submodule" has inconsistent indentation: the second
line "cd compact-tools && yarn install && yarn build && cd -" should be indented
to match the triple-backtick fenced block; update the README.md code block so
that the "cd compact-tools ..." line has the same leading spaces as the
surrounding fenced code (i.e., add three spaces before "cd compact-tools && yarn
install && yarn build && cd -") to maintain consistent indentation and
formatting.</details> </blockquote></details> <details> <summary>contracts/src/math/test/Uint64.test.ts (1)</summary><blockquote> `452-460`: **Consider removing "(duplicate)" from test name.** The test at line 452 is labeled `should fail when remainder >= divisor (duplicate)`, which may cause confusion. While this tests the same assertion via `divRem` (vs `div` at line 306), the label could be clearer—perhaps `should fail when remainder >= divisor via divRem`. <details> <summary>Suggested rename</summary> ```diff - test('should fail when remainder >= divisor (duplicate)', () => { + test('should fail when remainder >= divisor', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/math/test/Uint64.test.ts` around lines 452 - 460, Rename the test title string in the test that currently reads "should fail when remainder >= divisor (duplicate)" to a clearer label such as "should fail when remainder >= divisor via divRem"; locate the test by the unique call uint64Simulator.overrideWitness('wit_divUint64', ...) and the assertion using uint64Simulator.divRem(10n, 5n) and update only the test name string to remove "(duplicate)" and reflect that it exercises divRem.contracts/src/math/Types.compact (1)
2-2: Minor: Header comment path may be inconsistent.The SPDX header comment shows
(math/types/Types.compact), but based on the file structure, this file appears to be atcontracts/src/math/Types.compact(not in atypes/subdirectory). Consider updating the path in the comment for consistency.📝 Suggested fix
-// OpenZeppelin Midnight Apps Contracts v0.0.1-alpha.0 (math/types/Types.compact) +// OpenZeppelin Midnight Apps Contracts v0.0.1-alpha.0 (math/Types.compact)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/math/Types.compact` at line 2, The header comment path is inconsistent: update the file header string "(math/types/Types.compact)" to reflect the actual location used in this repo (e.g., "(math/Types.compact)" or matching the file's path) so the banner matches contracts/src/math/Types.compact; edit the top-of-file comment in the Types.compact file to the correct path text.contracts/src/math/test/Pack.test.ts (1)
12-16: Guardvec8against oversized input.Line 15 casts any array to
Vec8, sovec8(...)can silently return more than 8 elements if a future test passes too many values. A small length check keeps the tuple guarantee honest.Suggested fix
const vec8 = (...values: number[]): Vec8 => { + if (values.length > 8) { + throw new Error('vec8 expects at most 8 values'); + } const a = [...values]; while (a.length < 8) a.push(0); return a.map((x) => BigInt(x)) as Vec8; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/math/test/Pack.test.ts` around lines 12 - 16, The helper vec8 currently casts any-length array to Vec8 which can silently produce >8 elements; update the vec8 function to guard against oversized input by checking values.length and throwing a clear error (e.g., "vec8 expects at most 8 values") if values.length > 8, then proceed with the existing padding to 8 and mapping to BigInt; reference the vec8 function and the Vec8 type when making this change.contracts/src/lunarswap/README.md (1)
63-90: Avoid pinning exact source line numbers in the README.Lines 64-89 hard-code file line numbers and row counts. Those references will drift quickly on the next refactor and leave the doc stale; file/circuit names alone, or generated metadata, will age much better.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/README.md` around lines 63 - 90, The README currently hard-codes source line numbers and circuit row counts for items like Lunarswap constructor (Lunarswap.compact → LunarswapRouter_initialize), LunarswapRouter.initialize, LunarswapFactory.initialize, LunarswapPair.initialize, LunarswapLiquidity.initialize, and LunarswapFee.initialize; remove the exact line numbers and k/rows values and instead reference only file/component names and the called initialization functions (e.g., Lunarswap.compact → LunarswapRouter_initialize, LunarswapRouter.compact → LunarswapFactory_initialize and LunarswapFee_initialize, etc.), or replace them with generated metadata links/IDs if available so the doc stays accurate after refactors.contracts/src/lunarswap/tests/removeLiquidity.test.ts (3)
133-138: Remove unusedgetPairReservescall.Line 138 calls
getPairReservesbut the result is discarded. This appears to be a leftover from development. Consider removing it since the reserves are fetched again on lines 146-149 for the actual assertions.🧹 Proposed fix
// Get updated pair state const updatedLpTotalSupply = lunarswap.getLpTokenTotalSupply( usdcCoin, nightCoin, ); - lunarswap.getPairReserves(usdcCoin, nightCoin); // Verify LP token supply decreased🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/tests/removeLiquidity.test.ts` around lines 133 - 138, Remove the unused call to getPairReserves where its return value is discarded; specifically, delete the standalone lunarswap.getPairReserves(usdcCoin, nightCoin) invocation that appears immediately after computing updatedLpTotalSupply (from lunarswap.getLpTokenTotalSupply) so only the later reserves fetch used for assertions remains.
751-881: Complex try-catch pattern reduces test reliability.This test performs three sequential removals wrapped in a try-catch that accepts a specific error as a valid outcome. If the implementation changes and fails differently, this test would incorrectly pass. Consider:
- Computing expected values to ensure success, or
- Splitting into separate tests for success and failure scenarios
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/tests/removeLiquidity.test.ts` around lines 751 - 881, The test wraps three sequential removeLiquidity calls in one try-catch that treats a specific error message as acceptable, making the test brittle; split this into explicit success and failure assertions by (1) creating separate tests: one that computes expected LP removals using calculateRemoveLiquidityMinimums and asserts lunarswap.removeLiquidity and subsequent lunarswap.getLpTokenTotalSupply/getPairReserves change reserves and supply as predicted, and (2) a separate test that intentionally triggers the insufficient-liquidity path and asserts the thrown error message from removeLiquidity; locate usage of lunarswap.removeLiquidity, lunarswap.getLpTokenTotalSupply, lunarswap.getPairReserves, and calculateRemoveLiquidityMinimums in the current test and refactor accordingly so success paths don't rely on catching an error.
679-703: Overly permissive try-catch may mask unexpected errors.This test accepts either a successful operation or a specific error message. If an unexpected error occurs, it would still pass if the message contains "Insufficient liquidity burned". Consider asserting on the exact error or splitting into two separate tests—one for the success path and one for the failure path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/tests/removeLiquidity.test.ts` around lines 679 - 703, The current test around lunarswap.removeLiquidity is too permissive because the try-catch will hide unexpected errors; split it into two explicit tests or tighten the assertion: create one test that calls lunarswap.removeLiquidity with valid preconditions and asserts the post-condition via lunarswap.getLpTokenTotalSupply(...) to be > 0n, and a separate test that calls lunarswap.removeLiquidity with inputs expected to fail and asserts the thrown error exactly (e.g., check instance of Error and message === 'Insufficient liquidity burned' or use toThrow with that exact message) so unexpected errors are not accepted.contracts/src/math/README.md (2)
109-125: Clarify the division operation in the example.At line 115,
div(product.low, c)dividesproduct.low(a U128) byc(a Uint<128>). The comment says "Divide product by c" but only the low part is used. If the multiplication can overflow intoproduct.high, this would lose precision. Consider adding a comment explaining this is intentional for the example, or demonstrating full 256-bit division if available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/math/README.md` around lines 109 - 125, The example function computeAndConvert currently uses div(product.low, c), which only divides the lower 128 bits and can lose precision if mul(a,b) overflows into product.high; update the example to either (a) add a clear comment in computeAndConvert explaining that using product.low is an intentional simplification and that full 256-bit precision is not preserved here, or (b) replace the division with the library's full U256 division routine (using product and c as a 256-bit dividend) if available; reference product.low, product.high, div, and computeAndConvert so the change is applied to the correct code path and ensure the comment or implementation notes mention potential precision loss when only the low part is used.
89-96: Non-standard import paths in examples.The import paths use
./node_modules/@openzeppelin/...which is atypical. Most projects configure module resolution to allow direct imports like@openzeppelin/midnight-apps-contracts/math/Uint64. Consider updating the examples to use the simpler import style for consistency with standard practices.📝 Suggested change
-import { div as divU64, sqrt as sqrtU64 } from "./node_modules/@openzeppelin/midnight-apps-contracts/math/Uint64"; -import { mul, div, sqrt } from "./node_modules/@openzeppelin/midnight-apps-contracts/math/Uint128"; -import { toBytes } from "./node_modules/@openzeppelin/midnight-apps-contracts/math/Uint256"; -import { lt } from "./node_modules/@openzeppelin/midnight-apps-contracts/math/Bytes32"; +import { div as divU64, sqrt as sqrtU64 } from "@openzeppelin/midnight-apps-contracts/math/Uint64"; +import { mul, div, sqrt } from "@openzeppelin/midnight-apps-contracts/math/Uint128"; +import { toBytes } from "@openzeppelin/midnight-apps-contracts/math/Uint256"; +import { lt } from "@openzeppelin/midnight-apps-contracts/math/Bytes32";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/math/README.md` around lines 89 - 96, Update the example import statements to use standard module specifiers instead of relative node_modules paths: replace imports like "import { div as divU64, sqrt as sqrtU64 } from \"./node_modules/@openzeppelin/midnight-apps-contracts/math/Uint64\";" and the similar lines that import mul, div, sqrt (Uint128), toBytes (Uint256) and lt (Bytes32) with direct package imports such as "@openzeppelin/midnight-apps-contracts/math/Uint64", "@openzeppelin/midnight-apps-contracts/math/Uint128", "@openzeppelin/midnight-apps-contracts/math/Uint256", and "@openzeppelin/midnight-apps-contracts/math/Bytes32" so the example uses standard module resolution and matches typical project configurations.contracts/src/lunarswap/tests/Lunarswap.test.ts (1)
630-639: Edge case test uses tokens without creating a pair first.The test "should handle edge cases with zero values" creates a pair with
usdcCoinandnightCoin, but then callsgetReserveIdwithzeroTokenCoinandusdcCoin— a pair that was never created. This may not test the intended behavior. Consider clarifying whether this tests reserve ID generation for non-existent pairs or if it should use tokens from the created pair.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/tests/Lunarswap.test.ts` around lines 630 - 639, The test "should handle edge cases with zero values" is using zeroTokenCoin with usdcCoin even though the created pair is (usdcCoin, nightCoin); update the test around getReserveId to either create the pair for zeroTokenCoin/usdcCoin before calling lunarswap.getReserveId, or switch zeroTokenCoin to use nightCoin so it uses the existing pair, or explicitly assert the expected behavior for non-existent pairs (e.g., that getReserveId still deterministically returns a non-zero id). Locate the test and adjust the setup or expectation for lunarswap.getReserveId, referencing the variables zeroTokenCoin, usdcCoin, nightCoin and the method getReserveId.contracts/src/lunarswap/tests/addLiquidity.test.ts (2)
861-878: Track the skipped test.This test is skipped with a TODO indicating it requires compiler team review. Consider creating a tracking issue to ensure this edge case is eventually addressed.
Would you like me to open an issue to track this compiler-related edge case for maximum amount handling?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/tests/addLiquidity.test.ts` around lines 861 - 878, Open a tracking issue for the skipped test "should handle maximum amounts" and link the compiler team for follow-up; include the test name and reproduction steps (minting with maxAmount = 2n ** 128n - 1n via usdc.mint and night.mint to LP_USER and calling lunarswap.addLiquidity), paste the observed error string ('failed to decode for built-in type u64 after successful typecheck'), attach the skipped test context and expected behavior, label the issue with "compiler", "skip-test" and "needs-repro", assign to the compiler owners or triage team, and set a reasonable milestone so it isn’t lost.
891-917: Duplicate test cases.The test
"should handle edge case with minimum viable amounts"appears twice - once in theedge casesdescribe block (Lines 891-917) and again in theerror handlingdescribe block (Lines 1247-1274). Both tests have identical logic. Remove the duplicate.Remove duplicate test
Remove Lines 1247-1274 (the second occurrence in the
error handlingblock) since the test logically belongs with edge cases and is already covered there.Also applies to: 1247-1274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/tests/addLiquidity.test.ts` around lines 891 - 917, Remove the duplicate test named "should handle edge case with minimum viable amounts" that appears a second time inside the "error handling" describe block; keep the original in the "edge cases" describe block, delete the redundant test (the one with the identical minting, calculateAddLiquidityAmounts call, lunarswap.addLiquidity invocation and expect on getLpTokenTotalSupply) so the suite only contains the single intended test, then run tests to confirm no other references rely on the removed duplicate.contracts/src/math/test/mocks/contracts/Field255.mock.compact (1)
1-4: Missing SPDX license header.Other mock files in this PR include the license header (
// SPDX-License-Identifier: MIT). Add it for consistency.Suggested fix
+// SPDX-License-Identifier: MIT +// OpenZeppelin Midnight Apps Contracts v0.0.1-alpha.0 (math/test/Field255.mock.compact) + pragma language_version >= 0.21.0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/math/test/mocks/contracts/Field255.mock.compact` around lines 1 - 4, Add the missing SPDX license header by inserting the comment "// SPDX-License-Identifier: MIT" at the very top of the file before the existing pragma line (the file containing "pragma language_version >= 0.21.0;" and "import CompactStandardLibrary;") so the header matches other mock files in the PR.contracts/src/lunarswap/LunarswapFee.compact (1)
109-111: Address TODO comments before release.These TODO comments indicate open design questions about admin vs. feeToSetter distinction and access control implementation. Consider creating tracking issues for these.
Would you like me to open issues to track these design decisions?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/LunarswapFee.compact` around lines 109 - 111, Resolve the TODOs in LunarswapFee.compact by (1) documenting the intended roles: add a clear comment above setFeeTo explaining the difference between Admin and feeToSetter (which one is the immutable owner vs a delegatable setter) and record any unresolved decision as a tracking issue; and (2) implement access control in the exported circuit setFeeTo(feeTo_: ZswapCoinPublicKey) so it checks the caller identity (e.g., require caller == Admin or an explicit feeToSetter role) and reverts/returns if unauthorized—use the existing Admin and feeToSetter identifiers to locate where to add the check and add a unit test exercising both authorized and unauthorized calls.contracts/src/lunarswap/LunarswapLiquidity.compact (2)
210-212: Redundant type casting and repeateddisclose()calls.The code has multiple redundant patterns:
pairId as PairIdwhenpairIdis already of typePairIddisclose(pairId as PairId)repeated multiple times with the same valueConsider storing the disclosed value once:
♻️ Suggested refactor
export circuit mint(pairId: PairId, recipient: Either<ZswapCoinPublicKey, ContractAddress>, amount: Uint<64> ): ShieldedCoinInfo { counter.increment(1); const newNonce = evolveNonce(counter, nonce); + const disclosedPairId = disclose(pairId); // pairId is used as domainSep in mintShieldedToken - const ret = mintShieldedToken(pairId as Bytes<32>, amount, newNonce, disclose(recipient)); + const ret = mintShieldedToken(disclosedPairId as Bytes<32>, amount, newNonce, disclose(recipient)); - if (!color.member(disclose(pairId as PairId))) { - color.insert(disclose(pairId as PairId), ret.color); + if (!color.member(disclosedPairId)) { + color.insert(disclosedPairId, ret.color); } // Update total supply tracking - simple balance increment - const currentTotalSupply = totalSupply.lookup(disclose(pairId as PairId)); + const currentTotalSupply = totalSupply.lookup(disclosedPairId); const newTotalSupply = Uint128_addChecked(currentTotalSupply, amount); - totalSupply.insert(disclose(pairId as PairId), disclose(newTotalSupply)); + totalSupply.insert(disclosedPairId, disclose(newTotalSupply));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/LunarswapLiquidity.compact` around lines 210 - 212, Remove the redundant casts and duplicate disclose() calls by computing and reusing the disclosed PairId once: call disclose(pairId) into a local (e.g., disclosedPairId) then use that variable in the color.member(...) and color.insert(...) calls; also drop the unnecessary "as PairId" cast since pairId is already PairId and keep passing ret.color to color.insert as before.
114-125:initializecircuit lacks idempotency protection.The
initializecircuit can be called multiple times, overwriting the existing configuration. The docstring says "should only be called once during contract deployment" but there's no enforcement.Consider adding a guard similar to
initializePairLpToken:🛡️ Proposed fix
export circuit initialize( nonce_: Bytes<32>, name_: Opaque<"string">, symbol_: Opaque<"string">, decimals_: Uint<8> ): [] { + assert(Bytes32_isZero(nonce), + "LunarswapLiquidity: initialize() - Already initialized" + ); nonce = disclose(nonce_); name = disclose(name_); symbol = disclose(symbol_); decimals = disclose(decimals_); return []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/LunarswapLiquidity.compact` around lines 114 - 125, The initialize circuit currently allows repeated calls and must be made idempotent: add a storage flag (e.g., initialized or isInitialized) checked at the start of the initialize circuit (similar to initializePairLpToken's guard) to revert/abort if already set, and set that flag to true after assigning nonce, name, symbol, decimals; update any constructor/deploy flow to disclose the flag where required and use the same flag name in other circuits as the canonical initialized guard.contracts/src/lunarswap/Lunarswap.compact (1)
94-97: Address TODO comments before production.The TODOs mention critical missing features:
- Deadline support for transaction expiry
- Access control for administrative functions
- Double-initialization prevention
These should be tracked and addressed, especially the double-initialize protection which is a security concern.
Do you want me to open issues to track these TODOs?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/Lunarswap.compact` around lines 94 - 97, Lunarswap's TODOs must be implemented: add a deadline parameter to on-chain-exposed functions (e.g., swap, mint, burn) and enforce require(block.timestamp <= deadline) to prevent expired txs; add access control (e.g., Ownable or AccessControl) and apply it to administrative methods (e.g., initialize, setFee, withdraw) so only authorized accounts can call them; and add a double-initialize guard in the Lunarswap contract by introducing an initialized boolean and an initializer modifier that reverts if already true, applying it to initialize and any setup routines.contracts/src/lunarswap/tests/mocks/contracts/LunarswapLibrary.mock.compact (1)
19-33: Inconsistentdisclose()usage betweensplitByValueandsortByColorvariants.
splitByValueCoinandsplitByValueQualifiedCoinreturn results directly, whilesortByColorCoinandsortByColorQualifiedCoinwrap returns indisclose(). The underlying implementations inLunarswapLibrary.compactare also plain returns without disclosure, suggesting the wrapping pattern is intentional at the mock level. However,getIdentitysimilarly returns withoutdisclose(), creating inconsistency among return types and deserves clarification for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/tests/mocks/contracts/LunarswapLibrary.mock.compact` around lines 19 - 33, Mocks are inconsistent: splitByValueCoin and splitByValueQualifiedCoin return raw SplitResult while sortByColorCoin/sortByColorQualifiedCoin wrap returns in disclose(), and getIdentity returns raw—make the mock behavior consistent by wrapping the splitByValue* return values in disclose() (i.e., return disclose(LunarswapLibrary_splitByValue<...>(...))) and likewise wrap getIdentity's return in disclose() to match the existing sortByColor* pattern; update references to LunarswapLibrary_splitByValue and getIdentity in the mock so all mock-returning functions use disclose() consistently.packages/lunarswap-sdk/src/index.ts (2)
127-147: Consider implications of slippage tolerance on new pair creation.For new pairs (Line 128), applying slippage tolerance to calculate minimum amounts (Lines 132-139) may cause unexpected behavior. The first liquidity provider sets the price ratio, so there's no price slippage risk—only the risk of front-running pair creation.
If the intent is to protect against front-running, the minimum amounts should equal the desired amounts. If someone front-runs the pair creation, the subsequent add would fail the optimal amount checks anyway.
💡 Alternative: Use desired amounts as minimums for new pairs
// Check if this is a new pair (no existing liquidity) if (reserveA === 0n && reserveB === 0n) { // For new pairs, optimal amounts are the desired amounts const amountAOptimal = amountADesired; const amountBOptimal = amountBDesired; - const amountAMin = calculateMinimumAmount( - amountAOptimal, - slippageTolerance, - ); - const amountBMin = calculateMinimumAmount( - amountBOptimal, - slippageTolerance, - ); + // For new pairs, minimums equal optimals since first depositor sets the ratio + const amountAMin = amountAOptimal; + const amountBMin = amountBOptimal;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lunarswap-sdk/src/index.ts` around lines 127 - 147, For new-pair handling in the add-liquidity logic (check using reserveA and reserveB), avoid applying slippageTolerance via calculateMinimumAmount when reserveA === 0n && reserveB === 0n; instead set amountAMin and amountBMin equal to amountADesired and amountBDesired so the first LP defines the ratio (update the block that currently computes amountAMin/amountBMin using calculateMinimumAmount to assign the desired amounts directly for new pairs).
34-47: Consider validatingoptimalAmountis positive.The function doesn't validate that
optimalAmount > 0n. While callers may ensure this, defensive validation would prevent unexpected behavior when combined with edge-case slippage values.Also, when
slippageTolerance = 10000(100%), the result is0n, which may cause downstream failures in contracts expecting non-zero minimums.🛡️ Proposed defensive validation
export function calculateMinimumAmount( optimalAmount: bigint, slippageTolerance: number, ): bigint { + if (optimalAmount <= 0n) { + throw new Error("Invalid optimal amount. Must be positive"); + } if (slippageTolerance < 0 || slippageTolerance > 10000) { throw new Error( "Invalid slippage tolerance. Must be between 0 and 10000 basis points", ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lunarswap-sdk/src/index.ts` around lines 34 - 47, The calculateMinimumAmount function lacks validation for optimalAmount and allows producing zero when slippageTolerance is 10000; add defensive checks at the top of calculateMinimumAmount to ensure optimalAmount is a positive bigint (optimalAmount > 0n) and reject slippageTolerance === 10000 (or require slippageTolerance < 10000) so the computed minimum cannot be zero; throw clear Errors mentioning calculateMinimumAmount, optimalAmount and slippageTolerance when inputs are invalid.contracts/src/lunarswap/LunarswapRouter.compact (1)
672-675: Remove duplicate@returnstag in docstring.* `@circuitInfo` k=6, rows=26 * * `@returns` {Bytes<32>} - The pairId for the given token pair. - - * `@returns` {Bytes<32>} - The pairId for the given pair. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/LunarswapRouter.compact` around lines 672 - 675, Remove the duplicate `@returns` tag in the docstring that documents the pairId return value: keep a single `@returns {Bytes<32>}` entry with a clear description (e.g., "The pairId for the given token pair") in the docblock associated with the function that returns the pairId (the pairId-returning docstring in LunarswapRouter), deleting the redundant second `@returns` line so the docstring has only one return annotation.contracts/src/math/test/mocks/contracts/Uint128.mock.compact (1)
240-253: Inconsistent use ofdisclosebetweenminU128andmaxU128.
minU128(Line 242) returns directly withoutdisclose, whilemaxU128(Line 252) usesdisclose. For consistency, both should follow the same pattern:export circuit minU128(a: U128, b: U128): U128 { toImpure = true; - return Uint128_min(a, b); + return disclose(Uint128_min(a, b)); }Or remove
disclosefrommaxU128to matchminU128.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/math/test/mocks/contracts/Uint128.mock.compact` around lines 240 - 253, minU128 currently returns Uint128_min(a, b) directly while maxU128 wraps Uint128_max(a, b) in disclose, causing inconsistent behavior; pick one pattern and make both functions match: either add disclose around Uint128_min in minU128 or remove disclose from maxU128 so both use the same reveal semantics—update the return statements in the circuit functions minU128 and maxU128 accordingly (also check the similarly-named max to ensure consistency).contracts/src/lunarswap/LunarswapFactory.compact (2)
213-262: Potential inconsistency inaddReserveswhen reserve states differ.Lines 220-232 handle the case where neither reserve exists. Lines 234-260 handle existing reserves but check each reserve's value independently. However, if only one reserve exists (which shouldn't happen in normal operation but could due to a bug), the logic could behave unexpectedly:
- If
reserve0Iddoesn't exist butreserve1Iddoes, Line 234'slookupon a non-existent key may fail or return default values.Consider adding explicit existence checks before lookups:
🛡️ Proposed defensive check
if (!reserves.member(disclose(reserve0Id)) && !reserves.member(disclose(reserve1Id))) { // ... initial deposit logic return []; } + + // Sanity check: both reserves should exist at this point + assert(reserves.member(disclose(reserve0Id)) && reserves.member(disclose(reserve1Id)), + "LunarswapFactory: addReserves() - Reserve state inconsistency"); if (reserves.lookup(disclose(reserve0Id)).value == 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/LunarswapFactory.compact` around lines 213 - 262, The addReserves circuit calls reserves.lookup(...) without first ensuring the key exists, which can misbehave if only one reserve exists; modify addReserves to use reserves.member(disclose(reserve0Id)) and reserves.member(disclose(reserve1Id)) before any reserves.lookup calls and branch on each member check: if a reserve member is false, call reserves.insertCoin with disclose(tokenX) directly (as in the initial-create branch); if true, call mergeCoinImmediate(reserves.lookup(disclose(reserveXId)), disclose(tokenX)) and then reserves.insertCoin; keep using disclose(...), mergeCoinImmediate, reserves.insertCoin and right<...>(kernel.self()) as in existing code to preserve semantics.
394-405: Clarify token color validation policy.The commented-out validation (Lines 399-401) and TODO indicate uncertainty about whether zero-color tokens (like tDust) should be allowed. This should be resolved before production:
- If zero-color tokens are valid, document this explicitly and remove the commented code
- If they should be blocked, uncomment and fix the validation
Leaving this unresolved creates ambiguity for integrators and potential security gaps.
Would you like me to help determine the appropriate validation policy based on the Midnight protocol specifications?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/LunarswapFactory.compact` around lines 394 - 405, The createPair function leaves token color validation commented out and ambiguous; decide the policy and apply it: if zero-color tokens (e.g., tDust) are permitted, remove the commented assert and add a clear comment in createPair/LunarswapFactory explaining that zero color is allowed and why; if zero-color tokens must be rejected, restore and correct the validation (use the same check for both token0.color and token1.color, e.g. by calling Utils_isKeyOrAddressZero on right<ZswapCoinPublicKey, ContractAddress>(ContractAddress { bytes: tokenX.color }) for each token) and throw/assert with a descriptive "LunarswapFactory: Invalid token color" message; make the change in createPair and ensure LunarswapPair_initializePair and pool.insert behavior remains consistent with the chosen policy.contracts/src/lunarswap/LunarswapPair.compact (3)
236-239: Acknowledged risk: LP token total supply may desync in UTXO model.The TODO accurately identifies that users can send LP tokens directly to a burn address without using the
burnfunction, causingtotalSupplyto be higher than actual circulating supply. This could affect:
- Liquidity calculations for subsequent providers
- Fee distribution accuracy
Consider implementing a mechanism to periodically reconcile supply, or document this as a known limitation for integrators.
Would you like me to propose a design pattern for supply reconciliation, or should this be tracked as a known limitation?
173-183: TODO:lpTokenTypeis not being calculated.The
lpTokenTypefield is initialized todefault<Bytes<32>>(Line 176) but should be calculated based on the pair's LP token color. This affects thepair.lpTokenTypechecks in other functions likeremoveLiquidity(Line 325 in Router).Would you like me to help implement the LP token type calculation, or open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/LunarswapPair.compact` around lines 173 - 183, The Pair construction currently sets lpTokenType to default<Bytes<32>>; replace that with a deterministic derivation of the LP token color based on the two token colors (token0.color and token1.color) so pair.lpTokenType is correct for downstream checks (e.g., removeLiquidity). Implement or call a helper like deriveLPTokenType(token0.color, token1.color) (or implement a small deterministic combiner / hash of the two Colors) and use its result (cast to Bytes<32> if needed) in place of default<Bytes<32>> in the Pair return; ensure the helper is defined/visible in the same module or imported so other functions that check pair.lpTokenType behave correctly.
331-331: Simplify redundant ternary expression.- const isFeeOn = isFeeToZero == true ? false : true; + const isFeeOn = !isFeeToZero;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/lunarswap/LunarswapPair.compact` at line 331, The ternary "const isFeeOn = isFeeToZero == true ? false : true;" is redundant; replace it with a simpler boolean expression such as "const isFeeOn = isFeeToZero !== true;" (or "const isFeeOn = !isFeeToZero;" if you want truthiness semantics) to preserve the original logic while removing the unnecessary ternary; update the declaration of isFeeOn in LunarswapPair.compact accordingly.contracts/src/structs/Queue.compact (1)
30-30: Outdated version reference in documentation.The comment references "Compact (v0.14.0)" but the pragma now requires
>= 0.21.0. Consider updating or removing this specific version reference to avoid confusion about whether the described limitations still apply in the current language version.📝 Suggested fix
- * - Lack of Iteration: Compact (v0.14.0) doesn't support + * - Lack of Iteration: Compact doesn't supportAlternatively, verify if variable-bound loops are now supported in 0.21.0 and update the documentation accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/src/structs/Queue.compact` at line 30, Update the outdated version reference in the comment inside Queue.compact that currently reads "Compact (v0.14.0) doesn’t support" to reflect the current pragma (>= 0.21.0): either remove the specific v0.14.0 mention or change it to a generic note about "Compact < 0.21.0" (or explicitly to "Compact (>= 0.21.0)" after verifying behavior); if variable-bound loops are now supported in 0.21.0, edit the sentence to state the current capability instead of the old limitation so the comment aligns with the pragma and actual language features.
| export circuit swapReserves( | ||
| pairId: PairId, | ||
| tokenIn: ShieldedCoinInfo, | ||
| tokenOut: ShieldedCoinInfo, | ||
| to: Either<ZswapCoinPublicKey, ContractAddress> | ||
| ): [] { | ||
| const reserveInType = | ||
| LunarswapLibrary_getIdentity( | ||
| disclose(pairId) as Bytes<32>, | ||
| disclose(tokenIn.color), | ||
| false | ||
| ); | ||
| const reserveOutType = | ||
| LunarswapLibrary_getIdentity( | ||
| disclose(pairId) as Bytes<32>, | ||
| disclose(tokenOut.color), | ||
| false | ||
| ); | ||
| const [reserveIn, reserveOut] = | ||
| getReserves(pairId, reserveInType as ReserveId, reserveOutType as ReserveId); | ||
|
|
||
| if (disclose(tokenIn.value) > 0) { | ||
| reserves.insertCoin( | ||
| reserveInType as ReserveId, | ||
| mergeCoinImmediate(reserves.lookup(reserveInType as ReserveId), disclose(tokenIn)), | ||
| right<ZswapCoinPublicKey, ContractAddress>(kernel.self()) | ||
| ); | ||
| } | ||
|
|
||
| if (disclose(tokenOut.value) > 0) { | ||
| const sendResults = sendShielded(reserveOut, disclose(to), disclose(tokenOut.value)); | ||
| if (sendResults.change.is_some) { | ||
| reserves.insertCoin( | ||
| reserveOutType as ReserveId, | ||
| sendResults.change.value, | ||
| right<ZswapCoinPublicKey, ContractAddress>(kernel.self()) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| assert(tokenIn.value > 0 || tokenOut.value > 0, | ||
| "LunarswapFactory: swapReserves() - Invalid token" | ||
| ); | ||
|
|
||
| return []; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move validation before state-modifying operations in swapReserves.
The assertion on Lines 364-366 validates that at least one of tokenIn.value or tokenOut.value is positive, but this check occurs after the reserve updates (Lines 345-361). If the assertion fails, the updates have already been attempted.
Move the assertion to the beginning of the function:
🔧 Proposed fix
export circuit swapReserves(
pairId: PairId,
tokenIn: ShieldedCoinInfo,
tokenOut: ShieldedCoinInfo,
to: Either<ZswapCoinPublicKey, ContractAddress>
): [] {
+ assert(tokenIn.value > 0 || tokenOut.value > 0,
+ "LunarswapFactory: swapReserves() - Invalid token"
+ );
+
const reserveInType =
LunarswapLibrary_getIdentity(
// ... rest of function
- assert(tokenIn.value > 0 || tokenOut.value > 0,
- "LunarswapFactory: swapReserves() - Invalid token"
- );
return [];📝 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.
| export circuit swapReserves( | |
| pairId: PairId, | |
| tokenIn: ShieldedCoinInfo, | |
| tokenOut: ShieldedCoinInfo, | |
| to: Either<ZswapCoinPublicKey, ContractAddress> | |
| ): [] { | |
| const reserveInType = | |
| LunarswapLibrary_getIdentity( | |
| disclose(pairId) as Bytes<32>, | |
| disclose(tokenIn.color), | |
| false | |
| ); | |
| const reserveOutType = | |
| LunarswapLibrary_getIdentity( | |
| disclose(pairId) as Bytes<32>, | |
| disclose(tokenOut.color), | |
| false | |
| ); | |
| const [reserveIn, reserveOut] = | |
| getReserves(pairId, reserveInType as ReserveId, reserveOutType as ReserveId); | |
| if (disclose(tokenIn.value) > 0) { | |
| reserves.insertCoin( | |
| reserveInType as ReserveId, | |
| mergeCoinImmediate(reserves.lookup(reserveInType as ReserveId), disclose(tokenIn)), | |
| right<ZswapCoinPublicKey, ContractAddress>(kernel.self()) | |
| ); | |
| } | |
| if (disclose(tokenOut.value) > 0) { | |
| const sendResults = sendShielded(reserveOut, disclose(to), disclose(tokenOut.value)); | |
| if (sendResults.change.is_some) { | |
| reserves.insertCoin( | |
| reserveOutType as ReserveId, | |
| sendResults.change.value, | |
| right<ZswapCoinPublicKey, ContractAddress>(kernel.self()) | |
| ); | |
| } | |
| } | |
| assert(tokenIn.value > 0 || tokenOut.value > 0, | |
| "LunarswapFactory: swapReserves() - Invalid token" | |
| ); | |
| return []; | |
| } | |
| export circuit swapReserves( | |
| pairId: PairId, | |
| tokenIn: ShieldedCoinInfo, | |
| tokenOut: ShieldedCoinInfo, | |
| to: Either<ZswapCoinPublicKey, ContractAddress> | |
| ): [] { | |
| assert(tokenIn.value > 0 || tokenOut.value > 0, | |
| "LunarswapFactory: swapReserves() - Invalid token" | |
| ); | |
| const reserveInType = | |
| LunarswapLibrary_getIdentity( | |
| disclose(pairId) as Bytes<32>, | |
| disclose(tokenIn.color), | |
| false | |
| ); | |
| const reserveOutType = | |
| LunarswapLibrary_getIdentity( | |
| disclose(pairId) as Bytes<32>, | |
| disclose(tokenOut.color), | |
| false | |
| ); | |
| const [reserveIn, reserveOut] = | |
| getReserves(pairId, reserveInType as ReserveId, reserveOutType as ReserveId); | |
| if (disclose(tokenIn.value) > 0) { | |
| reserves.insertCoin( | |
| reserveInType as ReserveId, | |
| mergeCoinImmediate(reserves.lookup(reserveInType as ReserveId), disclose(tokenIn)), | |
| right<ZswapCoinPublicKey, ContractAddress>(kernel.self()) | |
| ); | |
| } | |
| if (disclose(tokenOut.value) > 0) { | |
| const sendResults = sendShielded(reserveOut, disclose(to), disclose(tokenOut.value)); | |
| if (sendResults.change.is_some) { | |
| reserves.insertCoin( | |
| reserveOutType as ReserveId, | |
| sendResults.change.value, | |
| right<ZswapCoinPublicKey, ContractAddress>(kernel.self()) | |
| ); | |
| } | |
| } | |
| return []; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/lunarswap/LunarswapFactory.compact` around lines 324 - 369, The
assertion that ensures at least one of tokenIn.value or tokenOut.value is
positive is placed after state-modifying operations; move the check to the start
of the swapReserves circuit (immediately after parameters/initial local
declarations) so it runs before calling
getReserves/sendShielded/reserves.insertCoin; specifically, in function
swapReserves ensure the assert(tokenIn.value > 0 || tokenOut.value > 0, ...) is
executed before any calls to getReserves, reserves.insertCoin,
mergeCoinImmediate, or sendShielded to prevent partial state changes on invalid
input.
| if (ret.change.is_some) { | ||
| const tmpAddr = left<ZswapCoinPublicKey, ContractAddress>(ownPublicKey()); | ||
| sendImmediateShielded(ret.change.value, disclose(tmpAddr), ret.change.value.value); | ||
| } |
There was a problem hiding this comment.
Change coin from burn operation is sent but not returned to caller.
In the burn circuit, when there's change (line 264-267), it's sent to ownPublicKey() via sendImmediateShielded, but:
- The result isn't captured
- The change coin info isn't returned to the caller
This could lead to the caller losing track of their change. Consider returning the change info or documenting this behavior clearly.
🐛 Proposed fix to capture and return change
- export circuit burn(pairId: PairId, coin: ShieldedCoinInfo, amount: Uint<128>): ShieldedSendResult {
+ export circuit burn(pairId: PairId, coin: ShieldedCoinInfo, amount: Uint<128>): [ShieldedSendResult, ShieldedCoinInfo] {
// ... existing code ...
if (ret.change.is_some) {
const tmpAddr = left<ZswapCoinPublicKey, ContractAddress>(ownPublicKey());
- sendImmediateShielded(ret.change.value, disclose(tmpAddr), ret.change.value.value);
+ const changeSent = sendImmediateShielded(ret.change.value, disclose(tmpAddr), ret.change.value.value);
+ return [ret, changeSent.sent];
}
- return ret;
+ return [ret, coin]; // Return original coin info if no change
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/lunarswap/LunarswapLiquidity.compact` around lines 264 - 267,
The change coin sent via sendImmediateShielded when ret.change.is_some is
neither captured nor propagated back; modify the burn logic so that when
ret.change.is_some you capture the return value of sendImmediateShielded (called
with ret.change.value and disclose(left<ZswapCoinPublicKey,
ContractAddress>(ownPublicKey()))), store the resulting change coin/receipt, and
include that change info in the burn function's return object (e.g., extend the
existing return struct to carry change_coin or change_receipt and populate it
when handling ret.change). Ensure references to ret.change,
sendImmediateShielded, ownPublicKey(), and disclose are used to locate and
update the code paths.
| if (totalSupply == 0) { | ||
| // liquidity = (√amount0 * amount1) - MINIMUM_LIQUIDITY = Uint<128> | ||
| // TODO: review disclose | ||
| const liquidity = | ||
| Uint128_sub( | ||
| disclose(Uint128_sqrt(Uint128_mulChecked(amount0, amount1))), | ||
| MINIMUM_LIQUIDITY() | ||
| ); | ||
| assert(liquidity > 0, "LunarswapPair: mint() - New pair insufficient liquidity minted"); | ||
| // I think the liquidity won't exceed the Uint<64> | ||
| assert(liquidity <= Uint64_MAX_UINT64(), | ||
| "LunarswapPair: mint() - liquidity exceeds Uint<64> max" | ||
| ); | ||
| // TODO: WARNING! to be revised to prevent from inflation attack. | ||
| // TODO: IMPORTANT! that should be minted to the burn address. | ||
| //LunarswapLiquidity_mint(pairId, burn_address(), MINIMUM_LIQUIDITY()); | ||
| const lpTokens = LunarswapLiquidity_mint(pairId, to, liquidity as Uint<64>); | ||
| // Send the minimum liquidity to the contract address itself to lock the minimum liquidity | ||
| // sendImmediateShielded(lpTokens, shieldedBurnAddress(), MINIMUM_LIQUIDITY()); |
There was a problem hiding this comment.
Critical: Minimum liquidity is not being locked, enabling inflation attack.
The code calculates and subtracts MINIMUM_LIQUIDITY from the minted liquidity (Line 246), but the commented-out lines 254-258 show that the minimum liquidity is never actually minted to the burn address or locked. This leaves the pool vulnerable to the exact inflation attack the documentation describes preventing.
The TODO comments on lines 253-254 confirm this is an unresolved issue. Without locking the minimum liquidity, an attacker can manipulate the share value by donating assets to an empty pool.
🔒 Proposed fix to lock minimum liquidity
assert(liquidity > 0, "LunarswapPair: mint() - New pair insufficient liquidity minted");
// I think the liquidity won't exceed the Uint<64>
assert(liquidity <= Uint64_MAX_UINT64(),
"LunarswapPair: mint() - liquidity exceeds Uint<64> max"
);
- // TODO: WARNING! to be revised to prevent from inflation attack.
- // TODO: IMPORTANT! that should be minted to the burn address.
- //LunarswapLiquidity_mint(pairId, burn_address(), MINIMUM_LIQUIDITY());
+ // Mint minimum liquidity to the burn address to prevent inflation attack
+ LunarswapLiquidity_mint(pairId, Utils_eitherZPK(shieldedBurnAddress()), MINIMUM_LIQUIDITY() as Uint<64>);
const lpTokens = LunarswapLiquidity_mint(pairId, to, liquidity as Uint<64>);
- // Send the minimum liquidity to the contract address itself to lock the minimum liquidity
- // sendImmediateShielded(lpTokens, shieldedBurnAddress(), MINIMUM_LIQUIDITY());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/lunarswap/LunarswapPair.compact` around lines 240 - 258, The
pair mint currently subtracts MINIMUM_LIQUIDITY but never actually locks/mints
that minimum, leaving the pool vulnerable; update the mint flow in LunarswapPair
so you first mint MINIMUM_LIQUIDITY via LunarswapLiquidity_mint(pairId,
burn_address(), MINIMUM_LIQUIDITY()) (and if shielding is required call
sendImmediateShielded on that amount to shieldedBurnAddress()), then mint the
remaining liquidity (liquidity - MINIMUM_LIQUIDITY) to the recipient using
LunarswapLiquidity_mint(pairId, to, remainingLiquidity) ensuring types
(Uint<64>) and asserts remain valid; keep the burn mint separate and executed
before minting lpTokens to the user.
| if (reserve0.value != 0 && reserve1.value != 0) { | ||
| const price0 = disclose(Uint128_div(reserve1.value, reserve0.value)); | ||
| const price1 = disclose(Uint128_div(reserve0.value, reserve1.value)); | ||
|
|
||
| const price0VolCumulative = | ||
| Uint128_addChecked(pair.price0VolCumulative, Uint128_mulChecked(price0, amount0In)); | ||
| const price1VolCumulative = | ||
| Uint128_addChecked(pair.price1VolCumulative, Uint128_mulChecked(price1, amount1In)); | ||
|
|
||
| const volume0Cumulative = Uint128_addChecked(pair.volume0Cumulative, amount0In); | ||
| const volume1Cumulative = Uint128_addChecked(pair.volume1Cumulative, amount1In); | ||
|
|
||
| return [price0VolCumulative, price1VolCumulative, volume0Cumulative, volume1Cumulative]; |
There was a problem hiding this comment.
VWAP price calculation loses precision due to integer division.
Lines 658-659 compute prices using integer division:
const price0 = disclose(Uint128_div(reserve1.value, reserve0.value));
const price1 = disclose(Uint128_div(reserve0.value, reserve1.value));
This truncates fractional values, making the cumulative price oracle unreliable for tokens with different decimal scales or when reserves are imbalanced. For example, if reserve1 = 50 and reserve0 = 100, price0 becomes 0 instead of 0.5.
Uniswap v2 uses fixed-point UQ112.112 format for price accumulation. Consider scaling prices by a fixed factor (e.g., 1e18) before accumulation.
🔧 Proposed fix using scaled prices
if (reserve0.value != 0 && reserve1.value != 0) {
- const price0 = disclose(Uint128_div(reserve1.value, reserve0.value));
- const price1 = disclose(Uint128_div(reserve0.value, reserve1.value));
+ // Scale prices by 1e18 for precision (similar to UQ112.112)
+ const PRICE_SCALE: Uint<128> = 1000000000000000000; // 1e18
+ const price0 = disclose(Uint128_div(Uint128_mulChecked(reserve1.value, PRICE_SCALE), reserve0.value));
+ const price1 = disclose(Uint128_div(Uint128_mulChecked(reserve0.value, PRICE_SCALE), reserve1.value));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/lunarswap/LunarswapPair.compact` around lines 657 - 669, The
VWAP calculation currently uses integer division in
disclose(Uint128_div(reserve1.value, reserve0.value)) and
disclose(Uint128_div(reserve0.value, reserve1.value)) which truncates fractional
prices; change to compute fixed-point prices by first scaling the numerator by a
constant SCALE (e.g., 1e18) using Uint128_mulChecked and then dividing by the
denominator with Uint128_div to preserve precision (e.g., price0 =
Uint128_div(Uint128_mulChecked(reserve1.value, SCALE), reserve0.value)); then
use those scaled price0/price1 values in Uint128_mulChecked when computing
price0VolCumulative/price1VolCumulative and keep
pair.price0VolCumulative/price1VolCumulative in the same scaled units; ensure
you use the same SCALE everywhere and check for overflow via the existing
Checked ops (Uint128_mulChecked/Uint128_addChecked).
There was a problem hiding this comment.
Added TODO for future support, as currently that is not supported by the Math lib.
There was a problem hiding this comment.
@0xisk, understood — thanks for the context. Adding a TODO is a reasonable approach given the current Math library constraints. When the library gains support for scaled multiplication (e.g., Uint128_mulChecked with larger intermediate types), revisiting this to implement fixed-point pricing will be important for the accuracy of the VWAP oracle.
Would you like me to open a follow-up GitHub issue to track this precision gap so it doesn't get lost?
| it('should handle large output amounts correctly', () => { | ||
| const amountOut = 2000n; // Large amount relative to reserves | ||
| const amountInMax = 6000n; | ||
| const swapUsdcCoin = usdc.mint(recipient, amountInMax); | ||
| const preReserves = lunarswap.getPairReserves(usdcCoin, nightCoin); | ||
| const preUsdcReserve = preReserves[0]; | ||
| const preNightReserve = preReserves[1]; | ||
| try { | ||
| lunarswap.swapTokensForExactTokens( | ||
| swapUsdcCoin, // Use fresh token coin for swap | ||
| nightCoin, | ||
| amountOut, | ||
| amountInMax, | ||
| recipient, | ||
| ); | ||
| const postReserves = lunarswap.getPairReserves(usdcCoin, nightCoin); | ||
| const actualNightDelta = | ||
| preNightReserve.value - postReserves[1].value; | ||
| expect(actualNightDelta).toBeGreaterThanOrEqual(amountOut - 1n); | ||
| expect(actualNightDelta).toBeLessThanOrEqual(amountOut + 1n); | ||
| // Also check input constraint | ||
| const actualUsdcDelta = postReserves[0].value - preUsdcReserve.value; | ||
| expect(actualUsdcDelta).toBeLessThanOrEqual(amountInMax); | ||
| } catch (e) { | ||
| expect(() => { | ||
| throw e; | ||
| }).toThrow( | ||
| 'LunarswapRouter: swapTokensForExactTokens() - Insufficient input amount', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Don’t let this underfunded exact-output case pass on success.
Lines 564-565 request 2000n out with amountInMax = 6000n, but this fixture needs roughly 6.7k input by the exact-output formula. The current try/catch means the test still passes if swapTokensForExactTokens undercharges and succeeds, which hides the bug this case should catch.
Suggested fix
- const preReserves = lunarswap.getPairReserves(usdcCoin, nightCoin);
- const preUsdcReserve = preReserves[0];
- const preNightReserve = preReserves[1];
- try {
- lunarswap.swapTokensForExactTokens(
- swapUsdcCoin, // Use fresh token coin for swap
- nightCoin,
- amountOut,
- amountInMax,
- recipient,
- );
- const postReserves = lunarswap.getPairReserves(usdcCoin, nightCoin);
- const actualNightDelta =
- preNightReserve.value - postReserves[1].value;
- expect(actualNightDelta).toBeGreaterThanOrEqual(amountOut - 1n);
- expect(actualNightDelta).toBeLessThanOrEqual(amountOut + 1n);
- // Also check input constraint
- const actualUsdcDelta = postReserves[0].value - preUsdcReserve.value;
- expect(actualUsdcDelta).toBeLessThanOrEqual(amountInMax);
- } catch (e) {
- expect(() => {
- throw e;
- }).toThrow(
- 'LunarswapRouter: swapTokensForExactTokens() - Insufficient input amount',
- );
- }
+ expect(() => {
+ lunarswap.swapTokensForExactTokens(
+ swapUsdcCoin,
+ nightCoin,
+ amountOut,
+ amountInMax,
+ recipient,
+ );
+ }).toThrow(
+ 'LunarswapRouter: swapTokensForExactTokens() - Insufficient input amount',
+ );📝 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.
| it('should handle large output amounts correctly', () => { | |
| const amountOut = 2000n; // Large amount relative to reserves | |
| const amountInMax = 6000n; | |
| const swapUsdcCoin = usdc.mint(recipient, amountInMax); | |
| const preReserves = lunarswap.getPairReserves(usdcCoin, nightCoin); | |
| const preUsdcReserve = preReserves[0]; | |
| const preNightReserve = preReserves[1]; | |
| try { | |
| lunarswap.swapTokensForExactTokens( | |
| swapUsdcCoin, // Use fresh token coin for swap | |
| nightCoin, | |
| amountOut, | |
| amountInMax, | |
| recipient, | |
| ); | |
| const postReserves = lunarswap.getPairReserves(usdcCoin, nightCoin); | |
| const actualNightDelta = | |
| preNightReserve.value - postReserves[1].value; | |
| expect(actualNightDelta).toBeGreaterThanOrEqual(amountOut - 1n); | |
| expect(actualNightDelta).toBeLessThanOrEqual(amountOut + 1n); | |
| // Also check input constraint | |
| const actualUsdcDelta = postReserves[0].value - preUsdcReserve.value; | |
| expect(actualUsdcDelta).toBeLessThanOrEqual(amountInMax); | |
| } catch (e) { | |
| expect(() => { | |
| throw e; | |
| }).toThrow( | |
| 'LunarswapRouter: swapTokensForExactTokens() - Insufficient input amount', | |
| ); | |
| } | |
| it('should handle large output amounts correctly', () => { | |
| const amountOut = 2000n; // Large amount relative to reserves | |
| const amountInMax = 6000n; | |
| const swapUsdcCoin = usdc.mint(recipient, amountInMax); | |
| expect(() => { | |
| lunarswap.swapTokensForExactTokens( | |
| swapUsdcCoin, | |
| nightCoin, | |
| amountOut, | |
| amountInMax, | |
| recipient, | |
| ); | |
| }).toThrow( | |
| 'LunarswapRouter: swapTokensForExactTokens() - Insufficient input amount', | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/src/lunarswap/tests/swap.test.ts` around lines 563 - 592, The test
currently wraps lunarswap.swapTokensForExactTokens in a try/catch which allows a
silent success (hiding underfunding). Change the test so that a successful call
fails the test and only the thrown error is treated as a pass: after calling
swapTokensForExactTokens (or instead of the try block) assert a failure if it
returns (e.g. call fail()/throw new Error) and keep the expect(...).toThrow
assertion for the caught error; reference the swapTokensForExactTokens call,
amountOut, amountInMax, and the try/catch around it so the code ensures
underfunded exact-output must throw rather than succeed.
1 participant
Types of changes
What types of changes does your code introduce to OpenZeppelin Midnight Contracts?
Put an
xin the boxes that applyFixes #???
PR Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
tokenType()tocolor()in token contracts for clarity.Documentation