feat(billing): credit managed wallet from manual-credit Stripe invoices#3398
Open
baktun14 wants to merge 4 commits into
Open
feat(billing): credit managed wallet from manual-credit Stripe invoices#3398baktun14 wants to merge 4 commits into
baktun14 wants to merge 4 commits into
Conversation
Teach the Stripe webhook to recognize comped "manual_credit" invoices (created
out-of-band by the admin app with metadata {type:manual_credit,userId,reason})
and turn invoice.paid / invoice.payment_succeeded into a real, spendable
managed-wallet credit. There is no pre-created transaction row for these, so the
row is inserted keyed on the invoice id.
A partial unique index on stripe_invoice_id (WHERE NOT NULL) makes crediting
exactly-once across duplicate/retried and double-fire deliveries (invoice.paid +
invoice.payment_succeeded). The credit uses endTrial:false so a granted credit
never graduates a trial user. Unmatched non-manual-credit invoices keep
no-opping (double-credit guard).
Covered by unit, real-DB integration (dedupe), and functional (HTTP end-to-end)
tests.
Part of CON-625
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a ChangesManual credit invoice support
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3398 +/- ##
==========================================
- Coverage 71.69% 70.62% -1.08%
==========================================
Files 1149 1059 -90
Lines 28974 26643 -2331
Branches 7105 6648 -457
==========================================
- Hits 20774 18816 -1958
+ Misses 7203 6863 -340
+ Partials 997 964 -33
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/drizzle/0030_true_jetstream.sql`:
- Around line 1-2: The migration combines an enum alteration with a blocking
unique index build, so split the `CREATE UNIQUE INDEX` on `stripe_transactions`
out of `0030_true_jetstream.sql` into a separate non-transactional migration.
Keep the `ALTER TYPE` in the transactional migration, and add the index in a
follow-up migration that runs with `CONCURRENTLY` and outside Drizzle’s
transaction flow. Use the existing
`stripe_transactions_stripe_invoice_id_unique` index name to place the change
correctly.
In
`@apps/api/src/billing/services/stripe-webhook/stripe-webhook.service.integration.ts`:
- Around line 335-378: The webhook flow currently keeps
`refillService.topUpWallet(...)` inside the `@WithTransaction()` path in
`StripeWebhookService.tryToTopUpWalletFromInvoice`, even though it triggers
external authz/funding calls through
`ManagedUserWalletService.authorizeSpending()`. Refactor the method so the DB
transaction only covers invoice/transaction persistence, then invoke
`topUpWallet()` after the transaction has completed; update any affected
expectations in the `tryToTopUpWalletFromInvoice` tests to reflect the same
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b2a10255-ec06-4987-a356-ab322f2dc195
📒 Files selected for processing (9)
apps/api/drizzle/0030_true_jetstream.sqlapps/api/drizzle/meta/0030_snapshot.jsonapps/api/drizzle/meta/_journal.jsonapps/api/src/billing/model-schemas/stripe-transaction/stripe-transaction.schema.tsapps/api/src/billing/repositories/stripe-transaction/stripe-transaction.repository.integration.tsapps/api/src/billing/repositories/stripe-transaction/stripe-transaction.repository.tsapps/api/src/billing/services/stripe-webhook/stripe-webhook.service.integration.tsapps/api/src/billing/services/stripe-webhook/stripe-webhook.service.tsapps/api/test/functional/stripe-webhook.spec.ts
…oice metadata Switch the manual-credit crediting path to reuse the existing coupon-claim mechanism: the admin app (CON-626) pre-creates the pending stripe_transactions row via direct DB write, and apps/api credits the matching row through the existing updateTransactionAndTopUp path. Drops the metadata-driven webhook branch, creditManualCreditInvoice, and insertIfInvoiceAbsent. Keeps the manual_credit enum value, the partial unique index on stripe_invoice_id (now guarding one row per invoice for both paths), the invoice.paid subscription, and the endTrial handling. Exactly-once and the double-credit guard now come entirely from the existing row lock + already succeeded check. Tests updated: matched-row unit test, unique-index integration test, and pre-created-row functional tests. Part of CON-625
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/billing/services/stripe-webhook/stripe-webhook.service.integration.ts (1)
335-366: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider a dual-delivery idempotency test.
Coverage here proves a single
invoice.paidcall credits correctly, but the interesting new risk isinvoice.paidandinvoice.payment_succeededboth firing for the samemanual_creditinvoice. A test callingtryToTopUpWalletFromInvoicetwice (once per event variant) against the same transaction, assertingrefillService.topUpWalletfires only once, would directly validate the idempotency guard this PR relies on.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/billing/services/stripe-webhook/stripe-webhook.service.integration.ts` around lines 335 - 366, Add a dual-delivery idempotency test for Stripe manual credits: in the stripe-webhook.service.integration spec, exercise tryToTopUpWalletFromInvoice twice for the same manual_credit transaction using both invoice.paid and invoice.payment_succeeded event variants. Reuse the same mocked transaction from setup()/createMockTransaction and assert refillService.topUpWallet is invoked only once while the transaction is marked succeeded via stripeTransactionRepository.updateById.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/api/src/billing/services/stripe-webhook/stripe-webhook.service.integration.ts`:
- Around line 335-366: Add a dual-delivery idempotency test for Stripe manual
credits: in the stripe-webhook.service.integration spec, exercise
tryToTopUpWalletFromInvoice twice for the same manual_credit transaction using
both invoice.paid and invoice.payment_succeeded event variants. Reuse the same
mocked transaction from setup()/createMockTransaction and assert
refillService.topUpWallet is invoked only once while the transaction is marked
succeeded via stripeTransactionRepository.updateById.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c33391b9-9b49-434f-b730-4dea912ff683
📒 Files selected for processing (4)
apps/api/src/billing/repositories/stripe-transaction/stripe-transaction.repository.integration.tsapps/api/src/billing/services/stripe-webhook/stripe-webhook.service.integration.tsapps/api/src/billing/services/stripe-webhook/stripe-webhook.service.tsapps/api/test/functional/stripe-webhook.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/test/functional/stripe-webhook.spec.ts
Replace the inline ["coupon_claim","manual_credit"].includes(...) check with a
type-safe GRANTED_CREDIT_TRANSACTION_TYPES set co-located with StripeTransactionType,
so new credit-grant enum values live in one discoverable place.
Clarify the invoice partial-unique-index comment: it enforces one row per invoice;
exactly-once crediting comes from the locked status check in the webhook.
Simplify integration-test user creation to create({}); consolidate the two duplicate
routeStripeEvent tests via it.each; drop the never-passed amountPaid payload param.
Contributor
Author
|
Addressed the review feedback:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Part of CON-625.
An operator needs to grant spendable credit to a customer's managed wallet (e.g. enterprise GPU prepay via Brex ACH) without the customer paying or redeeming a coupon. The finance-approved mechanism is a comped Stripe invoice (marked paid out-of-band) that produces a paper trail, whose
invoice.paidwebhook then credits the wallet on-chain.This PR is the
apps/apicrediting slice, and it reuses the existing coupon-claim mechanism rather than inventing a new one: the admin app (CON-626,ovrclk/console-private) pre-creates the pendingstripe_transactionsrow — it has direct DB access, exactly as the coupon endpoint pre-creates its row internally — and marks the invoice paid out-of-band.apps/api's webhook then credits the matching row through the existing idempotent path. No new endpoint, no metadata parsing, no bespoke crediting method inapps/api.What
manual_creditto thestripe_transaction_typeenum.stripe_invoice_id(WHERE NOT NULL): enforces one row per invoice (guards both the coupon and manual-credit paths) and indexes the previously-unindexedfindByInvoiceIdlookup.invoice.paidalongsideinvoice.payment_succeeded(out-of-band-paid invoices fireinvoice.paid).manual_creditlikecoupon_claimforendTrial— a granted credit must not graduate a trial user.updateTransactionAndTopUp: it locks the matched row and no-ops if alreadysucceeded; an invoice with no matching row no-ops.The net
apps/apichange is small — enum value + index + migration + theinvoice.paidsubscription + theendTrialtweak, plus tests. (The PR's first commit implemented a metadata-driven variant; the second reverts that in favor of reusing the coupon path.)Tests: unit (matched-row
manual_creditcredits withendTrial:false;invoice.paid/invoice.payment_succeededrouting), real-DB integration (partial-unique-index invariant — duplicatestripe_invoice_idrejected, nulls allowed), functional (HTTP end-to-end — single credit keeps the user trialing;invoice.paid+invoice.payment_succeededdouble-fire credits once; an unmatched invoice creates no row and no credit).Migration —
0030_true_jetstream.sqlALTER TYPE "stripe_transaction_type" ADD VALUE 'manual_credit'+CREATE UNIQUE INDEX ... WHERE stripe_invoice_id IS NOT NULL.ADD VALUEis fine inside drizzle's single migration transaction.stripe_invoice_idtoday (one row per invoice). Pre-deploy check: confirm there are no existing duplicatestripe_invoice_idvalues in prod.Cross-repo contract (for CON-626, admin app)
Using the same DB and the same Stripe account/keys, the admin app must:
stripe_transactionsrow{ userId, type:"manual_credit", status:"pending", amount (cents), currency, stripeInvoiceId, description }before marking the invoice paid — otherwise theinvoice.paidwebhook can fire first, find no row, and no-op. Handle thestripe_invoice_idunique index on insert (e.g.ON CONFLICT DO NOTHING).stripeCustomerId.Deploy ordering: this migration (
0030, which adds themanual_creditenum value) must ship before the admin app writesmanual_creditrows, or those inserts fail.Coupling note: two services now write
stripe_transactions, so that table's shape becomes a shared contract. If that coupling becomes painful, revisit whether the admin app should instead go through anapps/apiendpoint (the original CON-626 direction).Summary by CodeRabbit
manual_creditStripe transactions for invoice-driven wallet top-ups.invoice.paid(manual credits) andinvoice.payment_succeeded(charged invoices).NULLinvoice IDs allowed).endTrialbehavior.