Add balance_before and balance_after to transactions

[rameerez]

Implements the "balance after transaction" feature requested in #8 — adding running balance information to transactions, similar to how banking apps display balance after each transaction.

Key additions:

• transaction.balance_after — the wallet balance immediately after this transaction
• transaction.balance_before — the wallet balance immediately before this transaction
• transaction.formatted_balance_after — formatted output using configured credit formatter

Why This Approach?

Goal

Add banking-like running balance to transaction history with:

• ✅ Minimal footprint — only 36 lines of production code added
• ✅ No migrations — uses existing metadata JSON column
• ✅ Full backwards compatibility — old transactions return nil gracefully
• ✅ Production-ready safety — ACID-compliant for money-like assets

Constraints Considered

1. No database schema changes — Users shouldn't need to run migrations
2. Backwards compatibility — Existing transactions must continue to work
3. Accuracy over convenience — For banking-like systems, data must be correct in ALL edge cases
4. Atomic operations — Credit operations must be all-or-nothing

Design Decisions

Why store in metadata vs. a new column?

• No migration required
• JSON column already exists and is used for audit data
• Backwards compatible — old transactions simply have no balance info
• Consistent with existing pattern of storing audit data in metadata

Why store BOTH balance_before AND balance_after?

Initially, I implemented balance_before as calculated: balance_after - amount. This works 99% of the time, but breaks in edge cases:

# With allow_negative_balance enabled:
# balance_before = 10, amount = -25, balance_after = 0 (floored)
# Calculated balance_before = 0 - (-25) = 25 ❌ (should be 10)

For a banking-like system handling money-equivalent assets, calculated values that can be wrong in edge cases are unacceptable. Explicit storage guarantees correctness.

Why update metadata after transaction creation (two writes)?

The balance can only be accurately computed AFTER the transaction exists (the credits method queries all transactions). Options considered:

1. Compute upfront (previous_balance ± amount) — Simpler formula but could diverge from actual credits calculation in edge cases
2. Update after create — Uses actual computed balance, guarantees consistency with wallet.balance column

Chose option 2 for safety. The extra write is negligible and fully protected by the database transaction.

Safety Analysis

ACID Compliance

Property	Implementation
Atomicity	All operations inside with_lock do...end — if update! fails, entire transaction rolls back
Consistency	balance_after always equals wallet.balance at commit time
Isolation	Row-level SELECT FOR UPDATE lock prevents concurrent modifications
Durability	Standard Rails/ActiveRecord guarantees

Callback Safety

Callbacks are executed inside execute_safely which rescues all errors — callbacks can never break credit operations or cause partial commits.

All Entry Points Covered

Audited every code path that creates transactions:

Entry Point	Method	Coverage
give_credits	→ add_credits	✅
spend_credits_on	→ deduct_credits	✅
Credit pack purchase	→ add_credits	✅
Subscription credits (trial/bonus/regular)	→ add_credits	✅
Subscription upgrade	→ add_credits	✅
Fulfillment job	→ add_credits	✅
Refunds	→ deduct_credits	✅

Test Coverage

Added 27 comprehensive tests covering:

• Basic accessor functionality
• Storage during give_credits, deduct_credits, spend_credits_on
• Mathematical consistency (balance_before + amount = balance_after)
• Edge cases:
  • Nil metadata handling
  • Zero starting balance
  • Exact deduction to zero
  • Large credit amounts (1 billion)
  • Negative balance scenarios (allow_negative_balance)
  • Transaction chain consistency
  • Metadata persistence across reloads
  • Concurrent metadata updates

Test Results: 700 tests, 1587 assertions, 0 failures, 0 errors

Usage Example

# Get transaction history with running balance (like a bank statement)
user.credit_history.each do |tx|
  puts "#{tx.created_at.strftime('%Y-%m-%d')}: #{tx.formatted_amount}"
  puts "  Balance: #{tx.balance_before} → #{tx.balance_after}"
end

# Output:
# 2024-12-16: +1000 credits
#   Balance: 0 → 1000
# 2024-12-26: +500 credits
#   Balance: 1000 → 1500
# 2025-01-14: -50 credits
#   Balance: 1500 → 1450

Files Changed

