From 4e891a147e3a743bb78fb81c2949023f7aae67e7 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:23:04 +0200 Subject: [PATCH 1/2] feat(custody): say outright whether the caller owns an account (#4424) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(custody): say outright whether the caller owns an account Clients had to infer ownership from the absence of the owner field, which is not a contract but a side effect: own accounts are loaded without the owner relation, foreign ones with it. Add the relation somewhere and the inference flips silently. It is not a cosmetic distinction. A client decides on it whether to offer transacting at all: orders carry no account, so an order placed while looking at someone else's Safe would be booked against the caller's own. Acting on another person's behalf does not exist here — initiatedBy is never set. isOwner states it. Creating an account yields true by definition; the update route answers it from the account, since a grantee passes the write guard too. * refactor(custody): ask the account who owns it The ownership comparison lived in seven places, one of them the controller — where the contributing guide says business logic does not belong. It is one method on the entity now, and the controller asks it. That also makes it testable, which the update route needed: it is the reason the flag exists, since a grantee passes the write guard without owning anything, and until now nothing exercised that. The entity spec covers it, including that an active grant does not make a grantee an owner. * refactor(custody): finish what the last commit claimed to have finished Three ownership comparisons were left behind: the multi-account shortcut in resolveOwnerAccountId, and the two grant guards. A textual search for the owner field misses the first, which is how the previous commit undercounted and then overclaimed. None of them behaved wrongly — they computed the same boolean — but three private copies of one predicate are three chances to drift. No raw comparison remains outside the entity. --- .../controllers/custody-account.controller.ts | 11 ++- .../custody/dto/output/custody-account.dto.ts | 3 + .../__tests__/custody-account.entity.spec.ts | 78 +++++++++++++++++++ .../entities/custody-account.entity.ts | 9 +++ .../mappers/custody-account-dto.mapper.ts | 4 +- .../__tests__/custody-account.service.spec.ts | 43 ++++++++++ .../services/custody-account.service.ts | 18 ++--- 7 files changed, 154 insertions(+), 12 deletions(-) create mode 100644 src/subdomains/core/custody/entities/__tests__/custody-account.entity.spec.ts diff --git a/src/subdomains/core/custody/controllers/custody-account.controller.ts b/src/subdomains/core/custody/controllers/custody-account.controller.ts index a8af59d97a..73723cddef 100644 --- a/src/subdomains/core/custody/controllers/custody-account.controller.ts +++ b/src/subdomains/core/custody/controllers/custody-account.controller.ts @@ -88,7 +88,8 @@ export class CustodyAccountController { dto.description, ); - return CustodyAccountDtoMapper.toDto(custodyAccount, CustodyAccessLevel.WRITE); + // The caller just created it, so it is theirs by definition. + return CustodyAccountDtoMapper.toDto(custodyAccount, CustodyAccessLevel.WRITE, true); } @Put(':id') @@ -107,7 +108,13 @@ export class CustodyAccountController { dto.description, ); - return CustodyAccountDtoMapper.toDto(custodyAccount, CustodyAccessLevel.WRITE); + // Reached through the write guard, which a grantee passes too — ownership is a separate + // question from the level and has to be answered from the account itself. + return CustodyAccountDtoMapper.toDto( + custodyAccount, + CustodyAccessLevel.WRITE, + custodyAccount.isOwnedBy(jwt.account), + ); } @Get(':id/balance') diff --git a/src/subdomains/core/custody/dto/output/custody-account.dto.ts b/src/subdomains/core/custody/dto/output/custody-account.dto.ts index 5f35f0dc31..87aebd0e15 100644 --- a/src/subdomains/core/custody/dto/output/custody-account.dto.ts +++ b/src/subdomains/core/custody/dto/output/custody-account.dto.ts @@ -22,6 +22,9 @@ export class CustodyAccountDto { @ApiProperty({ enum: CustodyAccessLevel, description: 'Access level for current user' }) accessLevel: CustodyAccessLevel; + @ApiProperty({ description: 'Whether the current user owns this account rather than being granted access to it' }) + isOwner: boolean; + @ApiPropertyOptional({ type: CustodyUserDto }) owner?: CustodyUserDto; } diff --git a/src/subdomains/core/custody/entities/__tests__/custody-account.entity.spec.ts b/src/subdomains/core/custody/entities/__tests__/custody-account.entity.spec.ts new file mode 100644 index 0000000000..b8c1fccdc1 --- /dev/null +++ b/src/subdomains/core/custody/entities/__tests__/custody-account.entity.spec.ts @@ -0,0 +1,78 @@ +import { UserData } from 'src/subdomains/generic/user/models/user-data/user-data.entity'; +import { CustodyAccessLevel, CustodyAccountStatus } from '../../enums/custody'; +import { CustodyAccountAccess } from '../custody-account-access.entity'; +import { CustodyAccount } from '../custody-account.entity'; + +describe('CustodyAccount', () => { + const ownerId = 100; + const granteeId = 200; + const strangerId = 300; + const custodyAccountId = 1; + + function userData(overrides: Partial = {}): UserData { + return Object.assign(new UserData(), { id: ownerId, users: [], custodyAccounts: [], ...overrides }); + } + + function custodyAccount(overrides: Partial = {}): CustodyAccount { + return Object.assign(new CustodyAccount(), { + id: custodyAccountId, + title: 'Own Safe', + description: 'Owner account', + owner: userData(), + requiredSignatures: 1, + status: CustodyAccountStatus.ACTIVE, + accessGrants: [], + ...overrides, + }); + } + + function accessGrant(params: { + id?: number; + account: CustodyAccount; + userData: UserData; + accessLevel: CustodyAccessLevel; + active: boolean; + }): CustodyAccountAccess { + return Object.assign(new CustodyAccountAccess(), { + id: params.id ?? 10, + account: params.account, + userData: params.userData, + accessLevel: params.accessLevel, + active: params.active, + }); + } + + describe('#isOwnedBy(...)', () => { + it("returns true for the owner's user-data id", () => { + const account = custodyAccount(); + + expect(account.isOwnedBy(ownerId)).toBe(true); + }); + + it('returns false for a grantee with an active write grant on the account', () => { + const account = custodyAccount(); + const grant = accessGrant({ + account, + userData: userData({ id: granteeId }), + accessLevel: CustodyAccessLevel.WRITE, + active: true, + }); + account.accessGrants = [grant]; + + expect(account.isOwnedBy(granteeId)).toBe(false); + }); + + it('returns false for a user-data id with no ownership or access grant', () => { + const account = custodyAccount(); + const grant = accessGrant({ + account, + userData: userData({ id: granteeId }), + accessLevel: CustodyAccessLevel.WRITE, + active: true, + }); + account.accessGrants = [grant]; + + expect(account.isOwnedBy(strangerId)).toBe(false); + }); + }); +}); diff --git a/src/subdomains/core/custody/entities/custody-account.entity.ts b/src/subdomains/core/custody/entities/custody-account.entity.ts index 5d20057f62..3a8125d867 100644 --- a/src/subdomains/core/custody/entities/custody-account.entity.ts +++ b/src/subdomains/core/custody/entities/custody-account.entity.ts @@ -24,4 +24,13 @@ export class CustodyAccount extends IEntity { @OneToMany(() => CustodyAccountAccess, (access) => access.account) accessGrants: CustodyAccountAccess[]; + + /** + * Whether this account belongs to the given user_data, as opposed to merely being reachable + * through a grant. Distinct from the access level: a grantee can hold WRITE without owning + * anything, and an owner can narrow themselves to READ while still owning it. + */ + isOwnedBy(accountId: number): boolean { + return this.owner.id === accountId; + } } diff --git a/src/subdomains/core/custody/mappers/custody-account-dto.mapper.ts b/src/subdomains/core/custody/mappers/custody-account-dto.mapper.ts index 225203f1b3..ed7de3a829 100644 --- a/src/subdomains/core/custody/mappers/custody-account-dto.mapper.ts +++ b/src/subdomains/core/custody/mappers/custody-account-dto.mapper.ts @@ -5,13 +5,14 @@ import { CustodyAccount } from '../entities/custody-account.entity'; import { CustodyAccessLevel } from '../enums/custody'; export class CustodyAccountDtoMapper { - static toDto(custodyAccount: CustodyAccount, accessLevel: CustodyAccessLevel): CustodyAccountDto { + static toDto(custodyAccount: CustodyAccount, accessLevel: CustodyAccessLevel, isOwner: boolean): CustodyAccountDto { return { id: custodyAccount.id, title: custodyAccount.title, description: custodyAccount.description, isLegacy: false, accessLevel, + isOwner, owner: custodyAccount.owner ? { id: custodyAccount.owner.id } : undefined, }; } @@ -23,6 +24,7 @@ export class CustodyAccountDtoMapper { description: undefined, isLegacy: true, accessLevel: CustodyAccessLevel.WRITE, + isOwner: true, owner: { id: userData.id }, }; } diff --git a/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts b/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts index 28f0cf8c90..7db1104b78 100644 --- a/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts +++ b/src/subdomains/core/custody/services/__tests__/custody-account.service.spec.ts @@ -355,6 +355,7 @@ describe('CustodyAccountService', () => { id: ownAccountId, accessLevel: CustodyAccessLevel.WRITE, isLegacy: false, + isOwner: true, }), ); }); @@ -378,6 +379,7 @@ describe('CustodyAccountService', () => { id: ownAccountId, accessLevel: CustodyAccessLevel.READ, isLegacy: false, + isOwner: true, }), ); }); @@ -423,6 +425,7 @@ describe('CustodyAccountService', () => { id: foreignAccountId, accessLevel: CustodyAccessLevel.READ, isLegacy: false, + isOwner: false, }), ); @@ -431,6 +434,46 @@ describe('CustodyAccountService', () => { expect(ownOccurrences).toHaveLength(1); }); + it('lists a shared foreign account with write access as not owned', async () => { + const foreignAccount = foreignCustodyAccount(); + const sharedWriteGrant = accessGrant({ + account: foreignAccount, + userData: ownerUserData(), + accessLevel: CustodyAccessLevel.WRITE, + active: true, + }); + userDataService.getUserData.mockResolvedValue(ownerUserData()); + mockFindActiveGrants([sharedWriteGrant]); + + const result = await service.getCustodyAccountsForUser(ownerId); + + expect(result).toHaveLength(1); + expect(result[0]).toEqual( + expect.objectContaining({ + id: foreignAccountId, + accessLevel: CustodyAccessLevel.WRITE, + isLegacy: false, + isOwner: false, + }), + ); + }); + + it('lists a legacy account as owned for a custody user without owned accounts', async () => { + const custodyUserId = 55; + userDataService.getUserData.mockResolvedValue(ownerUserData({ users: [custodyRoleUser(custodyUserId)] })); + mockFindActiveGrants([]); + + const result = await service.getCustodyAccountsForUser(ownerId); + + expect(result).toHaveLength(1); + expect(result[0]).toEqual( + expect.objectContaining({ + isLegacy: true, + isOwner: true, + }), + ); + }); + it("filters out inactive grants, another user's grants, and grants on non-active accounts", async () => { const account = ownCustodyAccount(); const legitimateGrant = accessGrant({ diff --git a/src/subdomains/core/custody/services/custody-account.service.ts b/src/subdomains/core/custody/services/custody-account.service.ts index 52eda89931..bf4c540b90 100644 --- a/src/subdomains/core/custody/services/custody-account.service.ts +++ b/src/subdomains/core/custody/services/custody-account.service.ts @@ -68,12 +68,12 @@ export class CustodyAccountService { }, relations: { account: { owner: true } }, }); - const sharedAccounts = activeGrants.filter((a) => a.account.owner.id !== accountId); + const sharedAccounts = activeGrants.filter((a) => !a.account.isOwnedBy(accountId)); // A grant on an own account narrows the owner's level — see checkAccess. Without this the // list would offer WRITE where the authorisation only grants inspection. const ownLevelByAccount = new Map( - activeGrants.filter((a) => a.account.owner.id === accountId).map((a) => [a.account.id, a.accessLevel]), + activeGrants.filter((a) => a.account.isOwnedBy(accountId)).map((a) => [a.account.id, a.accessLevel]), ); const custodyAccounts: CustodyAccountDto[] = [ @@ -81,9 +81,9 @@ export class CustodyAccountService { // No grant on an own account means the owner keeps full disposal — that is the rule, // not a fallback for a missing value. const level = ownLevelByAccount.get(ca.id) ?? CustodyAccessLevel.WRITE; - return CustodyAccountDtoMapper.toDto(ca, level); + return CustodyAccountDtoMapper.toDto(ca, level, true); }), - ...sharedAccounts.map((a) => CustodyAccountDtoMapper.toDto(a.account, a.accessLevel)), + ...sharedAccounts.map((a) => CustodyAccountDtoMapper.toDto(a.account, a.accessLevel, false)), ]; // Legacy Safe = absence of any owned account row; independent of shared grants. Hidden @@ -157,7 +157,7 @@ export class CustodyAccountService { // owner's own grant is the only way to express that, so it must not be overridden here. // Managing grants stays with the owner regardless (requireOwner), so this cannot lock // anyone out of their own account. - if (custodyAccount.owner.id === accountId && !access) { + if (custodyAccount.isOwnedBy(accountId) && !access) { return { custodyAccount, isLegacy: false }; } @@ -208,7 +208,7 @@ export class CustodyAccountService { const ownerId = custodyAccount.owner.id; // Owner already holds every Safe row; multi-account ambiguity only matters for grantees. - if (ownerId === callerAccountId) { + if (custodyAccount.isOwnedBy(callerAccountId)) { return ownerId; } @@ -384,7 +384,7 @@ export class CustodyAccountService { relations: { owner: true }, }); - if (!custodyAccount || custodyAccount.owner.id !== accountId) { + if (!custodyAccount || !custodyAccount.isOwnedBy(accountId)) { throw new ForbiddenException('Only the account owner can manage access grants'); } @@ -446,7 +446,7 @@ export class CustodyAccountService { account: CustodyAccount, newLevel: CustodyAccessLevel, ): void { - const isOwnGrant = access.userData.id === account.owner.id; + const isOwnGrant = account.isOwnedBy(access.userData.id); const isElevation = access.accessLevel === CustodyAccessLevel.READ && newLevel === CustodyAccessLevel.WRITE; if (!isOwnGrant && isElevation && account.status !== CustodyAccountStatus.ACTIVE) { @@ -461,7 +461,7 @@ export class CustodyAccountService { * account without an owner row and make the level unrecordable, so it stays refused. */ private rejectOwnerGrantRevocation(access: CustodyAccountAccess, account: CustodyAccount): void { - if (access.userData.id === account.owner.id) { + if (account.isOwnedBy(access.userData.id)) { throw new BadRequestException("Cannot revoke the account owner's access grant"); } } From 677e52341acff4574222f20767f78c9f1a9204e2 Mon Sep 17 00:00:00 2001 From: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:23:21 +0200 Subject: [PATCH 2/2] fix(custody): give the Safe balances a stable order (#4428) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(custody): give the Safe balances a stable order 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. * fix(custody): rank a non-finite balance last instead of comparing it 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. * test(custody): make the ordering test actually catch the bug it names 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. --- .../custody-asset-balance-dto.mapper.spec.ts | 274 ++++++++++++++++++ .../custody-asset-balance-dto.mapper.ts | 20 +- 2 files changed, 293 insertions(+), 1 deletion(-) diff --git a/src/subdomains/core/custody/mappers/__tests__/custody-asset-balance-dto.mapper.spec.ts b/src/subdomains/core/custody/mappers/__tests__/custody-asset-balance-dto.mapper.spec.ts index c254de057b..1711bf9ae4 100644 --- a/src/subdomains/core/custody/mappers/__tests__/custody-asset-balance-dto.mapper.spec.ts +++ b/src/subdomains/core/custody/mappers/__tests__/custody-asset-balance-dto.mapper.spec.ts @@ -100,5 +100,279 @@ describe('CustodyAssetBalanceDtoMapper', () => { expect(result[0].interest).toBe(interest); expect(result[0].interestValue).toEqual({ chf: interest, eur: interest, usd: interest }); }); + + it('returns balances ordered by descending CHF value regardless of input order', () => { + const aaaAsset: Asset = createCustomAsset({ + id: 20, + name: 'AAA', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const bbbAsset: Asset = createCustomAsset({ + id: 21, + name: 'BBB', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const cccAsset: Asset = createCustomAsset({ + id: 22, + name: 'CCC', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + + const balances: CustodyBalance[] = [ + Object.assign(new CustodyBalance(), { asset: cccAsset, balance: 50 }), + Object.assign(new CustodyBalance(), { asset: aaaAsset, balance: 100 }), + Object.assign(new CustodyBalance(), { asset: bbbAsset, balance: 500 }), + ]; + const interestByAssetName = new Map(); + + const result = CustodyAssetBalanceDtoMapper.mapCustodyBalances(balances, interestByAssetName); + + expect(result.map((balance) => [balance.asset.name, balance.value.chf])).toEqual([ + ['BBB', 500], + ['AAA', 100], + ['CCC', 50], + ]); + }); + + it('falls back to alphabetical asset name when CHF values are equal', () => { + const aaaAsset: Asset = createCustomAsset({ + id: 30, + name: 'AAA', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const bbbAsset: Asset = createCustomAsset({ + id: 31, + name: 'BBB', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const cccAsset: Asset = createCustomAsset({ + id: 32, + name: 'CCC', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + + const balances: CustodyBalance[] = [ + Object.assign(new CustodyBalance(), { asset: cccAsset, balance: 100 }), + Object.assign(new CustodyBalance(), { asset: bbbAsset, balance: 100 }), + Object.assign(new CustodyBalance(), { asset: aaaAsset, balance: 100 }), + ]; + const interestByAssetName = new Map(); + + const result = CustodyAssetBalanceDtoMapper.mapCustodyBalances(balances, interestByAssetName); + + expect(result.map((balance) => balance.asset.name)).toEqual(['AAA', 'BBB', 'CCC']); + }); + + it('returns the same order regardless of input order', () => { + const aaaAsset: Asset = createCustomAsset({ + id: 40, + name: 'AAA', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const bbbAsset: Asset = createCustomAsset({ + id: 41, + name: 'BBB', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const cccAsset: Asset = createCustomAsset({ + id: 42, + name: 'CCC', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const dddAsset: Asset = createCustomAsset({ + id: 43, + name: 'DDD', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + + const balances: CustodyBalance[] = [ + Object.assign(new CustodyBalance(), { asset: bbbAsset, balance: 100 }), + Object.assign(new CustodyBalance(), { asset: dddAsset, balance: 50 }), + Object.assign(new CustodyBalance(), { asset: aaaAsset, balance: 200 }), + Object.assign(new CustodyBalance(), { asset: cccAsset, balance: 100 }), + ]; + const firstInterestByAssetName = new Map(); + const secondInterestByAssetName = new Map(); + + const firstResult = CustodyAssetBalanceDtoMapper.mapCustodyBalances(balances, firstInterestByAssetName); + const secondResult = CustodyAssetBalanceDtoMapper.mapCustodyBalances( + [...balances].reverse(), + secondInterestByAssetName, + ); + + expect(firstResult.map((balance) => balance.asset.name)).toEqual( + secondResult.map((balance) => balance.asset.name), + ); + }); + + it('sorts negative CHF values after all positive values', () => { + const aaaAsset: Asset = createCustomAsset({ + id: 50, + name: 'AAA', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const bbbAsset: Asset = createCustomAsset({ + id: 51, + name: 'BBB', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const cccAsset: Asset = createCustomAsset({ + id: 52, + name: 'CCC', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + + const balances: CustodyBalance[] = [ + Object.assign(new CustodyBalance(), { asset: aaaAsset, balance: -50 }), + Object.assign(new CustodyBalance(), { asset: cccAsset, balance: 100 }), + Object.assign(new CustodyBalance(), { asset: bbbAsset, balance: 500 }), + ]; + const interestByAssetName = new Map(); + + const result = CustodyAssetBalanceDtoMapper.mapCustodyBalances(balances, interestByAssetName); + + expect(result.map((balance) => [balance.asset.name, balance.value.chf])).toEqual([ + ['BBB', 500], + ['CCC', 100], + ['AAA', -50], + ]); + }); + + it('places a non-finite CHF value last regardless of input order', () => { + const aaaAsset: Asset = createCustomAsset({ + id: 60, + name: 'AAA', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const bbbAsset: Asset = createCustomAsset({ + id: 61, + name: 'BBB', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const dddAsset: Asset = createCustomAsset({ + id: 62, + name: 'DDD', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const cccAsset: Asset = createCustomAsset({ + id: 63, + name: 'CCC', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + + // NaN plus two positive values is not enough: the old raw-value comparator and the ranked + // comparator happen to return the same order for both forward and reversed input, so the + // order-stability assertion cannot catch a regression. This four-value set adds a negative + // value: the old comparator returns BBB, CCC, DDD, AAA forward and BBB, AAA, CCC, DDD + // reversed, while the ranked comparator returns BBB, DDD, AAA, CCC both ways. Collapsing + // this back to three entries silently removes the only thing this test checks, so it must + // not be "simplified" later. + const balances: CustodyBalance[] = [ + Object.assign(new CustodyBalance(), { asset: aaaAsset, balance: -5 }), + Object.assign(new CustodyBalance(), { asset: bbbAsset, balance: 10 }), + Object.assign(new CustodyBalance(), { asset: dddAsset, balance: 0 }), + Object.assign(new CustodyBalance(), { asset: cccAsset, balance: NaN }), + ]; + const firstInterestByAssetName = new Map(); + const secondInterestByAssetName = new Map(); + + const firstResult = CustodyAssetBalanceDtoMapper.mapCustodyBalances(balances, firstInterestByAssetName); + const secondResult = CustodyAssetBalanceDtoMapper.mapCustodyBalances( + [...balances].reverse(), + secondInterestByAssetName, + ); + + expect(firstResult.map((balance) => balance.asset.name)).toEqual( + secondResult.map((balance) => balance.asset.name), + ); + expect(firstResult[firstResult.length - 1].asset.name).toBe('CCC'); + expect(secondResult[secondResult.length - 1].asset.name).toBe('CCC'); + expect(firstResult[firstResult.length - 1].value.chf).toBeNaN(); + expect(secondResult[secondResult.length - 1].value.chf).toBeNaN(); + }); + + it('places a positive Infinity CHF value last, the same as a non-finite NaN value', () => { + const cccAsset: Asset = createCustomAsset({ + id: 70, + name: 'CCC', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const aaaAsset: Asset = createCustomAsset({ + id: 71, + name: 'AAA', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + const bbbAsset: Asset = createCustomAsset({ + id: 72, + name: 'BBB', + approxPriceChf: 1, + approxPriceEur: 1, + approxPriceUsd: 1, + }); + + // Infinity is distinct from NaN because it is ordinarily comparable: the old raw-value + // comparator sorts it first instead of scrambling the order, so the position assertions + // catch the regression here. The order-stability assertion remains for consistency even + // though it does not distinguish the old and ranked comparators for this dataset. + const balances: CustodyBalance[] = [ + Object.assign(new CustodyBalance(), { asset: cccAsset, balance: Infinity }), + Object.assign(new CustodyBalance(), { asset: aaaAsset, balance: 200 }), + Object.assign(new CustodyBalance(), { asset: bbbAsset, balance: 100 }), + ]; + const firstInterestByAssetName = new Map(); + const secondInterestByAssetName = new Map(); + + const firstResult = CustodyAssetBalanceDtoMapper.mapCustodyBalances(balances, firstInterestByAssetName); + const secondResult = CustodyAssetBalanceDtoMapper.mapCustodyBalances( + [...balances].reverse(), + secondInterestByAssetName, + ); + + expect(firstResult.map((balance) => balance.asset.name)).toEqual( + secondResult.map((balance) => balance.asset.name), + ); + expect(firstResult[firstResult.length - 1].asset.name).toBe('CCC'); + expect(secondResult[secondResult.length - 1].asset.name).toBe('CCC'); + expect(firstResult[firstResult.length - 1].value.chf).toBe(Infinity); + expect(secondResult[secondResult.length - 1].value.chf).toBe(Infinity); + }); }); }); diff --git a/src/subdomains/core/custody/mappers/custody-asset-balance-dto.mapper.ts b/src/subdomains/core/custody/mappers/custody-asset-balance-dto.mapper.ts index 599d574a77..4f7a1a93aa 100644 --- a/src/subdomains/core/custody/mappers/custody-asset-balance-dto.mapper.ts +++ b/src/subdomains/core/custody/mappers/custody-asset-balance-dto.mapper.ts @@ -20,13 +20,31 @@ export class CustodyAssetBalanceDtoMapper { ): CustodyAssetBalanceDto[] { const groups = Util.groupByAccessor(custodyBalances, (b) => b.asset.name); - return Array.from(groups.values()).map((g) => { + const balances = Array.from(groups.values()).map((g) => { const asset = g[0].asset; const balance = Util.sumObjValue(g, 'balance'); const interestInfo = interestByAssetName.get(asset.name); return this.map(asset, balance, interestInfo); }); + + // The balances arrive in whatever order the database returned them — nothing orders that + // query — so the same holdings could come back in a different order on every request, with + // the list visibly reshuffling for no reason. Largest position first is both stable and the + // order someone reading a portfolio expects; equal values fall back to the name so the + // result is fully determined. + // + // A non-finite value is ranked last rather than compared. NaN is the reason: it makes every + // difference falsy, which would send the pair to the name comparison and cost the ordering + // its transitivity — the very unpredictability this sort exists to remove. Infinities are + // ranked the same way, not because they break the comparison but because they are the same + // class of corrupted data: no legitimate calculation produces one here, only an already + // broken balance can, and calculateAccruedInterest treats every non-finite figure alike for + // exactly that reason. It is not thrown, for the reason recorded there too: one damaged + // position must not take the customer's whole balance response down with it. + const rank = (value: number): number => (Number.isFinite(value) ? value : -Infinity); + + return balances.sort((a, b) => rank(b.value.chf) - rank(a.value.chf) || a.asset.name.localeCompare(b.asset.name)); } private static map(asset: Asset, balance: number, interestInfo?: CustodyInterestInfo): CustodyAssetBalanceDto {