Negative-balance correctness: fix transfer_to + :balance_depleted, audit + pin documented behavior#3
Merged
Conversation
The flag was half-applied: direct `wallet.debit` honored it via `apply_debit`, but the canonical `wallet.transfer_to` had its own balance pre-check that always rejected transfers exceeding the source balance. Apps using the flag for a "convenience overdraft" — ride-fare flows where passengers may briefly go negative until rewards land, family wallets with shared spend, telecom plans with bridging credit — worked for direct debits but had to monkey-patch the gem to make transfers (the actual user-to-user primitive) consistent. The fix: - The transfer pre-check is now gated on `!allow_negative_balance?`, matching `apply_debit`'s long-standing semantics. - When a transfer drives the source below zero AND the caller is using the default `:preserve` expiration policy, the policy automatically falls back to `:none` for that transfer. With no positive source buckets to inherit expirations from for the deficit portion, `build_preserved_transfer_inbound_credit_specs` would otherwise raise `InvalidTransfer` on the count mismatch. The receiver gets a single evergreen credit — the only honest representation of "value created without a source bucket". Explicit `:fixed` and `:none` are honored unchanged. - `:insufficient_balance` callback no longer fires for transfers that succeed under `allow_negative_balance = true`. It still fires (and the transfer still raises) when the flag is off. 13 new transfer tests cover: zero-balance source, positive→negative crossover, already-negative source, `:preserve`→`:none` fallback, mixed positive+overdraft buckets, `:fixed` and explicit `:none` policy parity, callback wiring (`:transfer_completed` fires, `:insufficient_balance` doesn't fire on success but still fires on rejection), `has_enough_balance?` semantics under the flag, and the wallet-pair lock holding under overdraft. Existing test asserting the old "transfers always reject below zero" behavior is amended to reflect the new contract. Bumps version to 0.2.0. README gains a "Negative balances and overdraft" section explaining the flag, the `:preserve`→`:none` fallback, the strict `has_enough_balance?` contract, and how apps should layer their own overdraft-floor policy on top. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Full audit of every code path that touches wallet balance under `allow_negative_balance = true`. Net result: one more behavior bug fixed and a layer of explicit tests around the documented subtleties so future changes can't quietly regress them. ### Bug fix — `:balance_depleted` callback Previously fired only on EXACT zero. With `allow_negative_balance` on, a single debit can take a wallet from +100 to -50 (skipping zero entirely), and the callback was silently missing. Widened the condition from `balance.zero?` to `!balance.positive?` so any positive→non-positive crossing fires it. With the flag off, balances can't go below zero, so the new condition collapses to the previous "exactly zero" semantic and existing callers see no behavior change. ### Documented behavior, now pinned by tests - Compounding debits accumulate `unbacked_amount` per row; the gem doesn't spread debt across debits. - A credit on a negative-balance wallet does NOT auto-allocate against existing unbacked debits. Both ledger entries persist; the `balance` accessor reconciles them on the fly. FIFO consumption on the next debit pulls from the new credit, not the old debt. - `has_enough_balance?` keeps strict semantics under the flag: it answers "do you have it on hand?", not "would the gem accept this debit?". Overdraft is a deliberate caller choice. - `balance_before` / `balance_after` snapshots correctly record signed integers across the positive→negative boundary. - Flipping `allow_negative_balance` OFF while wallets are below zero leaves them un-saveable until the flag is flipped back on. The model's `balance >= 0` validation is gated on the flag, and `refresh_cached_balance!` calls `save!` on every credit/debit. Test pins this gotcha + the recovery path. - `:low_balance_reached` is one-shot per crossing — going from -50 to -80 doesn't re-fire because the wallet was already low. - Concurrent overdraft debits on the same wallet serialize through `with_lock` (FOR UPDATE) and accumulate cleanly without double-counting `previous_balance`. ### Audit notes (no code change required) - `apply_credit` doesn't dispatch threshold callbacks (depleted / low_balance) — by design, those are one-way "you're going down" signals. - Allocation `amount > 0` constraint is correct: no allocation is written for the unbacked portion of a debit; that's how `unbacked_amount` is non-zero. - Transaction `amount` has no sign restriction (correct — credits +, debits -). - `Transfer#outbound_transactions` / `#inbound_transactions` filter by `amount` sign and work unchanged with negative-going transfers. - `has_wallets` `initial_balance` rejects negative values (intentional — you can't bootstrap negative). - `lock_wallet_pair!` correctly handles the same-wallet case (dead branch in practice because `transfer_to` rejects same-wallet earlier, but defensive). README gains explicit gotchas for: the `:preserve`→`:none` fallback, `:balance_depleted` semantics, `:low_balance` one-shot behavior, credit-doesn't-settle-prior-debt, the flag-flip lockup, and concurrency guarantees. Initializer template comment on `allow_negative_balance` warns against the runtime-toggle case; `on_balance_depleted` docstring updated to reflect the new crossing-into-non-positive semantic. 13 new tests across `wallet_test.rb` and `wallet_callbacks_test.rb`. 92 runs / 338 assertions, all green on Rails 7.2 and 8.1 appraisals. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Summary
Wallets.configuration.allow_negative_balancewas half-applied: directwallet.debithonored it viaapply_debit, but the canonicalwallet.transfer_tohad its own balance pre-check that always rejected transfers below zero — regardless of the flag. Apps using the flag for a "convenience overdraft" (ride-fare apps where passengers briefly go negative until rewards land, family wallets with shared spend, telecom plans with bridging credit) got inconsistent behavior and had to monkey-patch the gem.This PR fixes that, and follows up with a full audit of every code path that touches balance under
allow_negative_balance = true. The audit found one more real bug (:balance_depletedcallback was missing positive→negative crossings) and several documented subtleties that needed explicit test coverage so future changes can't quietly regress them.Bug fixes
Wallet#transfer_tohonorsallow_negative_balancelib/wallets/models/wallet.rb:!allow_negative_balance?, matchingapply_debit's long-standing semantics. When the flag is on, the transfer goes through; the source wallet records the deficit as an unbacked negative transaction (the same shapewallet.debitalready produced).:preserveexpiration policy, the policy automatically falls back to:nonefor that transfer. There are no positive source buckets to inherit expirations from for the deficit portion, sobuild_preserved_transfer_inbound_credit_specswould otherwise raiseInvalidTransferon the count mismatch. The receiver gets a single evergreen credit — the only honest representation of "value created without a source bucket".:fixed(withexpires_at:) and explicit:noneare honored unchanged — those policies don't depend on source-bucket walking.:insufficient_balancecallback no longer fires for successful overdraft transfers. It still fires (and the transfer still raisesWallets::InsufficientBalance) when the flag is off and the transfer is rejected.:balance_depletedcallback fires on positive→non-positive crossingsPreviously fired only on exact zero. With
allow_negative_balance = true, a single debit can take a wallet from e.g. +100 to -50, skipping zero entirely, and the callback was silently missing. Widened the condition frombalance.zero?to!balance.positive?. With the flag off, balances can't go below zero, so the new condition collapses to "exactly zero" and existing callers see no behavior change.Documented behavior, now pinned by tests
These were correct already but easy to break — the audit pass added explicit coverage and inline comments so they stay correct:
unbacked_amountper row; the gem doesn't spread debt across debits.balancereconciles them on the fly. FIFO consumption on the next debit pulls from the new credit, not the old debt.has_enough_balance?keeps strict semantics under the flag: it answers "do you have it on hand?", not "would the gem accept this debit?". Overdraft is a deliberate caller choice.balance_before/balance_aftersnapshots correctly record signed integers across the positive→negative boundary.allow_negative_balanceOFF while wallets are below zero leaves them un-saveable until the flag is flipped back on. The model'sbalance >= 0validation is gated on the flag, andrefresh_cached_balance!callssave!on every credit/debit. The flag is meant to be a stable config decision, not a runtime toggle. Test pins both the lockup and the recovery path.:low_balance_reachedis one-shot per crossing — going from -50 to -80 doesn't re-fire because the wallet was already low; coming back up to +200 then dipping low again does fire.with_lock(FOR UPDATE) and accumulate cleanly without double-countingprevious_balance. Concurrent transfers serialize throughlock_wallet_pair!.Tests
22 new tests across
transfer_test.rb,wallet_test.rb,wallet_callbacks_test.rb. Existing tests covering the old "transfers always reject below zero" and "depleted = exactly zero" semantics are amended to reflect the new contracts.bundle exec rake testgemfiles/rails_7.2.gemfilegemfiles/rails_8.1.gemfileCompatibility
allow_negative_balancedefaults tofalse, so apps that haven't opted in see no behavior change.:balance_depletedsemantic widening is a no-op under default config (balance can't go below zero).Wallets::InsufficientBalancefromtransfer_toafter flipping the flag on (i.e. were aware of the half-applied behavior and relied on it as a guardrail) need to know transfers will now go through. The README gains a "Negative balances and overdraft" section calling this out and the recommended pattern of layering an app-level floor policy in front.Documentation
wallet.debit/wallet.transfer_tocontract, the floor-is-yours-to-enforce model, the:preserve→:nonefallback,:balance_depletedsemantics,:low_balanceone-shot behavior, credit-doesn't-settle-prior-debt, the flag-flip lockup, concurrency guarantees, and the system-reversal use case.CHANGELOG.mdupdated for0.2.0.allow_negative_balancewarns against runtime-toggling;on_balance_depleteddocstring updated to reflect the crossing-into-non-positive semantic.Bumps version to 0.2.0.
🤖 Generated with Claude Code