feat(payments): gate priced invocations behind the transparent payment lifecycle - #111
Conversation
|
Great PR — the test discipline here is the best of the series (wall-time overlap bound, settle-poll positive controls, honest comments on the declared gaps). Both ts divergences verified against the reference and endorsed; we'll file them upstream. One real finding to address before merge, plus a few optional nits. Should fix in this PRA redelivered paid streaming request loses its open-stream writer. The PR documents that a duplicate delivery's chain run pops the route (snapshot compensates) — but the same drop-cleanup also removes and disposes the open-stream writer slot, which nothing compensates:
Reproduced on this branch: paid Since the slot-release is this PR's code, suggest fixing here: in the drop-cleanup, skip the slot release when a live payment snapshot exists for the event id — "a payment owns this request" is exactly what the snapshot map encodes. A test next to Optional / take or leave
Not asking to changeThe declared divergences (retention split, unconditional forward) are correct per spec — keep them, comments included. The client correlation bug stays in PR 10 (note: it's an rs port regression — the ts client already classifies by JSON-RPC type, so the fix has a reference implementation to mirror). The no-fragmentation gap, oversized PMI, and replay-after-TTL declarations all stand as documented. |
|
Thanks @ContextVM-org, the reproduction was exact. Fixed in fb04dc8, but I took the finding and not the witness. A live payment snapshot means "an invoice went out recently", not "a live delivery owns this request": it stays live after a payment fails, so guarding on it holds the slot and token index entry of every declined invoice until the transport closes, drivable by asking for priced streaming calls and never paying. That is a test, not an argument: an_unpaid_streaming_call_releases_its_open_stream_slot. It is also blind to the case with no payments at all. Instead the slot counts the inbound chain runs in flight for its event id and carries a flag set by the terminal with the successful tx.send; the cleanup reclaims only when the count is zero and the flag is unset, in one critical section. Happy to switch if you still prefer the snapshot guard. Your step 1 mattered on its own: a redelivery replacing the slot breaks a live stream even when nothing releases it, since send_response then reads has_started() == false and answers on the plain path. create_open_stream_writer is now first-writer-wins, with its own CHANGELOG entry as 0.2.2 code. Four tests cover all four timings (during the verify, after the forward mid-stream, after the forward before the first frame, and before the forward). The first two were watched failing on the pre-fix tree; all four fail when I delete the guard they pin. cap 0: taken in de769a2, as a deliberate divergence rather than a parity fix. ts's LruCache.set evicts at size >= capacity, so it steady-states at one payment rather than refusing. Caveat now in the rustdoc: gating never reads this cap, so after #113 a zero cap refuses transparent and serves gating. Say the word and I will drop it for parity. Join bound: deferred as you offered, not in this PR. The bound that matters is on the initiation phase, which is deliberately unbounded for parity, so it is a cross-SDK question rather than a local fix. Capture order: #113 already builds both senders after the announcement tags and pins the ordering in its tests. The Arc refactor stays open for hand-wirers. Divergences, gaps and the route pop are untouched. The PR 10 correlation fix landed in #114 as you described. Heads up for the stack: #112 needs a CHANGELOG merge and one line of code, since f2d9a69 gives run_inbound_chain an eighth parameter. |
Part of #100
Seventh piece of CEP-8, after the targeted sender in #109 and the handshake fix in #110, and the first one where money moves. A priced invocation now triggers
notifications/payment_required, waits for the processor to verify settlement, emitsnotifications/payment_accepted, and only then reaches the MCP handler. A pricing callback can reject (emitsnotifications/payment_rejected, drops) or waive (forwards untouched). Duplicate deliveries of one request event share one payment. And because a payment can outlive the 60 s stale-route sweep, the transport captures the request's routing fields when the invoice goes out and delivers the eventual result from that capture.Nothing registers itself yet: the middleware is exported and wired up only by tests. Registration, explicit gating, and client-side auto-pay are later PRs.
What's here
src/payments/server_payments.rs: options, the factory returningArc<dyn InboundMiddleware>, the pending-payment dedup, the lifecycle.src/payments/server_payments_utils.rs(crate-private): capability matching, PMI selection, timeout arithmetic, resolve-and-initiate. The explicit-gating middleware will share it.payment_notification_sender(snapshot_ttl)returning the notification publish as a closure for the detached middleware, the route-snapshot map, and a route-miss fallback insend_responsethat delivers a paid response after the sweep.InboundContextgains a per-event cancel token (child of the transport's shutdown token), and the drop-cleanup now releases the open-stream writer slot a gated request reserved.tests/payments_transparent_e2e.rs, 7 in-crate.Two deliberate divergences from ts
payment_acceptedpublish abort the forward, which is paid-but-undelivered. Here the failure is logged and the forward happens anyway: the notification is a SHOULD, the result is the point, and the failure is reachable (a paying client is idle by definition and the session LRU evicts idle sessions). The site carries a comment telling future refactors not to tidy it back into an early return.Both look worth reporting upstream as ts defects.
Why it's built this way
tools/callwith a progressToken that never streams takes the deferral's passthrough branch with its slot already deleted, so a fallback beside those arms would never run for it. When both a slot and a snapshot exist the open-stream arm wins and the snapshot is consumed unused. The same snapshot also covers a route popped by a duplicate delivery, a loss path ts shares.send_notification's body moved into a shared static that both the method and the injected sender call, so the two cannot drift on tags or wrap-kind policy.Heads up
Like the targeted sender, the closure captures the discovery tag set when built: build it after setting the announcement tags. Pass the middleware's payment TTL as
snapshot_ttlso snapshots outlive every payment (delivery is session-independent, so this works past the session timeout). And one finding outside this PR's scope: the client transport consumes its response-correlation entry on the first correlated message of any kind, sopayment_requiredmakes our own client drop the acceptance and the paid response. Server-side delivery is correct and asserted on the wire; the fix is recorded for the client payments PR, which without it never sees a paid result.Declared and bounded rather than fixed: the dedup is TTL-bounded while the spec's MUST is unbounded (ts identical; the options doc says so); a verify timeout emits nothing (ts parity, the spec has no signal for it); a waiver's metadata is dropped (ts behavioral parity, against its own docs); a stored priced request can be replayed by a third party after TTL, so the future auto-payer must ignore invoices for requests it has no live pending entry for; oversized priced requests always take the first-processor fallback because our clients put
pmitags on the start frame while identity comes from the end frame; and the snapshot delivery inherits the deferred path's no-fragmentation gap, now reachable for a paid result.