Skip to content

feat: cross flow wallet auth#18

Open
cyaiox wants to merge 6 commits into
mainfrom
feat/at-cross-flow-1-wallet-handoff
Open

feat: cross flow wallet auth#18
cyaiox wants to merge 6 commits into
mainfrom
feat/at-cross-flow-1-wallet-handoff

Conversation

@cyaiox

@cyaiox cyaiox commented May 22, 2026

Copy link
Copy Markdown
Member

Summary

Adds end-to-end cross-platform (@cross) coverage for the auth handoff between the Decentraland web dapp and the Unity desktop client — in both directions — plus the runner infrastructure that lets a single Playwright session orchestrate staged dotnet test assertions against a long-running, AltTester-instrumented Explorer.

The two stacks never call each other directly. They communicate only through flat files on disk (the same contract the real launcher/client use), so the C#/.NET and TS/Playwright suites stay fully decoupled.

Flow 1 — client → web wallet device-pairing (explorer-wallet-login.spec.ts)

Verifies the wallet sign-in a user starts inside the client and completes in the browser:

  1. C# WalletLoginCapture.TestCaptureWalletAuthHandoff clicks Metamask on Unity's auth screen. The client's auth-api round-trip ends in Application.OpenURL, which the #if ALTTESTER hook in UnityAppWebBrowser intercepts to write auth-url.txt instead of opening the system browser.
  2. In parallel, Playwright signs up a fresh DCL profile on /auth/login (existing-profile path via AUTH_TEST_WALLET_PRIVATE_KEY, or fresh-wallet + QuickSetup signup as a clean-env fallback) so the identity exists by the time we reach the requests page.
  3. Playwright reads auth-url.txt, derives requestId from the redirectTo query param, and navigates to /auth/requests/<id>?targetConfigId=default. That connection makes auth-api emit request-validation-status, which activates Unity's Verification.Dapp.Screen with the validation code.
  4. Device-pairing safety check (the real product feature): C# WalletLoginCodeRead.TestReadVerificationCode reads Unity's code and writes it to disk; Playwright deep-scans the verify-sign-in card for the web-side code, then asserts expect(webCode).toBe(unityCode). Skipping this makes the test green but doesn't validate the security property users rely on.
  5. Click "YES, THEY ARE THE SAME" → signature flows back over the auth-api websocket → WalletLoginInWorldEmote.TestInWorldAndRunEmote asserts the HUD is up and plays Fist Pump.

Flow 2 — web → client signup handoff (web-to-client-handoff.spec.ts)

