Rebuild usage_credits on wallets ledger core (v1.0.0)

[rameerez]

Summary

This release rebuilds usage_credits on top of the new wallets gem, which provides the ledger core: balances, transactions, allocations, transfers, and expiration handling.

┌─────────────────────────────────────────────────────────────────┐
│                      usage_credits 1.0                          │
│  ┌───────────────────────────────────────────────────────────┐  │
│  │  Subscriptions, Credit Packs, Pay Integration, Fulfillment │  │
│  │  Operations DSL, Pricing, Refunds, Webhook Handling        │  │
│  └───────────────────────────────────────────────────────────┘  │
│                            │                                    │
│                            ▼                                    │
│  ┌───────────────────────────────────────────────────────────┐  │
│  │                       wallets                              │  │
│  │    Balance, Credit, Debit, Transfer, Expiration, FIFO,    │  │
│  │    Audit Trail, Row-Level Locking, Multi-Asset            │  │
│  └───────────────────────────────────────────────────────────┘  │
└─────────────────────────────────────────────────────────────────┘

---

Architecture

• UsageCredits::Wallet, Transaction, Allocation, Transfer now extend their Wallets::* counterparts using the embeddability hooks
• Each subclass sets embedded_table_name, config_provider, callbacks_module, and related model class names
• Both gems can coexist in the same Rails app without table/config collision
• Cross-class transfers are rejected (UsageCredits::Wallet cannot transfer to Wallets::Wallet)

---

Key Changes

New Files

• lib/usage_credits/models/transfer.rb — credit transfers between users
• lib/generators/usage_credits/upgrade_generator.rb — upgrade migration for pre-1.0 installs
• test/integration/coexistence_test.rb — verifies both gems work independently

Schema Updates

• Migration templates updated with new schema:
  • expiration_policy column on transfers (preserve/none/fixed)
  • transfer_id FK on transactions (no singular outbound/inbound FKs)
  • asset_code column on wallets (default: "credits")
  • All value columns now bigint

Model Changes

• Wallet/Transaction/Allocation now extend Wallets::* base classes
• Re-declared associations point to UsageCredits::* classes
• Callback event mapping: credited → credits_added, debited → credits_deducted, etc.

---

Backwards Compatibility

All existing API preserved:

# Still works exactly the same
user.credits                           # => 1000
user.give_credits(500, reason: "bonus")
user.spend_credits_on(:send_email)
user.has_enough_credits_to?(:generate_report)
user.credit_history

Existing installs run the upgrade migration to add new columns/tables.

---

New Capabilities from Wallets Core

Feature	Description
Transfer expiration policies	:preserve (default), :none, :fixed
Multi-bucket transfer splitting	Preserves source expirations across multiple inbound legs
Row-level locking	Prevents race conditions on concurrent operations
Balance snapshots	balance_before / balance_after on every transaction

---

Test Results

729 runs, 1756 assertions, 0 failures, 0 errors, 0 skips

---

Test Plan

• All 729 tests pass
• Coexistence test verifies both gems work independently
• Upgrade migration test verifies schema changes
• Migration template test verifies correct schema generation
• Backwards compatibility preserved for all existing API

---

🤖 Generated with Claude Code

[claude]

placeholder test comment - will be replaced

[claude]

ignore this test comment

[claude]

ignore

[claude]

multi line
body test

[claude]

heading test:
this line has content
and the next line would be a heading but I cant test that inline

[claude]

line 1
## Heading test
line 3 content

[claude]

line 1

HTML Heading Test

line 3 content

[claude]

Code Review — PR #30: Rebuild on wallets ledger core (v1.0.0)

This is a substantial architectural refactor. The layering is clean and the backwards-compat story is well-thought-out. A few things worth addressing before merging:

---

Potential Bugs

1. deduct_credits return value not verified

The new wrapper returns whatever Wallets::Wallet#debit returns. The old implementation explicitly returned the spend_tx. Some callers (e.g., spend_credits_on) depend on the return value being the transaction record. If the parent's debit returns something different this would be a silent breakage — worth an explicit test or doc note confirming the contract.

