Migrate liquidity_pool_id.js file to TS#862
Migrate liquidity_pool_id.js file to TS#862quietbits wants to merge 3 commits intotypescript-migrationfrom
liquidity_pool_id.js file to TS#862Conversation
|
Size Change: +275 B (+0.01%) Total Size: 3.53 MB
|
There was a problem hiding this comment.
Pull request overview
This PR migrates the liquidity_pool_id.js file from JavaScript to TypeScript, maintaining the same functionality while adding type safety. The migration follows established patterns in the codebase for TypeScript conversions, using vitest for testing and generating appropriate type definitions.
Changes:
- Migrated
src/liquidity_pool_id.jsto TypeScript with proper type annotations - Added comprehensive TypeScript test file using vitest
- Generated type definition file with complete method signatures
- Updated
.gitignoreto include TypeScript build artifacts
Reviewed changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/liquidity_pool_id.ts |
New TypeScript implementation with type annotations and XDR type workarounds for compatibility |
src/liquidity_pool_id.js |
Removed old JavaScript implementation |
test/unit/liquidity_pool_id.test.ts |
New comprehensive test suite using vitest framework |
type_validation/liquidity_pool_id.d.ts |
Generated type definitions matching the implementation |
type_validation/strkey.d.ts |
Alphabetically sorted union type members for consistency |
.gitignore |
Added tsconfig.tsbuildinfo for TypeScript build cache |
💡 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 4 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static fromOperation(tlAssetXdr: xdr.TrustLineAsset): LiquidityPoolId { | ||
| const assetType = tlAssetXdr.switch(); | ||
| if (assetType === xdr.AssetType.assetTypePoolShare()) { | ||
| // tlAssetXdr.liquidityPoolId() is Buffer at runtime | ||
| const liquidityPoolId = ( | ||
| tlAssetXdr.liquidityPoolId() as unknown as Buffer | ||
| ).toString("hex"); | ||
| return new LiquidityPoolId(liquidityPoolId); |
There was a problem hiding this comment.
fromOperation previously returned new this(liquidityPoolId) (allowing subclasses to reuse the factory). The TS version always returns new LiquidityPoolId(...), which is a behavioral/API change. If subclassing support is intended, consider typing the static method with a this constructor type and returning new this(...) instead.
| static fromOperation(tlAssetXdr: xdr.TrustLineAsset): LiquidityPoolId { | |
| const assetType = tlAssetXdr.switch(); | |
| if (assetType === xdr.AssetType.assetTypePoolShare()) { | |
| // tlAssetXdr.liquidityPoolId() is Buffer at runtime | |
| const liquidityPoolId = ( | |
| tlAssetXdr.liquidityPoolId() as unknown as Buffer | |
| ).toString("hex"); | |
| return new LiquidityPoolId(liquidityPoolId); | |
| static fromOperation<T extends LiquidityPoolId>( | |
| this: new (liquidityPoolId: string) => T, | |
| tlAssetXdr: xdr.TrustLineAsset, | |
| ): T { | |
| const assetType = tlAssetXdr.switch(); | |
| if (assetType === xdr.AssetType.assetTypePoolShare()) { | |
| // tlAssetXdr.liquidityPoolId() is Buffer at runtime | |
| const liquidityPoolId = ( | |
| tlAssetXdr.liquidityPoolId() as unknown as Buffer | |
| ).toString("hex"); | |
| return new this(liquidityPoolId); |
There was a problem hiding this comment.
new this() was intentional in the original JS, but since no subclasses exist and TypeScript favors explicit return types, using new LiquidityPoolId(...) is the idiomatic choice here and does not constitute a breaking change (public facing code won't be affected by this).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TODO: check generated XDR types to make sure they are up to date. | ||
| // arm() exists at runtime on XDR union types but is not declared in types/curr.d.ts. | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any | ||
| expect((tlAsset as any).arm()).toBe("liquidityPoolId"); |
There was a problem hiding this comment.
The test asserts the union arm via (tlAsset as any).arm() and disables lint rules to allow any/unsafe access. Since TrustLineAsset.switch().name is already typed (see types/curr.d.ts), consider asserting on tlAsset.switch().name instead so the test stays type-safe and doesn’t depend on an undeclared runtime-only method.
| // TODO: check generated XDR types to make sure they are up to date. | |
| // arm() exists at runtime on XDR union types but is not declared in types/curr.d.ts. | |
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any | |
| expect((tlAsset as any).arm()).toBe("liquidityPoolId"); | |
| expect(tlAsset.switch().name).toBe("liquidityPoolId"); |
There was a problem hiding this comment.
This suggestion is wrong tlAsset.switch().name produces assetTypePoolShare instead of liquidityPoolId.
2 participants
No description provided.