Skip to content

fix(custody): give the Safe balances a stable order - #4428

Merged
TaprootFreak merged 3 commits into
developfrom
fix/custody-balance-stable-order
Jul 29, 2026
Merged

fix(custody): give the Safe balances a stable order#4428
TaprootFreak merged 3 commits into
developfrom
fix/custody-balance-stable-order

Conversation

@TaprootFreak

@TaprootFreak TaprootFreak commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Found while reviewing baseline screenshots in DFXswiss/services#1197: two runs against an unchanged dataset produced the same holdings in a different order.

The cause

getUserCustodyBalance loads the balances with findBy(...) and no ordering, and the mapper preserves whatever came back. Without an ORDER BY, Postgres is free to return rows in any order it likes — and does, once a table has seen updates.

What that looks like

A customer opening the Safe twice can see their positions swap places for no reason they can perceive. Nothing is wrong with the figures; the list simply reshuffles. It also makes any screenshot of the Safe unreproducible, which is how this surfaced.

The order

Largest position first, by CHF value, with the asset name breaking ties so the result is fully determined. That is stable, and it is the order someone reading a portfolio expects — the biggest holding at the top.

Sorting happens in the mapper rather than the query because the fiat value only exists after conversion.

A damaged value

NaN makes every difference falsy, so a broken figure fell through to the name comparison — against every other entry, whatever its worth. The ordering lost its transitivity and the position depended on the input order again: the very unpredictability this sort removes, only triggered by a bad figure instead of a missing ORDER BY.

Non-finite values are therefore ranked last rather than compared. They are not thrown, for the reason already recorded a few lines up in the caller: one damaged position must not take the customer's whole balance response down with it.

Tests

Five cases in the existing mapper suite, each feeding the input in an order that would let a naive pass-through slip by:

  • descending value, given ascending input
  • equal values fall back to the name, given reverse-alphabetical input
  • the same input in two different orders yields the same output — the guarantee this exists for
  • a negative value sorts behind every positive one; there is a negative balance row in production, so that is not hypothetical
  • a non-finite value lands last no matter which order it arrived in, and so does a real infinity

The ordering case earns its name: with three values it held for the broken comparator too, so it would have watched the defect return in silence. Four values with a negative among them separate the two, verified by putting the old line back and watching both tests go red.

npx jest src/subdomains/core/custody --silent
  Test Suites: 6 passed, 6 total
  Tests:       6 skipped, 79 passed, 85 total

npm run type-check      → clean
npx prettier --check    → clean

The balances came back in whatever order the database chose — nothing orders
that query. The same holdings could therefore appear in a different order on
every request, with the list visibly reshuffling for no reason a customer
could see. It also made screenshots of the Safe unreproducible.

Largest position first, name as tiebreak. That is both stable and the order
someone reading a portfolio expects.
NaN makes every difference falsy, so a damaged value fell through to the name
comparison — against every other entry, whatever its worth. The ordering lost
its transitivity and the position depended on the input order again: exactly
the unpredictability this sort was added to remove, only triggered by a broken
figure instead of a missing ORDER BY.

Reproduced before and after: the same four positions, fed forwards and
backwards, now come out identical.

It is ranked rather than thrown for the reason already recorded a few lines
up in the caller — one damaged position must not take the customer's whole
balance response down with it.

Tests cover the negative case too. There is a negative balance row in
production, so that is not hypothetical.
Its point is that the same holdings come back in the same order however they
went in — but with three values that held for the broken comparator too, so
the assertion would have watched the defect return without a word. Only the
position checks were doing any work.

Four values with a negative among them separate the two: the old comparator
yields different orders forwards and backwards, the new one does not. Verified
by putting the old line back and watching both tests go red.

Also covers a real Infinity, which the ranking treats like NaN, and says in
the comment why: not because infinities break the comparison, but because
nothing legitimate produces one here — the same reading of corrupted data the
interest calculation already applies.
@TaprootFreak

Copy link
Copy Markdown
Collaborator Author

Four review passes to zero findings.

The fix itself is small, but it grew a second half along the way. Ordering by value alone still left one case undecided: NaN makes every difference falsy, so a damaged figure fell through to the name comparison — against every other entry, whatever its worth. The ordering lost its transitivity and depended on input order again, which is exactly what this change exists to remove. Non-finite values are ranked last now, not thrown, for the reason already recorded a few lines up in the caller: one damaged position must not take the customer's whole balance response down with it.

The most useful finding was about a test rather than the code. The case named "returns the same order regardless of input order" held for the broken comparator too, given three values — it would have watched the defect return without a word, and only the position checks were doing any work. Four values with a negative among them separate the two. Verified the way such a claim has to be: by putting the old line back and watching both tests go red, then putting it back.

@TaprootFreak
TaprootFreak marked this pull request as ready for review July 28, 2026 12:10
@TaprootFreak
TaprootFreak merged commit 677e523 into develop Jul 29, 2026
12 checks passed
@TaprootFreak
TaprootFreak deleted the fix/custody-balance-stable-order branch July 29, 2026 09:23
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.

1 participant