From a6bf221b2f62a50fcc558925f9684fe7284e6fb3 Mon Sep 17 00:00:00 2001 From: Javi R <4920956+rameerez@users.noreply.github.com> Date: Wed, 18 Mar 2026 02:37:48 +0000 Subject: [PATCH] Make create_for_owner! idempotent and race-safe - Return existing wallet if one already exists for owner + asset_code - Handle race conditions with rescue for RecordNotUnique/RecordInvalid - Normalize asset_code before lookup/create - Extract initial_balance_credit_attributes for subclass overriding - Add test for idempotent behavior Gemspec cleanup: - Use git ls-files for cleaner file list - Exclude dev directories (.claude, .cursor, .github, test, etc.) - Add Rails version ceiling (< 9.0) Co-Authored-By: Claude Opus 4.6 --- lib/wallets/models/wallet.rb | 39 +++++++++++++++++++++++++----- test/models/wallets/wallet_test.rb | 25 +++++++++++++++++++ wallets.gemspec | 33 +++++++++++++++++++------ 3 files changed, 84 insertions(+), 13 deletions(-) diff --git a/lib/wallets/models/wallet.rb b/lib/wallets/models/wallet.rb index 99481fa..b4f2cee 100644 --- a/lib/wallets/models/wallet.rb +++ b/lib/wallets/models/wallet.rb @@ -88,8 +88,12 @@ def self.transfer_class class << self def create_for_owner!(owner:, asset_code:, initial_balance: 0, metadata: {}) initial_balance = normalize_initial_balance(initial_balance) + asset_code = normalize_asset_code(asset_code) metadata = metadata.respond_to?(:to_h) ? metadata.to_h : {} + existing_wallet = find_by(owner: owner, asset_code: asset_code) + return existing_wallet if existing_wallet.present? + transaction do wallet = create!( owner: owner, @@ -98,20 +102,32 @@ def create_for_owner!(owner:, asset_code:, initial_balance: 0, metadata: {}) metadata: metadata ) - if initial_balance.to_i.positive? - wallet.credit( - initial_balance, - category: :adjustment, - metadata: { reason: "initial_balance" } - ) + if initial_balance.positive? + wallet.credit(initial_balance, **initial_balance_credit_attributes) end wallet + rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid => error + wallet = find_by(owner: owner, asset_code: asset_code) + raise error if wallet.nil? + + if record_conflict_due_to_existing_wallet?(error) + wallet + else + raise error + end end end private + def initial_balance_credit_attributes + { + category: :adjustment, + metadata: { reason: "initial_balance" } + } + end + def normalize_initial_balance(value) return 0 if value.nil? raise ArgumentError, "Initial balance must be a whole number" unless value == value.to_i @@ -121,6 +137,17 @@ def normalize_initial_balance(value) value end + + def normalize_asset_code(value) + value.to_s.strip.downcase.presence || raise(ArgumentError, "Asset code is required") + end + + def record_conflict_due_to_existing_wallet?(error) + return true if error.is_a?(ActiveRecord::RecordNotUnique) + return false unless error.is_a?(ActiveRecord::RecordInvalid) + + error.record.errors.of_kind?(:asset_code, :taken) + end end # ========================================= diff --git a/test/models/wallets/wallet_test.rb b/test/models/wallets/wallet_test.rb index 83c82df..54fe7b5 100644 --- a/test/models/wallets/wallet_test.rb +++ b/test/models/wallets/wallet_test.rb @@ -62,6 +62,31 @@ class Wallets::WalletTest < ActiveSupport::TestCase end end + test "create_for_owner is idempotent for an existing owner and asset" do + owner = users(:new_user) + + wallet = Wallets::Wallet.create_for_owner!( + owner: owner, + asset_code: :ore, + initial_balance: 25 + ) + + assert_no_difference -> { Wallets::Wallet.where(owner: owner, asset_code: "ore").count } do + same_wallet = Wallets::Wallet.create_for_owner!( + owner: owner, + asset_code: " ORE ", + initial_balance: 75 + ) + + assert_equal wallet.id, same_wallet.id + end + + assert_equal 25, wallet.reload.balance + assert_equal 1, wallet.transactions.count + assert_equal "adjustment", wallet.transactions.sole.category + assert_equal "initial_balance", wallet.transactions.sole.metadata["reason"] + end + test "negative balances are tracked correctly when enabled" do original_setting = Wallets.configuration.allow_negative_balance Wallets.configuration.allow_negative_balance = true diff --git a/wallets.gemspec b/wallets.gemspec index 20fb6ed..32aa01a 100644 --- a/wallets.gemspec +++ b/wallets.gemspec @@ -16,15 +16,34 @@ Gem::Specification.new do |spec| spec.metadata["allowed_push_host"] = "https://rubygems.org" spec.metadata["homepage_uri"] = spec.homepage - spec.metadata["source_code_uri"] = spec.homepage + spec.metadata["source_code_uri"] = "#{spec.homepage}/tree/main" spec.metadata["changelog_uri"] = "#{spec.homepage}/blob/main/CHANGELOG.md" spec.metadata["rubygems_mfa_required"] = "true" - spec.files = Dir.chdir(__dir__) do - Dir.glob("**/*", File::FNM_DOTMATCH).reject do |file| - file.start_with?(".git/", "coverage/", "dist/", "test/dummy/log/", "test/dummy/tmp/", "test/dummy/storage/") || - file.end_with?(".gem") || - [".", "..", ".DS_Store", "Gemfile.lock", "test/.DS_Store"].include?(file) + gemspec = File.basename(__FILE__) + spec.files = IO.popen(%w[git ls-files -z], chdir: __dir__, err: IO::NULL) do |ls| + ls.readlines("\x0", chomp: true).reject do |file| + (file == gemspec) || + file.start_with?(*%w[ + .aux/ + .claude/ + .cursor/ + .github/ + bin/ + coverage/ + dist/ + gemfiles/ + spec/ + test/ + tmp/ + ]) || + %w[ + .DS_Store + .ruby-version + Appraisals + Gemfile + Gemfile.lock + ].include?(file) end end @@ -32,5 +51,5 @@ Gem::Specification.new do |spec| spec.executables = spec.files.grep(%r{\Aexe/}) { |file| File.basename(file) } spec.require_paths = ["lib"] - spec.add_dependency "rails", ">= 6.1" + spec.add_dependency "rails", ">= 6.1", "< 9.0" end