e2e: Add test for testing different address types#470
e2e: Add test for testing different address types#470Abhay349 wants to merge 1 commit intocaravan-bitcoin:mainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Legend101Zz
left a comment
Legend101Zz
left a comment
There was a problem hiding this comment.
just a minor comment , and rest it looks really good to me , can you please also attach a screenshot of the working e2e test ? Thanks
| { | ||
| type: "P2SH-P2WSH", | ||
| label: "Nested SegWit (P2SH-P2WSH)", | ||
| regex: /^2[MN][a-zA-Z0-9]{33}$/ |
There was a problem hiding this comment.
question here , Base58 addresses (like P2SH) are not fixed length. While they are usually 35 characters, they can technically be 33 or 34 characters depending on the number of leading zeros in the hash. maybe Loosen the length requirement and refine the allowed characters ?
There was a problem hiding this comment.
I agree and that makes sense, I will loosen regex to avoid enforcing a strict length.
I’m planning to change it to something like:
/^2[1-9A-HJ-NP-Za-km-z]{30,40}$/so that we can validate structure without assuming an exact length, let me know if it looks good.
Thanks for guidance :)
|
|
Hi @Legend101Zz I have attached screenshots of working e2e tests and also updated the regex for P2SH-P2WSH. |
|
Hi @Abhay349 I’m currently working on resolving that. Until the fix is implemented and merged, I’ll hold off on reviewing this PR since the changes from that fix will likely require some adjustments to your code as well. Thanks a lot for your patience, and really appreciate the contribution! |
|
Hi @Abhay349 Thanks for your patience. Could you please pull in the latest changes and review the updated E2E setup we’ve implemented, then adjust your changes accordingly? |
Signed-off-by: Abhay349 <pandeyabhay967@gmail.com>
|
Hi @bucko13 @Legend101Zz I have pushed a fix addressing updates, pls take a look and let me know if I can proceed for similer changes in other PRs.. |
There was a problem hiding this comment.
Pull request overview
Adds a new Playwright E2E spec to validate Caravan Coordinator behavior across multiple multisig wallet address types (currently P2WSH + P2SH-P2WSH, with P2TR scaffolded but skipped), expanding coverage beyond the default address type.
Changes:
- Introduces a parameterized E2E test that imports a wallet config per address type and verifies the generated receive address format.
- Adds a flow to send funds to the generated address and assert the receive-path index advances.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // Send 1 BTC | ||
| await btcClient?.sendToAddress(senderWallet, address, 1); | ||
|
|
||
| // Wait for the UI to reflect new tx / increment path | ||
| await expect.poll(async () => { | ||
| return await receiveTab.getCurrentPathSuffix(); | ||
| }, { | ||
| message: "Address path should increment after receiving funds", | ||
| timeout: 30000, | ||
| intervals: [1000] | ||
| }).not.toBe(initialSuffix); |
There was a problem hiding this comment.
After sendToAddress, the test doesn’t mine a block or trigger a UI refresh. Other e2e flows mine blocks and call walletNav.refresh() to ensure Bitcoin Core reports the UTXO and Caravan updates; without this, the path suffix may never change and the poll can time out. Also, btcClient is a required fixture, so avoid optional chaining here to fail loudly if it’s ever missing.
| }).not.toBe(initialSuffix); | ||
|
|
||
| } catch (error) { | ||
| throw new Error(`Failed testing ${type}: ${error}`); |
There was a problem hiding this comment.
The try/catch rethrows a new Error using ${error}, which drops the original stack and can hide Playwright assertion details. Prefer letting the original error bubble up, or rethrow while preserving cause (and use error instanceof Error ? error.message : String(error) for the message).
| throw new Error(`Failed testing ${type}: ${error}`); | |
| if (error instanceof Error) { | |
| throw new Error(`Failed testing ${type}: ${error.message}`, { cause: error }); | |
| } | |
| throw new Error(`Failed testing ${type}: ${String(error)}`); |
| { | ||
| type: "P2TR", | ||
| label: "Taproot (P2TR)", | ||
| regex: /^bcrt1p.{50,}$/, |
There was a problem hiding this comment.
The (currently skipped) P2TR address regex is very permissive (.{50,}) and could pass invalid addresses once the test is enabled. It would be safer to constrain it to the bech32m charset/length expected on regtest (e.g., lowercase alphanumerics with a fixed length).
| regex: /^bcrt1p.{50,}$/, | |
| regex: /^bcrt1p[023456789acdefghjklmnpqrstuvwxyz]{52}$/, |
| test.beforeEach(async ({ page }) => { | ||
| // Ensure we start with a slight delay to allow any previous cleanup | ||
| await page.waitForTimeout(1000); | ||
| }); |
There was a problem hiding this comment.
The fixed waitForTimeout(1000) introduces an unconditional 1s delay per test run and can hide real synchronization issues. Prefer waiting on a specific UI condition (e.g., tab button visible) or remove this delay if it’s not strictly required.
| test.beforeEach(async ({ page }) => { | |
| // Ensure we start with a slight delay to allow any previous cleanup | |
| await page.waitForTimeout(1000); | |
| }); |
| // 1. Get Base Config | ||
| const downloadedWalletFile = testStateManager.getDownloadedWalletFile(); | ||
| if (!fs.existsSync(downloadedWalletFile)) { | ||
| throw new Error(`Wallet file not found at ${downloadedWalletFile}. Please run previous tests.`); | ||
| } | ||
|
|
||
| const walletConfig = JSON.parse(fs.readFileSync(downloadedWalletFile, "utf-8")); | ||
|
|
There was a problem hiding this comment.
This test re-implements wallet-config precondition checks (existsSync + JSON.parse) inline. The e2e suite already has assertWalletFileDownloaded / assertModifiedWalletConfig helpers to fail fast with consistent diagnostics—consider using one of those in a beforeAll and then just read the file path here.
| const tempConfigPath = path.join( | ||
| path.dirname(downloadedWalletFile), | ||
| `wallet_config_${type}.json` | ||
| ); | ||
| fs.writeFileSync(tempConfigPath, JSON.stringify(walletConfig, null, 2)); | ||
|
|
There was a problem hiding this comment.
The temporary config file is written next to the downloaded wallet file and never cleaned up. Over time this can clutter the shared download dir and make debugging harder; consider writing it to the test state upload/temp dir and deleting it in a finally/afterEach.
| await page.locator("button[role=tab][type=button]:has-text('Receive')").click(); | ||
| const address = await receiveTab.getCurrentAddress(); |
There was a problem hiding this comment.
This test clicks the Receive tab using a raw selector even though a walletNav fixture exists with switchToTab(). Using the page object keeps selectors centralized and reduces flakiness if the UI markup changes.
What kind of change does this PR introduce?
Issue Number:
Fixes subtask of #442
If relevant, did you update the documentation?
Summary
This PR adds, parameterized end-to-end tests to validate different wallet address types in the Caravan coordinator.
Previously, E2E tests mainly covered the default address behavior. This change ensures comprehensive coverage for:
P2WSH (Native SegWit): Verifies correct address generation (bcrt1q...) and fund reception.
P2SH-P2WSH (Nested SegWit): Verifies correct address generation (2...) and fund reception.
P2TR (Taproot): Test scaffold included but currently skipped.
For each active type, the test validates:
Does this PR introduce a breaking change?
-No
Checklist
npm run changeset)Other information
P2TR (Taproot) skipped: The P2TR test case is explicitly marked as
skip: true. Investigation intocaravan-bitcoin/src/types/addresses.tsrevealed that while P2TR is defined as a validMultisigAddressType, it is currently commented as "not able to be used anywhere yet". The test is preserved to enable immediate verification once support is fully implemented in the underlying package.
All coordinator E2E tests (P2WSH and P2SH-P2WSH) pass locally using the Docker-based setup.
Have you read the contributing guide?
Yes