Add unit tests for createPool.ts #501

calintje · 2024-11-13T14:58:47Z

Title
Add unit tests for createPool.ts

Details

Fixed the calculation of estInitializationCosts in the createConcentratedLiquidityPool function.
Added tests to verify the correct calculation of estInitialization costs.
Updated setupWhirlpool to support the V2 instruction of our program.

…ees.

wjthieme · 2024-11-13T15:00:33Z

ts-sdk/whirlpool/src/createPool.ts

-    .send();
+  const nonRefundableRents: Lamports[] = await Promise.all(
+    stateSpaces.map(async (space) => {
+      const rentExemption = await rpc.getMinimumBalanceForRentExemption(BigInt(space)).send();


I wouldn't do this since it would change the rpc calls it needs to make from 1 to 5 (or 6)

wjthieme · 2024-11-13T15:00:59Z

ts-sdk/whirlpool/src/createPool.ts

-  stateSpace += getWhirlpoolSize();
+  stateSpaces.push(
+    tokenProgramA === TOKEN_2022_PROGRAM_ADDRESS
+      ? getTokenSize(getAccountExtensions(mintA.data))


Can we make this a separate function (should be able to check mint.owner so mint would be the only parameter)?

Suspect we will need the same thing in increase_liquidity too. We can put it in token.ts

And then we can write a test for it 😄

wjthieme · 2024-11-13T15:02:34Z

ts-sdk/whirlpool/tests/createPool.test.ts

+    setDefaultFunder(signer);
+  });
+
+  it("Should throw an error if token mints are not ordered", async () => {


nit: they are ordered, just not ordered correctly

wjthieme · 2024-11-13T15:03:23Z

ts-sdk/whirlpool/tests/createPool.test.ts

+    await assert.rejects(
+      createConcentratedLiquidityPoolInstructions(rpc, mintB, mintA, 64, 1),
+      {
+        name: 'AssertionError',


Not sure if it exists but is there a new AssertionError("Token order needs to be flipped to match the canonical ordering (i.e. sorted on the byte repr. of the mint pubkeys)")

wjthieme · 2024-11-13T15:03:50Z

ts-sdk/whirlpool/tests/createPool.test.ts

+      createConcentratedLiquidityPoolInstructions(rpc, mintB, mintA, 64, 1),
+      {
+        name: 'AssertionError',
+        message: 'Token order needs to be flipped to match the canonical ordering (i.e. sorted on the byte repr. of the mint pubkeys)'


nit: we use double quotes for strings in ts (but you can run yarn format and that should format your files correctly

wjthieme · 2024-11-13T15:07:29Z

ts-sdk/whirlpool/tests/createPool.test.ts

+      await createSplashPoolInstructions(rpc, mintA, mintB, price);
+    await sendTransaction(instructions);
+
+    const pool = await fetchWhirlpool(rpc, poolAddress);


I would structure the test like this:

create splash pool

fetch maybe pool before

fetch signer balance before

assert pool does not exist

send tx

fetch maybe pool

fetch signer blance after

assert pool exists

assert sqrt, mintA, mintB, and tick spacing

assert that estInitialization cost equals difference in signer balance

That way you don't need the Should estimate initialization costs correctly test case entirely

And then create more tests with similar structure for:

Splash pool with TE token

CL pool

CL pool with TE token

etc.

wjthieme · 2024-11-13T15:07:47Z

ts-sdk/whirlpool/tests/utils/program.ts

 import {
  SPLASH_POOL_TICK_SPACING,
  WHIRLPOOLS_CONFIG_ADDRESS,
 } from "../../src/config";
 import { tickIndexToSqrtPrice } from "@orca-so/whirlpools-core";
+import { fetchMint } from "@solana-program/token";
+import { a } from "vitest/dist/chunks/suite.B2jumIFP.js";


…xtensions array.

wjthieme · 2024-11-14T16:51:58Z

ts-sdk/whirlpool/src/createPool.ts

-    .send();
+  const nonRefundableRents: Lamports[] = await Promise.all(
+    stateSpaces.map(async (space) => {
+      const rentExemption = await calculateMinimumBalance(rpc, space);


This is still doing the same thing where it fetches the rent 5 (or 6 times).

The way I would do it. Change the function to:

export function calculateMinimumBalance( rent: Rent, dataSize: number, ): Lamports {

Fetcht he rent sysvar at the beginning of this function and then instead of keeping track of stateSpace: Array<bigint> you can just straight up keep track of estInitializationCost: Lamports.

And then instead of doing stateSpace.push(...) you can just do estInitializationCost += calculateMinimumBalance(rent, ...)

wjthieme · 2024-11-14T16:52:13Z

ts-sdk/whirlpool/src/sysvar.ts

+ * @param {number} dataSize - The size of the account data in bytes.
+ * @returns {Promise<BigInt>} The minimum balance required for rent exemption in lamports.
+ */
+export async function calculateMinimumBalance(


Can you write a test for this?

wjthieme · 2024-11-14T16:54:35Z

ts-sdk/whirlpool/tests/createPool.test.ts

+    const price = 10;
+    const sqrtPrice = priceToSqrtPrice(price, 6, 6);
+
+    let signerAccount = await rpc.getAccountInfo(signer.address).send();


Think nicer if we implement getBalance on the mockRpc so we don't have to duplicate this every time.

wjthieme · 2024-11-14T16:55:54Z

ts-sdk/whirlpool/tests/createPool.test.ts

+    assertAccountExists(pool);
+
+    signerAccount = await rpc.getAccountInfo(signer.address).send();
+    const balanceAfter = signerAccount.value?.lamports ?? lamports(0n);


nit: I would group all the fetch together before starting on any assertions. poolBefore together with balanceBefore, poolAfter together with balanceAfter.

wjthieme · 2024-11-14T16:56:42Z

ts-sdk/whirlpool/tests/createPool.test.ts

+    const balanceChange = balanceBefore - balanceAfter;
+    const txFee = 15000n; // 3 signing accounts * 5000 lamports
+    const minRentExempt = balanceChange - txFee;
+    assert.strictEqual(estInitializationCost, minRentExempt);


Let me see if I can make the tx fee go to 0 in bankrun so that this assertion can just become:

assert.strictEqual(estInitializationCost, balanceBefore - balanceAfter);

wjthieme · 2024-11-14T16:58:02Z

ts-sdk/whirlpool/tests/createPool.test.ts

+  });
+
+  it("Should create splash pool with 1 TE token", async () => {
+    const mint1 = await setupMint();


I would set up these mints in the beforeAll as well since that will get faster execution (init the TE mint once opposed to doing it for each TE test)

wjthieme · 2024-11-14T17:00:16Z

ts-sdk/whirlpool/tests/createPool.test.ts

+  });
+
+  it("Should create splash pool with 2 TE tokens", async () => {
+    const mint1 = await setupMintTEFee();


For the tests I would do (instead of trying to combine Transfer fee and 1/2 TE token tests):

1 TE token (without extensions)

2 TE tokens (without extensions)

1 TE token (with transfer fee)

wjthieme · 2024-11-14T17:02:21Z

ts-sdk/whirlpool/src/sysvar.ts

@@ -0,0 +1,28 @@
+import { fetchSysvarRent } from "@solana/sysvars";


nit: This should be re-exported by @solana/web3.js so you can use that as import

wjthieme · 2024-11-15T13:19:02Z

ts-sdk/whirlpool/src/sysvar.ts

 */
-export async function calculateMinimumBalance(
-  rpc: Rpc<GetAccountInfoApi>,
+export function calculateMinimumBalance(


nit: maybe we can be a bit more descriptive with this naming. getMinimumBalanceForRentExemption is what the rpc method that does the same is called

wjthieme · 2024-11-15T13:20:54Z

ts-sdk/whirlpool/tests/createPool.test.ts

+    mintTE1 = await setupMintTE();
+    mintTE2 = await setupMintTE();
+    mintTEFee1 = await setupMintTEFee();
+    mintTEFee2 = await setupMintTEFee();


Do you really need to have this second TEFee mint?

wjthieme · 2024-11-15T13:22:35Z

ts-sdk/whirlpool/tests/sysvar.test.ts

+  let rent: SysvarRent;
+
+  beforeAll(async () => {
+    rent = {


This doesn't necessarily have to be in the beforeAll (since it is not asynchronous). You can just init the rent directly as a const

wjthieme · 2024-11-15T13:22:37Z

ts-sdk/whirlpool/tests/sysvar.test.ts

+  });
+
+  it("Should calculate the correct minimum balance for a token account", async () => {
+    const mint = await setupMint();


You can make this test synchronous since you don't need to store/fetch anything in bankrun

wjthieme · 2024-11-15T13:27:06Z

ts-sdk/whirlpool/tests/utils/mockRpc.ts

@@ -103,7 +103,19 @@ const decoders: Record<string, VariableSizeDecoder<string>> = {
  base64: getBase64Decoder(),
 };

-async function getAccountData<T>(address: unknown, opts: unknown): Promise<T> {
+interface AccountData {


Not 100% sure if the AccountData is always exactly in this format. IIRC data depends on the encoding specified and is not always [string, string]. lamports/rentEpoch might be numbers I think (hence why I put generic T).

Instead of adding this interface you could also just do an assertion in getBalance to check if lamports exists in the object and then return that:

const accountDataForBalance = await getAccountData( addressForBalance, config.payload.params[1], ); assert("lamports" in accountDataForBalance); return getResponseWithContext<T>(accountDataForBalance.lamports);

wjthieme · 2024-11-15T16:17:42Z

ts-sdk/whirlpool/tests/createPool.test.ts

+      await createSplashPoolInstructions(rpc, mintA, mintB, price);
+
+    const balanceBefore = await rpc.getBalance(signer.address).send();
+    const maybePool = await fetchMaybeWhirlpool(rpc, poolAddress);


nit: I would name them poolBefore and poolAfter

wjthieme · 2024-11-15T16:18:28Z

ts-sdk/whirlpool/src/token.ts

@@ -33,6 +32,8 @@ import {
  getCreateAccountWithSeedInstruction,
  getTransferSolInstruction,
 } from "@solana-program/system";
+import { getTokenSize } from "@solana-program/token";
+import { getTokenSize as getToken22Size } from "@solana-program/token-2022";


nit: maybe getTokenWithExtensionsSize since if it is a t22 token with no extensions we still the other one

wjthieme · 2024-11-15T16:19:20Z

ts-sdk/whirlpool/tests/createPool.test.ts

+    const txFee = 15000n; // 3 signing accounts * 5000 lamports
+    const minRentExempt = balanceChange - txFee;
+
+    assertAccountExists(pool);


Interesting! Never knew this existed

wjthieme · 2024-11-15T16:20:00Z

ts-sdk/whirlpool/tests/createPool.test.ts

+    assert.strictEqual(SPLASH_POOL_TICK_SPACING, pool.data.tickSpacing);
+  });
+
+  it("Should create splash pool with 1 TE tokens (with extension)", async () => {


nit: maybe instead of with extension be explicit about that it is the TE extension

wjthieme · 2024-11-15T16:21:47Z

ts-sdk/whirlpool/tests/sysvar.test.ts

+      tokenSize,
+    );
+
+    const expectedMinimumBalance = lamports((165n + OVERHEAD) * 10n);


I wouldn't do the calculation here and just assert agains the actual value. OVERHEAD is something internal of the function and shouldn't be here. From a test you need to assume that you know nothing about the internal workings of the function and only test inputs and outputs

So just 1445n

wjthieme · 2024-11-15T16:23:01Z

ts-sdk/whirlpool/tests/token.test.ts

+    assert.strictEqual(tokenSize, 165);
+  });
+
+  it("Should get the correct token size for TOKEN_2022_PROGRAM mint with", async () => {


nit: mint with ...

calintje added 7 commits November 10, 2024 14:12

Base createPool test

c202a75

Process feedback. Improve createPool.ts tests. Attempt modify sysvarF…

f777c27

…ees.

Fix calculation of initizialization costs. Add createPool tests.

a3eab20

Merge branch 'main' into calintje/ts-sdk-unit-tests

502aae4

Clean up

08186d7

Clean up

47eba59

Clean up

413c5b0

calintje requested review from yugure-orca, wjthieme and odcheung as code owners November 13, 2024 14:58

wjthieme reviewed Nov 13, 2024

View reviewed changes

calintje added 7 commits November 13, 2024 16:33

Resolve minor comments

de84176

Restructure tests. Add getTokenSizeByProgram function, including tests.

71a7d7b

Resolve comments. Fix minor bug tih token size calculation based on e…

51bc3a0

…xtensions array.

Format

e8902bc

Update doc on core-sdks (unrelated)

3b4d976

Update doc on core-sdks (unrelated)

7e18813

Format

9a32a10

wjthieme reviewed Nov 14, 2024

View reviewed changes

Resolve comments

d54d855

calintje requested review from philcchen and rawfalafel as code owners November 15, 2024 02:42

Merge branch 'main' into calintje/ts-sdk-unit-tests

4fa6b40

calintje marked this pull request as draft November 15, 2024 02:46

calintje added 3 commits November 15, 2024 03:59

Revert changes

5c1bfc0

Revert changes

ccffacd

Revert changes

55b81c6

calintje marked this pull request as ready for review November 15, 2024 03:18

wjthieme approved these changes Nov 15, 2024

View reviewed changes

Resolve comments

c6a8823

wjthieme approved these changes Nov 15, 2024

View reviewed changes

calintje added 2 commits November 15, 2024 18:48

Resolve comments

7c359f9

Fromat

3c72de4

calintje merged commit 3ba9c08 into main Nov 15, 2024
5 checks passed

calintje deleted the calintje/ts-sdk-unit-tests branch November 15, 2024 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for createPool.ts #501

Add unit tests for createPool.ts #501

calintje commented Nov 13, 2024

wjthieme Nov 13, 2024

wjthieme Nov 13, 2024

wjthieme Nov 13, 2024

wjthieme Nov 13, 2024

wjthieme Nov 13, 2024

wjthieme Nov 13, 2024

wjthieme Nov 13, 2024

wjthieme Nov 13, 2024

wjthieme Nov 13, 2024

wjthieme Nov 13, 2024

wjthieme Nov 14, 2024

wjthieme Nov 14, 2024

wjthieme Nov 14, 2024

wjthieme Nov 14, 2024

wjthieme Nov 14, 2024

wjthieme Nov 14, 2024

wjthieme Nov 14, 2024

wjthieme Nov 14, 2024

wjthieme Nov 15, 2024

wjthieme Nov 15, 2024

wjthieme Nov 15, 2024

wjthieme Nov 15, 2024

wjthieme Nov 15, 2024

wjthieme Nov 15, 2024

wjthieme Nov 15, 2024

wjthieme Nov 15, 2024

wjthieme Nov 15, 2024

wjthieme Nov 15, 2024 •

edited

Loading

wjthieme Nov 15, 2024

wjthieme Nov 15, 2024

		@@ -0,0 +1,28 @@
		import { fetchSysvarRent } from "@solana/sysvars";

Add unit tests for createPool.ts #501

Add unit tests for createPool.ts #501

Conversation

calintje commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjthieme Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjthieme Nov 15, 2024 •

edited

Loading