Repository navigation
feat: add pay-api and pay-worker as first-party crates - #422
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
lgalabru
left a comment
lgalabru
left a comment
There was a problem hiding this comment.
CI is currently blocked on clippy.
Greptile SummaryThis change adds the Pay API, shared Rust API crates, channel-closing and session-settlement workers, runtime configuration, container images, and publishing workflows. Redemption coordination was exercised with concurrent claims and broadcast-error handling: exactly one of 32 concurrent attempts acquired the code, deterministic preflight failures released the claim, and ambiguous transport failures retained it. Focused tests also confirmed that stale close deadlines are rechecked, expired or non-increasing vouchers are not used for closure, and reclaim batches remain byte-bounded. Two failures remain in the worker paths. T-Rex validation blockedA full Redis- and Solana-backed settlement lifecycle could not run because the environment lacks the Confidence Score: 3/5Not safe to merge until the settlement lock and distribution transaction sizing failures are fixed. Two independent failures affect core financial worker behavior: overlapping settlement runs are possible under an accepted configuration, and an accepted channel distribution can never fit in a Solana transaction. The focused redemption, close-deadline, voucher, and reclaim-batching paths behaved as expected. Files Needing Attention: rust/crates/pay-worker/src/bin/settle_sessions.rs; rust/crates/pay-worker/src/channel.rs; rust/crates/pay-worker/src/bin/close_channels.rs
 T-Rex LogsWhat T-Rex did
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ad7931d to
311cd22
Compare
Moves pay-api (Solana stablecoin balance/onramp HTTP service, confidential transfer charges) and pay-worker (close-channels operator maintenance job, settle-sessions continuous session-settlement worker) in from solana-foundation/agent-gateway, where they originated as standalone Cargo workspaces pinning pay-kit independently. Both are now regular workspace members here, sharing this repo's single pay-kit pin (already at the same rev both services were already on) instead of duplicating the dependency. solana_mpp:: source references renamed to pay_kit:: to match this workspace's alias. No behavior change — all 134 existing tests pass unmodified; fixed the clippy/fmt lints this repo's stricter CI gate (-D warnings) newly surfaces (collapsible-if, manual is_multiple_of, field-reassign-with-default, one dead field) since agent-gateway's CI never ran clippy on this code. Adds Dockerfiles (context is the whole rust/ workspace, since both are now workspace members) and GHCR publish workflows, mirroring how the pay CLI image itself is published. agent-gateway will pull these images the same way it already mirrors the pay CLI's GHCR image into its own Artifact Registry, so its Terraform needs no changes. Dropped the per-crate release profile tuning (thin LTO, symbol strip, panic=abort) that agent-gateway had — profiles are workspace-level in Cargo, and applying it repo-wide would change release characteristics for the pay CLI binary too. Worth revisiting as a deliberate follow-up.
311cd22 to
dd21394
Compare
EfeDurmaz16
left a comment
EfeDurmaz16
left a comment
There was a problem hiding this comment.
Reviewed the moved crates against the money-path bar rather than as a pure relocation, since they become first-party here. Seven inline notes; the settle-sessions expiry sentinel and the close-channels seal are the two that fire on their own without an attacker. All pre-existing from agent-gateway, so a follow-up PR is the right home for them rather than this one.
Verified at 2cc1317 in a fresh clone: cargo fmt --check clean, cargo clippy --workspace --all-targets -- -D warnings clean, cargo test --workspace --exclude pay-integration 1900 passed 0 failed.
Redemption findings are covered separately and are not repeated here.
EfeDurmaz16
left a comment
EfeDurmaz16
left a comment
There was a problem hiding this comment.
The relocation itself is clean: rust/Cargo.toml needed no edit because members = ["crates/*"] already covers the new directories, Cargo.lock is additive only so no existing pin moved, and default-members = ["crates/cli"] keeps the new gcp_kms/worker/redis-store features out of the released CLI binary. Three inline notes below, all on redemption, all pre-existing in agent-gateway rather than introduced here. Sting covers settlement, send, subscriptions and CI separately.
The theme: commit 1ee954b makes the API key mandatory so operators believe dedup is on, but the two paths that silently return "never redeemed" are still open, so the failure got quieter rather than safer. Worth splitting into a follow-up PR so this one stays a pure move.
Verified at 2cc1317 in a fresh clone: read redeem.rs, config.rs, main.rs, state.rs in full, diffed Cargo.lock and Cargo.toml against merge-base e1172d4, and traced each finding to the payout call. I could not diff the moved sources against agent-gateway, so "no behavior change" is unverified from outside.
EfeDurmaz16
left a comment
EfeDurmaz16
left a comment
There was a problem hiding this comment.
Reviewed the follow-up commits rather than the move. dd21394 is production code from agent-gateway, and the relocation itself checks out: the shipped CLI feature graph is identical to main, Cargo.lock is additive with no pin moved, and no newly enabled feature changes behavior.
The four items below are different. They were introduced by this PR's own fix commits, not inherited from agent-gateway, so they are not covered by the "this is a move" framing.
Everything else I found is pre-existing in agent-gateway and I would rather not hold a relocation on it. Happy to open a follow-up PR with those and approve this one once the four below are settled. One of them, an uncapped priority fee on the operator-signed cancel transaction, is cheap to exploit, so I would not want it to land quietly in a backlog.
Separately, worth adding a redis service to the Rust CI jobs before merge. The claim store and both leases added during this review have never executed in a test, so the green run does not cover them.
| // A rejected preflight never submits the transaction, so the | ||
| // claim can be retried. Transport and timeout errors remain | ||
| // claimed because the RPC may have accepted the transaction. | ||
| release_redemption_claim_best_effort(cfg, &req.code, &destination_pk).await; |
There was a problem hiding this comment.
This release cannot fire. mark_redemption_broadcasting at line 198 has already rewritten the value to broadcasting:{destination}, but the release script compares against claimed:{destination}, so the Lua returns 0 and the DEL never runs. release_redemption_claim_best_effort only logs on Err, so the Ok(false) is silent. With no TTL on the claim key the code is then locked permanently for a failure that provably sent no funds. Matching either prefix in the release script closes it.
There was a problem hiding this comment.
Fixed in 22223a9. The owner-checked release script now deletes either the pre-broadcast claimed:<destination> value or the post-signing broadcasting:<destination> value, so a deterministic preflight rejection can be retried.
| return Ok(None); | ||
| } | ||
| cursor = txs.last().map(|t| t.signature.clone()); | ||
| if page + 1 == cfg.max_scan_pages { |
There was a problem hiding this comment.
For a code with no prior burn the scan can only reach Ok(None) from an empty or underfull page, so proving absence means walking the hot wallet's history to the end. That wallet is send.fee_payer, shared with /v1/send, and every successful redemption appends to it, so once it passes max_scan_pages * 100 transactions every fresh redemption returns 502. The fail-closed direction is right; bounding the scan at the claim-store cutoff keeps it without the cliff, since codes issued after that cutoff are already deduped by Redis.
There was a problem hiding this comment.
Fixed in 22223a9. Redemption now requires legacy_scan_cutoff_unix_seconds, the Redis claim-store rollout timestamp. The Helius scan stops once it reaches that boundary, retaining legacy memo detection beforehand while relying on durable Redis claims afterward instead of failing every fresh redemption once the hot wallet grows.
| method: query_param | ||
| key: "api-key" | ||
| value_from_env: HELIUS_API_KEY | ||
| value_from_env: SOLANA_RPC_API_KEY |
There was a problem hiding this comment.
This file predates the move by two months and its siblings all use provider-specific names (DASHSCOPE_API_KEY, GOOGLE_API_KEY), so this rename makes helius the only one off the pattern, for a proxy config that is specifically a Helius endpoint. It also means pay-api and pay gate api now read the same variable, so a host running both can no longer give them separate keys. If the rename is wanted anyway, worth doing the siblings in the same pass rather than one file.
There was a problem hiding this comment.
Reverted in 22223a9. The Helius proxy is back on HELIUS_API_KEY, keeping its provider-specific credential and preserving separate keys for the proxy and pay-api.
| hard_failures += reclaim_outcome.failures; | ||
| for lease in reclaim_leases { | ||
| lease.release().await; | ||
| } |
There was a problem hiding this comment.
Reopening greptile's thread here since it was resolved without a fix commit or a reply. The lease is acquired with a fixed 300s TTL, held across the whole channel loop and then the reclaim batches, and never renewed, while each confirmed broadcast can take up to confirm_timeout (60s default), so a handful of channels exceeds it. The same shape applies to the settlement lock and the redemption claim, which are also acquire-once with no renewal. If this is a deliberate accept, worth saying so on the thread.
There was a problem hiding this comment.
Fixed in 22223a9. Reclaim leases now use an owner-checked EXPIRE heartbeat every third of the TTL and cancel/await it before owner-checked release. I applied the same renewal to the settle-sessions lock. The redemption claim itself intentionally has no TTL, so it does not have this expiry race.
EfeDurmaz16
left a comment
EfeDurmaz16
left a comment
There was a problem hiding this comment.
Approved, previous blockers look fixed at 22223a9: claim release, scan cutoff, Helius env revert, and lease heartbeats.
Please track follow-ups before enabling cancel, redeem, or forced-close in prod: cap cancel priority fees, settle vouchers during CLOSING, and add Redis to Rust CI. Fine to merge the move with those tracked and all commits are signed.
Co-authored-by: Ludo Galabru <ludo.galabru@solana.org> Signed-off-by: Ludo Galabru <ludo.galabru@solana.org>
22223a9 to
d5e1caf
Compare
| DEFAULT_FINALIZED_CHANNEL_RETENTION.as_secs(), | ||
| )?); | ||
| let dry_run = parse_bool_env("DRY_RUN", true)?; | ||
| let lock_ttl = parse_u64_env("SETTLEMENT_LOCK_TTL_SECONDS", 300)?; |
There was a problem hiding this comment.
SETTLEMENT_LOCK_TTL_SECONDS=0 is accepted here. Initial acquisition clamps it to one second (ttl_seconds.max(1)), but the heartbeat is started with the original zero value and renews with EXPIRE <lock> 0, which deletes the Redis key. A second worker can then acquire the singleton lock while the first worker is still reconciling and broadcasting, allowing duplicate settlement or lifecycle submissions. Reject zero during configuration, or normalize the value once before both acquisition and heartbeat renewal.
Artifacts
Focused settlement-risk harness source
- Shell harness created without changing product code; it executes the compiled due-close and voucher safety tests, showing the exact executable paths reviewed.
Captured focused settlement-risk harness source
- Captured command output prints the exact harness content and its three worker-test invocations, showing reproducible focused validation.
Focused due-close and voucher harness passed
- Harness execution passed all three focused paths: stale-deadline recheck, due push-channel gating, and newer unexpired voucher selection, showing those unit paths behave safely.
All settle-sessions unit tests passed
- Complete compiled worker unit suite ran eight tests successfully, showing the existing worker-level pure logic tests pass.
Lease TTL source path showing zero-renewal defect
- Captured numbered source path shows acquisition clamps TTL with `max(1)` while heartbeat renews the original TTL, proving a configured zero deletes the lease.
| /// A distribution must fit in a Solana transaction together with the fixed | ||
| /// channel accounts. Keep malformed preimages from requesting an unbounded | ||
| /// allocation and reject plans that cannot be represented safely. | ||
| const MAX_DISTRIBUTION_RECIPIENTS: usize = 64; |
There was a problem hiding this comment.
The worker accepts up to 64 distribution recipients, but the corresponding distribute instruction cannot fit in a Solana transaction. The executed harness built the production instruction and measured a 64-recipient transaction at 4,794 bytes against the 1,232-byte maximum. process_channel sends non-reclaim steps without the byte-size validation used for reclaim batches, so an operator signs and submits a transaction that the RPC or validator rejects, leaving the channel undistributed. Validate every planned non-reclaim transaction before signing, and constrain recipient counts to the actual transaction budget.
Artifacts
Actual distribute serialization harness source
- The temporary integration harness calls the project's `build_distribute_ix` and pay-kit's production `tx_size` helper for one- and 64-recipient distributions, proving the measurement uses the actual instruction construction.
Actual distribute serialization harness manifest
- The manifest records the harness dependencies and the project worker dependency used for the executed integration test.
Actual distribute serialization harness output
- The executed harness passed both tests and recorded 572 bytes for one recipient versus 4,794 bytes for the actual 64-recipient distribute transaction, proving the accepted maximum cannot fit.
Close-channels reclaim batching test output
- The executed worker binary tests passed the two-channel, operation-cap, and varied-rent-payer byte-bounded reclaim batching cases.
Close-channels source locations and cleanup check
- The captured source lines show the 64-recipient limit, actual distribute account construction, unchecked non-reclaim send path, and that the temporary integration test file was removed.
Executed harness source capture
- The captured harness source and manifest show exactly what was copied into the temporary project integration-test location and executed.
Summary
pay-api(Solana stablecoin balance/onramp HTTP service, confidential-transfer charges) andpay-worker(close-channelsoperator maintenance job,settle-sessionscontinuous session-settlement worker) in fromsolana-foundation/agent-gateway, where they lived as standalone Cargo workspaces pinningpay-kitindependently.pay-kitpin already used everywhere else here (same rev they were already on — no wire-contract drift).solana_mpp::source references renamed topay_kit::to match this repo's dependency alias.Dockerfiles (build context is the wholerust/workspace, since both are workspace members now) and GHCR publish workflows, mirroring how thepayCLI image itself is published.Companion PR
solana-foundation/agent-gateway#17 removes the source from that repo and repoints its Docker-publish workflow to mirror these images from GHCR into its own Artifact Registry — no Terraform changes needed there, since the Artifact Registry image paths stay the same.
Notes
-D warnings) newly surfaces on this code (agent-gateway's CI never ran clippy against it): collapsible-if, manualis_multiple_of, field-reassign-with-default, one genuinely-dead field (#[allow(dead_code)]'d with a note).panic=abort) agent-gateway had — profiles are workspace-level in Cargo, and applying it repo-wide would change release characteristics for thepayCLI binary too. Flagging as a deliberate follow-up if we want it back.Test plan
cargo test --workspace --exclude pay-integrationpassescargo clippy --workspace --all-targets -- -D warningscleancargo fmt --checkcleandocker build -f rust/crates/pay-api/Dockerfile rustsucceeds, binary runs (fails closed on missing config, as expected)docker build -f rust/crates/pay-worker/Dockerfile rustsucceeds, both binaries run (fail closed on missing config, as expected)