Fix critical bugs (host-app shadowing, FK destroy, PG race), extract shared concerns, reach 100% line coverage#4
Open
rameerez wants to merge 2 commits into
Open
Conversation
usage_credits 1.0 builds on these hooks; writing them down makes the contract explicit so future wallets releases treat changes to it as breaking. Also note the association re-declaration requirement and add dynamic association class resolution to the TODO list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugs fixed (each verified with a reproduction before fixing): - Engine no longer adds gem lib dirs to host-app autoload paths, which made Zeitwerk claim top-level ::Wallet/::Transaction/::Transfer/ ::Allocation and shadow (break) host apps defining models with those names. - Transfer#transactions is now dependent: :nullify - destroying a wallet/owner with transfer history no longer raises ActiveRecord::InvalidForeignKey; the counterparty's ledger and balance survive, and metadata keeps transfer_id for audit. - create_for_owner! race recovery works on PostgreSQL: the duplicate insert now goes through a savepoint (requires_new: true) with the rescue outside the transaction, so a lost race no longer poisons the caller's transaction (PG::InFailedSqlTransaction). - All required belongs_to declare optional: false explicitly - the gem loads before Rails applies belongs_to_required_by_default, so orphan records were passing validation and dying on DB NOT NULL constraints. - has_wallets options are resolved lazily and inherited by subclasses (STI); config default_asset changes are honored regardless of load order. - Callbacks.dispatch ignores unknown events instead of raising NoMethodError mid-transaction. - transfer_to raises friendly InvalidTransfer for unpersisted source wallets; amount validation no longer leaks FloatDomainError/ NoMethodError; String expires_at parses instead of crashing; unsaved owner errors are Wallets::Error; expired/not_expired scopes partition cleanly at the boundary; history gets an id tiebreaker; transfers use self.class.transaction; Transfer validates category presence. - Removed the redundant no-op Wallets::Railtie; fixed the initializer template's category docs; synced Appraisals with gemfiles/ and CI. DRY/SSOT: - New Wallets::Embeddable concern (table-name/config plumbing, was 4 copies) and Wallets::HasMetadata concern (indifferent-access metadata, was 3 copies); Wallets.normalize_asset_code (was 5 inline copies); Wallet/Transfer reuse Transaction scopes instead of hand-written SQL predicates. Embeddability contract unchanged. Tests: 92 runs / 338 assertions -> 186 runs / 667 assertions. Coverage: 93.45% -> 100% line, 65.93% -> 99.39% branch; SimpleCov gate raised to 98/85. The install generator now runs in tests and its generated migration executes up and down against the real adapter. Verified on SQLite, PostgreSQL, and Rails 7.2/8.1 appraisals. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A full-codebase hardening pass: four serious bugs fixed (each verified with a reproduction before fixing), the embeddability/metadata plumbing consolidated into shared concerns, and the test suite doubled to 100% line / 99.39% branch coverage.
Bugs fixed
1. Installing the gem broke host apps with a
Transaction/Wallet/Transfer/AllocationmodelThe engine added the gem's
libdirs to the host app's autoload paths, so Zeitwerk claimed those top-level constants and shadowed the app's own models (NameError: uninitialized constant Transaction— reproduced in the dummy app). All gem code is eagerly required, so the paths were pure liability. Removed, with a regression test asserting no gem dir is ever autoloaded.2. Owners with transfer history couldn't be destroyed
Destroying a wallet destroyed its transfers while the counterparty's ledger row still referenced them →
ActiveRecord::InvalidForeignKey(reproduced).Transfer#transactionsis nowdependent: :nullify: the link object goes away, both sides' ledger rows and balances survive, and transaction metadata still carriestransfer_id+ counterparty details for audit.3.
create_for_owner!race recovery failed on PostgreSQLThe duplicate-insert rescue ran inside the same transaction as the failed
INSERT; on PG a unique-index violation aborts that transaction, so the recoverySELECTraisedPG::InFailedSqlTransaction— precisely in the concurrent-creation scenario the rescue exists for. The create now writes through a savepoint (requires_new: true) and rescues outside it, keeping the caller's surrounding transaction (e.g.after_createauto-creation) usable after a lost race. Pinned by a test that performs a real duplicate insert inside a caller's transaction, exercising true PG savepoint semantics in CI.4. All
belongs_toassociations were silently optionalThe gem's models load before Rails applies
belongs_to_required_by_default, so orphanTransaction/Allocation/Transferrecords passed validation and died later on DBNOT NULLconstraints. Every required association now declaresoptional: falseexplicitly. (Deliberately not centralized viabelongs_to_required_by_defaulton the abstract base: embedder subclasses redeclare associations and must keep following their own app's config.)Smaller fixes
has_walletsoptions now inherited by STI subclasses and resolved lazily againstWallets.configurationCallbacks.dispatchignores unknown events instead of raisingNoMethodErrormid-transactiontransfer_toon an unpersisted source raises friendlyInvalidTransferFloat::INFINITY/NaN/Symbolamounts raiseArgumentError(previouslyFloatDomainError/NoMethodErrorleaked);has_enough_balance?returnsfalseinstead of crashingexpires_atvalues parse ("2030-01-01"works; garbage raises a clear error)Wallets::Error(one rescuable hierarchy)expired/not_expiredscopes partition cleanly at the exact boundary instanthistoryordered withidtiebreaker; transfers useself.class.transaction(multi-DB embedders);TransfervalidatescategorypresenceWallets::Railtie; fixed initializer template's category docs; syncedAppraisalswithgemfiles/+ CI (7.2 / 8.1)DRY / SSOT
Wallets::Embeddable— table-name/config plumbing extracted from 4 identical copies; embedded subclasses withoutembedded_table_namederive"#{prefix}#{table_suffix}"Wallets::HasMetadata— indifferent-access metadata behavior extracted from 3 identical copies (incl. MySQL NULL-column healing)Wallets.normalize_asset_code— single source of truth (was 5 inline copies)Transactionscopes (credits/debits/not_expired) instead of hand-written SQL predicatesembedded_table_name,config_provider,callbacks_module,*_class_name,callback_event_map) is fully preserved — embeddability suite passes unchangedTests
New coverage includes: regression tests for every fix above, a real duplicate-insert race inside a caller's transaction, destroy cascades with transfer history, FIFO expiring-first allocation order, expiration boundary partitioning, amount/expiration edge cases, callback logging fallbacks, STI wallet-option inheritance, opposing-transfer deadlock test, and the install generator now runs in tests with its generated migration executed up and down against the real adapter.
A max-effort multi-angle review (5 correctness + 3 cleanup + 1 altitude finder, adversarial verification, gap sweep) ran over this diff; zero correctness findings survived, and the two confirmed cleanup findings are applied.
Verified: SQLite (multiple seeds), PostgreSQL (full suite locally), Rails 7.2 + 8.1 appraisals — all 186 green everywhere.
🤖 Generated with Claude Code