feat(contracts): `addLiquidity` circuit `v1` (Unpermissioned Shielded) by 0xisk · Pull Request #104 · OpenZeppelin/midnight-apps

0xisk · 2025-05-30T17:36:56Z

Description

Overview

Implements Lunarswap V1, a decentralized exchange protocol on the Midnight Network using Compact, with a focus on privacy-preserving features. Introduces the addLiquidity circuit for adding liquidity to trading pairs and minting LP tokens.
Blocked by #148.

Resolves

N/A

Key Changes

Added addLiquidity Circuit: Enables users to add liquidity to trading pairs in Lunarswap.compact and LunarswapRouter.compact. Optimizes token amounts to maintain price ratios, mints LP tokens, and returns excess tokens to users. Supports both new and existing pairs with slippage protection.
Added LunarswapFactory.compact: Manages pair creation and storage for addLiquidity operations.
Added LunarswapFee.compact: Handles fee collection for liquidity provision.
Added LunarswapLiquidity.compact: Manages LP token minting and tracking for addLiquidity.
Added LunarswapPair.compact: Updates pair reserves and tracks statistics during liquidity addition.
Added OpenZeppelin utilities and SDK (lunarswap-sdk) for liquidity calculations.

Testing

Added addLiquidity.test.ts: Tests liquidity addition for new and existing pairs, verifying optimal amounts, slippage protection, and reserve updates.
Added Lunarswap.test.ts and LunarswapSimulator.ts: Validates addLiquidity integration with pair management and token minting.
Tests cover edge cases like insufficient amounts and pair initialization.

Notes

TODO: Add support for removing liquidity, swaps, and transaction deadlines.
TODO: Export fee circuits and integrate initializable modules.

0xisk · 2025-07-08T13:27:58Z

@andrew-fleming this PR is ready to be reviewed, but I see that the pipeline is failing, I am thinking first to take the chance and update the whole gh workflows with the one that is in compact-contracts repo. WDYT?

andrew-fleming

Looking good, @0xisk! This is a partial review so you have some feedback :)

andrew-fleming · 2025-07-09T01:58:26Z

contracts/lunarswap-v1/package.json

@@ -0,0 +1,44 @@
+{
+	"name": "@midnight-dapps/lunarswap-v2",


The directory name is still lunarswap-v1

andrew-fleming · 2025-07-09T03:19:43Z

contracts/lunarswap-v1/src/tests/addLiquidity.test.ts

+import type { CoinInfo } from "@midnight-dapps/compact-std";
+import {
+	SLIPPAGE_TOLERANCE,
+	calculateAddLiquidityAmounts,


Suggested change

calculateAddLiquidityAmounts,

calculateLiquidityAmounts,

andrew-fleming · 2025-07-09T03:22:54Z

contracts/lunarswap-v1/src/tests/Lunarswap.test.ts

+	calculateAddLiquidityAmounts,
+	calculateRemoveLiquidityMinimums,
+} from "@midnight-dapps/lunarswap-sdk";


Suggested change

calculateAddLiquidityAmounts,

calculateRemoveLiquidityMinimums,

} from "@midnight-dapps/lunarswap-sdk";

calculateLiquidityAmounts,

} from "@midnight-dapps/lunarswap-sdk";

The other isn't exported from the sdk index and afaict it isn't being used here

andrew-fleming · 2025-07-09T03:27:57Z

contracts/lunarswap-v1/src/Lunarswap.compact

