fix(binance): value past days from Binance's own snapshot total (#763)
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=<id>` already exists — use it: ``` php artisan banking:sync --full --connection=<coinbase-connection-id> ``` 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.
This commit is contained in:
Víctor Falcón
GitHub
parent
9e493dc75b
commit
e6cb0459f6
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue