Skip to content

fix(asset): write decimals through the service that owns the cache - #4584

Merged
TaprootFreak merged 4 commits into
developfrom
fix/asset-decimals-cache
Aug 1, 2026
Merged

fix(asset): write decimals through the service that owns the cache#4584
TaprootFreak merged 4 commits into
developfrom
fix/asset-decimals-cache

Conversation

@TaprootFreak

@TaprootFreak TaprootFreak commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator

Closes #4578.

The defect

EvmDecimalsService.setDecimals() wrote through repoFactory.asset, the instance RepositoryFactory constructs itself. Every reader goes through AssetService, which holds a different instance of the same repository class. CachedRepository.invalidateCache() clears only the instance it is called on, so nothing told the readers' cache that decimals had just been filled — it kept serving the previous rows until the entry expired.

The impact was small: the job only fills rows that are still NULL, so it delayed a repair rather than serving a wrong value. The exposure belongs to the pattern, not to this field.

The fix

The write goes through AssetService, which owns the instance the cached reads are served from. AssetService.updatePrices did precisely that already and had exactly one caller, so instead of copying its body it is renamed to updateAssets and used by both jobs. Reading moves behind the service as well (getEvmAssetsWithoutDecimals), so RepositoryFactory.asset is left without a single caller.

updateAssets then took three corrections during review, each one closing a hole the previous version had left:

  1. Invalidation in finally. The first version invalidated after the loop, so a failing update skipped it and left the rows written before it out of the cache — the very staleness this change removes.
  2. Every update is attempted. Batching had silently given up the per-asset isolation the original loop had: one row that keeps failing would have blocked every asset queued behind it, on every run. Failures are collected instead of aborting.
  3. The original errors survive. Collecting failures first reduced them to text, dropping type, stack and every cause after the first. It now throws an AggregateError carrying all of them and naming the affected ids.

On (3) a dedicated error class exposing failedIds was considered and rejected: neither caller inspects the error, both hand it to the cron, so it would be structure without a consumer.

The empty case is guarded inside the service rather than at the call site, so it holds for every caller. That also changes the five-minute price job, which previously passed an empty list and dropped the whole asset cache for nothing.

Tests

asset.service.update.spec.ts (new)

  • every update written, cache invalidated once
  • cache invalidated even when an update fails, so earlier writes are not left stale
  • every update attempted when one in the middle fails, and the last one still reached
  • the original errors are carried, asserted by identity rather than by message text
  • nothing written and cache untouched for an empty list

evm-decimals.service.spec.ts (new)

  • the write goes through the service that owns the cache
  • several assets collected into a single write
  • a failing token lookup does not take the other assets with it
  • nothing collected when no asset is missing its decimals, and none when every lookup fails

Reverting the write turns three of the decimals tests red.

Not covered here

The pattern itself — a repository existing twice, in DI and in RepositoryFactory, with independent caches — stays as it is; nothing enforces the service boundary, so a future direct repository write could recreate this. Issue #4578 records the structural options.

Checks

npm run type-check, npm run lint and prettier --check clean; the new specs plus the asset.service and asset-prices-job suites green (13 tests).

The hourly decimals job wrote through the repository instance built by
RepositoryFactory, while every reader goes through AssetService and its own
instance. invalidateCache() only clears the instance it is called on, so the
freshly written decimals stayed invisible to readers until the entry expired.

The write now goes through AssetService, which invalidates the cache the readers
are served from. updatePrices did exactly that already and was the only caller
of its kind, so it is renamed to updateAssets and shared instead of copied.

The job also collects its updates and writes once: it runs hourly and usually
finds nothing, so invalidating unconditionally would drop the whole asset cache
every hour for no reason.
Batching the writes had given up the failure isolation the per-asset loop used
to have: if one update threw, invalidateCache() was never reached and the rows
written before it stayed out of the cache — the exact staleness this change
exists to remove.

The invalidation now sits in a finally block, so it also runs when an update
fails. The empty case is guarded in the service instead of at the call site,
which makes it hold for every caller: the price job passed an empty list every
five minutes and dropped the whole asset cache for nothing.
…lure

The finally block covered the cache but not the loop: the first failing update
still aborted the batch, so every asset queued behind it was skipped. Before
this change each asset was written on its own, and a row that keeps failing was
harmless — as a batch it would block all following assets on every run.

Every update is attempted now, failures are collected, the cache is invalidated
either way, and the collected ids are reported afterwards.
Collecting the failures had reduced them to text: the caller lost the type,
the stack and the code of the database error, and every cause after the first
was dropped. Whoever reads the log needs the error itself.

AggregateError carries all of them and names the affected ids in its message.
No dedicated error class with a failedIds field: neither caller inspects the
error, both hand it to the cron, so that would be structure without a consumer.
@TaprootFreak

Copy link
Copy Markdown
Collaborator Author

Four review passes, three defects fixed. Each one was a hole left by the previous version of the same method, so they are worth naming:

  1. The invalidation sat after the loop, so a failing update skipped it and left the rows written before it out of the cache — the exact staleness this PR removes.
  2. Batching the writes had silently dropped the per-asset failure isolation the original loop had. One row that keeps failing would have blocked every asset queued behind it, on every run.
  3. Collecting the failures reduced them to text, losing the type, the stack and every cause after the first.

The fourth pass found nothing.

One recommendation was deliberately not followed: a dedicated error type exposing failedIds. Neither caller inspects the error — both hand it to the cron — so it would have been structure without a consumer. AggregateError keeps the causes, which is the part that matters for whoever reads the log. Happy to be overruled on that.

CI green: build and checks, all three test shards, coverage, and the coverage ratchet.

@TaprootFreak
TaprootFreak marked this pull request as ready for review August 1, 2026 17:00
@TaprootFreak
TaprootFreak merged commit f4d0ebc into develop Aug 1, 2026
18 checks passed
@TaprootFreak
TaprootFreak deleted the fix/asset-decimals-cache branch August 1, 2026 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Writes through RepositoryFactory do not invalidate the DI instance's cache

1 participant