Migrate /numbers to TS part 2#853
Conversation
|
Size Change: +8.67 kB (+0.25%) Total Size: 3.52 MB
|
There was a problem hiding this comment.
Pull request overview
This PR continues the migration of the /numbers directory to TypeScript, completing "part 2" of the migration. The changes convert three key files (xdr_large_int.js, sc_int.js, and index.js) to TypeScript, along with their associated tests, while maintaining complete test coverage by migrating existing test cases from JavaScript to TypeScript.
Changes:
- Migrated
src/numbers/xdr_large_int.js,src/numbers/sc_int.js, andsrc/numbers/index.jsto TypeScript with proper type annotations - Created comprehensive TypeScript test files (
xdr_large_int.test.ts,sc_int.test.ts,index.test.ts) that preserve all test cases from the removed JS test files - Added type declaration files in
type_validation/numbers/for the migrated modules - Updated type declarations in
types/stellar__js-xdr/index.d.tsto include theslicemethod and reordered union types for consistency - Created
src/index.d.tsas a temporary type declaration file during the migration process - Updated documentation comments to use
new ScInt()consistently instead of justScInt() - Added proper
.jsfile extensions to imports as required for ES modules
Reviewed changes
Copilot reviewed 14 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| types/stellar__js-xdr/index.d.ts | Added slice method declaration to LargeInt and reordered union types for consistency |
| type_validation/numbers/xdr_large_int.d.ts | New type declarations for XdrLargeInt class |
| type_validation/numbers/sc_int.d.ts | New type declarations for ScInt class |
| type_validation/numbers/index.d.ts | New type declarations for numbers index module exports |
| type_validation/numbers/{uint256,uint128,int256,int128}.d.ts | Updated constructor parameter types to use consistent ordering |
| test/unit/scval_test.js | Deleted - tests migrated to TypeScript |
| test/unit/scint_test.js | Deleted - tests migrated to TypeScript |
| test/unit/i256_test.js | Deleted - tests migrated to TypeScript |
| test/unit/numbers/xdr_large_int.test.ts | New comprehensive TypeScript tests for XdrLargeInt |
| test/unit/numbers/sc_int.test.ts | New comprehensive TypeScript tests for ScInt, includes migrated tests |
| test/unit/numbers/index.test.ts | New TypeScript tests for scValToBigInt and related functions |
| test/unit/numbers/{uint256,uint128,int256,int128}.test.ts | Updated imports to use .js extensions and added migrated tests from i256_test.js |
| src/numbers/xdr_large_int.ts | Migrated from JS with full type annotations and improved type safety |
| src/numbers/sc_int.ts | Migrated from JS with proper type annotations and updated documentation |
| src/numbers/index.ts | Migrated from JS with type-safe scValToBigInt implementation |
| src/index.d.ts | Temporary type declarations to support ongoing migration |
Comments suppressed due to low confidence (1)
src/numbers/xdr_large_int.ts:269
- The
isTypemethod on line 269 should have an explicit return type annotation. It should bestatic isType(type: string): booleanto make the return type explicit and enable better type narrowing. This is especially important for type guard functions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 23 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/numbers/sc_int.ts:84
- The constructor creates
bigValueby converting the input to bigint on line 79, but line 90 (outside the changed region) still passes the originalvaluetonearestBigIntSizeinstead ofbigValue. This means string inputs like "123" will have their bit length calculated as the string length (3 characters) rather than the numeric value's bit length, potentially causing incorrect type selection. While this appears to be a pre-existing issue from the JS code, the TypeScript migration provides an opportunity to fix it by usingbigValueinstead.
src/numbers/sc_int.ts:116 - The
nearestBigIntSizefunction acceptsbigint | number | stringas input, but on line 114 callsbigI.toString(2)to convert to binary string. Whilebigint.toString(radix)andnumber.toString(radix)support the radix parameter,string.toString()does not accept parameters and will ignore the argument. For string inputs like "12345", this would return "12345" instead of a binary representation, leading to incorrect bit length calculation. The function should either convert string inputs to bigint first, or the type signature should bebigint | numberonly.
src/numbers/sc_int.ts:116 - The
nearestBigIntSizefunction parameter type isbigint | number | string, but the function callsbigI.toString(2)at line 114. Whilebigintandnumberhave atoString(radix)method,stringdoes not have this signature - it only hastoString()without parameters. When a string is passed, callingtoString(2)will ignore the argument and return the string itself, which would then be used to calculate bit length incorrectly. The function should either convert the input to bigint first, or the type signature should be narrowed to exclude string.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Ryang-21
left a comment
There was a problem hiding this comment.
Left a couple comments about type mismatches between the manual and generated type declarations
| const signed = value < 0; | ||
| constructor( | ||
| value: bigint | number | string, | ||
| opts?: { type?: string; [key: string]: unknown }, |
There was a problem hiding this comment.
If we want parity with the manual declarations we should create a type called ScIntType that contains the possible option types
There was a problem hiding this comment.
If we go this route we will need to add duration and timepoint
| * {@link XdrLargeInt.isType}) | ||
| * @param values - a list of integer-like values interpreted in big-endian order | ||
| */ | ||
| constructor(type: string, values: XdrLargeIntValues) { |
There was a problem hiding this comment.
Same thing here the manual type declaration has this type: ScIntType
| @@ -0,0 +1,374 @@ | |||
| /* eslint-disable @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-explicit-any */ | |||
|
|
|||
| import { describe, it, expect } from "vitest"; | |||
There was a problem hiding this comment.
Can we just include tests for scValToBigInt here?
| @@ -28,7 +29,7 @@ declare module "@stellar/js-xdr" { | |||
| } | |||
|
|
|||
| export class UnsignedHyper extends LargeInt { | |||
| constructor(values: Array<number | bigint | string>); | |||
| constructor(values: Array<bigint | number | string>); | |||
| static defineIntBoundaries(): void; | |||
| static MIN_VALUE: UnsignedHyper; | |||
| static MAX_VALUE: UnsignedHyper; | |||
There was a problem hiding this comment.
The js-xdr implementation looks like it spreads the args parameter
/**
* @param {Array<Number|BigInt|String>} parts - Slices to encode
*/
constructor(...args) {
super(args);
}
index,sc_int, andxdr_large_intfiles to TS.i256_test.js,scint_test.js, andscval_test.js)src/index.js