2. Upgrade migration has no down method

upgrade_usage_credits_to_wallets_core.rb.erb defines only up. rails db:rollback after running this migration will silently no-op. Consider adding def down; raise ActiveRecord::IrreversibleMigration; end to make the intent explicit rather than allowing a broken rollback.

3. TOCTOU in ensure_credit_wallet

wallet = original_credit_wallet || UsageCredits::Wallet.find_by(owner: self, asset_code: "credits")
return wallet if wallet.present?
UsageCredits::Wallet.create_for_owner!(...)

Two concurrent requests could both get nil from both checks and race to create_for_owner!. The unique index means the second will fail at the DB level, but the resulting ActiveRecord::RecordNotUnique won't be rescued gracefully. A rescue ActiveRecord::RecordNotUnique + re-lookup would harden this edge case.

---

Code Quality

4. CATEGORIES = DEFAULT_CATEGORIES is redundant

In transaction.rb, both constants now point to the same frozen object. The original code had a comment explaining CATEGORIES was a backwards-compat alias — that comment was removed in this PR. Either restore the comment or remove CATEGORIES if nothing external references it.

5. "credits" string is scattered across ~10 locations

The asset code "credits" is hard-coded in has_wallet.rb, wallet.rb, test fixtures, and migration templates. Extracting it to a constant (e.g., UsageCredits::DEFAULT_ASSET_CODE = "credits") would reduce typo risk and make future changes trivial.

6. table_prefix in Configuration looks like dead code

The table_prefix method always returns "usage_credits_" but the actual table routing is done via embedded_table_name on each model class. If this method is not part of the wallets embeddability API contract, it should be removed to avoid confusion.

7. UpgradeGenerator does not guard against duplicate migrations

Running rails generate usage_credits:upgrade twice produces two migration files with different timestamps. Consider checking for an existing upgrade migration file before generating a new one.

---

Architecture / Design

8. wallets pinned to ~> 0.2 (pre-1.0 gem)

Pre-1.0 gems conventionally allow breaking changes in minor bumps. The ~> 0.2 constraint locks out 0.3 even for pure bug fixes. Document this coupling so downstream consumers understand they will need a usage_credits update to pick up wallets fixes beyond 0.2.x.

9. transfer_completed: nil silently drops the event

Users who configure on_transfer_completed in UsageCredits.configure will get a silent no-op. There is a test confirming this is intentional, but there is no user-facing documentation. A brief note in the README that transfer callbacks are only available at the wallets gem layer would prevent confusion.

---

Testing

10. No test for transfer_credits_to (the backwards-compat wrapper)

wallet.rb introduces transfer_credits_to as a backwards-compat alias for transfer_to with error translation (Wallets::InvalidTransfer to UsageCredits::InvalidTransfer). All new tests call transfer_to directly. A test for the wrapper and its error mapping would be worth adding.

11. CoexistenceTest setup only cleans wallets-side tables

The setup block only calls Wallets::Wallet.where(owner_type: "Team").delete_all. A comment explaining why usage_credits-side cleanup is not needed (i.e., UsageCredits::Wallet records for Teams cannot exist given the model separation) would make the intent clearer.

---

Minor

• schema.rb bumps to ActiveRecord::Schema[8.1]. Teams on Rails 7.x will see a diff if they regenerate their schema. Worth a note in the migration guide.
• usage_credits_allocations in schema.rb still has an index named index_allocations_on_transaction_id (old naming convention) alongside the new properly-namespaced indexes. Not a functional issue, but worth fixing for consistency.

---

Overall this is solid work. The embeddability pattern is clean, the coexistence test is genuinely useful, and the upgrade migration test using a real SQLite database is exactly the right approach for verifying data-preserving migrations. The main items to address before merging are the missing down method (item 2), the wallets coupling documentation (item 8), and the "credits" constant extraction (item 5) to reduce future maintenance burden.

[claude]

Code Review: PR #30 — Rebuild usage_credits on wallets ledger core (v1.0.0)

This is a substantial, well-structured refactor. The layering strategy (extending Wallets::* base classes, re-declaring associations with correct namespaced classes, mapping events via callback_event_map) is clean and the coexistence test is a great addition. A few things worth addressing before merging:

---

🔴 High Priority

1. credits method may silently break the backwards-compatibility contract

wallet.rb now implements credits as:

def credits
  balance
end

The comment above it says:

usage_credits historically floors negative balances to zero even when allow_negative_balance is enabled. Keep that contract for backwards compatibility.

But the implementation doesn't enforce this floor. If the wallets layer allows balance to go negative (when allow_negative_balance is true), callers who previously received 0 will now receive a negative number. The old code was:

.yield_self { |sum| [sum, 0].max }.to_i

If the intent is truly to preserve that contract, it should be:

def credits
  [balance, 0].max
end

…or the comment should be revised to say the floor is intentionally removed.

---

2. add_credits passes fulfillment: to the wallets gem's credit() — is that supported?

wallet.rb:

def add_credits(amount, metadata: {}, category: :credit_added, expires_at: nil, fulfillment: nil)
  credit(
    amount,
    metadata: metadata,
    category: category,
    expires_at: expires_at,
    fulfillment: fulfillment   # ← is Wallets::Wallet#credit expecting this?
  )
end

If Wallets::Wallet#credit doesn't accept a fulfillment: keyword, this will raise ArgumentError: unknown keyword: fulfillment at runtime for every subscription fulfillment. This path is exercised heavily (subscription credits, credit packs). Worth double-checking the wallets gem's credit method signature.

---

3. Upgrade migration has no down method

upgrade_usage_credits_to_wallets_core.rb.erb only defines up. This means db:rollback will raise a IrreversibleMigration (or silently do nothing, depending on Rails version). Pre-1.0 users who run the upgrade and hit a problem can't roll back automatically. Either add a down that reverses each step, or explicitly mark it irreversible:

def down
  raise ActiveRecord::IrreversibleMigration, "Cannot roll back the 1.0 upgrade. Restore from a database backup."
end

---

🟡 Medium Priority

4. Locking behavior in deduct_credits — is it preserved?

The original deduct_credits wrapped everything in with_lock do with FOR UPDATE on the positive transaction rows. The new version delegates entirely to debit(...) from the wallets gem. Whether the wallets gem provides equivalent row-level locking is critical for preventing double-spending under concurrency. Worth making this explicit in a comment (or the wallets gem's docs) so future maintainers know the lock guarantee isn't lost.

5. table_prefix leaks an internal detail into the public Configuration API

configuration.rb:

def table_prefix
  "usage_credits_"
end

The comment says this is for wallets gem compatibility and is always "usage_credits_". If it's not user-configurable (and can't be changed without breaking things), it shouldn't be on Configuration as a public method — it gives the false impression it can be overridden. Consider moving it to an internal constant or the model layer.

6. transfer_to vs transfer_credits_to — two surfaces for the same operation

The README documents the wallet-level API as credit_wallet.transfer_to(...), which calls the method inherited from Wallets::Wallet directly. But there's also transfer_credits_to which wraps it. These two paths have different error types raised. Callers using the direct transfer_to won't get the InsufficientCredits/InvalidTransfer translation. Either document which one to use, or have transfer_to be overridden to go through the wrapper.

---

🟢 Low / Nits

7. Schema version vs. migration version mismatch

schema.rb declares ActiveRecord::Schema[8.1] but new migration files use ActiveRecord::Migration[7.2]. This is cosmetic in the test dummy, but could cause warnings or confusion in apps on Rails 7.x that run the new migrations.

8. ensure_credit_wallet double-lookup

wallet = original_credit_wallet || UsageCredits::Wallet.find_by(owner: self, asset_code: "credits")
if wallet.present?
  self.credit_wallet = wallet unless original_credit_wallet == wallet
  return wallet
