From e6cb0459f66b5d8a255be0edbf4e9f0045598e83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Tue, 11 Aug 2026 13:04:06 +0200 Subject: [PATCH] fix(binance): value past days from Binance's own snapshot total (#763) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Last of the crypto-pricing family, after #759 and #761. The issues queue has produced nothing new for four cycles, so this came out of reading the code the logs pointed at. ## What was wrong `syncHistoricalBalances` rebuilds past daily rows in `account_balances` — the data behind the net-worth chart. It priced each holding by handing its raw ticker to `CurrencyConversionService`: ```php $converted = $this->currencyConverter->convert($asset, $targetCurrency, $quantity, $date); if ($converted == 0.0) { $skippedAssets[$asset] = true; // and the holding leaves the day's total continue; } ``` That provider carries fiat currencies and a couple of majors. Everything it did not recognise was dropped from the day's total, so an altcoin portfolio produced a chart history far below what the user actually held — while **today's** balance was correct, because the live path prices through Binance's own tickers. The new test demonstrates it: a day holding 10 SOL comes out as **0** on the old code. I first went looking for the USD hop that fixed the Coinbase equivalent, and there isn't one to add here — a past day needs a *dated* price, and both the asset leg and any USD leg would come from the same provider that does not know the asset. ## What it does instead The snapshot already carried the answer and the service was throwing it away. Binance sends `totalAssetOfBtc` — what it valued the whole spot account at, on that day — in the same `data` object as the balances. One BTC→fiat conversion, on the one ticker the rate provider always covers, replaces the per-asset loop and covers every holding including the ones nothing else can price. Both are spot-only (`accountSnapshot?type=SPOT` and `api/v3/account`), so historical and current days stay on the same basis — no discontinuity at the join. Second commit is a review follow-up: the day's balance now rests entirely on that one field, so a response that stops carrying it gets its own counter in the summary log rather than looking like a day with no snapshot. ## ⚠️ Correction to the ops advice I gave in #759 and #761 I twice suggested `php artisan banking:sync --full` to repair the understated Coinbase rows. **Do not run that unscoped.** With no filter it forces `isFirstSync = true` for *every* active connection, which would also rewrite up to 180 days of history for all 11 Binance accounts in one shot as a side effect. `--connection=` already exists — use it: ``` php artisan banking:sync --full --connection= ``` For Binance the same command is the repair, but it should be a deliberate decision per connection, not collateral damage. **Nothing repairs the existing 11 accounts automatically**: incremental sync only fills the gap after `MAX(balance_date)` and never revisits older rows, so their charts stay understated until someone asks for it. ## Deliberate behaviour changes, called out - **An empty day now charts as zero.** Previously a snapshot with no balances was skipped, leaving a gap that the frontend forward-fills — so a day the account was actually empty showed the *previous* non-zero value. It now writes 0. More correct, but it is a change beyond the stated fix. - **A day whose BTC value is positive but unconvertible is left alone** rather than written as zero, so it disappears into the frontend's forward-fill instead of reading as a portfolio that briefly vanished. ## Tests `historical sync values a holding the rate provider cannot price` — a day holding SOL with deliberately no `sol` rate available. **Verified to fail on the old implementation: 0 instead of 100000.** The three touched historical fixtures gained `totalAssetOfBtc` and keep their original expected cents, recomputed on the new basis (2.0 BTC ÷ 0.000019 = 10526316; 1.0 ÷ 0.000018 = 5555556; 0.02 ÷ 0.00002 = 100000). `tests/Feature/OpenBanking` green at 341/341 locally. ## Not covered Coinbase's historical path already prices via dated per-asset candles with a USD route, so it is not exposed to this at the same scale, and Coinbase exposes no portfolio-level BTC equivalent to substitute. Bitpanda has no historical sync at all. --- .../Banking/BinanceBalanceSyncService.php | 56 +++++++++++-------- .../OpenBanking/BinanceBalanceSyncTest.php | 13 ++++- 2 files changed, 44 insertions(+), 25 deletions(-) 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); });