fix(wallet): check address-credited deposits against the player's RG deposit limit - #160
fix(wallet): check address-credited deposits against the player's RG deposit limit#160zaxovaiko wants to merge 1 commit into
Conversation
…deposit limit The PSP path refuses an over-limit deposit before charging; the custody webhook and poller path credited any amount with no check. The funds are already on chain there, so the credit still happens, but an over-limit deposit now files an rg_limit_breach reconciliation finding with the limit, period and prior usage so an operator can return the excess. A failing limit check is logged and never blocks the credit. Signed-off-by: Volodymyr Zakhovaiko <zaxovaiko@gmail.com>
jakubfilinger-b
left a comment
jakubfilinger-b
left a comment
There was a problem hiding this comment.
The gap is real and the shape of the answer is right: the funds are on chain, so refusing is not on the table, and a finding plus an audit row is the honest thing to leave behind. Crediting first and reporting second, with the check outside the credit transaction so a gate failure can never roll back money that already arrived, is the correct trade — and the three tests cover exactly the three outcomes that matter.
One blocking item and two smaller ones. The blocking one is the read placement: the sibling PSP path runs the same check inside its transaction and this one does not, and the difference is load-bearing under concurrency. Detail on that thread.
A correction for the description and changeset while you're in there: they both say "the custody webhook and poller path", but creditDepositByAddress has exactly one caller — the webhook route in wallet/router/index.ts — and reconciliation-finding.service.ts says as much in its own doc comment ("hits an unattributable deposit in real time, not on a poll"). I checked the other creditWalletBalance call sites: the PSP deposit is already gated, and the rest are manualAdjust, swap legs and withdrawal refunds, none of which are player deposits. So the coverage is right — it's just the sentence that claims more than it does, and this one lands in an operator-facing changeset.
| // The funds are already on chain, so the player's deposit limit cannot refuse this credit | ||
| // the way it refuses a PSP charge. It is still asked, with this deposit as the attempted | ||
| // move, and a refusal becomes a finding so a human can act on the excess. | ||
| const rgDecision = await this.rgDecisionForLandedDeposit( |
There was a problem hiding this comment.
The comment explains why this sits outside the credit transaction and I agree with the reasoning — a gate failure must not roll back on-chain funds. But it also reads used on this.drizzle.db before the transaction opens, and the sibling path does the opposite: assertWithinRgDepositLimit is handed txn and called from inside the deposit transaction at :961.
That asymmetry has a concrete consequence. Two deposits to the same player landing close together both compute used from the same pre-credit snapshot, so each is judged on its own against the limit. A player with a daily limit of 1 who lands 0.6 and 0.6 within the window has both credited, both judged allowed, and no finding filed — even though the day closes at 1.2 against a limit of 1. That is precisely the row an operator needs to return the excess, and it is the case a determined player would produce deliberately.
It also makes the numbers in the finding itself unreliable: used is the value from before this credit and before any concurrent one, so the (X already used) in detail can already be wrong by the time the row is written.
You don't need the check inside the transaction to fix it — you need it after the credit commits, reading post-credit state. Move the call below the transaction (still guarded by !replayed) and ask the gate about the state as it now stands. A gate failure there still cannot roll anything back, which is the property the comment is protecting, and the answer stops depending on how many deposits raced.
| txHash: event.txHash, | ||
| externalId: event.externalId, | ||
| transactionId, | ||
| detail: `credited over the player's ${rgDecision.period} ${rgDecision.limitType} limit of ${rgDecision.limit} (${rgDecision.used} already used) - the funds were already on chain`, |
There was a problem hiding this comment.
The changeset promises the finding carries "the limit, the period and what was already used", but all three exist only inside this English sentence. wallet_reconciliation_finding has no metadata column, so limit, period, limitType and used are unreachable to anything but a human reading detail — an operator UI cannot sort or filter by them, and the excess-return workflow this row exists to trigger has to regex the string to learn how much the excess actually was. The test pins the prose (toContain('daily deposit limit of 1')), which will also make the wording load-bearing the first time someone rephrases it.
The table already carries amount (the deposit) and currency; what is missing is the limit side. Either add a metadata jsonb column in the same migration that adds the enum value and put the decision in it, or if that is too much for this PR, say so in the changeset rather than implying the fields are queryable.
| amount: string, | ||
| currency: string, | ||
| ): Promise<RgLimitDecision | null> { | ||
| if (!this.rgLimits) { |
There was a problem hiding this comment.
An unbound RG_LIMITS returns null here and the deposit is credited with no check and no trace — same as before this PR, which is fair, but it is now the one path where a licence-facing gate can be absent and nothing anywhere says so. assertWithinRgDepositLimit at :799 has the identical early return, so an operator who never binds the port has both deposit routes silently ungated.
Not this PR's job to make the port required, but a one-time warning at plugin bind time ("wallet loaded without RG_LIMITS - deposit limits are not enforced") would turn a silent condition into a visible one, and it is the kind of thing that gets asked about in an audit. Worth a follow-up issue if not here.
Separately: the try/catch logs userId and currency but not the externalId or txHash, so the log line cannot be tied back to the deposit it failed on. Those are already in scope at the call site — worth threading through.
| expect(emittedTopics(events)).toEqual(['wallet.deposit.completed']); | ||
| expect(rgLimits.checkDeposit).toHaveBeenCalledWith(expect.anything(), w.userId, '2', 'BTC'); | ||
| const findings = await findingsFor(externalId); | ||
| expect(findings).toHaveLength(1); |
There was a problem hiding this comment.
Good set — over-limit, under-limit and gate-down are the three that matter, and asserting the balance in all three is what makes the "credit always happens" claim real rather than stated.
Worth one more, given the (kind, externalId) partial unique index and onConflictDoNothing in recordReconciliationFinding: a second over-limit deposit for the same player with a different externalId should file a second finding. The dedup key makes replay safe, which is the behaviour the changeset promises, but nothing currently pins that it does not also swallow a genuinely new breach.
2 participants
Summary
A deposit credited by address (the custody webhook and poller path) is now checked against the player's responsible-gambling deposit limit. The credit still happens, and an over-limit deposit files a
rg_limit_breachreconciliation finding with the limit, the period and what was already used, plus the usualwallet.reconciliation_finding.recordedaudit row.Why
The PSP path refuses an over-limit deposit before charging. The address path credited any amount with no check at all, so a player with a daily limit could deposit any amount on chain and nothing recorded that the limit had been passed. An operator had no list of deposits to return the excess on.
Alternatives considered
Risks
rg_limit_breachtoWALLET_RECONCILIATION_FINDING_KINDS; the wallet migration0020adds it to the finding kind enum.