From 32da4789974a67b5e54ca8d51afca96dfcde08bf Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Sun, 3 May 2026 23:03:16 +0100 Subject: [PATCH 1/2] Make transfer_to honor allow_negative_balance (v0.2.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CHANGELOG.md | 11 ++ README.md | 25 +++ lib/wallets/models/wallet.rb | 41 ++++- lib/wallets/version.rb | 2 +- test/models/wallets/transfer_test.rb | 254 ++++++++++++++++++++++++++- 5 files changed, 326 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3c8fd3..f1cb6df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,14 @@ +## [0.2.0] - 2026-05-03 + +### Fixed + +- `Wallet#transfer_to` now honors `Wallets.configuration.allow_negative_balance`, matching `Wallet#debit`. Previously the flag was half-applied: direct debits could go below zero, but the canonical transfer primitive (used to move value between users) silently rejected any transfer that would push the source below zero. Apps using the flag for a "convenience overdraft" (e.g. ride-fare apps where passengers may briefly go negative until rewards land) had to monkey-patch the gem to get consistent behavior. ([#3](https://github.com/rameerez/wallets/pull/3)) + +### Changed + +- When a transfer drives the source wallet below zero AND the caller is using the default `:preserve` expiration policy, the policy automatically falls back to `:none` for that transfer. Rationale: there are no positive source buckets to "preserve" expirations from for the deficit portion, and `build_preserved_transfer_inbound_credit_specs` would otherwise raise `InvalidTransfer`. The inbound credit becomes a single evergreen entry — the only honest representation of "value created without a source bucket". Explicit `:fixed` and `:none` policies 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 `Wallets::InsufficientBalance`) when the flag is off and the transfer is rejected. + ## [0.1.0] - 2026-03-18 Initial release. diff --git a/README.md b/README.md index 56bf828..24a6742 100644 --- a/README.md +++ b/README.md @@ -245,6 +245,31 @@ Transfers require both wallets to use the same asset and the same wallet class. > sender.transfer_to(receiver, 100, expiration_policy: :fixed, expires_at: 30.days.from_now) > ``` +### Negative balances and overdraft + +By default `wallets` rejects any debit or transfer that would push a wallet below zero. Some apps (ride-fare apps with rewards, family wallets with shared spend, telecom plans with bridging credit, etc.) want to allow a small overdraft so the user experience doesn't hard-stop on a low balance. + +Flip one flag and it applies consistently to direct `wallet.debit` and to `wallet.transfer_to`: + +```ruby +Wallets.configure do |config| + config.allow_negative_balance = true +end + +passenger.wallet(:eur).balance # => 100 (1€) +passenger.wallet(:eur).transfer_to(driver.wallet(:eur), 300, category: :ride_fare) +passenger.wallet(:eur).reload.balance # => -200 (-2€) +driver.wallet(:eur).reload.balance # => 300 (3€) +``` + +A few things to know: + +- **Apps own the floor.** The flag is binary — it doesn't cap how negative a wallet can go. If you want a "5€ convenience overdraft", enforce that in your domain code before calling `transfer_to` (e.g. a `WalletPolicy.can_afford?(wallet:, amount:)` service that checks `wallet.balance + MAX_OVERDRAFT >= amount`). +- **`has_enough_balance?` stays strict.** It answers "does this wallet have enough on hand right now?" — overdraft is a deliberate choice the caller makes by attempting the debit/transfer, not by querying. So `wallet.has_enough_balance?(amount)` returns `false` even when the gem would happily complete an overdraft. +- **`:preserve` expiration falls back to `:none` when a transfer goes negative.** With `allow_negative_balance = true` and the default `:preserve` policy, transfers that exceed the source's positive buckets can't honestly "preserve" an expiration on the deficit portion (there is no source bucket). The transfer's `expiration_policy` is automatically downgraded to `:none` for that transfer; the receiver gets a single evergreen credit. Explicit `:fixed` (with `expires_at:`) and `:none` are honored as-is. +- **System-initiated reversals.** Refunds and payout reversals via `transfer_to` will also go through, even if the recipient's wallet ends up below zero. That's correct: the ledger has to settle, and a negative wallet records a real debt instead of a silent failure. Apps that want to *block* user-initiated overdrafts but *allow* system reversals should keep the floor check in their service layer, not toggle the flag mid-request. +- **`:insufficient_balance` callback.** Fires only when a debit or transfer is actually rejected (i.e. flag off or your service-layer floor refused). Successful overdraft transfers do not fire it. + ### Expiring balances Credits can expire: diff --git a/lib/wallets/models/wallet.rb b/lib/wallets/models/wallet.rb index b4f2cee..61d574a 100644 --- a/lib/wallets/models/wallet.rb +++ b/lib/wallets/models/wallet.rb @@ -243,11 +243,50 @@ def transfer_to(other_wallet, amount, category: :transfer, metadata: {}, expirat lock_wallet_pair!(other_wallet) previous_balance = balance - if amount > previous_balance + + # Insufficient-balance gate. + # + # `allow_negative_balance` is the single source of truth for whether + # the gem permits a wallet to drop below zero. Direct `wallet.debit` + # has always honored it via `apply_debit`; transfers must too, or + # the flag is half-applied and surprising (debits go through, the + # canonical transfer primitive doesn't). + # + # When the flag is OFF, this is the only chance to refuse cleanly: + # we want to dispatch `:insufficient_balance` for the observability + # callback, then raise BEFORE creating the Transfer row. (The later + # `apply_debit` call would also catch this, but only after a + # pointless Transfer row was created.) + # + # When the flag is ON, we let the transfer through. `apply_debit` + # below will allocate as many positive buckets as exist; the + # remaining shortfall lands as an unbacked negative transaction the + # wallet's `balance` accounting accommodates via + # `unbacked_negative_balance`. + if amount > previous_balance && !allow_negative_balance? dispatch_insufficient_balance!(amount, previous_balance, metadata) raise InsufficientBalance, "Insufficient balance (#{previous_balance} < #{amount})" end + # When a transfer dips below zero we can't faithfully apply the + # `:preserve` expiration policy: there are no positive source + # buckets to inherit expirations from for the deficit portion. + # `build_preserved_transfer_inbound_credit_specs` would refuse with + # `InvalidTransfer` on the count mismatch, defeating the very + # callers who opted into negative balances. The only honest + # fallback is to collapse the inbound side to a single evergreen + # credit (`:none`) — value created without a source bucket has no + # source expiration to preserve. + # + # Callers who care about a specific expiration on the inbound side + # should pass `expiration_policy: :fixed, expires_at: …`. Those are + # honored unchanged because they don't depend on source-bucket + # walking. Same for an explicit `:none`. + if resolved_policy == "preserve" && amount > previous_balance + resolved_policy = "none" + inbound_expires_at = nil + end + transfer = transfer_class.create!( from_wallet: self, to_wallet: other_wallet, diff --git a/lib/wallets/version.rb b/lib/wallets/version.rb index 96773c0..93bad3a 100644 --- a/lib/wallets/version.rb +++ b/lib/wallets/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module Wallets - VERSION = "0.1.0" + VERSION = "0.2.0" end diff --git a/test/models/wallets/transfer_test.rb b/test/models/wallets/transfer_test.rb index 9325f9b..f8ce9f3 100644 --- a/test/models/wallets/transfer_test.rb +++ b/test/models/wallets/transfer_test.rb @@ -153,18 +153,262 @@ class Wallets::TransferTest < ActiveSupport::TestCase assert_includes error.message, "expiration policy" end - test "rejects transfers that exceed available balance even when negatives are enabled" do - original_setting = Wallets.configuration.allow_negative_balance + test "rejects transfers that exceed available balance when negatives are disabled" do + sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 10) + recipient = create_wallet(users(:peer_user), asset_code: :credits, initial_balance: 0) + + assert_raises(Wallets::InsufficientBalance) do + sender.transfer_to(recipient, 25, category: :gift) + end + end + + # ─────────────────────────────────────────────────────────────────────────── + # `allow_negative_balance` parity for transfers + # ─────────────────────────────────────────────────────────────────────────── + # + # `wallet.debit` has always honored `Wallets.configuration.allow_negative_balance` + # via `apply_debit`, but `wallet.transfer_to` had its own pre-check that + # ALWAYS rejected transfers exceeding the source balance — the flag only + # half-applied. Apps that flipped the flag on for a "convenience overdraft" + # (e.g. ride-fare apps where passengers may briefly go negative until + # rewards land) found their direct debits worked, but the canonical + # transfer primitive — used to move value between users — silently still + # required positive balance. + # + # Tests below cement the new contract: with `allow_negative_balance = true`, + # transfers behave like debits — they go through, drive the source negative + # (with the inbound credit forced to "none" expiration since the source has + # no positive buckets to "preserve" from), and otherwise leave the rest of + # the transfer flow untouched. + + test "transfers drive the source wallet negative when allow_negative_balance is enabled" do + Wallets.configuration.allow_negative_balance = true + + sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 10) + recipient = create_wallet(users(:peer_user), asset_code: :credits, initial_balance: 0) + + transfer = sender.transfer_to(recipient, 25, category: :peer_payment) + + assert transfer.persisted? + assert_equal 25, transfer.amount + assert_equal(-15, sender.reload.balance, "source wallet went negative within the gem-level overdraft") + assert_equal 25, recipient.reload.balance + assert_equal sender.id, transfer.from_wallet_id + assert_equal recipient.id, transfer.to_wallet_id + end + + test "transfers from a wallet with zero balance succeed when allow_negative_balance is enabled" do + Wallets.configuration.allow_negative_balance = true + + sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + recipient = create_wallet(users(:peer_user), asset_code: :credits, initial_balance: 0) + + transfer = sender.transfer_to(recipient, 100, category: :peer_payment) + + assert transfer.persisted? + assert_equal(-100, sender.reload.balance) + assert_equal 100, recipient.reload.balance + end + + test "transfers from an already-negative wallet keep going further negative when allow_negative_balance is enabled" do + Wallets.configuration.allow_negative_balance = true + + sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + recipient = create_wallet(users(:peer_user), asset_code: :credits, initial_balance: 0) + sender.debit(50, category: :purchase) + assert_equal(-50, sender.reload.balance) + + transfer = sender.transfer_to(recipient, 30, category: :peer_payment) + + assert transfer.persisted? + assert_equal(-80, sender.reload.balance) + assert_equal 30, recipient.reload.balance + end + + test "preserve policy falls back to none when the transfer drives the source below zero" do + # With "preserve", the gem normally allocates inbound credits across the + # same expiration buckets the outbound debit consumed. When the source + # has no positive buckets to consume from (or the transfer exceeds them), + # there is nothing to preserve — falling back to "none" produces an + # evergreen inbound credit on the receiver, which is the only honest + # representation of "value created without a source bucket". Without + # this fallback, `build_preserved_transfer_inbound_credit_specs` would + # raise `InvalidTransfer` on the count mismatch. + Wallets.configuration.allow_negative_balance = true + + sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + recipient = create_wallet(users(:peer_user), asset_code: :credits, initial_balance: 0) + + transfer = sender.transfer_to(recipient, 40, category: :peer_payment) + inbound = transfer.inbound_transactions.sole + + assert_equal "none", transfer.expiration_policy, "falls back to none when no positive buckets exist" + assert_equal 40, inbound.amount + assert_nil inbound.expires_at + end + + test "preserve fallback hits when partial overdraft mixes positive buckets with new debt" do + # Sender has a 30-credit positive bucket; transfers 100. The transfer + # consumes the 30 (preserve would inherit its expiration) plus needs 70 + # more from thin air. There is no honest single expiration to assign + # to that 70, so we collapse the inbound side to a single "none" credit + # for the full amount. + Wallets.configuration.allow_negative_balance = true + + sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + recipient = create_wallet(users(:peer_user), asset_code: :credits, initial_balance: 0) + sender.credit(30, category: :top_up, expires_at: 60.days.from_now) + + transfer = sender.transfer_to(recipient, 100, category: :peer_payment) + inbound = transfer.inbound_transactions.sole + + assert_equal "none", transfer.expiration_policy + assert_equal 100, inbound.amount + assert_nil inbound.expires_at + assert_equal(-70, sender.reload.balance) + assert_equal 100, recipient.reload.balance + end + + test "preserve policy still preserves expiration buckets when the transfer fits within positive balance" do + # Sanity check — the negative-balance fallback must not regress the + # default behavior for transfers that fit normally inside the source's + # positive buckets. Same flag on, but the source has enough; preserve + # works as before. + Wallets.configuration.allow_negative_balance = true + + sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + recipient = create_wallet(users(:peer_user), asset_code: :credits, initial_balance: 0) + expiration = 90.days.from_now + sender.credit(200, category: :top_up, expires_at: expiration) + + transfer = sender.transfer_to(recipient, 75, category: :peer_payment) + inbound = transfer.inbound_transactions.sole + + assert_equal "preserve", transfer.expiration_policy + assert_equal 75, inbound.amount + assert_equal expiration.to_i, inbound.expires_at.to_i + end + + test "fixed expiration policy is honored when transfer drives the source negative" do + # If the caller chose "fixed" with an explicit expires_at, the inbound + # credit takes that expiration regardless of source bucket coverage — + # the user explicitly opted into a single inbound expiration, no need + # for the preserve→none fallback. + Wallets.configuration.allow_negative_balance = true + + sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + recipient = create_wallet(users(:peer_user), asset_code: :credits, initial_balance: 0) + expiration = 14.days.from_now + + transfer = sender.transfer_to(recipient, 50, category: :peer_payment, expiration_policy: :fixed, expires_at: expiration) + inbound = transfer.inbound_transactions.sole + + assert_equal "fixed", transfer.expiration_policy + assert_equal 50, inbound.amount + assert_equal expiration.to_i, inbound.expires_at.to_i + assert_equal(-50, sender.reload.balance) + end + + test "explicit none policy is honored when transfer drives the source negative" do + Wallets.configuration.allow_negative_balance = true + + sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + recipient = create_wallet(users(:peer_user), asset_code: :credits, initial_balance: 0) + + transfer = sender.transfer_to(recipient, 50, category: :peer_payment, expiration_policy: :none) + inbound = transfer.inbound_transactions.sole + + assert_equal "none", transfer.expiration_policy + assert_nil inbound.expires_at + assert_equal(-50, sender.reload.balance) + end + + test "transfer that drives the source negative dispatches the transfer_completed callback" do + Wallets.configuration.allow_negative_balance = true + completed = [] + Wallets.configuration.on_transfer_completed { |ctx| completed << ctx } + + sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + recipient = create_wallet(users(:peer_user), asset_code: :credits, initial_balance: 0) + + transfer = sender.transfer_to(recipient, 25, category: :peer_payment) + + assert_equal 1, completed.size + assert_equal transfer.id, completed.first.transfer.id + assert_equal 25, completed.first.amount + end + + test "insufficient_balance callback does NOT fire on a successful negative-going transfer" do + # The pre-check used to dispatch :insufficient and then raise. Now that + # negative balances are allowed end-to-end, neither side-effect should + # happen for a transfer that goes through. This guards against a + # regression where the pre-check was kept but the raise was conditioned + # — the callback would still fire for callers who installed + # `on_insufficient_balance` for top-up nudge UX. Wallets.configuration.allow_negative_balance = true + insufficient = [] + Wallets.configuration.on_insufficient_balance { |ctx| insufficient << ctx } + + sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + recipient = create_wallet(users(:peer_user), asset_code: :credits, initial_balance: 0) + + sender.transfer_to(recipient, 25, category: :peer_payment) + + assert_empty insufficient, "successful negative-going transfer must not fire insufficient_balance" + end + + test "insufficient_balance callback still fires when negatives are disabled and the transfer is rejected" do + insufficient = [] + Wallets.configuration.on_insufficient_balance { |ctx| insufficient << ctx } sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 10) recipient = create_wallet(users(:peer_user), asset_code: :credits, initial_balance: 0) assert_raises(Wallets::InsufficientBalance) do - sender.transfer_to(recipient, 25, category: :gift) + sender.transfer_to(recipient, 25, category: :peer_payment) end - ensure - Wallets.configuration.allow_negative_balance = original_setting + + assert_equal 1, insufficient.size, "with the flag off the rejection path keeps its observability" + assert_equal 25, insufficient.first.metadata[:required] + assert_equal 10, insufficient.first.metadata[:available] + end + + test "has_enough_balance? still reflects strict balance even when allow_negative_balance is true" do + # `has_enough_balance?` is the opt-in pre-flight check apps use to decide + # whether to attempt a transfer / debit at all. It intentionally keeps + # strict semantics — overdraft is a deliberate choice the caller makes + # by attempting the transfer; the predicate just answers "do they have + # enough on hand?". + Wallets.configuration.allow_negative_balance = true + + sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 10) + + refute sender.has_enough_balance?(25) + assert sender.has_enough_balance?(10) + end + + test "concurrent overdraft transfers serialize through the wallet pair lock" do + # `lock_wallet_pair!` already serialized concurrent transfers under the + # positive-balance contract. With overdraft the lock matters more, not + # less — two threads scanning a QR shouldn't double-debit a passenger + # whose remaining headroom only covers one transfer. Verify the lock + # is still acquired before the balance read used by the transfer. + Wallets.configuration.allow_negative_balance = true + sender = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + recipient = create_wallet(users(:peer_user), asset_code: :credits, initial_balance: 0) + + lock_calls = 0 + sender.singleton_class.send(:define_method, :lock_wallet_pair!) do |other| + lock_calls += 1 + first, second = [self, other].sort_by(&:id) + first.lock! + second.lock! unless first.id == second.id + end + + sender.transfer_to(recipient, 25, category: :peer_payment) + + assert_equal 1, lock_calls + assert_equal(-25, sender.reload.balance) end test "rejects transfers across different assets" do From a91db0835d0198ee0beb6c710a69c591d6aee097 Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Sun, 3 May 2026 23:13:03 +0100 Subject: [PATCH 2/2] Negative-balance audit: fix :balance_depleted + pin documented behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CHANGELOG.md | 7 + README.md | 7 +- .../wallets/templates/initializer.rb | 11 +- lib/wallets/models/wallet.rb | 14 +- test/integration/wallet_callbacks_test.rb | 97 ++++++++++++ test/models/wallets/wallet_test.rb | 139 ++++++++++++++++++ 6 files changed, 271 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f1cb6df..3b26999 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,12 +3,19 @@ ### Fixed - `Wallet#transfer_to` now honors `Wallets.configuration.allow_negative_balance`, matching `Wallet#debit`. Previously the flag was half-applied: direct debits could go below zero, but the canonical transfer primitive (used to move value between users) silently rejected any transfer that would push the source below zero. Apps using the flag for a "convenience overdraft" (e.g. ride-fare apps where passengers may briefly go negative until rewards land) had to monkey-patch the gem to get consistent behavior. ([#3](https://github.com/rameerez/wallets/pull/3)) +- `:balance_depleted` callback now fires when a debit takes a wallet from a positive balance to **zero or lower** (was: exactly zero). Previously, with `allow_negative_balance = true`, a single debit that drove a wallet from e.g. +100 to -50 would skip the callback because the wallet never landed on exact zero. The callback semantic is "ran out of available value", which is at least as true at -50 as at 0. With `allow_negative_balance = false` the behavior is unchanged because balances cannot go below zero. ### Changed - When a transfer drives the source wallet below zero AND the caller is using the default `:preserve` expiration policy, the policy automatically falls back to `:none` for that transfer. Rationale: there are no positive source buckets to "preserve" expirations from for the deficit portion, and `build_preserved_transfer_inbound_credit_specs` would otherwise raise `InvalidTransfer`. The inbound credit becomes a single evergreen entry — the only honest representation of "value created without a source bucket". Explicit `:fixed` and `:none` policies 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 `Wallets::InsufficientBalance`) when the flag is off and the transfer is rejected. +### Documented + +- `apply_credit` does NOT auto-allocate a new credit against existing unbacked debits. The FIFO ledger is intentionally append-only — both the unbacked debit and the new credit persist as independent rows; `balance` reconciles them on the fly. Apps that want automatic debt settlement should layer that on in their own service code. +- `allow_negative_balance` is meant to be a stable config decision, not a runtime toggle. Flipping it OFF while wallets are below zero leaves them un-saveable, since the model's `balance >= 0` validation is gated on the flag and any subsequent `credit` / `debit` calls `refresh_cached_balance!` (which calls `save!`). +- `has_enough_balance?` keeps strict semantics under `allow_negative_balance = true`: it still answers "does this wallet have enough on hand right now?", returning `false` when the gem would happily complete an overdraft. Overdraft is a deliberate caller choice via `debit` / `transfer_to`, not a query semantic. + ## [0.1.0] - 2026-03-18 Initial release. diff --git a/README.md b/README.md index 24a6742..add5db9 100644 --- a/README.md +++ b/README.md @@ -267,8 +267,13 @@ A few things to know: - **Apps own the floor.** The flag is binary — it doesn't cap how negative a wallet can go. If you want a "5€ convenience overdraft", enforce that in your domain code before calling `transfer_to` (e.g. a `WalletPolicy.can_afford?(wallet:, amount:)` service that checks `wallet.balance + MAX_OVERDRAFT >= amount`). - **`has_enough_balance?` stays strict.** It answers "does this wallet have enough on hand right now?" — overdraft is a deliberate choice the caller makes by attempting the debit/transfer, not by querying. So `wallet.has_enough_balance?(amount)` returns `false` even when the gem would happily complete an overdraft. - **`:preserve` expiration falls back to `:none` when a transfer goes negative.** With `allow_negative_balance = true` and the default `:preserve` policy, transfers that exceed the source's positive buckets can't honestly "preserve" an expiration on the deficit portion (there is no source bucket). The transfer's `expiration_policy` is automatically downgraded to `:none` for that transfer; the receiver gets a single evergreen credit. Explicit `:fixed` (with `expires_at:`) and `:none` are honored as-is. -- **System-initiated reversals.** Refunds and payout reversals via `transfer_to` will also go through, even if the recipient's wallet ends up below zero. That's correct: the ledger has to settle, and a negative wallet records a real debt instead of a silent failure. Apps that want to *block* user-initiated overdrafts but *allow* system reversals should keep the floor check in their service layer, not toggle the flag mid-request. +- **`:balance_depleted` fires on positive→non-positive crossings**, not on exact zero. A debit that takes a wallet from +100 to -50 in one shot still fires the callback. With `allow_negative_balance = false` the behavior collapses back to "exactly zero" (since balances can't go below zero), so existing callers don't see a change. +- **`:low_balance_reached` is one-shot per crossing**, regardless of how deep the dip goes. Going from +200 to -100 fires once; the next debit from -100 to -200 does not re-fire because the wallet was already below threshold. - **`:insufficient_balance` callback.** Fires only when a debit or transfer is actually rejected (i.e. flag off or your service-layer floor refused). Successful overdraft transfers do not fire it. +- **Credits don't auto-settle prior debt.** If a wallet is at -50 and you `credit(80)`, the wallet's balance becomes 30 — but the unbacked -50 debit and the +80 credit persist as independent ledger rows. The next debit consumes from the +80 bucket (FIFO) without back-filling the older unbacked debit. The math is consistent; the audit story is "we never settled the original debt with this credit". If you want auto-settlement, do it in your service layer (e.g. when crediting, `wallet.transactions.where("amount < 0").where("ABS(amount) > spent_amount") …` and create allocations explicitly). +- **System-initiated reversals.** Refunds and payout reversals via `transfer_to` will also go through, even if the recipient's wallet ends up below zero. That's correct: the ledger has to settle, and a negative wallet records a real debt instead of a silent failure. Apps that want to *block* user-initiated overdrafts but *allow* system reversals should keep the floor check in their service layer, not toggle the flag mid-request. +- **Don't toggle the flag at runtime while wallets are negative.** `allow_negative_balance` is meant to be a stable config decision. The model carries a `balance >= 0` validation gated on the flag; flipping it OFF while wallets sit below zero leaves them un-saveable until you flip it back on (any subsequent `credit` / `debit` calls `refresh_cached_balance!`, which raises a `RecordInvalid`). If you need to turn the flag off, drain or settle any negative wallets first. +- **Concurrency holds.** `wallet.debit` / `wallet.credit` use `with_lock` (row-level `SELECT … FOR UPDATE`); `wallet.transfer_to` locks both wallets in id order via `lock_wallet_pair!`. Two concurrent overdraft debits on the same wallet serialize cleanly — they don't double-count `previous_balance`. ### Expiring balances diff --git a/lib/generators/wallets/templates/initializer.rb b/lib/generators/wallets/templates/initializer.rb index 714f767..7701660 100644 --- a/lib/generators/wallets/templates/initializer.rb +++ b/lib/generators/wallets/templates/initializer.rb @@ -17,6 +17,14 @@ # config.table_prefix = "wallets_" # Set to true only if your domain explicitly supports debt or overdrafts. + # Applies consistently across `wallet.debit` and `wallet.transfer_to` — a + # debit or a transfer can drive the source wallet below zero. Cap how + # negative a wallet can go in your own service layer (the flag is binary). + # + # Heads up: this is meant to be a stable config decision, not a runtime + # toggle. Flipping it OFF while wallets are negative leaves those wallets + # un-saveable until you flip it back on. Drain or settle negative wallets + # before disabling. # # config.allow_negative_balance = false @@ -49,7 +57,8 @@ # on_balance_debited - After value is deducted from a wallet # on_transfer_completed - After a transfer between wallets succeeds # on_low_balance_reached - When balance drops below the threshold - # on_balance_depleted - When balance reaches exactly zero + # on_balance_depleted - When a debit drives balance from positive + # to zero or below (one-shot per crossing) # on_insufficient_balance - When a debit or transfer is rejected # # Context object properties (available depending on event): diff --git a/lib/wallets/models/wallet.rb b/lib/wallets/models/wallet.rb index 61d574a..5c4fba6 100644 --- a/lib/wallets/models/wallet.rb +++ b/lib/wallets/models/wallet.rb @@ -621,11 +621,21 @@ def dispatch_balance_threshold_callbacks!(previous_balance) ) end - if previous_balance.positive? && balance.zero? + # `:balance_depleted` fires when the wallet crosses from a positive + # balance to ZERO OR LOWER on this debit. We use `<= 0` rather than + # `== 0` so the callback still fires when `allow_negative_balance` is + # true and a single debit takes the wallet from e.g. +100 to -50 + # (skipping exact zero). With negatives disabled, `balance` cannot + # go below zero, so `<= 0` collapses to `== 0` and the original + # "exactly zero" semantic is preserved for default-config users. + # Either way the callback is one-shot per crossing — going from -50 + # to -80 doesn't re-fire because `previous_balance` was already + # non-positive. + if previous_balance.positive? && !balance.positive? dispatch_callback(:depleted, wallet: self, previous_balance: previous_balance, - new_balance: 0 + new_balance: balance ) end end diff --git a/test/integration/wallet_callbacks_test.rb b/test/integration/wallet_callbacks_test.rb index 7399853..7de27b5 100644 --- a/test/integration/wallet_callbacks_test.rb +++ b/test/integration/wallet_callbacks_test.rb @@ -83,6 +83,103 @@ class WalletCallbacksTest < ActiveSupport::TestCase assert_equal 0, events.first.new_balance end + # ─────────────────────────────────────────────────────────────────────────── + # depleted under `allow_negative_balance` + # ─────────────────────────────────────────────────────────────────────────── + # + # The original semantic was "fires when balance reaches exactly zero". + # That breaks for apps with negative balances enabled — a single debit + # can take a wallet from +100 to -50, skipping zero entirely. The + # callback was conceptually about "ran out of available value", and a + # negative balance is even more "out" than zero. The condition is + # widened to `previous > 0 && new <= 0` so positive→negative crossings + # also fire it. With negatives disabled, balance can't go below zero, + # so `new <= 0` collapses back to `new == 0` and existing callers see + # no behavior change. + + test "balance_depleted fires when a debit crosses zero straight into negative" do + Wallets.configuration.allow_negative_balance = true + wallet = create_wallet(users(:new_user), asset_code: :wood, initial_balance: 50) + events = [] + + Wallets.configuration.on_balance_depleted { |ctx| events << ctx } + + wallet.debit(120, category: :purchase) + + assert_equal 1, events.size + assert_equal 50, events.first.previous_balance + assert_equal(-70, events.first.new_balance) + assert_equal(-70, wallet.reload.balance) + end + + test "balance_depleted does not re-fire on already-negative wallets that go more negative" do + Wallets.configuration.allow_negative_balance = true + wallet = create_wallet(users(:new_user), asset_code: :wood, initial_balance: 0) + wallet.debit(20, category: :purchase) # balance goes 0 → -20 (depleted fires once) + events = [] + + Wallets.configuration.on_balance_depleted { |ctx| events << ctx } + wallet.debit(30, category: :purchase) # -20 → -50 (already depleted) + + assert_empty events, "depleted is one-shot per crossing — going deeper into negative does not re-fire" + end + + test "balance_depleted re-fires after a positive bounce-back" do + Wallets.configuration.allow_negative_balance = true + wallet = create_wallet(users(:new_user), asset_code: :wood, initial_balance: 100) + events = [] + + Wallets.configuration.on_balance_depleted { |ctx| events << ctx } + + wallet.debit(150, category: :purchase) # 100 → -50, fires (1) + wallet.credit(80, category: :reward) # -50 → 30, no fire (credit) + wallet.debit(60, category: :purchase) # 30 → -30, fires (2) + + assert_equal 2, events.size, "depleted fires once per fresh crossing of the positive→non-positive boundary" + assert_equal [ 100, 30 ], events.map(&:previous_balance) + assert_equal [ -50, -30 ], events.map(&:new_balance) + end + + test "balance_depleted does not fire when a debit lands a wallet that was already non-positive" do + Wallets.configuration.allow_negative_balance = true + wallet = create_wallet(users(:new_user), asset_code: :wood, initial_balance: 0) + events = [] + + Wallets.configuration.on_balance_depleted { |ctx| events << ctx } + wallet.debit(40, category: :purchase) # 0 → -40, previous was already non-positive + + assert_empty events, "previous balance must be strictly positive for depleted to fire" + end + + test "low_balance_reached fires once when a debit drops a wallet below threshold and into negative" do + Wallets.configuration.allow_negative_balance = true + wallet = create_wallet(users(:new_user), asset_code: :gems, initial_balance: 200) + events = [] + + Wallets.configuration.low_balance_threshold = 50 + Wallets.configuration.on_low_balance_reached { |ctx| events << ctx } + + wallet.debit(300, category: :purchase) # 200 → -100, well below threshold + + assert_equal 1, events.size + assert_equal 50, events.first.threshold + assert_equal 200, events.first.previous_balance + assert_equal(-100, events.first.new_balance) + end + + test "low_balance_reached does not re-fire when an already-low wallet dips deeper" do + Wallets.configuration.allow_negative_balance = true + wallet = create_wallet(users(:new_user), asset_code: :gems, initial_balance: 30) + Wallets.configuration.low_balance_threshold = 50 + events = [] + + Wallets.configuration.on_low_balance_reached { |ctx| events << ctx } + + wallet.debit(80, category: :purchase) # 30 → -50, was already below threshold + + assert_empty events + end + test "insufficient_balance fires before raising" do wallet = wallets_wallets(:poor_coins_wallet) events = [] diff --git a/test/models/wallets/wallet_test.rb b/test/models/wallets/wallet_test.rb index 54fe7b5..b4343d5 100644 --- a/test/models/wallets/wallet_test.rb +++ b/test/models/wallets/wallet_test.rb @@ -100,4 +100,143 @@ class Wallets::WalletTest < ActiveSupport::TestCase ensure Wallets.configuration.allow_negative_balance = original_setting end + + # ─────────────────────────────────────────────────────────────────────────── + # Negative-balance audit coverage + # ─────────────────────────────────────────────────────────────────────────── + # + # The full audit pass (see PR #3 description) walked every code path that + # touches balance and found one real bug (`:depleted` callback) plus + # several documented subtleties. The tests below pin the documented + # behavior so future changes don't quietly regress them. + + test "compounding debits on a zero-balance wallet accumulate the unbacked deficit" do + Wallets.configuration.allow_negative_balance = true + wallet = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + + first_debit = wallet.debit(40, category: :purchase) + second_debit = wallet.debit(30, category: :purchase) + + # Each debit's `unbacked_amount` is independent — the gem never + # "spreads" debt across debits. With no positive buckets to consume, + # both debits are 100% unbacked and the wallet's balance is the sum + # of both deficits. + assert_equal 40, first_debit.unbacked_amount + assert_equal 30, second_debit.unbacked_amount + assert_equal(-70, wallet.reload.balance) + end + + test "a credit on a negative-balance wallet does not auto-allocate against existing unbacked debits" do + # Documented behavior: the FIFO model's audit story is intentionally + # immutable — once a debit is unbacked, the gem won't quietly back- + # fill it from a later credit. Both ledger entries persist and the + # `balance` accessor reconciles them on the fly. Apps that want + # "settle the debt automatically on the next credit" should layer + # that on top in their own service code; the gem deliberately keeps + # the ledger append-only and unsurprising. + Wallets.configuration.allow_negative_balance = true + wallet = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + debit = wallet.debit(50, category: :purchase) + credit = wallet.credit(80, category: :reward) + + assert_equal 50, debit.reload.unbacked_amount, "the unbacked debit stays unbacked" + assert_equal 80, credit.reload.remaining_amount, "the new credit is fully unspent" + assert_equal 30, wallet.reload.balance, "balance reconciles the two sides" + end + + test "fifo consumption pulls from the oldest non-expired positive bucket and ignores unbacked debits" do + # Companion to the test above: walks the explicit allocation chain so + # callers reading the audit log can see how the gem reasoned about + # which credit covered which debit. + Wallets.configuration.allow_negative_balance = true + wallet = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + wallet.debit(50, category: :purchase) # 0 → -50, unbacked 50 + new_credit = wallet.credit(80, category: :reward) # -50 → 30, credit untouched + spend = wallet.debit(60, category: :purchase) # 30 → -30 + spend.reload + + allocations = spend.outgoing_allocations.includes(:source_transaction).order(:id).to_a + assert_equal 1, allocations.size, "only the new credit was used; the old unbacked debit stays unbacked" + assert_equal new_credit.id, allocations.first.source_transaction_id + assert_equal 60, allocations.first.amount + assert_equal(-30, wallet.reload.balance) + end + + test "has_enough_balance? returns false on a negative wallet for any positive amount" do + Wallets.configuration.allow_negative_balance = true + wallet = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + wallet.debit(40, category: :purchase) + + assert_equal(-40, wallet.reload.balance) + refute wallet.has_enough_balance?(1) + refute wallet.has_enough_balance?(40) + refute wallet.has_enough_balance?(0_000_000_000) + end + + test "transactions on a negative wallet record correct balance_before and balance_after snapshots" do + Wallets.configuration.allow_negative_balance = true + wallet = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 10) + debit = wallet.debit(40, category: :purchase) + credit = wallet.credit(15, category: :reward) + + assert_equal 10, debit.balance_before + assert_equal(-30, debit.balance_after) + assert_equal(-30, credit.balance_before) + assert_equal(-15, credit.balance_after) + end + + test "flipping allow_negative_balance off while a wallet is negative makes subsequent saves fail" do + # Documented gotcha: `allow_negative_balance` is meant to be a stable + # config decision, not a runtime toggle. The wallet model carries a + # `balance >= 0` validation gated on the flag; flipping the flag off + # while wallets are below zero leaves them un-saveable, which means + # any further `credit` / `debit` (both call `refresh_cached_balance!` + # internally, which calls `save!`) raises a validation error. + Wallets.configuration.allow_negative_balance = true + owner = users(:new_user) + wallet = create_wallet(owner, asset_code: :credits, initial_balance: 0) + wallet.debit(20, category: :purchase) + assert_equal(-20, owner.wallet(:credits).balance) + + Wallets.configuration.allow_negative_balance = false + + # Re-fetch the wallet so the lock acquired by `with_lock` doesn't trip + # AR's "no unpersisted changes" guard. Apps reach this branch by + # calling `user.wallet(:foo).credit(...)` after a flag flip; this + # mirrors that. + error = assert_raises(ActiveRecord::RecordInvalid) do + owner.wallet(:credits).credit(5, category: :reward) + end + assert_includes error.message, "Balance must be greater than or equal to 0" + + # Recovery: flip the flag back on. + Wallets.configuration.allow_negative_balance = true + owner.wallet(:credits).credit(5, category: :reward) + assert_equal(-15, owner.wallet(:credits).balance) + end + + test "concurrent debits on the same wallet serialize through with_lock and accumulate correctly" do + # `wallet.debit` wraps `apply_debit` in `with_lock`, which acquires a + # row-level FOR UPDATE on the wallet row. With `allow_negative_balance` + # on, two concurrent debits should NOT both observe the same + # `previous_balance` and double-count: they serialize, each sees the + # post-prior-debit state, and the resulting balance is the sum. + Wallets.configuration.allow_negative_balance = true + wallet = create_wallet(users(:new_user), asset_code: :credits, initial_balance: 0) + + threads = 4.times.map do + Thread.new do + # Each thread needs its own AR connection; integration tests rely + # on the connection-per-thread pool. Without checkout, threads + # would share the test connection and serialize incidentally. + ActiveRecord::Base.connection_pool.with_connection do + wallet.class.find(wallet.id).debit(10, category: :purchase) + end + end + end + threads.each(&:join) + + assert_equal(-40, wallet.reload.balance) + assert_equal 4, wallet.transactions.where("amount < 0").count + end end