-
Notifications
You must be signed in to change notification settings - Fork 293
Test migration #1115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test migration #1115
Conversation
a2c33c3 to
547cae9
Compare
3d75ad9 to
8ab1348
Compare
jshiohaha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops - i was reviewing commit by commit, but it looks like some of the commits no longer exist 🙈 fortunately, it looks like GH still recognizes the comments and the general area in which they apply. i'm just posting comments for the first 2 commits before they become too outdated, and i will continue reviewing the rest of the commits 🌱
| ), | ||
| ); | ||
| // Initialize tick arrays sequentially to avoid AccountBorrowFailed errors in LiteSVM | ||
| const results = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you could probably add a utility to handle this sequential execution under the hood while maintaining a functional interface (if you want). under the hood, i imagine the utility could do just this or use smth like p-limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha! I was just thinking of this when I looking over the code earlier today! Will look into it
|
|
||
| // Normalize logs so Anchor's EventParser can decode events reliably. | ||
| // Solana RPC emits binary event data as: "Program data: <base64>". | ||
| // Some LiteSVM builds may emit hex or numeric arrays instead; convert them to base64. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting! Could you explain more? Is behavior version dependent? Is it dependent on the underlying program?
and maybe dumb question, but is pretty_logs a utility that could help here? https://www.litesvm.com/docs/testing-your-program/executing-instructions#working-with-program-logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is being defensive to handle all possible formats LiteSVM might emit, making tests robust across different environments. It's not dependent on the underlying program.
pretty_logs is for debugging/viewing logs, while convertPayloadToBase64 is for programmatically parsing events in your automated tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh interesting.. i didn't realize LiteSVM would conditionally output different formats 🤔 i was thinking pretty_logs might be useful to convert the logs into the desired format w/o all the extra logic, e.g. pretty_logs -> split by new line -> parse events
b4707fb to
62f842b
Compare
jshiohaha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i added comments for the rest of the commits! overall, it seems like most tests look good & many comments are related to styling or design. there are a few related to modified test logic that i wasn't sure about, so i tried to comment just to double check those cases
and apologies, i thought i added more comments last night, but it looks like they didn't actually get posted because they're still marked as pending 🤦🏻♂️
| // SplashPool has only 2 TickArrays for negative and positive ticks | ||
| -1, +1, | ||
| ]))!.buildAndExecute(); | ||
| const fullRange = TickUtil.getFullRangeTickIndex( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious, is it only whitespace/formatting changes in this area? i'm having a little trouble seeing what changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yea git's diff does not make it clear. The only relevant changes are in the beforeAll and beforeEach callbacks.
| resetLiteSVM(); | ||
| await startLiteSVM(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do you recommend we think about calling resetLiteSVM and startLiteSVM?
i see they're paired together here within beforeEach, but below it seems like sometimes it's just startLiteSVM in a beforeAll and resetLiteSVM in beforeEach (without startLiteSVM)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tests share the litesvm state, since startLiteSVM follows a singleton pattern. Each reset reloads multiple .so programs from disk and considerably increases test time, so I only reset state where necessary to prevent AccountAlreadyInUse errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, interesting! so your process was smth like don't reset b/c latency unless you run into issues?
legacy-sdk/whirlpool/tests/integration/multi-ix/adaptive_fee.test.ts
Outdated
Show resolved
Hide resolved
...y-sdk/whirlpool/tests/integration/migration/migrate_repurpose_reward_authority_space.test.ts
Show resolved
Hide resolved
legacy-sdk/whirlpool/tests/integration/token-badge/delete_token_badge.test.ts
Outdated
Show resolved
Hide resolved
| // Skip decimals assertions in event due to LiteSVM decoding quirks; verify via mints below | ||
| // LiteSVM event may misreport tickSpacing and initialSqrtPrice; verify on-chain below instead | ||
| // LiteSVM may also misdecode pubkeys in the event payload; verify on-chain below instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, this is pretty interesting. is this due to the event payload being decoded incorrectly? iirc, there was some behavior related to logs requiring a bunch of custom logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a transaction is built:
- It captures a blockhash from the blockchain
- The transaction is serialized with that blockhash
- That specific blockhash has a limited lifetime
After expireBlockhash():
- The old blockhash is marked expired
- LiteSVM strictly rejects transactions with expired blockhashs
- The tx object still has the old, expired blockhash cached
solana-test-validator is more forgiving around using the same transaction object. In LiteSVM, once a transaction object has an expired blockhash, it required creating a completely new transaction object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks for explaining! although, i think this might be in response to this comment: #1115 (comment)
legacy-sdk/whirlpool/tests/integration/v2-ix/initialize/initialize_pool_v2.test.ts
Show resolved
Hide resolved
legacy-sdk/whirlpool/tests/integration/v2-ix/initialize/initialize_reward_v2.test.ts
Outdated
Show resolved
Hide resolved
legacy-sdk/whirlpool/tests/integration/v2-ix/liquidity/increase_liquidity_v2.test.ts
Show resolved
Hide resolved
legacy-sdk/whirlpool/tests/integration/v2-ix/two-hop-swap/two_hop_swap_v2.test.ts
Show resolved
Hide resolved
f00da43 to
7c72880
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i think i made it through all the files again. a couple last comments, but approving to merge after you address remaining comments at your discretion. maybe the most concerning (b/c i lack context) are the value changes here.
nice work! this is a beast 🐗
| run: yarn install | ||
| - name: Build packages | ||
| run: yarn build --output-style static | ||
| - name: Checkout repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert ws changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yea I thought they had been altered by the yarn format command, but it was just my code editor - will do!
| }); | ||
| } | ||
|
|
||
| export async function initializeLiteSVMEnvironment() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for adding, removes a lot of redundant lines from the code base + review 🙏🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea I agreed that this was the bulk of the entire PR!
| poolInitInfo.whirlpoolPda.publicKey, | ||
| IGNORE_CACHE, | ||
| )) as WhirlpoolData; | ||
| whirlpool = await (async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious - do we need the same for lines 111-119? i see it's doing something similar, i.e. setting emissions to a non-zero value first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! After setting a non-zero emissions value, LiteSVM can also delay the pool update. We can use the same polling pattern there.
| startTickIndex: 25344, | ||
| arrayCount: 12, | ||
| aToB: false, | ||
| }); | ||
|
|
||
| // Add positions | ||
| const fundParams: FundedPositionV2Params[] = [ | ||
| { | ||
| liquidityAmount: new anchor.BN(10_000_000), | ||
| tickLowerIndex: 29440, | ||
| tickUpperIndex: 33536, | ||
| }, | ||
| ]; | ||
| aqConfig.initPositionParams.push({ poolIndex: 0, fundParams }); | ||
| aqConfig.initPositionParams.push({ poolIndex: 1, fundParams }); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason to change some of these values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we change them only to exercise specific scenarios; otherwise we keep them constant
| .getInfos() | ||
| .positions.slice(0, 5) | ||
| .map((p) => p.publicKey); | ||
| const posExt = fixture |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this update got lost amongst all the others? 😅
| const resultAllFalse = { | ||
| positions: new Map(), | ||
| positionsWithTokenExtensions: new Map(), | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think it's worth adding a comment for context?
| const configKey = | ||
| configParams!.configInitInfo.whirlpoolsConfigKeypair.publicKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i vaguely remember that we might've updated configParams to have a default so we didn't need the shebang?
| const actualBump = Array.isArray(whirlpool.whirlpoolBump) | ||
| ? whirlpool.whirlpoolBump[0] | ||
| : (whirlpool.whirlpoolBump as unknown as number); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it verifies the program ignores a caller-supplied bump and uses the correct PDA-derived bump, preventing clients from influencing PDA derivation and ensuring deterministic, secure address generation.
| let _eventVerified = false; | ||
| let _detectedSignature: string | null = null; | ||
| const listener = ctx.program.addEventListener( | ||
| "poolInitialized", | ||
| (event, _slot, signature) => { | ||
| detectedSignature = signature; | ||
| (_event, _slot, signature) => { | ||
| _detectedSignature = signature; | ||
| // verify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like this listener doesn't do anything either? worth removing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes you're right! removing!
6e2ad75 to
14a4a1b
Compare
0dbaff8 to
237eaad
Compare
8627888 to
ab304ec
Compare
Summary
This PR completes the test migration from Anchor-based integration tests to LiteSVM with Vitest across all categories. It removes the old Anchor integration test flow and introduces a single workspace-driven command that runs the LiteSVM test suite.New test command:
yarn workspace @orca-so/whirlpools-sdk-integration testCompletes in 171s!! 🥳🥳

What changed