Verifies the web-first path: fresh-user OTP signup on the dapp boots the client authenticated as the right user:

  1. OTP signup on the dapp. The spec races two post-OTP URLs to detect new-user (/auth/quick-setup) vs. recurrent (/) and resolves the username it will later assert.
  2. The personalised download CTA's URL embeds an auth-redeemable session UUID. The POM intercepts the underlying network request (the dapp re-serves the .dmg as a Blob, so download.url() is useless), and parseAuthTokenFromDownloadUrl derives the UUID in pure JS — a verbatim port of the launcher's auto_auth.rs algorithm, so we exercise the dapp's URL-mint contract without running the launcher binary.
  3. The UUID is written to auth-token-bridge.txt; the expected username to expected-username.txt.
  4. The instrumented Explorer is launched fresh (its TokenFileAuthenticator reads the bridge on first-launch — pre-launching would invalidate Flow 2's contract).
  5. Identity guard (load-bearing): C# WebFirstLoginUsernameAssert.TestInWorldUsernameMatches polls the welcome-screen greeting and accepts either Welcome <name> (new user) or Welcome back <name> (returning). It then runs the shared TestInWorldAndRunEmote. The order matters — a mismatch aborts the suite before any in-world side effects land on the wrong account.

Infrastructure & supporting changes

  • explorer-runner.tssetupExplorerStack / teardownExplorerStack spawn AltTester Desktop + Explorer as detached background processes that outlive individual dotnet test --filter runs; runExplorerTest(filterName) does plain staged dotnet test; EXPLORER_BUILD_TARGET selects the Unity build (absolute path → mf --local-build; branch/PR/version → mf explorer install). Also launchInstrumentedExplorer / relaunchExplorer (sharing an openExplorerApp helper) and clearExplorerIdentityCache (recursive fs.rmSync) for Flow 2's fresh-boot requirement.
  • Helpersauth-request-bridge.ts (Flow 1 file IPC), token-bridge.ts + download-gateway.ts (Flow 2 bridge + URL→UUID parser), persistent-data-path.ts (single macOS-only getCrossStackPath() resolver replacing duplicated platform switches), plus shared sleep() and uniqueUsername() to drop local copies.
  • C# fixtures & viewsCrossWalletLoginTests.cs (capture + code-read stages + shared CrossPlatformPaths), CrossWalletLoginTestsWithEmoteRun.cs (in-world + emote), CrossWebFirstLoginTests.cs (identity guard); new VerificationDappAuthScreenView, additions to AuthenticationMainScreenView, and ViewContainer registration.
  • DiagnosticsDiagnosticTests gains a [Category("Diagnostic")] scene-tree dump (consolidated from the deleted SceneInspector.cs) for future locator discovery; doesn't run in default invocations.
  • Docsweb/CLAUDE.md documents both flows, the file-IPC contract, and the runner conventions; .env.example gains the EXPLORER_BUILD_TARGET block.

Notes

  • The runner stack and IPC paths are macOS-only (open/pkill/lsof, ~/Library/Application Support/...). Flow 1 files live under the Explorer's persistentDataPath (.../Decentraland/Explorer/ — verified at runtime; see the comment in CrossWalletLoginTests.cs re: the PR-prose folder-name slip); Flow 2 files live under .../DecentralandLauncherLight/.
  • If any of those paths or fixture names change, the TS helpers + spec headers + explorer-runner.ts must be updated together.

Dependencies

Depends on: decentraland/unity-explorer#8862 — the #if ALTTESTER OpenUrl hook (writes auth-url.txt, suppresses Application.OpenURL in ALTTESTER builds) that Flow 1 relies on. Pin via EXPLORER_BUILD_TARGET until merged.

cyaiox added 4 commits May 21, 2026 19:24
… assertion

Adds an end-to-end @cross spec that exercises the real Decentraland
wallet sign-in flow:

  1. C# clicks Metamask on Unity's auth screen
  2. unity-explorer's `UnityAppWebBrowser.OpenUrl` (#if ALTTESTER) writes
     the auth URL to `~/Library/Application Support/DecentralandLauncherLight/
     auth-url.txt` instead of opening the system browser
  3. Playwright reads the file, derives `requestId`, signs in with a mocked
     wallet (existing-profile path via `AUTH_TEST_WALLET_PRIVATE_KEY` env, or
     fresh-wallet + QuickSetup signup as a clean-env fallback)
  4. Navigates to `/auth/requests/<id>?targetConfigId=default` → server emits
     `request-validation-status` → Unity's `Verification.Dapp.Screen` activates
     with the same validation code
  5. C# `TestReadVerificationCode` reads Unity's code; Playwright scans the DOM
     for the web-side code (per-parent digit-leaf concatenation, no testid
     dependency)
  6. **`expect(webCode).toBe(unityCode)`** — the actual device-pairing safety
     check end-users perform visually
  7. Click "YES, THEY ARE THE SAME" → signature flows back over auth-api WS →
     Unity transitions in-world
  8. C# `TestInWorldAndRunEmote` asserts MainMenu, equips Fist Pump, triggers
     the slot-0 hotkey

Requires the matching unity-explorer commits on branch
`chore/expose-requestid-for-cross-tests`:
  - d5ffd8851 — write auth-url.txt under #if ALTTESTER
  - 262994fea — suppress Application.OpenURL in ALTTESTER builds

Key infra changes:
  - `setupExplorerStack` / `teardownExplorerStack` (explorer-runner.ts) spawn
    AltTester Desktop + Explorer as detached background processes that outlive
    individual `dotnet test --filter` invocations. mf's own lifecycle would
    tear them down between stages.
  - `runExplorerTest(filterName)` reverts to plain `dotnet test --filter` per
    the convention in `explorer/CLAUDE.md`. Fast (no install/launch overhead
    per stage), matches existing patterns.
  - `EXPLORER_BUILD_TARGET` env var picks the Unity build: absolute path →
    `mf --local-build`, branch/PR/version → `mf explorer install`.

Companion follow-ups (not blocking this PR):
  - `web/shared/alttester/` scaffold for a Node AltTester WebSocket client;
    protocol shapes + typed exceptions + documented `AltDriver` API but
    `connect`/`send` bodies stubbed. Lets future cross flows observe Unity
    scene state from Playwright directly instead of file-flag IPC.
  - Flow 2 spec (`web-to-client-handoff.spec.ts`) replaces the old skipped
    `web-to-inworld-handoff.spec.ts`. Still `.skip` pending codegen of the
    landing-page "Jump Into Decentraland" CTA selector.

Diagnostic: `SceneInspector.cs` is a `[Category("Diagnostic")]` fixture that
dumps the AltTester-visible scene tree on demand — keep around for future
locator-discovery debugging; doesn't run in default test invocations.
Renames the scenario-specific files for clarity:
  - CrossPlatformVerificationTests.cs → CrossWalletLoginTests.cs
    (capture stage + code-read stage + the shared CrossPlatformPaths helper)
  - new CrossWalletLoginTestsWithEmoteRun.cs (in-world + Fist Pump emote)
  - client-to-web-handoff.spec.ts → explorer-wallet-login.spec.ts

The "Cross" prefix mirrors the existing CrossPlatform* naming convention and
the [Category("CrossVerify")] NUnit attribute, so the cross-stack scope is
obvious from both the directory listing and the dotnet test --filter.

Splits TestInWorldAndRunEmote (formerly the convergence test) into its own
file scoped to the wallet-login scenario. If a future flow needs the same
emote convergence, copy-or-share at that point — premature abstraction now
would obscure the per-scenario reading.

Also aligns the cross-stack file paths with the new
UnityAppWebBrowser.OpenUrl hook in unity-explorer (which now writes to
Application.persistentDataPath = ~/Library/Application Support/Decentraland/
Explorer/ instead of the launcher dir).
Resolve .env.example: keep EXPLORER_BUILD_TARGET block added on this branch.
…entity assert

Implements the web-first cross-platform handoff: OTP signup on the dapp →
capture the personalised download-gateway URL → derive the auth-redeemable
session UUID in pure JS (mirrors launcher-rust auto_auth.rs) → write it to
auth-token-bridge.txt → launch the instrumented Explorer fresh so its
TokenFileAuthenticator lands the user authenticated as the just-signed-up
identity → assert the welcome-screen greeting matches the QuickSetup
username before the in-world emote convergence.

The identity assert accepts either "Welcome <name>" (new-user first launch)
or "Welcome back <name>" (returning-user) — both are valid evidence the
bridge token landed on the right account. Catches stale-bridge and
profile-cache regressions that "main menu visible + emote plays" would
silently pass.
@cyaiox
cyaiox marked this pull request as ready for review June 3, 2026 13:50

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — feat: cross flow wallet auth

Files changed: 25 (+2130 -117) · CI: pending

Substantial cross-platform auth E2E infrastructure. Well-documented, clear orchestration design with file-based IPC between Playwright (TS) and Unity (C#). The two-flow architecture (client→web and web→client) is sound. That said, there are a few correctness issues that should be resolved before merge.


P1 — Major (should fix before merge)

1. [Correctness] Likely macOS path mismatch between C# test paths and Unity's Application.persistentDataPath
CrossPlatformPaths.AuthUrlPath in CrossWalletLoginTests.cs builds:

~/Library/Application Support/Decentraland/Explorer/auth-url.txt

But PR #8862's description states Unity's Application.persistentDataPath on macOS resolves to:

~/Library/Application Support/Decentraland/Decentraland Unity Explorer/auth-url.txt

The product name in ProjectSettings.asset determines the folder name. If it's "Decentraland Unity Explorer" (as the companion PR states), the C# test will poll a directory that Unity never writes to — and the test will always time out. The same mismatch applies to AuthVerificationCodePath.

Please verify the actual value of Application.persistentDataPath at runtime and align the C# CrossPlatformPaths constants. Same for the TS-side getAuthUrlPath() and getAuthVerificationCodePath() in auth-request-bridge.ts.

2. [Correctness] Windows persistentDataPath uses LocalLow, not Roaming
The TS helpers (getAuthUrlPath, getAuthVerificationCodePath) use process.env['APPDATA'] (= %APPDATA% = Roaming) but Unity's Application.persistentDataPath on Windows maps to %USERPROFILE%\AppData\LocalLow\<company>\<product>. These are different directories. While the tests are currently macOS-only (the process management uses pkill, open, lsof), the path functions claim cross-platform support. Either:

  • Fix the Windows path to use LOCALAPPDATA + Low, or
  • Remove the win32/linux branches and throw "macOS only" until cross-platform support is actually needed.

3. [Correctness] readWebCode() is fragile — will match any digit-only element on the page
In explorer-wallet-login.spec.ts, the DOM deep-scan finds ALL leaf elements with digit-only text and returns the first group (grouped.find(s => s.length >= 1)). If the page contains other numeric content (counters, timestamps, prices, user counts), this will silently return the wrong value and the code-match assertion may produce confusing false passes or failures.
Suggestion: scope the scan to the verify-sign-in container (e.g. page.locator('[data-testid="verify-sign-in-approve-button"]').locator('..').locator('..')) rather than the entire document. Or wait for the TODO data-testid on the code element.


P2 — Minor (non-blocking improvements)

4. [Duplication] Platform-path switch blocks repeated 4×
getAuthUrlPath(), getAuthVerificationCodePath(), getExpectedUsernamePath(), and getTokenBridgePath() each have identical switch(process.platform) structures differing only in the directory and filename. Extract a shared getPersistentDataPath(app: 'explorer' | 'launcher', filename: string): string helper.

5. [Duplication] sleep() utility defined in multiple files
Both auth-request-bridge.ts and explorer-runner.ts define their own sleep(). Consider using a shared utility from shared/helpers/.

6. [Duplication] uniqueUsername() defined identically in two spec files
explorer-wallet-login.spec.ts and web-to-client-handoff.spec.ts both define const uniqueUsername = (): string => \QA${randomBytes(3).toString('hex')}``. Extract to a shared test helper.

7. [YAGNI] AltTester WebSocket client is entirely unimplemented
shared/alttester/ adds ~400 lines (client + protocol + index + README) where every method throws "not yet implemented". The README explicitly says "scaffold only — not yet functional" and lists a 5-step follow-up plan. Consider deferring this to the PR that actually implements it — shipping dead code adds review surface and maintenance burden without runtime value.

8. [Style] SceneInspector.cs overlaps with DiagnosticTests.DiagnoseAuthScreenUsernameLabel
Both are diagnostic scene-dump tools with similar patterns (enumerate all elements, filter, dump). Consider consolidating into a single diagnostic fixture.


Security Review

No security issues found.

  • Private keys are either from env vars or generated ephemerally via viem — none hardcoded.
  • File-based IPC is local-only, limited to well-known paths under Application.persistentDataPath and the launcher data dir.
  • Auth tokens written to disk are test-only artifacts.
  • Process spawning (mf, dotnet, open, pkill) uses controlled arguments — no user-input injection vectors.
  • .env.example contains only placeholder values.
  • The AltTester WebSocket client connects to localhost:13000 only.

Consumer Impact

Test-only code — no public API surface changes. No downstream consumers affected.

Verdict

REQUEST CHANGES — P1 items #1 (path mismatch) and #3 (fragile code reader) should be addressed. #1 in particular is likely a correctness bug that would cause both flows to fail at runtime. #2 is worth addressing for code hygiene even though it's not exercised on macOS.


Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz via Slack

File.Delete(CrossPlatformPaths.AuthUrlPath);

Assert.That(Views.AuthenticationMainScreen.LoginSelectionScreen.IsPresent(), Is.True,
"Expected the LoginSelection sub-screen after LoggedOutAuthBaseTest.EnsureInWorld.");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] The path components "Decentraland", "Explorer" may not match Unity's actual Application.persistentDataPath, which uses companyName/productName from ProjectSettings.asset. PR #8862's description states the macOS path is ~/Library/Application Support/Decentraland/Decentraland Unity Explorer/. If the product name is indeed "Decentraland Unity Explorer" (not just "Explorer"), this path will never find the file Unity writes.

Please verify by logging Application.persistentDataPath at runtime and aligning this constant.

export function getAuthUrlPath(): string {
switch (process.platform) {
case 'darwin':
return path.join(os.homedir(), 'Library', 'Application Support', 'Decentraland/Explorer', 'auth-url.txt')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Same path concern as the C# side — 'Decentraland/Explorer' must exactly match Unity's Application.persistentDataPath folder name. If the Unity product name is "Decentraland Unity Explorer", this should be 'Decentraland/Decentraland Unity Explorer'.

Also: the 'Decentraland/Explorer' string passed to path.join creates a single segment containing a / on Windows. Use separate arguments: 'Decentraland', 'Explorer'.

// (`<div>1</div><div>3</div>`) — the latter we still need to
// concatenate per parent.
const groups = new Map<Element, string[]>()
document.querySelectorAll('*').forEach(el => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] This DOM scan finds any digit-only leaf element on the page. If there's a page-visit counter, user count, or other numeric element, grouped.find(s => s.length >= 1) will return whichever appears first in DOM order — not necessarily the verification code.

Consider scoping the scan to the verify-sign-in container:

const container = page.locator('[data-testid="verify-sign-in-approve-button"]').locator('xpath=ancestor::div[contains(@class, "verify")]')
const webCode = await container.evaluate(/* ... */)

Or, use a more targeted selector matching the known MUI class pattern for the code digits.

await sleep(pollIntervalMs)
}
throw new Error(`auth-url.txt did not appear at ${getAuthUrlPath()} within ${timeoutMs / 1000}s`)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] The win32 case uses APPDATA (Roaming), but Unity's Application.persistentDataPath on Windows resolves to %USERPROFILE%\AppData\LocalLow\<company>\<product> — a different directory. This will silently fail if anyone runs these tests on Windows.

Comment thread web/shared/alttester/client.ts Outdated
type WebSocketHandle = unknown

/**
* Minimal AltTester WebSocket client — vendor implementation targeting

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2 — YAGNI] Every method in this file throws 'not yet implemented'. The README says this is a scaffold for a follow-up session. Consider deferring the entire shared/alttester/ directory to the PR that actually implements the WebSocket transport — it avoids shipping ~400 lines of dead code that reviewers and maintainers need to track.

…de reader, drop dead scaffold

- Extract getCrossStackPath() (tests/auth/helpers/persistent-data-path.ts): a
  single macOS-only resolver for the Explorer persistentDataPath + launcher
  data dir, replacing 4 duplicated platform switch blocks. Drops the incorrect
  Windows (Roaming, should be LocalLow) / Linux branches by throwing macOS-only
  — the @cross runner is macOS-bound (open/pkill/lsof). [review #2, #4]
- Extract shared sleep() (shared/helpers/async.ts); replace 4 local copies
  (auth-request-bridge, token-bridge, explorer-runner, otp-mailbox). [review #5]
- Use the existing shared uniqueUsername() in both @cross specs; drop the two
  local copies + now-unused randomBytes imports. [review #6]
- Scope explorer-wallet-login readWebCode to the verify-sign-in card (climb from
  the approve button to the nearest numeric-leaf ancestor) instead of a
  whole-document digit scan that could match unrelated numeric content. [review #3]
- Remove the unimplemented web/shared/alttester scaffold (client/protocol/index/
  README + todo) — dead code, deferred to its implementation PR. [review #7]
- Consolidate SceneInspector.DumpVisibleObjects into DiagnosticTests behind a
  shared FilterByName helper; delete SceneInspector.cs. [review #8]
- Document the runtime-verified persistentDataPath folder ("Explorer", not
  "Decentraland Unity Explorer") in CrossWalletLoginTests. The current path is
  correct: the OpenUrl hook (PR #8862) writes auth-url.txt to .../Decentraland/
  Explorer/; the PR's prose naming the folder "Decentraland Unity Explorer" is a
  documentation slip and was not applied. [review #1]

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — feat: cross flow wallet auth

Files changed: 22 (+1770 -150) · CI: in progress

All six findings from the prior review have been addressed — nice work. The path mismatch is thoroughly documented with runtime verification, the Windows/Linux dead branches are gone (replaced by a clean macOS-only throw in persistent-data-path.ts), the web code reader is now scoped to the approve button's ancestor, the duplicated sleep/uniqueUsername utilities are extracted, and the AltTester scaffold was dropped. The codebase is cleaner and the orchestration design is solid.

One new P1 and a handful of P2s from this pass:


P1 — Major (should fix before merge)

1. [Documentation] CLAUDE.md directory tree references a non-existent file
The tree listing shows:

└── client-to-web-handoff.spec.ts     # @cross — Flow 1

But the actual file created in this PR is explorer-wallet-login.spec.ts. No file named client-to-web-handoff.spec.ts exists on this branch. CLAUDE.md is the primary developer navigation aid for this repo — a wrong filename will send anyone looking for the Flow 1 spec on a dead-end hunt.

Fix: replace client-to-web-handoff.spec.ts with explorer-wallet-login.spec.ts in the directory tree.


P2 — Minor (non-blocking improvements)

2. [Robustness] clearExplorerIdentityCache will throw on subdirectories
fs.unlinkSync only removes files — if Thirdweb/EcosystemWallet ever contains subdirectories, the function throws EISDIR (not caught by the ENOENT guard). Consider fs.rmSync(thirdwebDir, { recursive: true, force: true }) followed by re-creating the empty dir, or just fs.rmSync with force: true since the dir itself isn't needed.

3. [Duplication] relaunchExplorer duplicates launchInstrumentedExplorer's open-command logic
Both independently resolve the app path and construct the same open args (--skip-version-check, --position 100,100, --dclenv org, --alttester). Extract a shared openExplorerApp(appPath) helper so the args stay in sync.

4. [Precision] readWebCode() match threshold is very permissive
.find(s => s.length >= 1) matches any group containing a single digit. The scoping to the approve button's ancestor is a big improvement, but a s.length >= 2 threshold would further reduce the chance of matching a stray "2" or "0" in non-code UI elements.


Security Review

No security issues found.

  • Private keys are ephemeral (viem generatePrivateKey) or from env vars — none hardcoded.
  • File-based IPC is local-only, limited to well-known paths under ~/Library/Application Support/.
  • Process spawning uses controlled, non-user-derived arguments — no injection vectors.
  • .env.example contains only placeholder values.
  • Auth tokens and bridge files are test-only artifacts.
  • The clearExplorerIdentityCache function operates on a known fixed path.

Consumer Impact

Test-only code — no public API surface changes. No downstream consumers affected.

Previous Review Findings — All Addressed ✅

# Finding Resolution
P1-1 macOS path mismatch (Explorer vs Decentraland Unity Explorer) Documented runtime verification in CrossPlatformPaths + persistent-data-path.ts
P1-2 Windows %APPDATA% vs LocalLow Replaced with macOS-only throw in persistent-data-path.ts
P1-3 readWebCode() document-wide scan Scoped to approve button ancestor tree
P2-4 Platform-path duplication × 4 Extracted to getCrossStackPath in persistent-data-path.ts
P2-5 Duplicate sleep() Extracted to shared/helpers/async.ts
P2-6 Duplicate uniqueUsername() Now imports from existing helpers/test-user.ts
P2-7 Unimplemented AltTester scaffold Removed from PR
P2-8 SceneInspector overlap with DiagnosticTests Folded into DiagnosticTests.cs

Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz via Slack

Comment thread web/CLAUDE.md
│ ├── web-to-client-handoff.spec.ts # @cross — Flow 2: web OTP → token-bridge → Explorer in-world + emote (skipped, see TODO)
│ └── client-to-web-handoff.spec.ts # @cross — Flow 1: client launches → browser OTP → in-world + emote (skipped, see TODO)
└── marketplace/ # marketplace dapp tests
├── helpers/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] This references client-to-web-handoff.spec.ts which does not exist on this branch. The actual Flow 1 spec file is explorer-wallet-login.spec.ts.

Suggested fix:

    │       └── explorer-wallet-login.spec.ts      # @cross — Flow 1: client launches → browser wallet pairing → in-world + emote

/**
* Launches the AltTester-instrumented Explorer and waits for it to
* ESTABLISH a TCP connection to AltTester Desktop on port 13000.
*

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] fs.unlinkSync only removes files — if Thirdweb/EcosystemWallet ever contains subdirectories, this throws EISDIR which isn't caught by the ENOENT guard. Consider:

fs.rmSync(thirdwebDir, { recursive: true, force: true })

This handles both files and subdirectories, and force: true suppresses ENOENT.


/**
* Wipes the Explorer-side identity cache (Thirdweb EcosystemWallet) so that
* the next launch can't authenticate against a prior session and ignore the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] This duplicates the app-path resolution + open args from launchInstrumentedExplorer (lines 93-106). If the args ever diverge (e.g., adding a new flag), one callsite will be stale. Consider extracting a shared openExplorerApp(appPath: string) helper that both functions call.

// approve/deny buttons.
// TODO(request-page-code-selector): tighten to a data-testid on the code
// element once the dapp adds one, dropping the climb entirely.
const readWebCode = (): Promise<string | null> =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] .find(s => s.length >= 1) matches any group with a single digit — e.g., a "0 notifications" badge or a step indicator like "2". The scoping to the approve button's ancestor tree reduces this risk significantly, but a s.length >= 2 threshold would be a cheap additional guard:

.find(s => s.length >= 2) ?? null

(Assuming the device-pairing code is always at least 2 digits.)

…harden runner, tighten code reader

- web/CLAUDE.md: the Flow 1 spec was renamed to explorer-wallet-login.spec.ts
  (commit aa3a6f0) but the directory tree + prose still referenced the old
  client-to-web-handoff.spec.ts. Update both; also correct the tree's "browser
  OTP" description (that's Flow 2's mechanism — Flow 1 is wallet device-pairing). [review #1]
- clearExplorerIdentityCache: replace readdirSync + per-file unlinkSync (throws
  EISDIR on any subdirectory) with fs.rmSync(dir, { recursive: true, force: true });
  force suppresses ENOENT so the try/catch is gone. [review #2]
- Extract openExplorerApp(context): the appPath resolution + `open` args +
  status check were duplicated between launchInstrumentedExplorer and
  relaunchExplorer; now one helper keeps the launch flags in sync. [review #3]
- readWebCode: raise the per-parent group threshold from length >= 1 to >= 2.
  Pairing codes are always multi-digit (e.g. "96"), so a length-2 floor rejects
  a stray single digit that could survive within the scoped card. [review #4]

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review (3rd pass) — feat: cross flow wallet auth

Files changed: 22 (+1770 -150) · CI: ✅ passing

All four findings from the prior review have been addressed — nice clean-up. The codebase is in solid shape: the two-flow architecture is well-orchestrated, the file-based IPC is well-documented with verified-at-runtime annotations, the shared helper extraction (persistent-data-path.ts, openExplorerApp, sleep, uniqueUsername) removes the duplication, clearExplorerIdentityCache now uses rmSync(recursive, force) correctly, and the readWebCode DOM scan is properly scoped with a >= 2 digit floor.


P2 — Minor (non-blocking)

1. [Documentation] CLAUDE.md and spec JSDoc claim specs are skipped / blocked, but they aren't
Three documentation surfaces are out of sync with the actual code:

  • CLAUDE.md tree annotations say (skipped, see TODO) for both spec files, but neither test.describe uses .skip.
  • CLAUDE.md prose says Flow 2 is "Blocked on: (a) the post-signup CTA selector in PostSignupPage.downloadSignedLauncher" and Flow 1 is "Blocked on locating where the client surfaces the code and URL" — but the PR implements both (captureSignedDownloadUrl for Flow 2, VerificationDappAuthScreenView.CodeText + auth-url.txt for Flow 1). Also PostSignupPage.downloadSignedLauncher doesn't exist — the actual method is captureSignedDownloadUrl.
  • explorer-wallet-login.spec.ts JSDoc says "Stays test.describe.skip until two prereqs land" but the code isn't skipped. Meanwhile web-to-client-handoff.spec.ts correctly says "Both locators that previously blocked this spec are resolved."

Suggestion: update the CLAUDE.md tree annotations to remove (skipped, see TODO), update the prose paragraphs to say "Blocked on" → "Depends on" (for Flow 1's unity-explorer branch dep) or remove the "Blocked on" entirely (for Flow 2 where blockers are resolved), fix the stale method name reference, and update the explorer-wallet-login.spec.ts JSDoc to match reality.

2. [Style] waitForNoEstablishedConnection lacks JSDoc
Unlike its siblings waitForPort and waitForEstablishedConnection, this function has no documentation comment. A one-liner describing its purpose ("Polls until the ESTABLISHED connection drops on the given port") would be consistent.


Previous Review Findings — All Addressed ✅

# Finding Resolution
P1-1 CLAUDE.md references client-to-web-handoff.spec.ts (non-existent) Tree now lists the correct filenames
P2-2 clearExplorerIdentityCache throws EISDIR on subdirs Uses fs.rmSync({ recursive: true, force: true })
P2-3 relaunchExplorer duplicates open command logic Shared openExplorerApp(context) helper
P2-4 readWebCode() match threshold >= 1 too permissive Now requires s.length >= 2

Security Review

No security issues found.

  • Private keys are ephemeral (viem generatePrivateKey) or loaded from env vars — none hardcoded.
  • File-based IPC uses fixed, well-known paths under ~/Library/Application Support/ — no user input in path construction.
  • Process spawning (mf, dotnet, open, pkill, lsof) uses controlled, non-user-derived arguments — no injection vectors.
  • .env.example contains only placeholder values (all-zeros private keys, commented-out vars).
  • Auth tokens written to disk are test-only artifacts, local to the CI runner.
  • The clearExplorerIdentityCache function operates on a known fixed path.

Consumer Impact

Test-only code — no public API surface changes. No downstream consumers affected.

Verdict

APPROVE — no P0 or P1 issues. The architecture is sound, the code is clean, all prior findings are addressed. The P2 documentation items can be cleaned up in a follow-up.


Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz via Slack

Comment thread web/CLAUDE.md
│ ├── switch-method.spec.ts # @web @auth — switch from OTP to web3 in same context
│ ├── web3-avatar-setup.spec.ts # @webgpu — Unity 3D avatar editor
│ └── web-to-inworld-handoff.spec.ts # @cross — web → desktop (currently skipped)
│ ├── web-to-client-handoff.spec.ts # @cross — Flow 2: web OTP → token-bridge → Explorer in-world + emote (skipped, see TODO)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Documentation says (skipped, see TODO) for both spec files, but neither test.describe block actually uses .skip. Also, the prose below (lines 308-310) says both flows are "Blocked on" things that this PR actually implements — the blockers are resolved. Consider removing the (skipped, see TODO) annotations and updating the prose to reflect the current state.

* 5. **Dispatch the in-world C# stage.** `TestInWorldAndRunEmote` asserts
* the HUD is up and plays Fist Pump.
*
* Stays `test.describe.skip` until two prereqs land:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] This JSDoc says "Stays test.describe.skip until two prereqs land" but the test.describe on line 65 doesn't use .skip. Meanwhile web-to-client-handoff.spec.ts correctly notes "Both locators that previously blocked this spec are resolved." Consider updating this header to match reality.

throw new Error(`Timed out waiting for ${host}:${port} after ${timeoutMs / 1000}s`)
}

/**

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] waitForNoEstablishedConnection is the only polling function in this file without a JSDoc comment. Its siblings waitForPort and waitForEstablishedConnection both have documentation — a one-liner here would be consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants