Skip to content

Negative-balance correctness: fix transfer_to + :balance_depleted, audit + pin documented behavior#3

Merged
rameerez merged 2 commits into
mainfrom
fix/transfer-respects-allow-negative-balance
May 3, 2026
Merged

Negative-balance correctness: fix transfer_to + :balance_depleted, audit + pin documented behavior#3
rameerez merged 2 commits into
mainfrom
fix/transfer-respects-allow-negative-balance

Conversation

@rameerez
Copy link
Copy Markdown
Owner

@rameerez rameerez commented May 3, 2026

Summary

Wallets.configuration.allow_negative_balance 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 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_depleted callback 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_to honors allow_negative_balance

lib/wallets/models/wallet.rb:

  • The insufficient-balance pre-check is now gated on !allow_negative_balance?, matching apply_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 shape wallet.debit already produced).
  • 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. There are no positive source buckets to inherit expirations from for the deficit portion, so 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 (with expires_at:) and explicit :none are honored unchanged — those policies don't depend on source-bucket walking.
  • The :insufficient_balance callback no longer fires for successful overdraft transfers. It still fires (and the transfer still raises Wallets::InsufficientBalance) when the flag is off and the transfer is rejected.

:balance_depleted callback fires on positive→non-positive crossings

Previously 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 from balance.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:

  • 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; balance 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. 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_reached is 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.
  • Concurrent overdraft debits on the same wallet serialize through with_lock (FOR UPDATE) and accumulate cleanly without double-counting previous_balance. Concurrent transfers serialize through lock_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.

Run Result
bundle exec rake test 92 / 338 assertions, all green
gemfiles/rails_7.2.gemfile 92 / 338 assertions, all green
gemfiles/rails_8.1.gemfile 92 / 338 assertions, all green

Compatibility

  • allow_negative_balance defaults to false, so apps that haven't opted in see no behavior change.
  • :balance_depleted semantic widening is a no-op under default config (balance can't go below zero).
  • Apps that previously caught Wallets::InsufficientBalance from transfer_to after 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

  • README gains a "Negative balances and overdraft" section covering: the unified wallet.debit / wallet.transfer_to contract, the floor-is-yours-to-enforce model, the :preserve:none fallback, :balance_depleted semantics, :low_balance one-shot behavior, credit-doesn't-settle-prior-debt, the flag-flip lockup, concurrency guarantees, and the system-reversal use case.
  • CHANGELOG.md updated for 0.2.0.
  • Initializer template comment on allow_negative_balance warns against runtime-toggling; on_balance_depleted docstring updated to reflect the crossing-into-non-positive semantic.

Bumps version to 0.2.0.

🤖 Generated with Claude Code

rameerez and others added 2 commits May 3, 2026 23:03
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>
@rameerez rameerez changed the title Make transfer_to honor allow_negative_balance Negative-balance correctness: fix transfer_to + :balance_depleted, audit + pin documented behavior May 3, 2026
@rameerez rameerez merged commit 43afc41 into main May 3, 2026
11 of 12 checks passed
@rameerez rameerez deleted the fix/transfer-respects-allow-negative-balance branch May 3, 2026 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant