Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
馃挕 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca4a33d569
鈩癸笍 About Codex in GitHub
Codex has been enabled to automatically 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 馃憤.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /** Days from the grant until it expires and what is left of it is forfeited. */ | ||
| expiryDays: number; | ||
| /** Weight profile that scores this grant's bets. */ | ||
| weightProfileId: string; |
There was a problem hiding this comment.
This is only a pointer to mutable configuration, not a terms snapshot. weightedContribution reloads the profile rows by this id for every bet, so editing or deleting a profile changes or stops wagering for grants that already exist. Store the resolved weights here, or point to an immutable profile version.
There was a problem hiding this comment.
This one is already covered in 3bf82f22 - snapshotTerms copies the profile rows into terms.weights, and wagering scores from the snapshot, never from the live profile. Editing or deleting the profile leaves an existing grant untouched (there are tests for both).
|
Are you planning on adding more for our "Stage 0"? |
4324b28 to
2d340ad
Compare
2d340ad to
12aac7c
Compare
| * was credited, so there is nothing to lose. Everything after funding ends in `completed`, | ||
| * `expired` or `forfeited`. | ||
| */ | ||
| export const BONUS_GRANT_STATUSES = [ |
There was a problem hiding this comment.
These grant value sets already exist in contracts/schemas/promo.ts, which is the shared event and adapter contract. Keeping another tuple, schema, and type here lets the database, input port, and event payloads drift. Import the shared values instead.
There was a problem hiding this comment.
Done in 97923d04 - dropped the copies, the module imports them from contracts/schemas/promo.ts now.
| .notNull() | ||
| .default('0'), | ||
| status: promoGrantStatusEnum().$type<BonusGrantStatus>().notNull().default('active'), | ||
| forfeitReason: promoForfeitReasonEnum().$type<BonusForfeitReason>(), |
There was a problem hiding this comment.
This only rejects a reason on a non-forfeited grant. It still allows { status: "forfeited", forfeitReason: null }, even though every forfeit needs a reason for the audit trail. Enforce both directions in the database constraint.
There was a problem hiding this comment.
Done in 97923d04 - the check is now (status = 'forfeited') = (forfeit_reason is not null), so both directions are enforced.
| amount: MoneyAmountSchema.refine(isPositiveMoney, 'must be greater than zero'), | ||
| source: BonusGrantSourceSchema, | ||
| sourceRef: z.string().min(1), | ||
| actor: z.union([ |
There was a problem hiding this comment.
source and actor are validated independently, so { source: "manual", actor: { type: "system" } } is accepted. That records an admin-issued bonus as a system action. Require an admin actor for a manual grant.
There was a problem hiding this comment.
Done in 97923d04 - a manual grant is refused unless the actor is an admin.
|
|
||
| async grant(tx: DrizzleTx, rawArgs: BonusGrantArgs): Promise<BonusGrantOutcome> { | ||
| const args = grantArgsSchema.parse(rawArgs); | ||
| const wageringRequired = moneyScaleBy(args.amount, args.terms.wageringMultiplier); |
There was a problem hiding this comment.
MoneyAmountSchema allows 20 integer digits, and the 1000 multiplier can produce a 23-digit wageringRequired. For example, 99999999999999999999 times 2 reaches the insert and overflows numeric(38,18). Validate the derived amount, or cap the amount relative to the multiplier.
There was a problem hiding this comment.
Done in 97923d04 - the derived wageringRequired is validated against MoneyAmountSchema before the insert.
| async grant(tx: DrizzleTx, rawArgs: BonusGrantArgs): Promise<BonusGrantOutcome> { | ||
| const args = grantArgsSchema.parse(rawArgs); | ||
| const wageringRequired = moneyScaleBy(args.amount, args.terms.wageringMultiplier); | ||
| const terms = await this.snapshotTerms(tx, args.terms); |
There was a problem hiding this comment.
This reads the live weight profile before checking the idempotency key. If a grant already exists and its profile was later deleted, an exact retry throws here instead of returning the original grant. Resolve an existing key before requiring live configuration, while keeping the insert race-safe.
There was a problem hiding this comment.
Done in 97923d04 - an existing key is resolved before the profile is read. The insert keeps onConflictDoNothing, so the concurrent case is still race-safe.
Wagering is scored from the weights copied onto the grant terms at grant time, so editing or deleting a weight profile no longer changes a bonus a player already holds. A grant against a missing profile is refused.
The separate-balance model needs somewhere for bonus funds to live, and the answer is the grant row itself rather than a second `wallet_balance` row. `wallet_balance` is unique on (wallet, currency) and is read from three dozen places - reconciliation, custody sweep, swap, withdrawal, the balance stream - each of which would need a "real money only" predicate, and the one that gets missed is a player withdrawing bonus money. Reconciliation is the sharper argument: it compares internal balances against on-chain custody, and bonus funds were never deposited, so holding them there manufactures a permanent unexplained surplus on every run. Keeping them out makes withdrawal correct by construction instead of by a subtractive lock formula. Funds cross into the real balance exactly once, at conversion, and are fully visible to withdrawal, limits and reconciliation from that instant. `activated_at` and `closed_at` bracket the grant's life; `status` already says which terminal state it closed in, so there is one column rather than four. The 0001 migration is regenerated in place rather than followed by a column-add migration: the table has never existed in a deployed database, so the second file would carry no information. Also registers both promo migration sets in the test database bootstrap. They were missing, which the module's self-migrating integration tests hid; anything booted through the shared test app would have failed on a missing relation.
Every grant was audited as `actorType: 'system'` with no actor id, and the port had no field to carry one. `manual` is in the source enum because an admin hands a player a bonus through it, so the only record of who did that said the system did. `BonusGrantArgs` now carries the actor and a manual grant records the admin behind it. Three input bounds, all on the money path, all at the boundary rather than after the fact. A wagering multiplier of zero produces a grant that converts the moment it is created; there is now an explicit floor, and a ceiling above it so a fat-fingered multiplier is refused rather than written. Currency is uppercased at entry, because a grant stored as `usd` against a wallet holding `USD` is invisible to every bet in that currency and expires untouched. The replay path returned success for any row that matched the idempotency key, without checking it was the same grant. Two payouts sharing one source reference would have reported a credit that never happened. It now compares the currency and the amounts and refuses on a mismatch. Three CHECK constraints hold the money invariants where no caller can route around them: a bonus balance cannot go negative, wagering progress cannot pass its requirement, and a forfeit reason cannot exist without a forfeit. The consumption path is written against these rather than being trusted to respect them. The two raw `throw new Error` calls became typed errors the router can map, and the FIFO index is now partial on live statuses - a terminal grant is never consumed again, and long term they are almost the whole table. ADR-0040 records why bonus funds live on the grant rather than in `wallet_balance`, since that decision is what the rest of the stack is built on.
Import the grant value sets from the shared promo contract instead of redeclaring them. Require a forfeit reason exactly when a grant is forfeited, refuse a manual grant with no admin actor, refuse a requirement that would overflow numeric(38,18), and resolve an existing idempotency key before the weight profile is read so a replay survives a deleted profile.
c5720f5 to
97923d0
Compare
2 participants
Summary
Stacked on #177. Review that one first; this diff is only the last three commits. Adds
promo_grant- the row a player's bonus actually is - andGrantService.grant()bound to theBONUS_GRANTSport. BF-592.Why
The grant row is the bonus balance. There is no separate balance table and no second
wallet_balancerow:promo_grant.bonus_balanceholds the funds, and they cross into the real balance exactly once, at conversion, as awallet_transactionof typebonus.That is the load-bearing decision in this PR, so the reasoning in full.
wallet_balanceis unique on(wallet_id, currency), and it is read from roughly three dozen places - reconciliation, the custody sweep, swap, deposit, withdrawal, auto-withdrawal, the balance stream, admin reporting. Discriminating a bonus row by akindcolumn means every one of those reads needs a "real money only" predicate, and the one that gets missed is a player withdrawing bonus money as if it were their own.Reconciliation is the sharper argument. It compares internal balances against on-chain custody. Bonus money was never deposited and has no chain side, so holding it in
wallet_balancemanufactures a permanent unexplained surplus on every reconciliation run.Keeping it out also makes withdrawal correct by construction rather than by formula: there is nothing to subtract, because there is nothing there.
The idempotency guard is the unique index, not a read.
unique (user_id, source, source_ref)withonConflictDoNothing, then a select for the existing row. A read-then-write check would let two concurrent replays of the same deposit both pass and grant the bonus twice; the index cannot. There is a test that fires three concurrent duplicates and asserts exactly one row and onecreated: true.termsis a snapshot, never a lookup, and it snapshots the weight rows, not a reference to the profile. Editing an offer - or its weight profile - must not change a bonus a player already holds. Storing a profile id would have left that property true only by discipline; storing the resolved rows makes it true by construction. This is the fix for the one correction the design review raised against the original shape.Input is validated at the boundary. A caller is another module, but a malformed multiplier or a non-positive amount has to be refused before it reaches the ledger rather than corrected after. Zod parse on entry.
expires_atcomes from Postgres,now() + make_interval(days => ...), so there is no clock to inject and no drift between the row'screated_atand its expiry.Alternatives considered
A second
wallet_balancerow with akinddiscriminator - rejected above, on the reconciliation and the thirty-odd unguarded reads.A
promo_grant_balancetable - one row per grant per currency, which is one row per grant, which is a column.Four terminal timestamps (
completed_at,expired_at,forfeited_at,cancelled_at) -statusalready says which terminal state a grant closed in, soclosed_atis one column carrying the same information.A column-add migration on top of the existing one. The
0001migration is regenerated in place instead. The table has never existed in a deployed database, so a follow-upALTER TABLEwould carry no information a reader could use.Input bounds and attribution
Three bounds sit at the boundary rather than after the fact. A wagering multiplier of zero
produces a grant that converts the moment it is created, so there is a floor, and a ceiling above
it so a fat-fingered multiplier is refused rather than written. Currency is uppercased at entry:
a grant stored as
usdagainst a wallet holdingUSDis invisible to every bet in that currencyand quietly expires.
BonusGrantArgscarries an actor.manualis in the source enum because an admin hands a playera bonus through it, and before this the only record of that said the system did it, with a null
actor.
The replay path used to return success for any row matching the idempotency key without checking
it was the same grant, so two payouts sharing one source reference would have reported a credit
that never happened. It compares the currency and the amounts now and refuses on a mismatch.
Three
CHECKconstraints hold the money invariants where no caller can route around them: a bonusbalance cannot go negative, wagering progress cannot pass its requirement, and a forfeit reason
cannot exist without a forfeit. The consumption path is written against these rather than trusted
to respect them.
ADR-0040 records the balance-model decision this PR implements.
Risks
No wallet credit yet.
GrantServicewrites the grant row and moves no money into a wallet; conversion is what moves money, and it arrives with the consumption path. A grant created today is a correct record of an obligation with no settlement behind it.No events emitted.
promo.bonus.grantedis declared in #177 and nothing subscribes. Emitting it from inside the caller's transaction would require the transactional outbox, which is optional and throws when unbound, so the emit belongs to whoever owns the commit - the deposit subscription, once offers exist.No offer table, so terms arrive from the caller. Eligibility (
ok: falsewithineligible/offer_inactive/currency_unsupported) is declared on the port and unreachable until offers land.This PR also registers both promo migration sets in the shared test-database bootstrap. They were missing; the module's self-migrating integration tests hid it, but anything booted through the shared test app would have failed on a missing relation.