Repository navigation
feat(connect): connect plan --run walks a plan's Solana legs on the same page - #186
Conversation
…ame page (#184) A plan with a Solana leg used to be refused by --run and run one page per leg. Now every leg runs on the one plan page. - Server: prepareConfirmSolana is the Solana confirm core without its page (runConfirmSolana unchanged in behaviour). A Solana leg is held to its acknowledged digest and gate-checked when it starts, and its transaction is built again at the operator's press and verified from what landed. - Each chain family's connection is required only when the plan has a leg on it. - Page: an EVM-only plan renders as before. A Solana-only plan signs in as a single Solana confirmation does. A mixed plan keeps the EVM sign-in and connects the same wallet's Solana account at Start, checked against the connected Solana account before any leg runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Kevin Chen <13785749+Kevin-Chen0@users.noreply.github.com>
… an EVM vault (#184) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Kevin Chen <13785749+Kevin-Chen0@users.noreply.github.com>
- docs/tools.md, docs/security-model.md, the earn skill and both changelogs describe Solana legs on the one plan page. - llms.txt regenerated; dist/ and the plugin copy rebuilt. - The page-source pin now lists the two Solana imports the plan page adds, and pins the only Wallet Standard features it may call: standard:connect and solana:signAndSendTransaction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Kevin Chen <13785749+Kevin-Chen0@users.noreply.github.com>
Kevin-Chen0
left a comment
There was a problem hiding this comment.
Review, at 9c88b03
Verdict: one worth-fixing finding, nothing blocking. I'll sign off once it's in, or on Kevin's word to ship without it.
What I ran
- Typecheck:
tsc -b, the scripts tsconfig, and the page typecheck are clean. - Full suite on one worker (Windows): 622 passed, 7 failed, 46 skipped. The 7 are the ones that also fail on the base on this platform:
connectfile mode,entrypoint, and 6×registry-check.chain. - Rebuild: the page bundle,
tsup,llms.txtand the plugin sync were rebuilt from this head. They differ from the committed files only in line endings. - Mutations: 11, one guard each, each restored from a saved copy and byte-compared.
Mutations
| # | Guard disabled | Result |
|---|---|---|
| M1 | stale-leg check after a Solana build | killed |
| M2 | a Solana leg's receiver check | killed |
| M3 | the Solana connection required for a plan with a Solana leg | killed |
| M4 | the gate and simulation when a Solana leg starts | killed (4 tests) |
| M5 | not prepared: recorded as an operator stop |
killed |
| M6 | Solana core: refuse a build after a signature | survived |
| M7 | Solana core: a failed verdict read as finished | killed |
| M8 | Solana core: the built blockhash not recorded | killed (8 tests) |
| M9 | Solana core: refuse a second signature | survived |
| M10 | a Solana leg's digest not compared before the page opens | killed |
| M11 | the replaced/started re-check after preparing a leg | killed (shared EVM test) |
W1 (worth fixing): two guards in the Solana confirm core have no test
prepareConfirmSolanarefuses a build once a signature is on record ("the transaction was already sent"). Itsacceptrefuses a second signature ("the transaction was already reported"). Disabling either leaves every test green.- The same was true on the base. That gap now covers two routes: the single Solana page and
--run. - Suggested pins, one each: a
buildafter a signature was accepted is refused; a second signature for the same leg is refused and the first verdict stands.
Checked, and not a finding
- The plan page cannot sign a Solana leg with a different, silently connected wallet. The page passes
shouldAutoConnect: falsetotoSolanaWalletConnectors. Privy 3.47.0 wires that straight into its connectors (shouldAutoConnect ?? true) and gates its silentstandard:connecton it. So Phantom, Solflare and the like are not auto-connected, and the only Solana wallets on a mixed plan are Privy's own embedded wallet and the same-named wallet the page connects itself. The account is then checked against the connected Solana account. - Using the wallet's own Wallet Standard features is fine.
standard:connectandsolana:signAndSendTransactionare what Privy itself calls for an external Solana wallet, and the new source pin limits the page to exactly those two. - An EVM-only plan is unchanged: the same provider config and the same page component, and every existing
--runtest passes untouched.
Not covered by this review
- A real wallet. The live run (Base, Arbitrum, Robinhood Chain and Solana on one page) is next, on this head.
- Privy's runtime behaviour, which was read from its shipped code, not observed. That includes MetaMask's Solana connect prompt and whether
meta.nameequals the Wallet Standard name. - The page's React code, which is typechecked only.
Kevin-Chen0
left a comment
There was a problem hiding this comment.
Review at 9c88b03: signed off, with four worth-fixing items
Nothing blocking. The server guards for a Solana leg on the plan page held under every attack below. The items listed are wording, test and record-keeping gaps. None of them lets a wrong transaction be sent or verified.
What was run
- Tests: Linux, one worker (
vitest run --no-file-parallelism). 635 passed, 0 failed, 40 skipped. - Typechecks:
tsc -b,tsc -p tsconfig.scripts.jsonand the page'stsc --noEmitare clean.llms:checkpasses. - Build: a fresh
npm run buildreproducesdist/treasury.mjsand the plugin copy exactly, andconnect-page/build.mjsreproduces both committedconnect-page.jsbundles byte for byte. - Base: the head contains
release/v0.1.4at 412041b. - Mutations (one site each, byte-checked before the run, md5-restored after):
- All 11 of the PR's kills reproduce, and the survivor (the digest check at leg start) cannot be reached. The digest is a pure function of the plan's copy, the Solana session read once, and the registry, and the check before the page covers every pending leg.
- Six further mutants drawn from the PR's claims:
- fee-payer check off: killed, by a
solana-connecttest only; - blockhash binding off: killed, by
connect/solana-connecttests only; - leg-start gate made non-fatal: killed;
- the build's
done.length > 0guard deleted: survived, no effect, because the HTTP lock serialises requests; - verify deadline set to
Infinity: survived (item 3); - one written but not run.
- fee-payer check off: killed, by a
- Attacks that held:
- a transaction paid for by another account;
- another blockhash;
- a build finishing after a stop;
- a second build while a signature is being checked (409);
- an EVM leg naming a Solana receiver (refused before any page);
- a Solana vault on a leg whose chain says EVM (refused);
- a plan with an EVM leg and no EVM wallet connected (refused).
- Page: an EVM-only plan uses the same provider config, line for line, and the same component path. No page path moves past a step without the process's verdict. A wallet error with no signature ends the plan as "may have been sent".
Worth fixing
- A failed Solana wallet check partway through says nothing was sent, after money moved. (
connect-page/src/plan.tsx:385-393,src/connect/commands.ts:1110)solana.ensure()runs before every step while a Solana step is still pending, not only at Start, and its cached wallet resets when the page reloads.- Scenario: an EVM, EVM, Solana plan; step 1 lands; the tab reloads; "Continue with step 2". The wallet's Solana connect prompt is declined. The page then reads "The plan was stopped before any step ran, and nothing was sent", and the recorded reason says "before the plan started".
- Both are false: step 1 moved money.
- Suggest naming the step and saying "nothing was sent" only when no step has landed.
- One signature can complete two legs. (
src/connect/commands.ts:1164)- Nothing compares an accepted Solana signature with those already recorded on earlier legs.
- A scratch test with two identical legs (
deposit tlCashPlusUSDC2G 2.5twice) completed both on one signature. The plan recorded 5 USDC deposited when 2.5 moved. - On a real chain this needs leg 2's build to get leg 1's blockhash after leg 1 landed and verified, so a lagging or caching RPC. It is unlikely, but the record would overstate what moved.
- A one-line refusal of a signature already on an earlier leg closes it.
- The same check on EVM (#181) was not looked at.
- Test gaps.
- The plan suite's RPC stub returns one blockhash for every build, so no plan test can tell one leg's transaction from another's. The blockhash binding and fee-payer check are covered only by single-confirm tests.
- Changing the verify deadline to
Infinitysurvives. The record stays right; the run would keep polling after the page closes. - Suggest a stub that returns a new blockhash per call, plus a "leg 1's signature reported for leg 2 is a mismatch" test.
- The page-source pin misses optional chaining.
wallet.features?.["solana:signMessage"]matches neither pattern intests/connect-page-source.unit.test.ts:84-88, and the pin passed 9/9 with it in place.- Destructuring and aliasing the features object also get through. The file calls itself a tripwire, so those two are acceptable, but
?.[is ordinary code. - It also does not strip comments: a forbidden feature named in a
//comment fails it. That is strict but harmless.
Minor
- The page's account check: it compares the address list the wallet reports. A same-named Wallet Standard wallet could list the expected address, so the guard that holds is the process's fee-payer and landed-transaction check. "The account check is the guard" in the PR text overstates the page's part. The account is also not re-checked after Start, which is safe for the same reason.
- Wallet choice on a mixed plan: the Privy-wallet branch wins over the name rule. A wallet Privy auto-connected that holds the expected address is used even when the EVM sign-in was another wallet. The account still matches, so this is safe, but the docs say "the same wallet the operator signed in with".
- Errors before the wallet is asked: a missing target, a malformed wire or a missing feature are classed as "may have been sent". That errs on the safe side.
- Stopped plans: a Solana leg already running still gets a freshly built transaction at the press after the plan is cleared or replaced. Its outcome stays on the run's own copy of the plan, as on EVM.
Not covered
- No real wallet and no browser run; the page was reviewed by reading it.
- Privy 3.47's runtime behaviour, read from its shipped code by the PR, was not re-checked.
- A
--plan --legrun in another shell while the page drives a Solana leg.
…d-plan wallet check says what already went - A Solana signature already on record for an earlier leg of the plan is refused as a mismatch, and the plan ends. Two identical legs built on the same blockhash produce the same transaction, so the blockhash binding alone could let leg 1's transaction complete leg 2 and the plan record twice what moved. - The page's Solana wallet check runs before every leg while a Solana leg is still to run, so after a reload it can fail once earlier legs have sent transactions. The page and the recorded reason now name the leg it stopped before and say nothing more was sent, instead of "before the plan started ... nothing was sent". - The plan tests' stub chain hands out a new blockhash on every read, and two tests pin that a transaction built for an earlier leg, or the signature already on it, does not complete a later leg. - The page-source check on a Solana wallet's features is an allowlist: every `features` in the page's code must be one of the two literal lookups, so `features?.[...]`, destructuring and aliasing fail it. Comments are not held to it. - Bundles rebuilt; plugin copy synced. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Kevin Chen <13785749+Kevin-Chen0@users.noreply.github.com>
|
Fix commit ec37ec6 for the four worth-fixing items in the review above. That review was signed at 9c88b03 and does not cover this commit; it needs its own review.
Checks at ec37ec6
Not covered: the page change is typechecked only, as the page has no automated tests. The verify deadline (item 3 of the review) is unchanged and still untested; the record was already correct. |
Kevin-Chen0
left a comment
There was a problem hiding this comment.
Reviewed at ec37ec676c8457d783400eba3f02d1291124c03a, starting with the delta from 9c88b030, then the rest of the source diff: src/connect/solana.ts, src/connect/commands.ts (plan run), src/connect/ack.ts and connect-page/src/plan.tsx.
Signed off at ec37ec67. Nothing blocking. Two worth-fixing items and one nit are below; none needs to hold this PR.
What I ran
- Full suite (
npx vitest run --no-file-parallelism, aftertsc -b): all pass, 40 skipped. CI is green at this head. - Mutations, each restored afterwards:
| mutation | result |
|---|---|
drop the recordedElsewhere callback in commands.ts |
"two identical Solana legs built in one slot" fails |
drop the blockhash binding (built.has(...)) in verifySolanaLanded |
"a transaction built for an earlier Solana leg does not complete a later one" fails |
add w.features.evil to plan.tsx |
the features pin fails |
add const s = "a // b"; return w.features.evil; on one line |
the features pin passes (see item 1) |
The two new plan tests each fail on the defect they name, not on setup. With the per-read blockhash stub, the earlier mixed-plan test asserts blockhash: lastBlockhash, and its landed transaction defaults to that same blockhash, so it still goes through the binding check rather than around it.
The four questions
- (a) Holds. A leg starts only when
startable()sees itpendingin both the run's copy and the file. No path leaves apendingleg with transactions:runPlanLegputs a leg back topendingonly when no page opened.acceptrefuses a second signature on the same leg (done.length > 0) before the cross-leg check runs, so the check cannot match the current leg's own entry. - (b) Real false negative, but nothing in the current source triggers it. See item 1. I scanned the page sources for a
//that the stripper removes after a quote on the same line, and found none. - (c) Correct.
solana.ensure()runs before every leg while a Solana leg is still pending, not only after a reload. So the old "nothing was sent" note was also wrong in an uninterrupted run once an EVM leg had gone; the newfirsttest covers both cases. A completed leg always carries at least one transaction, and the server sendstxs: []rather than leaving it undefined (legView). - (d) No vacuous pass found; see the mutation table.
Worth fixing (non-blocking)
- The comment stripper in the features pin can remove code.
(^|[^:"'\])//.*$treats any//inside a string as a comment unless a quote or:sits right before it. Everything after it on that line goes, including a forbiddenfeatures.x, and the pin passes; the fourth mutation shows this. Stripping comments with the TypeScript scanner (ts.createScanner`, skipping comment trivia) would close this. Alternatively, fail the pin whenever a stripped span starts after a quote on its line. - The cross-leg signature check covers
connect plan --runonly. A leg run on its own page (connect deposit|withdraw … --plan <code> --leg <n>,commands.ts:628→runConfirmSolana) still callsprepareConfirmSolanawithoutrecordedElsewhere. Those are separate commands seconds apart, so two identical legs sharing a blockhash is much less likely there. Passing the same callback, read from the plan file, would make both paths hold the same rule; it could be a follow-up.
Nit
- The new comment in
plan.tsxsays the wallet check "runs again after a reload". It runs before every leg while a Solana leg is pending. "Before each leg" would describe what the code does.
Not covered
I did not open the page in a browser or use a real wallet, and I did not review the rebuilt bundles beyond the tests that load them. Verification deadlines and timing were not re-checked; they are unchanged in this delta.
…nature too; parse comments out of the page check - `connect deposit|withdraw --plan <code> --leg <n>` on Solana now refuses, as a mismatch, a signature already on record for another leg of the same plan, as `connect plan --run` does. A test runs two identical Solana legs leg by leg on one blockhash. - The page-source check strips comments with the TypeScript parser instead of a pattern, so a `//` inside a string no longer hides the code after it. - The plan page's comment on its Solana wallet check says it runs before every leg. - Bundles rebuilt; plugin copy synced. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Kevin Chen <13785749+Kevin-Chen0@users.noreply.github.com>
|
Fix commit 7c6a987 for the follow-ups in review 5525727074 (signed at ec37ec6; it does not cover this commit).
Checks at 7c6a987:
|
Kevin-Chen0
left a comment
There was a problem hiding this comment.
Re-reviewed the delta ec37ec67..7c6a987d. The earlier sign-off at ec37ec67 covers the rest of the PR.
Signed off at 7c6a987dcc3cabe5decf9017c61475133b51eac3. All three items from the previous review are addressed, and nothing new came up.
What I ran
- Full suite (
npx vitest run --no-file-parallelism, aftertsc -b): 638 passed, 0 failed. - Mutations, each restored afterwards:
| mutation | result |
|---|---|
drop recordedElsewhere from the --plan --leg Solana path |
"run leg by leg, two identical Solana legs built in one slot" fails |
const s = "a // b"; return w.features.evil; on one line in plan.tsx |
the features pin fails (it passed at ec37ec67) |
w.features.evil and features["x:y"] in a line comment and a block comment only |
the pin passes, as intended |
w.features['standard:connect'] (single quotes) |
the pin passes: the printer re-emits the literal with double quotes, so the lookup is still counted as an allowed one |
Questions
- (a) Holds. The printer only changes how a lookup is written; the name inside it stays the same. Single quotes come out as double quotes (last row above), so a single-quoted forbidden name is still caught by the name check. A template-literal or computed key keeps its form, adds to the
featurescount without being an allowed lookup, and fails the pin. - (b) Holds. The lookup returns false unless
file.code === plan.code. While the leg runs, its own entry isrunningwith no transactions (runPlanLeg), andacceptrefuses a second signature before this check runs, so the check cannot match the current leg. - (c) Holds. With the guard removed, the new test fails on the verdict, not on setup. Both pages are built on the same blockhash (asserted), so the blockhash binding alone cannot be what refuses the signature.
Not covered
I did not open the page in a browser or use a real wallet. The two bundle diffs (dist/treasury.mjs, plugin/dist/treasury.mjs, +6/−2 each) were checked only through the tests that load them.
Closes #184 once v0.1.4 reaches
main. Intorelease/v0.1.4.connect plan --run(#179) walked every leg of a plan on one signing page, but only EVM legs: a plan with a Solana leg was refused and ran one page per leg. Now every leg runs on the one page, in order, Solana legs included.What changes
Server (
src/connect/solana.ts,commands.ts)prepareConfirmSolanais the Solana confirm flow without its page, as connect plan --run: one signing page for every leg of a plan (#174) #179'sprepareConfirmis for EVM.runConfirmSolanauses it and is unchanged in behaviour: the gate before the page, the build at the operator's press,built/simulatedkeyed by blockhash, andverifySolanaLanded.solanaIntentDigest) before the page opens and again when it starts. When it starts, its shares are worked out (a withdrawal by amount) andbuildForIntentruns: the gate, the simulation, and the partial-withdrawal refusal. Only then is it marked running. Its transaction is built again when the page asks for it ({ build: true }), so the blockhash is fresh. Only a signature for a transaction built for that leg, paid for by the connected Solana account, verifies. A build that was still running when the leg was stopped on the page is refused.solanaRequest) now gives a Solana deposit or withdrawal its acknowledged form and its intent, for a single run, a--plan --legrun and--runalike.unprepared: truerecords the first pending leg asstopped,not prepared: …. The page shows that leg as "Couldn't prepare · nothing sent".--ackresult'snext_stepand the command descriptions now say one signing page for every plan.Page (
connect-page/src/plan.tsx,main.tsx)walletChainTypestays"ethereum-only"and the login methods are the same, so the window offers no Solana wallet. The provider also gets Privy's Solana connectors (WalletConnect stripped, as on the Solana pages) and its Solana RPC plugin.The page-source pin (
tests/connect-page-source.unit.test.ts), changed on purpose; please look at it. The page now importsuseStandardWalletsand theSolanaStandardWallettype. For the same-named external wallet on a mixed plan, it calls that wallet's own Wallet Standard features, as Privy does for an external Solana wallet. The pin lists the two new imports and adds one check: the only features the page may call arestandard:connectandsolana:signAndSendTransaction, never one named at run time. As a positive control, swapping one of them forsolana:signMessagefails it.Docs:
docs/tools.md,docs/security-model.md, the earn skill, both changelogs.llms.txtregenerated; bundles rebuilt; plugin copy synced.Verified
Tests: 16 new tests in
tests/connect-plan.unit.test.ts, driving the page's HTTP protocol against a stub Solana RPC built on the recorded vault snapshot. They cover:ComputationalBudgetExceeded(stopped,reverted, plan ended);Three existing tests changed, because they pinned the old split: the acknowledgement's wording, the
--acknext_step, and--runrefusing a Solana plan.Full suite, one worker (
vitest run --no-file-parallelism), base vs head:release/v0.1.4at 412041b (fix(connect): replace a crashed run's page lock safely #185): 622 passed, 7 failed, 46 skipped. The 17 more tests are the 16 above and the page-source pin, and fix(connect): replace a crashed run's page lock safely #185 brings 5 more.client429 wait, passed in both head runs.registry-check.chain, 1 inclient(the 429 wait), and 1 inconnect(the owner-only file mode).Mutations: each new guard was disabled in turn. Each mutant was applied to exactly one site, run against the five connect and Solana test files, restored from a saved copy, and byte-compared before the next. A test that already fails on the base was not counted as a kill.
unpreparedreason; the Solana-session requirement; the EVM session required only for EVM legs; a late signature overwriting the time limit's verdict; a build after the leg was stopped; the Solana-family check.checkPlanLegwith the rebuilt Solana digest). It rebuilds from the same plan copy and connected account that the check before the page opens just passed, so no test can reach it separately. It is the same second check the EVM path makes, kept for parity.Typechecks and checks:
tsc -b,tsc -p tsconfig.scripts.json, the page typecheck,llms:check.sync-plugin.sh --checkpasses on a freshly built tree. In a Windows checkout it flagsplugin/package.jsononly because git writes it with CRLF; the committed blob is byte-identical to the generated manifest.Not verified
externalWallets.solana.connectorswhateverwalletChainTypesays (only the WalletConnect-Solana entry is gated), so the Solana hooks work under the EVM sign-in.meta.nameand the extension's Wallet Standard Solana wallet carry the same name (MetaMask in both) is also unmeasured.🤖 Generated with Claude Code