From 77a229018a6ace7eed5ad854b7b93ec0a5ea2317 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 2 Mar 2026 05:59:21 +0000 Subject: [PATCH] fix: address review feedback on holdout helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract rpcCall into helpers/rpc.ts to eliminate the duplicate copy in wallet.ts and assertions.ts (warning: code duplication) - Fix waitForReceipt() in swap.ts to assert receipt.status === '0x1': reverted transactions (status 0x0) now throw immediately with a clear message instead of letting sellAllKrk silently succeed and fail later at the balance assertion (bug) - Add screen.width debug log to connectWallet() before the isVisible check, restoring the regression signal from always-leave.spec.ts (warning) - Fix expectPoolHasLiquidity() to only assert sqrtPriceX96 > 0 (pool is initialised); drop the active-tick liquidity() check which gives false negatives when price moves outside all LiquidityManager ranges after a sovereign exit (warning) - Add WETH balance snapshot before/after the swap in sellAllKrk() and log a warning when WETH output is 0, making pool health degradation visible despite amountOutMinimum: 0n (warning) - Add before/after screenshots in buyKrk() (holdout-before-buy.png, holdout-after-buy.png) to restore CI debugging artefacts (nit) - Move waitForTimeout(2_000) settle buffer in buyKrk() to the catch path only; when the Submitting→idle transition is observed the extra wait is redundant (nit) Co-Authored-By: Claude Sonnet 4.6 --- scripts/harb-evaluator/helpers/assertions.ts | 33 +++-------- scripts/harb-evaluator/helpers/rpc.ts | 17 ++++++ scripts/harb-evaluator/helpers/swap.ts | 58 ++++++++++++++------ scripts/harb-evaluator/helpers/wallet.ts | 17 ++---- 4 files changed, 70 insertions(+), 55 deletions(-) create mode 100644 scripts/harb-evaluator/helpers/rpc.ts diff --git a/scripts/harb-evaluator/helpers/assertions.ts b/scripts/harb-evaluator/helpers/assertions.ts index 5ce3ec7..e4a5296 100644 --- a/scripts/harb-evaluator/helpers/assertions.ts +++ b/scripts/harb-evaluator/helpers/assertions.ts @@ -6,20 +6,10 @@ */ import { expect } from '@playwright/test'; import { Interface } from 'ethers'; +import { rpcCall } from './rpc'; // ── Internal helpers ───────────────────────────────────────────────────────── -async function rpcCall(rpcUrl: string, method: string, params: unknown[]): Promise { - const resp = await fetch(rpcUrl, { - method: 'POST', - headers: { 'content-type': 'application/json' }, - body: JSON.stringify({ jsonrpc: '2.0', id: Date.now(), method, params }), - }); - const payload = await resp.json(); - if (payload.error) throw new Error(`RPC ${method}: ${payload.error.message}`); - return payload.result; -} - async function getTokenBalance(rpcUrl: string, token: string, address: string): Promise { if (token.toLowerCase() === 'eth') { const result = (await rpcCall(rpcUrl, 'eth_getBalance', [address, 'latest'])) as string; @@ -33,7 +23,6 @@ async function getTokenBalance(rpcUrl: string, token: string, address: string): const POOL_ABI = [ 'function slot0() external view returns (uint160 sqrtPriceX96, int24 tick, uint16 observationIndex, uint16 observationCardinality, uint16 observationCardinalityNext, uint8 feeProtocol, bool unlocked)', - 'function liquidity() external view returns (uint128)', ]; const poolIface = new Interface(POOL_ABI); @@ -58,25 +47,17 @@ export async function expectBalanceIncrease( } /** - * Assert that the Uniswap V3 pool at `poolAddress` is initialised and has - * non-zero active liquidity at the current tick. + * Assert that the Uniswap V3 pool at `poolAddress` is initialised. * - * Used after a sovereign exit to confirm the pool remains functional and the - * LiquidityManager's positions are intact. + * Checks that sqrtPriceX96 > 0, which confirms the pool has been seeded and + * can execute swaps. Active-tick liquidity is intentionally not checked here: + * after a sovereign exit the LiquidityManager's three range positions (Floor, + * Anchor, Discovery) may all sit outside the current tick while the pool + * itself remains functional. */ export async function expectPoolHasLiquidity(rpcUrl: string, poolAddress: string): Promise { - // slot0() — check pool is initialised (sqrtPriceX96 > 0) const slot0Encoded = poolIface.encodeFunctionData('slot0', []); const slot0Hex = (await rpcCall(rpcUrl, 'eth_call', [{ to: poolAddress, data: slot0Encoded }, 'latest'])) as string; const [sqrtPriceX96] = poolIface.decodeFunctionResult('slot0', slot0Hex); expect(sqrtPriceX96).toBeGreaterThan(0n); - - // liquidity() — active liquidity at the current tick must be non-zero - const liquidityEncoded = poolIface.encodeFunctionData('liquidity', []); - const liquidityHex = (await rpcCall(rpcUrl, 'eth_call', [ - { to: poolAddress, data: liquidityEncoded }, - 'latest', - ])) as string; - const [liquidity] = poolIface.decodeFunctionResult('liquidity', liquidityHex); - expect(liquidity).toBeGreaterThan(0n); } diff --git a/scripts/harb-evaluator/helpers/rpc.ts b/scripts/harb-evaluator/helpers/rpc.ts new file mode 100644 index 0000000..cb18bac --- /dev/null +++ b/scripts/harb-evaluator/helpers/rpc.ts @@ -0,0 +1,17 @@ +/** + * Shared JSON-RPC utility for holdout helpers. + * + * Exported from one place so wallet.ts, assertions.ts, and future helpers + * share a single implementation rather than embedding the same fetch + + * error-check block in each file. + */ +export async function rpcCall(rpcUrl: string, method: string, params: unknown[]): Promise { + const resp = await fetch(rpcUrl, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ jsonrpc: '2.0', id: Date.now(), method, params }), + }); + const payload = await resp.json(); + if (payload.error) throw new Error(`RPC ${method}: ${payload.error.message}`); + return payload.result; +} diff --git a/scripts/harb-evaluator/helpers/swap.ts b/scripts/harb-evaluator/helpers/swap.ts index 16378e3..7670daa 100644 --- a/scripts/harb-evaluator/helpers/swap.ts +++ b/scripts/harb-evaluator/helpers/swap.ts @@ -9,7 +9,7 @@ import type { Page } from '@playwright/test'; import { expect } from '@playwright/test'; import { Interface } from 'ethers'; import { navigateSPA } from '../../../tests/setup/navigate'; -import { getKrkBalance } from './wallet'; +import { rpcCall } from './rpc'; // Infrastructure addresses stable across Anvil forks of Base Sepolia const SWAP_ROUTER = '0x94cC0AaC535CCDB3C01d6787D6413C739ae12bc4'; @@ -23,25 +23,33 @@ const ROUTER_ABI = [ // ── Internal helpers ───────────────────────────────────────────────────────── +/** Read an ERC-20 balanceOf in the Node.js context via direct RPC. */ +async function erc20BalanceOf(rpcUrl: string, tokenAddress: string, account: string): Promise { + const selector = '0x70a08231'; // balanceOf(address) + const data = selector + account.slice(2).padStart(64, '0'); + return BigInt((await rpcCall(rpcUrl, 'eth_call', [{ to: tokenAddress, data }, 'latest'])) as string); +} + /** * Poll eth_getTransactionReceipt until the tx is mined or maxAttempts exceeded. * Anvil with automine resolves almost immediately; the loop guards against * instances configured with a block interval or high RPC latency. + * + * Throws if the transaction was mined but reverted (status 0x0) so callers + * get a clear failure rather than a confusing downstream balance-assertion error. */ async function waitForReceipt(rpcUrl: string, txHash: string, maxAttempts = 20): Promise { for (let i = 0; i < maxAttempts; i++) { - const resp = await fetch(rpcUrl, { - method: 'POST', - headers: { 'content-type': 'application/json' }, - body: JSON.stringify({ - jsonrpc: '2.0', - id: Date.now(), - method: 'eth_getTransactionReceipt', - params: [txHash], - }), - }); - const payload = await resp.json(); - if (payload.result !== null) return; + const receipt = (await rpcCall(rpcUrl, 'eth_getTransactionReceipt', [txHash])) as Record< + string, + unknown + > | null; + if (receipt !== null) { + if (receipt.status === '0x0') { + throw new Error(`Transaction ${txHash} reverted (status 0x0)`); + } + return; // status === '0x1' — success + } await new Promise(r => setTimeout(r, 500)); } throw new Error(`Transaction ${txHash} not mined after ${maxAttempts * 500}ms`); @@ -50,7 +58,7 @@ async function waitForReceipt(rpcUrl: string, txHash: string, maxAttempts = 20): // ── Public config type ─────────────────────────────────────────────────────── export interface SellConfig { - /** Anvil JSON-RPC endpoint (used to wait for receipt and query KRK balance). */ + /** Anvil JSON-RPC endpoint (used to wait for receipt and query token balances). */ rpcUrl: string; /** Deployed KRAIKEN (KRK) ERC-20 contract address. */ krkAddress: string; @@ -80,6 +88,8 @@ export async function buyKrk(page: Page, ethAmount: string): Promise { const buyButton = page.getByRole('button', { name: 'Buy KRK' }); await expect(buyButton).toBeVisible({ timeout: 5_000 }); + + await page.screenshot({ path: 'test-results/holdout-before-buy.png' }); console.log('[swap] Clicking Buy KRK...'); await buyButton.click(); @@ -90,9 +100,12 @@ export async function buyKrk(page: Page, ethAmount: string): Promise { await page.getByRole('button', { name: 'Buy KRK' }).waitFor({ state: 'visible', timeout: 60_000 }); console.log('[swap] Swap completed'); } catch { + // Swap completed before the Submitting state could be observed console.log('[swap] Button state not observed (swap may have completed instantly)'); + await page.waitForTimeout(2_000); } - await page.waitForTimeout(2_000); + + await page.screenshot({ path: 'test-results/holdout-after-buy.png' }); } /** @@ -102,13 +115,19 @@ export async function buyKrk(page: Page, ethAmount: string): Promise { * * This is the "sovereign exit" path — it bypasses the UI swap widget and * sends transactions directly so the test is not gated on the sell-side UI. + * + * Logs a warning if the WETH balance does not increase after the swap, which + * indicates the pool returned 0 output (possible with amountOutMinimum: 0n on + * a partially-drained pool). */ export async function sellAllKrk(page: Page, config: SellConfig): Promise { - const krkBalance = await getKrkBalance(config.rpcUrl, config.krkAddress, config.accountAddress); + const krkBalance = await erc20BalanceOf(config.rpcUrl, config.krkAddress, config.accountAddress); if (krkBalance === 0n) throw new Error('sellAllKrk: KRK balance is 0 — nothing to sell'); console.log(`[swap] Selling ${krkBalance} KRK...`); + const wethBefore = await erc20BalanceOf(config.rpcUrl, WETH, config.accountAddress); + const erc20Iface = new Interface(ERC20_ABI); const routerIface = new Interface(ROUTER_ABI); @@ -154,4 +173,11 @@ export async function sellAllKrk(page: Page, config: SellConfig): Promise ); await waitForReceipt(config.rpcUrl, swapTxHash); console.log('[swap] Swap mined'); + + const wethAfter = await erc20BalanceOf(config.rpcUrl, WETH, config.accountAddress); + if (wethAfter <= wethBefore) { + console.warn('[swap] WARNING: WETH balance did not increase after sell — pool may have returned 0 output'); + } else { + console.log(`[swap] Received ${wethAfter - wethBefore} WETH`); + } } diff --git a/scripts/harb-evaluator/helpers/wallet.ts b/scripts/harb-evaluator/helpers/wallet.ts index 1b004c7..ddd1e9d 100644 --- a/scripts/harb-evaluator/helpers/wallet.ts +++ b/scripts/harb-evaluator/helpers/wallet.ts @@ -7,19 +7,7 @@ */ import type { Page } from '@playwright/test'; import { expect } from '@playwright/test'; - -// ── RPC helpers ───────────────────────────────────────────────────────────── - -async function rpcCall(rpcUrl: string, method: string, params: unknown[]): Promise { - const resp = await fetch(rpcUrl, { - method: 'POST', - headers: { 'content-type': 'application/json' }, - body: JSON.stringify({ jsonrpc: '2.0', id: Date.now(), method, params }), - }); - const payload = await resp.json(); - if (payload.error) throw new Error(`RPC ${method}: ${payload.error.message}`); - return payload.result; -} +import { rpcCall } from './rpc'; // ── Balance readers ────────────────────────────────────────────────────────── @@ -51,6 +39,9 @@ export async function connectWallet(page: Page): Promise { await page.evaluate(() => window.dispatchEvent(new Event('resize'))); await page.waitForTimeout(2_000); + const screenWidth = await page.evaluate(() => window.screen.width); + console.log(`[wallet] screen.width = ${screenWidth}`); + let panelOpened = false; const connectButton = page.locator('.connect-button--disconnected').first();