File	Changes
lib/usage_credits/models/transaction.rb	+24 lines — accessors and formatted helper
lib/usage_credits/models/wallet.rb	+12 lines — store balance info in metadata
test/models/usage_credits/transaction_test.rb	+392 lines — 27 new tests

Checklist

• No migrations required
• Backwards compatible (old transactions return nil)
• All 700 tests pass
• ACID-compliant implementation
• All transaction creation paths covered
• Edge cases tested (negative balance, zero balance, large amounts)
• Follows existing code patterns and style

Closes #8

---

🤖 Generated with Claude Code

[claude]

Code Review for PR #27: Add balance_before and balance_after to transactions

Summary

This PR implements a banking-like running balance feature for transactions, adding balance_after and balance_before fields stored in transaction metadata. Overall, this is a well-designed and thoroughly tested implementation that follows the existing codebase patterns.

Strengths

Code Quality

• Minimal footprint: Only 36 lines of production code
• Backwards compatible: Returns nil for old transactions, no migration required
• Excellent test coverage: 27 comprehensive tests covering edge cases
• Clean implementation following existing patterns

Design Decisions

• ACID compliance: Operations inside with_lock ensure atomicity
• Explicit storage: Storing both balance_before and balance_after avoids calculation errors in edge cases
• Proper timing: Balance information stored after transaction creation using actual computed values

Testing

• Thorough edge case coverage: nil metadata, negative balances, zero balances, large amounts, interleaved operations
• Validates mathematical consistency
• Tests persistence across reloads and metadata updates
• Tests all entry points

Areas for Consideration

1. Race Condition in Two-Write Pattern (Medium Priority)

Location: lib/usage_credits/models/wallet.rb:187-190 and 287-290

The implementation uses two database writes. Between the first and second write, the transaction exists without balance metadata. If the second update! fails, transactions would be created without balance information.

Recommendation: Consider computing balance upfront and including in initial create!, or add defensive backfill logic.

2. Performance Consideration (Low Priority)

The current implementation calls credits method twice per transaction, which executes a complex SQL query each time. For high-volume operations, this adds 2x query overhead.

Recommendation: Cache the balance calculation within the lock block, or use the balance column directly since it is kept in sync.

3. Metadata Merge Ordering (Low Priority)

The code merges balance information into existing metadata. If custom metadata happens to include balance_before or balance_after keys, they would be overwritten silently.

Recommendation: Consider namespacing system metadata or documenting that these keys are reserved.

Security

No security concerns identified. Uses parameterized queries, proper escaping, and row-level locks.

Test Coverage

The test suite is exceptionally comprehensive with 27 tests covering all major scenarios and edge cases.

Suggested additions (optional):

1. Test what happens if update! fails in the two-write pattern
2. Performance test with large transaction histories
3. Concurrent transaction test

Documentation

The PR description is excellent with clear explanation of design decisions, trade-offs, safety analysis, and usage examples.

Minor suggestion: Add a usage example to README.md showing the new feature in action.

Recommendation

APPROVE with minor suggestions

This is a high-quality PR with excellent testing and documentation. The two-write pattern is the only notable concern, but it is within acceptable risk since operations are within a lock, old transactions gracefully return nil, and the feature is additive.

Great work on this feature!

[rameerez]

Addressing the review points:

1. Two-Write Pattern (Medium Priority)

Good catch on the pattern, but this is actually zero risk rather than "acceptable risk":

The with_lock block in Rails wraps everything in a single database transaction. If the update! fails:

1. An exception is raised
2. The entire DB transaction rolls back (including the create!)
3. No orphaned transaction can exist

However, I agree the code could be clearer. I've added an explanatory comment in d9dcda6:

# Store balance information in transaction metadata for audit trail.
# Note: This update! is in the same DB transaction as the create! above (via with_lock),
# so if this fails, the entire transaction rolls back - no orphaned records possible.

2. Metadata Merge Ordering (Low Priority)

The current behavior is intentional - we want to overwrite any user-supplied balance_before/balance_after keys. The system must be authoritative about balance values; letting user-supplied values through would be a data integrity risk.

This follows the same pattern used elsewhere in the codebase (e.g., executed_at, gem_version are always system-set).

I've added a comment documenting this is intentional:

# We intentionally overwrite any user-supplied balance_before/balance_after keys
# to ensure system-set values are authoritative.