Repository navigation
Swaps e2e tests - #850
Swaps e2e tests#850
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying wallet-bitcoin with
|
| Latest commit: |
23a6bb9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f86277ed.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://swaps-e2e-tests.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
23a6bb9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5304bd39.arkade-wallet.pages.dev |
| Branch Preview URL: | https://swaps-e2e-tests.arkade-wallet.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build-markets-json.mjs`:
- Line 10: Update the build-markets script to create each registry’s parent
directory before calling writeFile, using recursive directory creation so clean
runs succeed when gitignored directories are absent.
In `@src/test/e2e/boltz.test.ts`:
- Around line 161-162: Await the exec call that cancels the invoice in the test
flow before submitting the swap or starting payment processing. Update the
surrounding cancellation step in boltz.test.ts so the subsequent assertion
reaches Payment failed only after cancelinvoice has completed.
In `@src/test/e2e/swaps.test.ts`:
- Line 16: Await each receive-drawer click before selecting its asset in the
swaps e2e test: update the clicks at src/test/e2e/swaps.test.ts lines 16, 46,
and 85 to use awaited Locator.click() calls, preserving the existing selection
flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6734d112-6f01-445c-9279-2de70d0a466c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.env.regtest.gitignorepackage.jsonregtestscripts/build-markets-json.mjssrc/lib/constants.tssrc/providers/assets.tsxsrc/screens/Wallet/Swap/Index.tsxsrc/test/e2e/boltz.test.tssrc/test/e2e/swaps.test.tssrc/test/e2e/utils.ts
💤 Files with no reviewable changes (1)
- .env.regtest
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build-markets-json.mjs`:
- Line 10: Update the build-markets script to create each registry’s parent
directory before calling writeFile, using recursive directory creation so clean
runs succeed when gitignored directories are absent.
In `@src/test/e2e/boltz.test.ts`:
- Around line 161-162: Await the exec call that cancels the invoice in the test
flow before submitting the swap or starting payment processing. Update the
surrounding cancellation step in boltz.test.ts so the subsequent assertion
reaches Payment failed only after cancelinvoice has completed.
In `@src/test/e2e/swaps.test.ts`:
- Line 16: Await each receive-drawer click before selecting its asset in the
swaps e2e test: update the clicks at src/test/e2e/swaps.test.ts lines 16, 46,
and 85 to use awaited Locator.click() calls, preserving the existing selection
flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6734d112-6f01-445c-9279-2de70d0a466c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.env.regtest.gitignorepackage.jsonregtestscripts/build-markets-json.mjssrc/lib/constants.tssrc/providers/assets.tsxsrc/screens/Wallet/Swap/Index.tsxsrc/test/e2e/boltz.test.tssrc/test/e2e/swaps.test.tssrc/test/e2e/utils.ts
💤 Files with no reviewable changes (1)
- .env.regtest
🛑 Comments failed to post (3)
scripts/build-markets-json.mjs (1)
10-10: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== files ==" git ls-files | sed -n '1,120p' || true echo "== target script ==" if [ -f scripts/build-markets-json.mjs ]; then nl -ba scripts/build-markets-json.mjs | sed -n '1,90p' else fd -a 'build-markets-json\.mjs$' . fi echo "== gitignore generated registry rules ==" rg -n "asset-registry|solver-registry|regtest\.json|public/" .gitignore **/.gitignore 2>/dev/null | sed -n '1,120p' || true echo "== package scripts for regtest:start ==" if [ -f package.json ]; then node - <<'JS' const fs = require('fs'); const pkg = JSON.parse(fs.readFileSync('package.json','utf8')); console.log(JSON.stringify(pkg.scripts || {}, null, 2)); JS fi echo "== repo references to registry paths ==" rg -n "asset-registry|solver-registry|regtest\.json|build-markets-json|regtest:start" -S . | sed -n '1,200p' || trueRepository: arkade-os/wallet
Length of output: 3241
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== target script with line numbers ==" awk '{printf "%6d\t%s\n", NR, $0}' scripts/build-markets-json.mjs | sed -n '1,120p' echo "== gitignore generated registry rules ==" awk '{printf "%6d\t%s\n", NR, $0}' .gitignore | rg -n "asset-registry|solver-registry|regtest\.json|public/" -C 2 || true echo "== package scripts ==" node - <<'JS' const fs = require('fs'); const pkg = JSON.parse(fs.readFileSync('package.json','utf8')); console.log(JSON.stringify(pkg.scripts || {}, null, 2)); JS echo "== Node writeFile parent-directory behavior probe ==" node - <<'JS' const { mkdir, symlink, rm, writeFile, access } = require('node:fs/promises'); async function probe() { const work = '/tmp/coderabbit-probe-no-parent-registry-' + process.hrtime.bigint().toString(); const file = work + '/public/solver-registry/regtest.json'; try { await rm(work, { recursive: true, force: true }); await writeFile(file, '{}'); const ok = await access(file); console.log('unexpected-write-success', true); } catch (err) { console.log('write-fails-with-missing-parent', err.code === 'ENOENT'); } await rm(work, { recursive: true, force: true }); } probe().then(() => process.exit(0)).catch(err => { console.log(err); process.exit(1); }); JS echo "== references to registry paths and command ==" rg -n "asset-registry|solver-registry|regtest\.json|build-markets-json|regtest:start" -S . || trueRepository: arkade-os/wallet
Length of output: 6563
Create the registry directories before writing.
fs/promises.writeFiledoes not create missing parent directories, and the JSON files are gitignored. Clean run-after-starts can fail atpnpm build:marketswithENOENTbefore the registries are generated.🛠️ Proposed fix
-import { writeFile } from 'node:fs/promises' +import { mkdir, writeFile } from 'node:fs/promises' async function main() { const solverCard = await fetchJson(options.solverApiUrl) const normalized = normalizeCard(solverCard) + await mkdir('./public/solver-registry', { recursive: true }) + await mkdir('./public/asset-registry', { recursive: true }) + const solverRegistry = JSON.stringify(normalized, null, 2)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build-markets-json.mjs` at line 10, Update the build-markets script to create each registry’s parent directory before calling writeFile, using recursive directory creation so clean runs succeed when gitignored directories are absent.Source: MCP tools
src/test/e2e/boltz.test.ts (1)
161-162: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Await invoice cancellation before starting the payment flow.
The unawaited
exec()races the cancellation against the swap submission, so this test can intermittently settle successfully instead of reachingPayment failed.Proposed fix
- exec(`docker exec lnd lncli --network=regtest cancelinvoice ${hash}`) + await execAsync(`docker exec lnd lncli --network=regtest cancelinvoice ${hash}`)📝 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.// cancel invoice to make the swap fail await execAsync(`docker exec lnd lncli --network=regtest cancelinvoice ${hash}`)🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 162-162: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/e2e/boltz.test.ts` around lines 161 - 162, Await the exec call that cancels the invoice in the test flow before submitting the swap or starting payment processing. Update the surrounding cancellation step in boltz.test.ts so the subsequent assertion reaches Payment failed only after cancelinvoice has completed.src/test/e2e/swaps.test.ts (1)
16-16: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -nP '^\s*(?!await\s+)page\.locator\(.+?\)\.first\(\)\.click\(\)\s*$' src/test/e2e/swaps.test.tsRepository: arkade-os/wallet
Length of output: 322
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "File outline and relevant snippets:" wc -l src/test/e2e/swaps.test.ts sed -n '1,110p' src/test/e2e/swaps.test.ts echo echo "Package Playwright type check context:" if [ -f package.json ]; then jq -r '.scripts, .devDependencies, .dependencies' package.json fi echo echo "Read-only semantic check for unawaited Playwright Locator click statements:" python3 - <<'PY' import re from pathlib import Path path = Path("src/test/e2e/swaps.test.ts") text = path.read_text() lines = text.splitlines() pat = re.compile(r"^\s*page\.locator\([^)]*\)\.first\(\)\.click\(\)\s*$") for i, line in enumerate(lines, start=1): if pat.match(line): print(f"{path}:{i}: unawaited Locator.click()") PYRepository: arkade-os/wallet
Length of output: 8656
Await each receive-drawer click before choosing its asset.
Each
Locator.click()returns a Promise in Playwright; withoutawait, the test can proceed to the asset selection before the click is settled, making the flow nondeterministic.Proposed fix
- page.locator('.swap-receive-card').first().click() + await page.locator('.swap-receive-card').first().click()📝 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.await page.locator('.swap-receive-card').first().click()📍 Affects 1 file
src/test/e2e/swaps.test.ts#L16-L16(this comment)src/test/e2e/swaps.test.ts#L46-L46src/test/e2e/swaps.test.ts#L85-L85🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/e2e/swaps.test.ts` at line 16, Await each receive-drawer click before selecting its asset in the swaps e2e test: update the clicks at src/test/e2e/swaps.test.ts lines 16, 46, and 85 to use awaited Locator.click() calls, preserving the existing selection flow.
|
@copilot review |
Addressed one regression found during review: |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review at 32665e1. PR is already merged; leaving notes for follow-up. No protocol-critical code changed — this is test/dev-tooling only.
Real bugs
src/test/e2e/swaps.test.ts — missing await on .swap-receive-card clicks (race).
Three of the four .swap-receive-card clicks are un-awaited, one is awaited — inconsistent within the same file:
- L17
page.locator('.swap-receive-card').first().click()— no await - L46
page.locator('.swap-receive-card').first().click()— no await - L83
page.locator('.swap-receive-card').first().click()— no await - L110
await page.locator('.swap-receive-card').first().click()— awaited
Locator.click() returns a Promise. Without await, the next page.getByTestId('swap-asset-row-…').click() can race the drawer-open animation and fail intermittently in CI. Fix: await all four.
scripts/build-markets-json.mjs — silent crash paths.
- L57
normalized.markets[0].quote_asset.idthrowsTypeError: Cannot read properties of undefinedif the card has no markets or the first market lacksquote_asset. Sinceregtest:startchains&& pnpm build:markets, a solver returning an empty card fails the whole start with an unhelpful error. Guard it and print something actionable. - L14 solver name
frenchmanis hardcoded. If the regtest solver changes its name, the script 404s from/v1/card?name=frenchmanand startup fails. Consider?name=(if the API supports first-of-list) or wiring the name through an env var. - L28
JSON.stringify(obj).replaceAll('http://pricefeed', 'http://localhost:8088')will happily rewrite any string in the payload that contains the substringhttp://pricefeed, not just the intended URL fields. Low risk today but fragile; iterate over the known URL fields instead.
Minor
src/test/e2e/utils.tsL380navigateToSwapsusestimeout: 2000forwaitForSelector('text=Choose asset to swap'). Every other navigate helper here uses 30–60s. On a cold-loaded wallet or slow CI this will flake; bump to 30000 to match neighbours..env.regtestdropsARKD_VTXO_TREE_EXPIRY=5120andARKD_BOARDING_EXIT_DELAY=7200. Confirm the bumpedregtestsubmodule (7784f5cd) sets equivalent defaults — otherwise timing-sensitive e2e tests (unilateral exit, boarding) may behave differently across contributors' machines vs. CI..env.regtestno longer ends with a newline — some editors/tools complain.src/lib/constants.tsL49 hardcodeshttp://localhost:3002for the regtest solver-registry URL. That's the wallet dev-server port; anyone runningvite --porton a different port silently loses the registry. A comment noting the coupling to the dev server would help.src/screens/Wallet/Swap/Index.tsxL856data-testid={\swap-asset-row-${asset.ticker.toLowerCase()}`}` — collides if two assets share a ticker (case-insensitive). Fine for the current fixtures, worth remembering when more assets are added.
Untrusted-content check
Nothing prompt-injection-shaped in the PR title/body/diff.
`getCovclaimdPubkeyForNetwork` now returns `undefined` where no covclaimd is configured, which only works against a `@arkade-os/swap` that skips the seal. ts-sdk#850 does that and merged as d652ab6d; no published version has it. Published 0.0.12 seals unconditionally, so the `undefined` would reach `sealWithEntropy`'s `input.covclaimdPubkey.length` and throw on every Lightning receive on bitcoin, signet and testnet -- strictly worse than the silent guarantee-loss of #952 this fixes. That is why the PR sat in draft. Two tarballs, not one. The swap build at d652ab6d imports `assertNoAssets` from `@arkade-os/sdk` in five places and published 0.4.69 exports it nowhere, so vendoring swap alone fails to load 44 test files. Both come from the same commit. Both pin through `pnpm.overrides` as well as the direct dependency, because each tarball carries the SAME version string as the published-but-different build it replaces -- #850 and the sdk change both landed without a version bump. A bare specifier resolves silently against the registry copy, which is the trap the earlier solver-discovery vendoring hit in 24b39ab. Temporary. Delete `vendor/` and restore the plain specifiers once both are published.
Summary by CodeRabbit
New Features
Bug Fixes
Tests