+}
+
+/**
+ * @title getPairIdentity circuit


Nit: Any reason not to shorten the name to getPairId?

andrew-fleming · 2025-07-09T03:35:28Z

contracts/lunarswap-v1/src/LunarswapFactory.compact

+     * @title isIdentityExists circuit
+     * @description Checks if a trading pair exists for the given identity hash.


Similar to my other question: why not just idExists or isPair?

andrew-fleming · 2025-07-10T03:11:25Z

contracts/lunarswap-v1/vitest.config.ts

+		environment: "node",
+		globals: true,
+		include: ["**/*.test.ts"],
+		hookTimeout: 100000,
+		coverage: {
+			provider: "v8",
+			reporter: ["text", "json", "html", "lcov"],
+			exclude: [
+				"node_modules/",
+				"**/*.d.ts",
+				"**/*.test.ts",
+				"**/*.spec.ts",
+				"**/coverage/**",
+				"**/dist/**",
+				"**/build/**",
+				"**/.next/**",
+				"**/vitest.config.*",
+				"**/tsconfig.*",
+				"**/package.json",
+				"**/package-lock.json",
+				"**/pnpm-lock.yaml",
+				"**/yarn.lock",
+			],
+			all: true,
+			clean: true,
+			cleanOnRerun: true,
+			reportsDirectory: "./coverage",
+			thresholds: {
+				global: {


Okay, my brain is now trying to decipher the shapes that this formatting is making. This is definitely a seahorse!

andrew-fleming · 2025-07-10T03:18:51Z

contracts/math/package.json

    "compact": "pnpm exec compact-compiler",
    "compact:fast": "pnpm exec compact-compiler --skip-zk",
-    "build": "pnpm exec compact-builder && tsc",
+    "build": "rm -rf dist && pnpm exec compact-builder && tsc",


Hmm idk about this..it's destructive without warning, no? I'd suggest moving rm -rf to something like a clean script (what we do in the contracts lib)

If you really want to keep it as is, we should at least document that build will destroy the dist because this is surprising behavior. This could also cause hard-to-find issues with CI and release pipelines once in place

andrew-fleming · 2025-07-10T03:24:55Z

packages/compact/src/Builder.ts

-        cmd: 'mkdir -p dist && find src -type f -name "*.compact" -exec cp {} dist/ \\; 2>/dev/null && rm dist/Mock*.compact 2>/dev/null || true',
+        /**
+         * Shell command to copy and clean `.compact` files from `src` to `dist`.
+         * - Creates `dist` directory if it doesn't exist.
+         * - Copies `.compact` files from `src` root to `dist` root (e.g., `src/Math.compact` → `dist/Math.compact`).
+         * - Copies `.compact` files from `src` subdirectories to `dist` with preserved structure (e.g., `src/interfaces/IMath.compact` → `dist/interfaces/IMath.compact`).
+         * - Excludes files in `src/test` and `src/src` directories.
+         * - Removes `Mock*.compact` files from `dist`.
+         * - Redirects errors to `/dev/null` and ensures the command succeeds with `|| true`.
+         */
+        cmd: [
+          'mkdir -p dist && \\', // Create dist directory if it doesn't exist
+          'find src -maxdepth 1 -type f -name "*.compact" -exec cp {} dist/ \\; && \\', // Copy .compact files from src root to dist root
+          'find src -type f -name "*.compact" \\', // Find .compact files in src subdirectories
+          '  -not -path "src/test/*" \\', // Exclude src/test directory
+          '  -not -path "src/src/*" \\', // Exclude src/src directory
+          '  -path "src/*/*" \\', // Only include files in subdirectories
+          '  -exec sh -c \\', // Execute a shell command for each file


Are these changes necessary in this PR?

andrew-fleming · 2025-07-10T03:38:27Z

contracts/lunarswap-v1/src/witnesses/ShieldedFungibleToken.ts

Can we not use the simple empty object that we use elsewhere?

andrew-fleming · 2025-07-10T04:29:22Z

contracts/lunarswap-v1/src/LunarswapPair.compact

@@ -0,0 +1,471 @@
+// SPDX-License-Identifier: MIT
+
+pragma language_version >= 0.14.0;


Suggested change

pragma language_version >= 0.14.0;

pragma language_version >= 0.15.0;

andrew-fleming

Sorry for the delay @0xisk. Nice work on getting this iteration done! Obviously, this is very big and quite complex in how all of the modules and circuits are supposed to interact. I left some comments and questions

Also, we should get the CI working and properly enforce formatting

andrew-fleming · 2025-07-11T03:41:33Z

contracts/lunarswap-v1/src/LunarswapPair.compact

+        amount0: Uint<128>,
+        amount1: Uint<128>
+    ): [Pair, Uint<128>] {
+        // We use addChecked here because the max amount in CoinInfo is Uint<128>


Good call adding the comment. CoinInfo.value is Uint<64> though, no?

andrew-fleming · 2025-07-11T03:45:37Z

contracts/lunarswap-v1/src/LunarswapPair.compact

+        // TODO: review the risk of using totalSupply here. Because of the nature of UTXO that 
+        // a user could send the LP tokens to the burn address directly without using the burn function, 
+        // but the totalSupply will not be updated.


andrew-fleming · 2025-07-11T03:56:58Z

contracts/lunarswap-v1/src/LunarswapPair.compact

+            ), liquidityTotal];
+        } else { 
+            // TODO: In case of using Uint<128> for reserves that results a conflict with 
+            // the ERC20 Compact standard, since the result here is Uint<256> while the max min tokens are Uint<128>


Nit but important: there's currently no ERC20 in midnight land. The FungibleToken Compact standard, instead?

Same with the other ERC20 refs below if you agree

andrew-fleming · 2025-07-11T03:59:39Z

contracts/lunarswap-v1/src/LunarswapPair.compact

+                disclose(Uint128_div(Uint128_mulChecked(amount0, totalSupply), reserve0)),
+                disclose(Uint128_div(Uint128_mulChecked(amount1, totalSupply), reserve1))
+            ); 
+            // TODO: that is TBD later in the future after doing benchmarks on the performance of MathU256


What's TBD? It's not clear what this is referring to

andrew-fleming · 2025-07-11T04:17:05Z

contracts/lunarswap-v1/src/LunarswapPair.compact

+        const balance0 = Uint128_addChecked(reserve0, amount0);
+        const balance1 = Uint128_addChecked(reserve1, amount1);
+
+        const [isFeeOn, kLast] = _mintFee(identity, reserve0, reserve1, pair.kLast);


Is this always called and structured this way because of immutable vars?

andrew-fleming · 2025-07-15T22:51:37Z

contracts/lunarswap-v1/src/tests/Lunarswap.test.ts

+			expect(lunarswap.getLiquidityTotalSupply(usdcCoin, dustCoin).value).toBe(
+				14142n,
+			);


I mentioned it in the other test file, but again, idk how 14142 is calculated and why it's correct here

andrew-fleming · 2025-07-15T23:01:42Z

contracts/lunarswap-v1/src/tests/Lunarswap.test.ts

+			expect(identity).toBeDefined();
+			expect(identity.length).toBe(32);


This isn't confirming that the correct pair hash is calculated though. It's just confirming that it's defined and has a len of 32. I think it's worth creating a helper to confirm the ids

andrew-fleming · 2025-07-15T23:02:46Z

contracts/lunarswap-v1/src/tests/Lunarswap.test.ts

+		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.getPairIdentity(usdcCoin, nightCoin);
+			const hash2 = lunarswap.getPairIdentity(usdcCoin, nightCoin);
+			expect(hash1).toEqual(hash2);


The pairs in hash1 and hash2 are in the same order, no?

andrew-fleming · 2025-07-15T23:05:36Z

contracts/lunarswap-v1/src/tests/Lunarswap.test.ts

+			const identity = lunarswap.getPairIdentity(usdcCoin, nightCoin);
+			expect(lunarswap.getLiquidityTotalSupply(usdcCoin, nightCoin).value).toBe(
+				7071n,


identity isn't used to get the correct order

andrew-fleming · 2025-07-15T23:08:35Z

contracts/lunarswap-v1/src/tests/Lunarswap.test.ts

In the PR comment under testing, it says:

- Added addLiquidity.test.ts: Tests liquidity addition for new and existing pairs, verifying optimal amounts, slippage protection, and reserve updates.
- Added Lunarswap.test.ts and LunarswapSimulator.ts: Validates addLiquidity integration with pair management and token minting.

Correct me if I'm wrong, but the setup looks pretty much the same for both. Why not combine them into a single test file?

andrew-fleming · 2025-07-16T00:06:43Z

From the PR description:

Blocked by #148.

Just curious, how is this blocked by the compiler output fix?

0xisk requested review from andrew-fleming and emnul as code owners May 30, 2025 17:36

0xisk marked this pull request as draft May 30, 2025 17:37

0xisk added the compact label May 30, 2025

0xisk self-assigned this May 30, 2025

0xisk added this to OZ Development for Midnight Jun 4, 2025

github-project-automation bot moved this to Backlog in OZ Development for Midnight Jun 4, 2025

0xisk changed the title ~~feat(contracts): addLiquidity circuit v2 UTXO~~ feat(contracts): addLiquidity circuit v1 UTXO Jun 26, 2025

0xisk changed the title ~~feat(contracts): addLiquidity circuit v1 UTXO~~ feat(contracts): addLiquidity circuit v1 (Unpermissioned Shielded - UTXO) Jun 27, 2025

0xisk force-pushed the feat/add-liquidity-utxo branch from c70ee3b to def6798 Compare June 27, 2025 14:37

0xisk mentioned this pull request Jul 4, 2025

feat(contracts): removeLiquidity circuits v1 #168

Closed

feat: add liquidity UTXO

6cf7376

0xisk force-pushed the feat/add-liquidity-utxo branch from def6798 to 6cf7376 Compare July 8, 2025 11:36

0xisk added 4 commits July 8, 2025 14:47

chore: rebase and update

828424d

chore: rebase and update

106e0a5

chore: update readme

8958533

chore: revert

29831b9

0xisk changed the title ~~feat(contracts): addLiquidity circuit v1 (Unpermissioned Shielded - UTXO)~~ feat(contracts): addLiquidity circuit v1 (Unpermissioned Shielded) Jul 8, 2025

fix: remove npmrc

198c3fc

0xisk marked this pull request as ready for review July 8, 2025 13:23

0xisk requested a review from a team as a code owner July 8, 2025 13:23

chore: update

398dee5

andrew-fleming reviewed Jul 10, 2025

View reviewed changes

andrew-fleming reviewed Jul 15, 2025

View reviewed changes

0xisk mentioned this pull request Sep 3, 2025

feat(e2e): Lunarswap application e2e #192

Closed

18 tasks

0xisk closed this Dec 29, 2025

0xisk moved this from Backlog to Complete/Stable Release in OZ Development for Midnight Feb 20, 2026

		* @title isIdentityExists circuit
		* @description Checks if a trading pair exists for the given identity hash.

		@@ -0,0 +1,471 @@
		// SPDX-License-Identifier: MIT

		pragma language_version >= 0.14.0;

	pragma language_version >= 0.14.0;
	pragma language_version >= 0.15.0;

		expect(identity).toBeDefined();
		expect(identity.length).toBe(32);

Comments

Conversation

0xisk commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Overview

Resolves

Key Changes

Testing

Notes

Uh oh!

0xisk commented Jul 8, 2025

Uh oh!

andrew-fleming left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrew-fleming left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrew-fleming commented Jul 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

0xisk commented May 30, 2025 •

edited

Loading

andrew-fleming left a comment •

edited

Loading