Conversation
kajoseph
added
the
BWS
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect sorting behavior in Bitcore Wallet Service where values were being coerced to strings, causing lexicographic ordering (e.g. "9" > "10") instead of numeric ordering. This impacts transaction ordering (notably sortDesc(txs, 'height') in the V8 explorer integration) and other utility sort call sites.
Changes:
- Reworks
Utils.sortAscto compare values numerically when appropriate (including multi-key sorting), with a string fallback. - Keeps
sortDescbehavior by delegating tosortAsc(...).reverse(). - Adds a unit test covering ascending sort with multi-digit numeric values.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/bitcore-wallet-service/src/lib/common/utils.ts | Replaces string/Buffer-based sorting with a normalized compare function supporting numeric comparisons and multi-key ordering. |
| packages/bitcore-wallet-service/test/utils.test.ts | Adds a regression-style test for multi-digit numeric sorting in sortAsc. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Numeric comparison for numbers, bigints, and non-empty numeric strings | ||
| const aIsNumeric = typeof a !== 'string' ? !isNaN(Number(a)) : a.trim() !== '' && !isNaN(Number(a)); | ||
| const bIsNumeric = typeof b !== 'string' ? !isNaN(Number(b)) : b.trim() !== '' && !isNaN(Number(b)); | ||
| if (aIsNumeric && bIsNumeric) { | ||
| const na = Number(a); | ||
| const nb = Number(b); | ||
| return na < nb ? -1 : na > nb ? 1 : 0; |
There was a problem hiding this comment.
compareVals calls Number(a)/Number(b) during numeric detection. Number(Symbol(...)) throws a TypeError, so sorting arrays (or object keys) that contain symbols will crash (previous implementation only relied on toString() and wouldn’t throw here). Consider explicitly limiting numeric comparison to number | bigint | string (and maybe boolean after normalization), or wrap the Number(...) conversion in a safe guard/try-catch so non-coercible types fall back to string comparison instead of throwing.
| it('should sort a simple array with multi-digit values', function() { | ||
| const res = Utils.sortAsc([30, 1, 200, 2]); | ||
| res.should.deep.equal([1, 2, 30, 200]); | ||
| }); |
There was a problem hiding this comment.
The regression that triggered this PR happens via sortDesc(..., 'height'), but the added coverage only exercises sortAsc on a primitive array. Please add a test that covers sortDesc (and ideally sorting objects by a numeric key with multi-digit values), so the original failure mode is protected against future changes.
Description
The BWS sort function turned everything to strings. This is a problem because string comparison uses different rules than numerical comparison. E.g.: "9" > "10" because the "9" char > the "1" char. This PR re-works the BWS sort function to better handle various data types.
This caused issues in
v8.ts>getTransactionswhen it callssortDesc(txs, 'height')and the txs heights would vary in number of digits (e.g. block height 999999 > 1000000).Changelog
Testing Notes
Call tx history for a wallet that has txs with block heights that increase in number of digits.
Checklist
BWCif modifying the bitcore-wallet-client package,CLIif modifying the bitcore-cli package, etc.)