diff --git a/app/Services/Banking/BinanceBalanceSyncService.php b/app/Services/Banking/BinanceBalanceSyncService.php index 314d9571..b16e0e9a 100644 --- a/app/Services/Banking/BinanceBalanceSyncService.php +++ b/app/Services/Banking/BinanceBalanceSyncService.php @@ -95,7 +95,18 @@ class BinanceBalanceSyncService } /** - * Fetch historical snapshots and convert each day's balances using the currency conversion API. + * Rebuild past daily balances from Binance's own daily snapshots. + * + * Each snapshot carries `totalAssetOfBtc`: what Binance valued the whole + * spot account at, on that day. One BTC->fiat conversion turns it into the + * user's currency, and BTC is the one ticker the rate provider always + * covers. + * + * The alternative - re-pricing each holding ourselves - needs a dated price + * per asset, and the only dated source here is a fiat rate provider that + * knows almost no altcoins. Anything it could not price was dropped from + * the day's total, so a portfolio of tokens it does not carry produced a + * chart history far below what the user actually held. * * @return bool Whether any API calls were made */ @@ -121,36 +132,36 @@ class BinanceBalanceSyncService } $count = 0; - $skippedAssets = []; + $unpricedDays = 0; + $daysWithoutValuation = 0; foreach ($snapshots as $snapshot) { $updateTime = $snapshot['updateTime'] ?? null; - $balances = $snapshot['data']['balances'] ?? []; + $btcValue = $snapshot['data']['totalAssetOfBtc'] ?? null; + + if ($updateTime === null) { + continue; + } + + // The whole valuation now rests on this one field, so a Binance + // response that stops carrying it must be visible rather than look + // like a day they simply had no snapshot for. + if ($btcValue === null) { + $daysWithoutValuation++; - if ($updateTime === null || empty($balances)) { continue; } $date = Carbon::createFromTimestampMs($updateTime)->toDateString(); - $totalValue = 0.0; + $btcValue = (float) $btcValue; + $totalValue = $this->currencyConverter->convert('BTC', $targetCurrency, $btcValue, $date); - foreach ($balances as $balance) { - $asset = $balance['asset']; - $quantity = (float) ($balance['free'] ?? 0) + (float) ($balance['locked'] ?? 0); + // A held-but-unpriceable day would land as a zero balance and read as + // a portfolio that briefly vanished, so leave the day alone instead. + if ($btcValue > 0 && $totalValue <= 0) { + $unpricedDays++; - if ($quantity <= 0 || isset($skippedAssets[$asset])) { - continue; - } - - $converted = $this->currencyConverter->convert($asset, $targetCurrency, $quantity, $date); - - if ($converted == 0.0) { - $skippedAssets[$asset] = true; - - continue; - } - - $totalValue += $converted; + continue; } $account->balances()->updateOrCreate( @@ -165,7 +176,8 @@ class BinanceBalanceSyncService 'account_id' => $account->id, 'days_synced' => $count, 'currency' => $targetCurrency, - ...($skippedAssets ? ['skipped_assets' => array_keys($skippedAssets)] : []), + ...($unpricedDays ? ['unpriced_days' => $unpricedDays] : []), + ...($daysWithoutValuation ? ['days_without_valuation' => $daysWithoutValuation] : []), ]); return true; diff --git a/tests/Feature/OpenBanking/BinanceBalanceSyncTest.php b/tests/Feature/OpenBanking/BinanceBalanceSyncTest.php index 79d70aca..9e8b0d05 100644 --- a/tests/Feature/OpenBanking/BinanceBalanceSyncTest.php +++ b/tests/Feature/OpenBanking/BinanceBalanceSyncTest.php @@ -309,6 +309,7 @@ test('first sync fetches historical snapshots and converts using currency API', 'balances' => [ ['asset' => 'BTC', 'free' => '2.0', 'locked' => '0.0'], ], + 'totalAssetOfBtc' => '2.0', ], ], [ @@ -318,6 +319,7 @@ test('first sync fetches historical snapshots and converts using currency API', 'balances' => [ ['asset' => 'BTC', 'free' => '2.0', 'locked' => '0.0'], ], + 'totalAssetOfBtc' => '2.0', ], ], ], @@ -389,6 +391,7 @@ test('subsequent sync only fetches snapshots since last balance date', function 'balances' => [ ['asset' => 'BTC', 'free' => '1.0', 'locked' => '0.0'], ], + 'totalAssetOfBtc' => '1.0', ], ], ], @@ -425,7 +428,7 @@ test('subsequent sync only fetches snapshots since last balance date', function expect($todayBalance->balance)->toBe(5600000); }); -test('historical sync converts assets using currency API', function () { +test('historical sync values a holding the rate provider cannot price', function () { $user = User::factory()->onboarded()->create(['currency_code' => 'EUR']); $connection = BankingConnection::factory()->binance()->create([ 'user_id' => $user->id, @@ -451,13 +454,16 @@ test('historical sync converts assets using currency API', function () { 'balances' => [ ['asset' => 'SOL', 'free' => '10.0', 'locked' => '0.0'], ], + 'totalAssetOfBtc' => '0.02', ], ], ], ]), + // Deliberately no `sol` rate: the provider carries fiat and a couple of + // majors, which is why re-pricing each holding dropped everything else. 'cdn.jsdelivr.net/*currencies/eur*' => Http::response([ 'eur' => [ - 'sol' => 0.01, // 1 EUR = 0.01 SOL → 1 SOL = 100 EUR + 'btc' => 0.00002, // 1 EUR = 0.00002 BTC → 0.02 BTC = 1000 EUR ], ]), 'api.binance.com/api/v3/account*' => Http::response([ @@ -475,7 +481,8 @@ test('historical sync converts assets using currency API', function () { $service = app(BinanceBalanceSyncService::class); $service->sync($account, $client, isFirstSync: true); - // Historical: 10 SOL / 0.01 = 1000 EUR → 100000 cents + // Binance valued the whole account at 0.02 BTC that day → 1000 EUR. + // Re-pricing the SOL ourselves would have found no rate and written zero. $yesterdayBalance = $account->balances()->where('balance_date', $yesterday->toDateString())->first(); expect($yesterdayBalance->balance)->toBe(100000); });