end

When original_credit_wallet is nil, the unless nil == wallet is always true, making the conditional redundant. Minor, but it adds noise.

9. Old empty_wallet fixture removal

The empty_wallet fixture (user 3, balance 0) was removed. If any test was relying on user 3 having no wallet for "wallet creation" tests, that scenario is now covered by the new walletless_user fixture. Just confirm no test references empty_wallet explicitly (looks like it was fully cleaned up, but worth a grep).

---

✅ What's working well

• The callback_event_map approach for translating wallets events to usage_credits events is elegant and makes the mapping explicit.
• Re-declaring associations with namespaced class names rather than relying on STI-style inference is the right call.
• The coexistence test (test/integration/coexistence_test.rb) is thorough and directly validates the multi-gem scenario.
• The upgrade migration covers all the schema changes (bigint columns, asset_code, transfers table) needed for pre-1.0 installs.
• CI change to run db:migrate:reset before tests ensures schema drift is caught early.

Overall this is a clean layering. The credits floor issue (#1) and the fulfillment: parameter (#2) are the ones most likely to cause silent regressions in production.

[claude]

Code Review — PR #30: Rebuild usage_credits on wallets ledger core (v1.0.0)

Great architectural direction — delegating the ledger core to wallets cleans up a lot of complexity and the backwards-compatible API surface is well-preserved. The 729-test suite passing is a healthy signal. Below are issues found, ordered by severity.

---

🔴 Critical

Upgrade migration is missing add_column :usage_credits_wallets, :metadata

The upgrade template adds new columns to the wallets schema but never adds the metadata column to usage_credits_wallets. Fresh installs get it (the install template includes it), but upgrading apps will hit PG::UndefinedColumn / SQLite3::Exception every time the wallets core reads or writes metadata on a wallet row post-upgrade. The create_pre_1_0_schema! helper in the test confirms this column didn't exist pre-1.0, which makes the omission in the upgrade template a silent data-access crash.

Fix: Add add_column :usage_credits_wallets, :metadata, :jsonb, default: {} (Postgres) / :text (SQLite/MySQL) to the upgrade migration, guarded with unless column_exists?.

---

🟠 Major

credits method does not floor negative balances

wallet.rb documents that credits preserves backwards-compatible floor-at-zero semantics, but the implementation just calls balance directly. The wallets core's balance method can return a negative value if allow_negative_balance is enabled on the config. The old implementation had .yield_self { |sum| [sum, 0].max }. This is silently broken.

Fix: def credits; [balance, 0].max; end

---

allocation_does_not_exceed_remaining_amount validation was removed

This validation guarded against allocations exceeding the available source credits in the FIFO ledger. It has been removed as part of delegating to Wallets::Allocation, but there is no confirmation in the diff that the parent class carries an equivalent guard. If it doesn't, allocations can silently persist that corrupt the ledger.

Action needed: Verify Wallets::Allocation has this guard, or re-add it to UsageCredits::Allocation.

---

transfer_credits_to only rescues specific subclasses, not Wallets::Error

wallet.rb rescues Wallets::InvalidTransfer and Wallets::InsufficientBalance but not Wallets::Error (the base class). Any new error subclass added to the wallets gem will surface as an unwrapped Wallets::* exception, breaking the UsageCredits error contract.

Fix: Add rescue Wallets::Error => e as a catch-all that re-raises as the corresponding UsageCredits::Error.

---

🟡 Minor

Transaction::CATEGORIES constant is stale

CATEGORIES = DEFAULT_CATEGORIES is a frozen snapshot that won't include additional_categories added at runtime via configuration. def self.categories computes dynamically and is correct, but the constant is still publicly accessible. Any caller using Transaction::CATEGORIES directly gets the wrong set. Either remove the constant or make it a method alias.

---

Upgrade migration change_column calls unguarded for existing bigint columns

The integer → bigint change_column calls aren't guarded with a type check. On MySQL this will re-run the column alteration (which locks the table) even when columns are already bigint. Suggest wrapping with if column_info(:usage_credits_wallets, :amount)&.sql_type =~ /^int/.

---

Coexistence test mutates global config without ensure

test/integration/coexistence_test.rb mutates both UsageCredits.configuration and Wallets.configuration inline and restores state via instance_variable_set at the end of the test. If the test raises before reaching those lines, all subsequent tests run with polluted config. Wrap in begin/ensure or extract to setup/teardown.

---

TOCTOU gap in spend_credits_on

spend_credits_on calls has_enough_credits_to? (no lock) and then acquires lock! inside a transaction before debiting. The insufficient-credits check fires the callback and raises before the lock, so a concurrent caller can slip past the guard. The wallets core's debit should be atomic, but the pre-lock guard callback fires spuriously on races. Consider moving the balance check to after acquiring the lock.

---

Coexistence isolation assertion references wrong column

coexistence_test.rb asserts UsageCredits::Transfer.where(from_wallet_id: wallets_wallet.id).count == 0 — but from_wallet_id on usage_credits_transfers refers to a usage_credits_wallets row ID, not a wallets_wallets row ID. The assertion passes vacuously (no uc transfers created) and would silently pass even if isolation broke differently.

---

Upgrade migration test only covers SQLite

The upgrade migration test always uses adapter: "sqlite3". The change_column for bigint and the jsonb → text type differences are not exercised on Postgres or MySQL, which is where most production installs will run.

---

🔵 Nit / Polish

• gemspec:50 — ~> 0.2 is too permissive for a pre-1.0 dependency. wallets is at 0.x with no API stability guarantee. ~> 0.2 allows any 0.2.x patch to be picked up automatically; since usage_credits couples deeply to embeddability hooks (embedded_table_name, config_provider, create_for_owner!), a wallets patch that changes those hooks would silently break production. Consider ~> 0.2.0 until wallets reaches 1.0.

• Dead table_prefix method in configuration.rb. It returns a hardcoded "usage_credits_" string and is never read by any code in the diff. Either wire it into the embeddability setup or remove it — as-is it implies table names are configurable when they're not.

• CHANGELOG upgrade instructions don't mention the irreversible migration. The down method on the upgrade migration raises IrreversibleMigration. The instructions say "deploy gem + db:migrate together" which is correct, but there's no mention that rollback isn't possible. Worth a one-liner for ops clarity.

• README coexistence section references find_wallet(:asset) without a link. This method comes from the wallets gem API and may change before 1.0. Linking to the wallets docs (or noting the method is from wallets) would make this more robust.

---

Summary

Severity	Count
🔴 Critical	1
🟠 Major	3
🟡 Minor	4
🔵 Nit	4

The most important items to address before merging are the missing metadata column in the upgrade migration (will crash production upgrades) and the credits floor not being applied (silent regression in the backwards-compatibility contract). The major items around allocation validation and error handling are also worth confirming before ship.

🤖 Reviewed with Claude Code

[claude]

Code Review: Rebuild usage_credits on wallets ledger core (v1.0.0)

This is a well-executed major refactor. The architecture is sound, the backwards-compatibility story is carefully preserved, and the test suite is substantive. Below are issues from critical to low, then a summary of what is done well.

---

Critical

credits method no longer floors at zero

# wallet.rb
def credits
  balance  # returns the raw stored column
end

The old implementation did [sum, 0].max to guarantee a non-negative result even when allow_negative_balance is enabled. The PR says this contract is preserved, but whether it still holds depends entirely on what the wallets gem stores in the balance column when an overdraft occurs. The test assert_equal 0, wallet.balance when overdraft is allowed passes, so the wallets core appears to floor too — but this is an implicit dependency on upstream behaviour, not an explicit guarantee in this gem. A cheap safeguard:

def credits
  [balance, 0].max
end

This makes the contract explicit and immune to a future change in how the wallets core stores overdrafts.

---

Medium

Category validation removed from Transaction

The old class had:

validates :category, presence: true, inclusion: { in: ->(record) { Transaction.categories } }

The new Transaction < Wallets::Transaction removes this. Transaction.categories is still defined and extended with usage_credits-specific entries (transfer_in, transfer_out, etc.), but there is no enforcement that created transactions actually belong to that list. If the wallets base class validates against its own smaller category list, usage_credits categories like signup_bonus or subscription_credits could fail at the wrong layer; if the base class does not validate categories at all, invalid categories can now be persisted silently.

Allocation validations removed

Similarly, the old Allocation class had:

validates :amount, numericality: { only_integer: true, greater_than: 0 }
validate :allocation_does_not_exceed_remaining_amount

These are gone in Allocation < Wallets::Allocation. Whether the wallets core re-applies them is unclear from this diff. If it does not, over-allocation is no longer caught at the model layer.

Upgrade migration: change_column calls are not guarded

All four change_column calls are unguarded, so re-running the migration after a partial MySQL failure could error. The surrounding operations are correctly guarded with column_exists? / index_exists? / table_exists?, but these four are not:

change_column :usage_credits_wallets, :balance, :bigint, null: false, default: 0
change_column :usage_credits_transactions, :amount, :bigint, null: false
change_column :usage_credits_allocations, :amount, :bigint, null: false
change_column :usage_credits_fulfillments, :credits_last_fulfillment, :bigint, null: false

A column-type guard before each call would close this gap.

---

Low

initial_balance: credit_options[:initial_balance].to_i silently converts nil to 0

NilClass#to_i returns 0 which is the desired default, but it masks the case where initial_balance is explicitly set to nil. credit_options.fetch(:initial_balance, 0) makes the intent explicit.

Hardcoded error message string in coexistence test

assert_equal "Wallet classes must match", error.message

Coupling a test to an upstream gem's error string means any rephrasing in the wallets gem breaks this test. Checking only the exception type is more resilient.

Test uses instance_variable_get to reach private config state

original_wallets_callback = Wallets.configuration.instance_variable_get(:@on_balance_credited_callback)

Workable for now, but brittle if the wallets gem ever exposes a public accessor or reset helper.

index_allocations_on_transaction_id is still in schema.rb

The new naming convention uses fully-qualified names but one old name (index_allocations_on_transaction_id) survives because the upgrade migration intentionally does not rename it. The upgrade migration template comment explains this well, but a corresponding note directly in schema.rb near that index name would help future maintainers avoid accidentally dropping and recreating it.

Coexistence test migration hardcodes ActiveRecord::Migration[7.2]

test/dummy/db/migrate/20250417000000_create_wallets_coexistence_tables.rb pins to [7.2] while the rest of the dummy schema was updated to [8.1]. Low impact but worth aligning.

---

What is done well

• Pre-flight duplicate wallet check in the upgrade migration is excellent production safety. Aborting before touching the schema and providing a runnable console snippet to fix the data is exactly the right approach.
• Idempotency of every step in the upgrade migration (column_exists?, index_exists?, table_exists? guards) is essential for MySQL compatibility and safe re-runs after interruption.
• Callback event mapping with nil suppression (transfer_completed: nil) is a clean way to opt out of wallets events that have no usage_credits equivalent, and the test coverage for it is solid.
• Coexistence test is thorough — it checks table isolation, class types, transfer direction, and callback isolation in one file.
• ensure_credit_wallet race-proofing via find_by fallback + create_for_owner! correctly handles the stale-association case that could previously create duplicate wallets.
• bigint promotion across all amount/balance columns is the right long-term choice, and documenting the production impact on large tables in both the migration and README is appreciated.
• Exception wrapping (Wallets::InsufficientBalance to InsufficientCredits, Wallets::InvalidTransfer to InvalidTransfer) preserves the public error contract cleanly.
• db:migrate:reset in CI ensures the real migration path is exercised in SQLite so schema drift is caught before release.

---

Overall this is a well-structured integration. The critical point about the zero-floor contract and the medium points about removed validations are worth resolving before release; the rest are polish.