feat(payments-v2): transfer a coinless token - #782
Conversation
|
@codex review Out of draft — the implementation is complete. Since the draft, this branch gained Worth the attention, in rough order of consequence:
Verification on Still owed and stated in the description: a staging NFT round-trip leg, alongside the coinless READ |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54235c9d00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…to read both shapes Reader before writer: nothing in this commit writes a token-addressed intent. It lands the union and the resume path that understands it, so a client can never meet a payload shape older code cannot parse. `IntentPayload` becomes `CoinIntentPayload | TokenIntentPayload` on a still-`v:2` envelope — the coin payload's stored shape did not change. `kind` is REQUIRED on both arms, never optional: a missed writer must be a compile error, not a payload that silently reads as a coin spend. The compiler duly found all thirteen coin-shaped reads, now narrowed through `machine/payload-view.ts` rather than by scattered `kind ===` checks. A `TokenIntentPayload` names EXACTLY one source (`direct: [string]` by type) and can never carry a split. `resume.validatePayload` re-checks both rather than trusting the type across a decrypt boundary, because a second leg would make `settlePartial` reachable with every conflict amount 0n — a remainder of '0' completes the intent and reports SUCCESS for a leg that never landed. An ABSENT `kind` reads as `'coin'`. That is a migration rather than a guess: it is the only shape any client wrote before this change. Also fixed, found while re-reading the mirror against §16: `applyOne` inherited `assets` on an ACTIVE row that omitted them. Absent assets on an active row STATE coinlessness, so inheriting left stale assets in `tokens()` while `coinless` was true — the same row in both reads at once, shown as an NFT while holding 100 coins. Only a tombstone may inherit now, which is what `recoverRemoved` actually needs. Caught by a test written to fail first. A token-addressed send records `assets: []` plus its `tokenId` in history, the shape wallet-api#151 accepts; `PendingTransfer` gains `tokenId` so an open token intent renders as the token instead of `coinId: ''` / `amount: ''`. Verified: 450 payments-v2 tests green; typecheck, typecheck:tests and lint clean. Refs #777.
payments.sendToken({ recipient, tokenId, memo? }): Promise<TransferResult>
A second verb rather than a widened `send()`: the addressing model is genuinely
different. A coin spend SELECTS sources to cover an amount and may queue for a
combination that frees up; a token spend reserves the one it was NAMED. There is
no amount, no combination, no split and no change.
**One money path, not two.** The same `TransferMachine`, the same durable intent,
the same checkpoints, mailbox deposit and applyDelta. `sendWithPolicy` is one loop
over a `SendJob` union; the two spends diverge in exactly one place —
`planAndMaterialize` — and nowhere else.
`SpendQueue.planToken` reserves the named source behind the SAME #738 fail-closed
gate `freeView()` applies: while the held-set is unproven nothing is spendable, or
a restart could double-spend a source an open intent already holds. It never
queues, because queueing waits for some other combination to free up and a named
token has none — it would block until a timeout on a spend that cannot become
possible. A token already held by another reservation is refused outright.
**A proven conflict is TERMINAL for a named source.** #625's bounded re-plan exists
to pick a DIFFERENT source after a lost race; a named token has no alternative, so
re-planning would re-pick the same one or nothing. Retrying it would spin.
Three independent gates keep a valued token out, because each covers a different
failure: `spendableCoinless` on the mirror (the fast, ordinary refusal),
`planToken`'s reservation check (concurrency), and a re-check of the decoded BLOB
in `materializeTokenSpend` — the blob is the authority on what a token carries, and
the mirror disagreeing with it is exactly the case where coins would move
unaccounted for.
`pool()`'s eligibility gates are now `isSpendable()`, shared by both verbs, so one
probe covers both and they cannot drift apart.
Extracted to stay under the 800-line ceiling the additions crossed:
`modules/payments-v2/mint.ts` (the journal-first mint and its finalize, a pure move
behind an explicit deps object) and `send-token.ts`.
Verified: 2288 tests green (7 new, incl. a valued-token refusal, an unknown-token
refusal that reserves nothing, a concurrent double-spend refusal, and a coin send
proving selection never saw the coinless token); 6 new mutation probes; typecheck,
typecheck:tests and lint clean.
Refs #777.
Connect 2.1 → 2.2. Additive: the `send` intent and `transfer:request` scope are untouched, and the handshake gate is MAJOR-only, so no existing dApp is cut off. `send_token` gets its OWN scope rather than reusing `transfer:request`. Mapping it onto the existing one would silently widen every dApp that already holds it — a page granted coin transfers could move an NFT it was never authorised for. The two are independent in both directions, pinned by tests: `transfer:request` does not authorise `send_token`, and `token:transfer` does not authorise `send`. ConnectHost needs no change: an intent frame is `params: Record<string, unknown>` forwarded verbatim after the channel gates, so the wallet app's `onIntent` is what interprets it. The protocol surface guard did its job — it failed with "bump SPHERE_CONNECT_VERSION + update EXPECTED", which is exactly the prompt it exists to give. Counts move 14 methods / 6 intents / 13 scopes → 14 / 7 / 14. Verified: 2291 tests green (3 new on scope independence); typecheck, typecheck:tests, lint and build clean. Refs #777.
Covers the transfer half of #777 across the API reference, CLAUDE.md's method table and Key Concepts, and the CHANGELOG: why it is a separate verb, why a proven conflict is terminal for a named source, the three gates that keep a valued token out, and why send_token carries its own scope rather than reusing transfer:request.
…ken spend All three survivors were mine and all three were in the money path — each because the test asserted at the wrong layer, not because the probe was wrong. - The #738 fail-closed gate on planToken had no test at all: the facade tests never drive an unproven ledger. Now asserted directly on SpendQueue, both ways. - The blob re-check was unreachable from the facade: spendableCoinless refuses a valued token first, so the mirror gate killed every case before the blob guard ran. Tested against materializeTokenSpend directly, which is the layer the probe mutates — a mirror that says coinless while the blob carries coins. - Nothing drove a conflict on a token send. The new test seeds a SECOND coinless token, so if the re-plan ever ran it would have somewhere to go: moving a token the caller never named is worse than the failure.
Both from `codex exec review` of the transfer branch, and both in the resume path — which is the one I built reader-first specifically to be safe, so they matter more than their P2 label suggests. **An EXPLICIT unknown `kind` migrated to 'coin'.** Only an ABSENT discriminant may do that: it is the one shape written before #777. An explicit unknown one — a newer client's payload, or a corrupted one — was falling through to coin semantics it was never written for, and a coin-SHAPED payload makes the discriminant the only thing telling them apart. Now refused; the intent stays open and untouched rather than executing under the wrong meaning. **Resume did not re-check the blob.** `materializeTokenSpend` treats the decoded blob as the authority on what a source carries, but the resume path decoded its sources and went straight to transferring them. A durable intent labelled `kind: 'token'` whose named source actually holds coins would have moved them while the history row for it records `assets: []` — value moved, nothing accounted for. `runOne` now applies the same check and fails closed, which is what it already does for a missing source. Verified: 3 new tests staging raw intents the way a foreign client would write them (an explicit unknown kind, an absent kind still migrating, and a token intent naming a valued source); 2 new mutation probes; full suite green. Refs #777.
From the review. `sendToken` is on the public `PaymentsV2` surface but its request type was not re-exported, so a consumer of the `./payments-v2" subpath could see the method and not type a call to it. `CoinlessToken` was already reachable via the root `export * from './types'`; only this one was missing. Verified against the BUILT declarations, not just the source.
`sendToken` had no e2e coverage at all. Everything about the transfer was proven against fakes plus the aggregator-backed engine — which shows this client is self-consistent, never that it and the service agree. Two wallets, a real testnet2-certified coinless token with the registry's non-fungible type, moved through the deployed backend: - the send certifies on-chain and reports one direct leg, never a split - B ACCEPTING it implies the full real trust-base verify + isOwnedBy passed, since Receive screens before it stores or claims - the payload comes back byte-identical through a DIFFERENT wallet's blob fetch — the round trip survives CBOR encode, content addressing, S3, the mailbox, the claim and a second wallet's decode - the token leaves A, and A's balance is untouched throughout - a VALUED token is refused by sendToken against the real backend, before any chain op: the coin is still spendable afterwards Verified: 5/5 green against wallet-api staging on testnet2.
`token:transfer` sitting next to `transfer:request` distinguished nothing — the two read as the same thing, and a permission scope appears in a consent dialog where it has to be self-evident. A code comment is the wrong place to carry that. `send_token` had the same defect from the other side: coins are tokens too, so "send token" does not say which kind moves. INTENT_ACTIONS.SEND_TOKEN 'send_token' -> SEND_NFT 'send_nft' PERMISSION_SCOPES.TOKEN_TRANSFER 'token:transfer' -> NFT_TRANSFER 'nft:transfer' `TOKEN_TRANSFER` was also already taken in this repo: it named the removed Nostr event kind 31113 (docs/LEGACY-INVENTORY.md), so the constant collided with prior art that still appears in a test assertion. Pre-release: Connect 2.2 has not shipped, so nothing is renamed out from under a dApp. The spec's "coinless, never non-fungible" rule is about naming the PROPERTY in prose — for a scope a user reads in a consent prompt, "nft" is the word that communicates. Verified: full suite green; the protocol surface guard's EXPECTED updated with it.
…at names no coin Follow-through on the Connect rename: the same defect ran through the SDK side. "Token" was used to mean "coinless token" in a dozen places, and coins are tokens too — so the name distinguished nothing, exactly as `token:transfer` beside `transfer:request` did not. kind: 'coin' | 'token' -> 'coin' | 'coinless' (durable intent field) sendToken -> sendCoinless SendTokenRequest -> SendCoinlessRequest planToken -> planCoinless materializeTokenSpend -> materializeCoinlessSpend buildTokenPayload -> buildCoinlessPayload TokenIntentPayload -> CoinlessIntentPayload isTokenIntent -> isCoinlessIntent TokenSpendDeps/Input -> CoinlessSpendDeps/Input send-token.ts -> send-coinless.ts The discriminant is why this could not wait. It is DURABLE state — field-encrypted into the intent payload the server stores, and read back on resume — so renaming it after a release is a data migration, not a refactor. Nothing has shipped and the only `kind: 'token'` payloads that exist came from this branch's own staging runs, so the change is free exactly now and never again. One vocabulary per audience: the SDK says *coinless*, matching wallet-api's spec rule, and the Connect wire keeps *nft* (`send_nft`, `nft:transfer`) because a consent prompt is read by a person, for whom "coinless" communicates nothing. Unchanged deliberately: `tokenData(tokenId)` works on ANY token and is named correctly, and `Token`/`tokens()` keep meaning coin tokens — that ambiguity predates this work and renaming it is a breaking change for every consumer. Verified: 2302 tests green; typecheck, typecheck:tests and lint clean. The historical CHANGELOG entries about the removed `sendTokenTransfer` transport member were deliberately left alone.
The runner flagged them STALE rather than passing silently — the `find` strings pointed at `kind: 'token'` comparisons the rename replaced. Updated in place with the refactor, never dropped: a probe whose target moved is a probe guarding nothing, and deleting one hides that its invariant lost coverage. `sendtoken-conflict-replans` renamed to `sendcoinless-conflict-replans` to match the verb it guards.
fd7fd59 to
ec59c6d
Compare
…ue === null From review (P1). `materializeCoinlessSpend` and the resume twin both asked `token.value !== null`. But `bare_collection` — the bridged dialect carrying coins this SDK cannot decode — decodes to a null value exactly like a genuinely coinless token. Either guard would therefore have moved a valued token and recorded `assets: []` for it: coins gone, nothing accounted. This is the ambiguity `valueEnvelope` exists to resolve, and the rule is written into CLAUDE.md — `none_*` means "names no coin", `bare_collection` means "cannot read". I documented it, fixed one instance of it in the receive path after an earlier review, and then wrote the same bug into both new guards. `isCoinlessEnvelope` moves to `token-engine/value-envelope.ts` beside the type it interprets, and all three call sites use it. It was a private helper in Receive.ts — three copies of a question this subtle is how they drift apart again. Test seams needed for it: `engineWith` takes an explicit envelope, and the resume harness gains `forceEnvelope` so a bridged source can be staged at all. Neither path could express the case before, which is why neither caught it. Also fixes the CI lint break: a duplicated comment block in resume.ts (two paragraphs saying the same thing about the absent-kind migration) and the ratio in value-envelope.ts. Verified: 2307 tests green over two clean runs; typecheck, typecheck:tests, lint. Probes retargeted onto the envelope check, plus one new on the shared helper.
|
@codex review Head is Your P1 is fixed — and it was the sharp one. Naming changed since you last read this, which is worth knowing before you diff: Worth your attention, in order of consequence:
Verification on this head: 2307 tests / 132 files, typecheck, typecheck:tests, lint, build; e2e |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Closes #777.
Completes the half of #777 that #779 (merged) deliberately left out. #779 was display and receive;
this changes the money path, which is why it got its own review rather than riding along.
A coinless token — one whose genesis data carries no value envelope — can now be moved:
A second verb, not a widened
send()The addressing model is genuinely different. A coin spend selects sources to cover an amount and
may queue for a combination that frees up; a coinless spend reserves the one token it was named.
No amount, no combination, no split, no change.
planCoinlesstherefore never queues: queueing waits for some other combination to becomeavailable, and a named token has none — it would block until timeout on a spend that cannot become
possible. It applies the #738 fail-closed gate itself, because it bypasses
freeView()wherethat gate lives for coin spends; without it a restart could double-spend a source an open intent
already holds.
One money path, not two. Same
TransferMachine, durable intent, checkpoints, mailbox depositand
applyDelta.sendWithPolicyis a single loop over aSendJobunion and the two spendsdiverge in exactly one function.
The durable intent is discriminated
IntentPayloadbecomesCoinIntentPayload | CoinlessIntentPayloadon a still-v:2envelope — thecoin payload's stored shape did not change.
kindis REQUIRED on both arms. A missed writer must be a compile error, not a payload thatsilently reads as a coin spend. The compiler duly found all thirteen coin-shaped reads, now
narrowed through
machine/payload-view.ts.direct: [string]by type) and can never carry asplit — re-checked in
resume.validatePayloadrather than trusted across a decrypt boundary,because a second leg makes
settlePartialreachable with every conflict amount0n, so aremainder of
'0'completes the intent and reports success for a leg that never landed.kindmigrates to'coin'(the only shape written before feat(payments-v2): represent and expose non-fungible tokens — tokens() drops every coinless token #777). An explicitunknown one is refused — a coin-shaped payload from a newer client would otherwise execute
under semantics it was never written for.
kind: 'coinless'whose named source actually holds coins would move them while its history row records
assets: [].Three gates keep a valued token out
Separate because each covers a different failure:
spendableCoinless(mirror)materializeCoinlessSpendA proven conflict is TERMINAL for a named source. #625's bounded re-plan exists to pick a
different source after a lost race; a named token has no alternative. Re-planning also replaces the
true cause (
source state already consumed by foreign) with a misleading "not a spendable coinlessholding", hiding from the caller that someone else spent their token.
Connect 2.1 → 2.2
A
send_nftintent with its ownnft:transferscope. Additive, and the handshake gate isMAJOR-only, so no existing dApp is cut off. The scope is deliberately not
transfer:request—reusing it would silently widen every dApp already holding it. Both directions are pinned by tests.
ConnectHost needs no change: an intent frame is
params: Record<string, unknown>forwarded verbatim.Naming (post-review)
Review flagged that
token:transferbesidetransfer:requestdistinguished nothing — and apermission scope is read by a human in a consent prompt, so a code comment is the wrong place to
carry the distinction. The same defect ran through the SDK: "token" meant "coinless token" in a dozen
places, and coins are tokens too.
TOKEN_TRANSFERwas also already taken here — it named theremoved Nostr event kind 31113 (
docs/LEGACY-INVENTORY.md), which a live test still asserts against.One vocabulary per audience: the SDK says coinless (matching wallet-api's spec rule), the Connect
wire says nft, because that surface is read by a person for whom "coinless" communicates nothing.
The discriminant is why this could not wait: it is field-encrypted into the intent payload the
server stores and read back on resume, so renaming it after a release is a data migration rather than
a refactor. Nothing had shipped, and the only
kind: 'token'payloads that ever existed came fromthis branch's own staging runs.
Left alone deliberately:
tokenData(tokenId)works on ANY token and is named correctly, andToken/tokens()keep meaning coin tokens — that ambiguity predates this work and renaming itbreaks every consumer.
Also fixed here
A bug in #779, found re-reading the mirror against §16:
applyOneinheritedassetson an ACTIVErow that omitted them. But absent assets on an active row state coinlessness, so inheriting left
stale assets in
tokens()whilecoinlesswas true — the same row in both reads, shown as anNFT while holding 100 coins. Only a tombstone inherits now, which is what
recoverRemovedneeds.Test written to fail first.
Verification
npm run test:run— 2302 tests / 132 files green.npm run test:mutation— 150/150 KILLED.npm run test:e2e— 5/5 green against wallet-api staging on testnet2, including a realcoinless token moved A→B: the send certifies on-chain, B accepting it implies the full real
trust-base verify +
isOwnedBypassed (Receive screens before storing), the payload returnsbyte-identical through a different wallet's blob fetch, and a valued token is refused before any
chain op with its coin still spendable afterwards.
codex exec reviewover three rounds — five findings, all fixed and pinned by tests plus probes:the two resume gaps above, the
mintDataTokenpost-certification stranding, and a missingSendCoinlessRequestexport.The mutation gate separately surfaced three coverage gaps, two of them real holes rather than
bookkeeping: nothing tested the #738 gate on
planCoinless, and the blob guard was unreachable fromthe layer it was tested at because the mirror gate refused first.
Rebased onto
mainafter #779 squash-merged.