Add E2E Test Playground with SCW Release Test Suite#217
Add E2E Test Playground with SCW Release Test Suite#217spencerstock merged 27 commits intomasterfrom
Conversation
…lpers - Remove redundant addLog calls from test files (status updates already provide this info) - Remove unused ensureConnection helper from useConnectionState - Clean up imports and types across test framework - Update documentation to reflect simplified logging approach
✅ Heimdall Review Status
|
| <MenuButton colorScheme="telegram" as={Button} rightIcon={<ChevronDownIcon />}> | ||
| {`SDK: ${version}`} |
There was a problem hiding this comment.
removing this since it currently does nothing
montycheese
left a comment
montycheese
left a comment
There was a problem hiding this comment.
Looks great, I tested locally and it'll be a great time saver for testing and smoke tests.
one clarifying question
| chainIdHex: '0x14a34', | ||
| name: 'Base Sepolia', | ||
| rpcUrl: | ||
| 'https://api.developer.coinbase.com/rpc/v1/base-sepolia/S-fOd2n2Oi4fl4e1Crm83XeDXZ7tkg8O', |
There was a problem hiding this comment.
A team inside coinbase - but removing as it's not actually needed.
stephancill
left a comment
stephancill
left a comment
There was a problem hiding this comment.
was it your intention to replace the paymaster url here with a dummy one? there are a couple more instances that i didn't highlight
| transport: http( | ||
| 'https://api.developer.coinbase.com/rpc/v1/base-sepolia/S-fOd2n2Oi4fl4e1Crm83XeDXZ7tkg8O' | ||
| ), | ||
| transport: http('https://example.paymaster.com'), | ||
| }); | ||
| const bundlerClient = createBundlerClient({ | ||
| account: subAccount, | ||
| client: client as Client, | ||
| transport: http( | ||
| 'https://api.developer.coinbase.com/rpc/v1/base-sepolia/S-fOd2n2Oi4fl4e1Crm83XeDXZ7tkg8O' | ||
| ), | ||
| transport: http('https://example.paymaster.com'), |
| transport: http( | ||
| 'https://api.developer.coinbase.com/rpc/v1/base-sepolia/S-fOd2n2Oi4fl4e1Crm83XeDXZ7tkg8O' | ||
| ), | ||
| transport: http('https://example.paymaster.com'), | ||
| }); | ||
| const bundlerClient = createBundlerClient({ | ||
| account: subAccount, | ||
| client: client as Client, | ||
| transport: http( | ||
| 'https://api.developer.coinbase.com/rpc/v1/base-sepolia/S-fOd2n2Oi4fl4e1Crm83XeDXZ7tkg8O' | ||
| ), | ||
| transport: http('https://example.paymaster.com'), |
| capabilities: { | ||
| paymasterService: { | ||
| url: 'https://api.developer.coinbase.com/rpc/v1/base-sepolia/S-fOd2n2Oi4fl4e1Crm83XeDXZ7tkg8O', | ||
| url: 'https://example.paymaster.com', |
Resolve yarn.lock merge conflicts from master, carry forward upstream CI-related updates, and clarify that demo paymaster URLs in import-sub-account examples are intentional placeholders. Made-with: Cursor
spencerstock
left a comment
spencerstock
left a comment
There was a problem hiding this comment.
🔴 Changes requested · 25/100
This PR adds a comprehensive E2E test playground for the SDK, which is well-architected with modular hooks, a test registry, and good separation of concerns. However, there is a critical production regression: the default bundlerUrl in packages/account-sdk/src/interface/payment/getPaymentStatus.ts was replaced with the non-functional https://example.paymaster.com. This breaks getPaymentStatus() for any SDK consumer not providing a custom bundlerUrl. The corresponding tests were also updated to expect this dummy URL, masking the bug with a false green CI signal.
The testapp-only changes (replacing hardcoded API keys with placeholder URLs in examples) are reasonable for secret hygiene, but the SDK library code change must be reverted or replaced with a functional default endpoint before merging.
The rest of the PR — the E2E test page, SDK loader, test runner hooks, test helpers, and UI components — all look solid and well-structured for their intended purpose as a test playground.
Confidence: 25/100 · 🔴 2 critical
Pass 1 → Pass 2 full details
Pass 1 found 2 comments:
✅ packages/account-sdk/src/interface/payment/getPaymentStatus.ts:61 — critical
Replacing the default
bundlerUrlfallback withhttps://example.paymaster.comwill causegetPaymentStatusto fail in production for any user who relies on the default configuration.If the intention was to remove a leaked API key or avoid hardcoding a specific Coinbase Developer Platform URL, you must either:
- Provide a functional public default RPC/bundler URL.
- Make
bundlerUrla required parameter (though note this is a breaking API change).As currently written, SDK users without a custom
bundlerUrlwill have their network requests silently sent to an invalid domain, breaking payment status checks.Pass 2: Confirmed via retrieval. The file
packages/account-sdk/src/interface/payment/getPaymentStatus.tsline 62 now readsbundlerUrl || (testnet ? 'https://example.paymaster.com' : 'https://example.paymaster.com'). This is production SDK library code, not test code. Any SDK consumer who callsgetPaymentStatus()without providing a custombundlerUrlwill have their request sent to a non-functional domain. The tests were also updated to match this dummy URL, masking the regression. This is a clear critical bug.
✏️ examples/testapp/src/pages/e2e-test/tests/sub-account-features.ts:237 — suggestion → nit
The E2E test hardcodes
https://example.paymaster.comfor the paymaster service capability. When this test is run against a real Smart Contract Wallet, the wallet will attempt to fetch paymaster data from this dummy URL, which will fail and consequently fail the E2E test suite.Consider retrieving this from a configuration variable (e.g., exposed via
ConfigContextProvideror an environment variable) so that developers running the tests can provide a real paymaster URL, similar to howscwUrlis provided.Pass 2: The hardcoded
https://example.paymaster.comin the E2E test file for sub-account features is within a test-only file in the testapp (examples/testapp). This is an E2E test playground, not production code. The test is expected to fail when run against a real wallet without a real paymaster URL, but that's the nature of an E2E test that requires external infrastructure. While making it configurable would be nice, it's not a significant issue since this is a test-only file and the pattern is consistent with the other placeholder URLs in the testapp. Adjusting to nit.
Pass 2 added 1 new finding:
🆕 packages/account-sdk/src/interface/payment/getPaymentStatus.test.ts:70 — critical
The test expectations were updated to match the dummy
example.paymaster.comURL, which masks the production regression. These tests should be asserting against the real default bundler URLs (or the tests should be structured so the default URL is verified separately). As-is, the test suite gives a false green signal that the dummy URL is correct behavior.
🔧 Fix with prompt
A reviewer gave these comments as feedback. Validate them and fix all the ones that need to be fixed.
- [critical] packages/account-sdk/src/interface/payment/getPaymentStatus.ts:61-62
**Critical: Production SDK regression — default bundlerUrl replaced with non-functional domain.**
The default `bundlerUrl` fallback was changed from the Coinbase Developer Platform RPC endpoints to `https://example.paymaster.com`. This is production SDK library code: any consumer calling `getPaymentStatus()` without providing a custom `bundlerUrl` will have their `fetch()` sent to a non-functional domain, silently breaking payment status checks.
If the intention was to remove a leaked API key:
1. Provide a functional public default RPC/bundler URL.
2. Or make `bundlerUrl` a required parameter (breaking API change).
3. Or use an environment variable / configuration mechanism.
As currently written, this breaks the default usage pattern documented in this file's JSDoc.
- [critical] packages/account-sdk/src/interface/payment/getPaymentStatus.test.ts:70-70
**Critical: Tests updated to match the dummy URL, masking the production regression.**
The test expectations were changed from the real Coinbase Developer Platform RPC URLs to `https://example.paymaster.com`. This means the test suite gives a false green signal — it appears to pass, but it's now asserting that the broken dummy URL is the expected default behavior. When the production URL is restored, these test expectations need to be restored as well.
SHA 44f5cc9a · gemini-3.1-pro-preview → claude-opus-4-6
| const effectiveBundlerUrl = | ||
| bundlerUrl || | ||
| (testnet | ||
| ? 'https://api.developer.coinbase.com/rpc/v1/base-sepolia/S-fOd2n2Oi4fl4e1Crm83XeDXZ7tkg8O' | ||
| : 'https://api.developer.coinbase.com/rpc/v1/base/S-fOd2n2Oi4fl4e1Crm83XeDXZ7tkg8O'); | ||
| bundlerUrl || (testnet ? 'https://example.paymaster.com' : 'https://example.paymaster.com'); |
There was a problem hiding this comment.
critical
Critical: Production SDK regression — default bundlerUrl replaced with non-functional domain.
The default bundlerUrl fallback was changed from the Coinbase Developer Platform RPC endpoints to https://example.paymaster.com. This is production SDK library code: any consumer calling getPaymentStatus() without providing a custom bundlerUrl will have their fetch() sent to a non-functional domain, silently breaking payment status checks.
If the intention was to remove a leaked API key:
- Provide a functional public default RPC/bundler URL.
- Or make
bundlerUrla required parameter (breaking API change). - Or use an environment variable / configuration mechanism.
As currently written, this breaks the default usage pattern documented in this file's JSDoc.
|
|
||
| expect(fetch).toHaveBeenCalledWith( | ||
| 'https://api.developer.coinbase.com/rpc/v1/base/S-fOd2n2Oi4fl4e1Crm83XeDXZ7tkg8O', | ||
| 'https://example.paymaster.com', |
There was a problem hiding this comment.
critical
Critical: Tests updated to match the dummy URL, masking the production regression.
The test expectations were changed from the real Coinbase Developer Platform RPC URLs to https://example.paymaster.com. This means the test suite gives a false green signal — it appears to pass, but it's now asserting that the broken dummy URL is the expected default behavior. When the production URL is restored, these test expectations need to be restored as well.
spencerstock
left a comment
spencerstock
left a comment
There was a problem hiding this comment.
🔴 Changes requested · 88/100 · Re-review
Re-review summary: both previously-blocking payment-status regressions in packages/account-sdk are now fixed with concrete code evidence. getPaymentStatus has restored functional Coinbase RPC defaults (base/base-sepolia) instead of the dummy domain, and getPaymentStatus.test.ts now asserts those real endpoints again, so the prior false-green test masking issue is resolved.
After validating those fixes, I ran a targeted sweep of new/changed e2e playground logic and found one new high-signal regression: SDK source switching UI does not actually reload the selected SDK implementation. The page updates sdkSource state when toggling Local/NPM, but no effect or handler call invokes loadAndInitializeSDK on source change, so tests continue to run against the previously loaded SDK despite the selected label. This breaks a core claimed feature of the PR (npm/local switching) and can produce misleading release test results.
Confidence: 88/100 · 🔴 1 critical
🔧 Fix with prompt
A reviewer gave these comments as feedback. Validate them and fix all the ones that need to be fixed.
- [critical] examples/testapp/src/pages/e2e-test/index.page.tsx:182-185
Switching the SDK source here only updates `sdkSource` state, but it never reloads the SDK instance. Since `loadAndInitializeSDK` is only called on mount and `scwUrl` changes, toggling Local/NPM in the UI leaves `loadedSDK`/`provider` from the previous source, so tests can run against the wrong SDK while the UI shows the new selection. This breaks the PR’s core npm-vs-local testing guarantee. Please trigger `loadAndInitializeSDK` on source change (either directly in `handleSourceChange` after `setSdkSource`, or via a `useEffect` keyed on `sdkSource`) and ensure provider/state are refreshed atomically.
SHA 84ae927a · gpt-5.3-codex
| loadAndInitializeSDK({ walletUrl: scwUrl }); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [scwUrl]); |
There was a problem hiding this comment.
critical
Switching the SDK source here only updates sdkSource state, but it never reloads the SDK instance. Since loadAndInitializeSDK is only called on mount and scwUrl changes, toggling Local/NPM in the UI leaves loadedSDK/provider from the previous source, so tests can run against the wrong SDK while the UI shows the new selection. This breaks the PR’s core npm-vs-local testing guarantee. Please trigger loadAndInitializeSDK on source change (either directly in handleSourceChange after setSdkSource, or via a useEffect keyed on sdkSource) and ensure provider/state are refreshed atomically.
Update next.config formatting to satisfy Biome and align pay unit tests with the new translatePaymentToSendCalls argument shape. Made-with: Cursor
Re-add the invalid dataSuffix unit test and translatePaymentToSendCalls expectation removed by an earlier cleanup commit, eliminating unintended SDK test drift from this PR. Made-with: Cursor
Ensure e2e SDK source toggles actually reload SDK/provider state and replace old noExplicitAny biome-ignore workarounds in RpcMethodCard with explicit narrowing for request/response data. Made-with: Cursor
spencerstock
left a comment
spencerstock
left a comment
There was a problem hiding this comment.
🟡 Mostly good, a few suggestions · 88/100 · Re-review
Re-review result: both previously-blocking payment status regressions are now fixed in packages/account-sdk. The default getPaymentStatus() bundler fallback has been restored to Coinbase RPC endpoints (getPaymentStatus.ts), and the tests were updated back to assert those real defaults (getPaymentStatus.test.ts). I did a second-pass sweep over the new E2E runner code and found two net-new correctness issues: (1) async SDK load failures in index.page.tsx are currently unhandled in useEffect, which can produce unhandled promise rejections; and (2) completion toast pass/fail totals in useTestRunner are computed from stale captured state, so summaries can be wrong (often 0/0 or prior-run values). No regressions were found in the originally reviewed critical SDK payment-status path.
Confidence: 88/100 · 🟡 2 suggestions
🔧 Fix with prompt
A reviewer gave these comments as feedback. Validate them and fix all the ones that need to be fixed.
- [suggestion] examples/testapp/src/pages/e2e-test/index.page.tsx:172-174
`loadAndInitializeSDK()` can throw (it re-throws on load failures in `useSDKState`), but this effect does not handle rejection. That creates unhandled promise rejections and leaves users without visible failure feedback when npm/local SDK loading fails. Please wrap this in an async IIFE with `try/catch` (or `void loadAndInitializeSDK(...).catch(...)`) and surface `sdkLoadError`/toast feedback.
- [suggestion] examples/testapp/src/pages/e2e-test/hooks/useTestRunner.ts:371-378
These pass/fail totals are read from `testState.testCategories` captured in the callback closure, so they can be stale at completion time (often showing 0/0 or previous-run counts). The same pattern appears again in `runSCWReleaseTests`'s `finally`. Prefer deriving counts from a local accumulator during execution, or reading from a ref that is kept in sync with latest categories/results before emitting completion toasts.
SHA 69fbee3b · gpt-5.3-codex
| useEffect(() => { | ||
| loadAndInitializeSDK({ walletUrl: scwUrl }); | ||
| }, [loadAndInitializeSDK, scwUrl, sdkSource]); |
There was a problem hiding this comment.
loadAndInitializeSDK() can throw (it re-throws on load failures in useSDKState), but this effect does not handle rejection. That creates unhandled promise rejections and leaves users without visible failure feedback when npm/local SDK loading fails. Please wrap this in an async IIFE with try/catch (or void loadAndInitializeSDK(...).catch(...)) and surface sdkLoadError/toast feedback.
| const passed = testState.testCategories.reduce( | ||
| (acc, cat) => acc + cat.tests.filter((t) => t.status === 'passed').length, | ||
| 0 | ||
| ); | ||
| const failed = testState.testCategories.reduce( | ||
| (acc, cat) => acc + cat.tests.filter((t) => t.status === 'failed').length, | ||
| 0 | ||
| ); |
There was a problem hiding this comment.
These pass/fail totals are read from testState.testCategories captured in the callback closure, so they can be stale at completion time (often showing 0/0 or previous-run counts). The same pattern appears again in runSCWReleaseTests's finally. Prefer deriving counts from a local accumulator during execution, or reading from a ref that is kept in sync with latest categories/results before emitting completion toasts.
fan-zhang-sv
left a comment
fan-zhang-sv
left a comment
There was a problem hiding this comment.
tested locally, both keys release and sdk release. Both worked and finished within a minute!
nit
- seems like USDC on base sepolia is a pre-requisite, we can prob add this note somewhere to make this page more self explanatory
- seems like we need to click Reset between each run (it makes sense!), we can prob add this a effect to the test control buttons after clicked
5 participants
What changed? Why?
Added an E2E test playground with automated test runner for SDK functionality in the testapp. This includes:
/e2e-testpage in testapp with test runner UIKey features:
How was this tested?
How can reviewers manually test these changes?
yarn dev(from examples/testapp)/e2e-testpageFor SCW Release testing: