Repository navigation
feat(strategies): support volatile asset symbols - #1032
Conversation
|
@Fi-zzy Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
@Fi-zzy is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
collinsezedike
left a comment
There was a problem hiding this comment.
@Fi-zzy thanks for the work. CI is red for two reasons.
pnpm format:check fails on packages/strategies/src/feeds.ts, packages/strategies/src/regime-scenarios.ts and packages/strategies/src/scenario-backtest.test.ts. Running pnpm exec prettier --write on those three clears the Lint & Typecheck job.
@meridian/strategies#build fails with TS2352 at regime-scenarios.ts:218, and that is what fails the Test job. Once the build is green, please confirm pnpm typecheck is clean with the Partial<Record<...>> holdings change in report.ts, because the failing tsc step stopped before consumers of finalState.holdings were checked.
| AssetSymbol, | ||
| Array<{ timestamp: number; price: FixedPointDecimal }> | ||
| > = { USDC: [], EURC: [] }; | ||
| > = Object.fromEntries( |
There was a problem hiding this comment.
@Fi-zzy Object.fromEntries here infers { [k: string]: never[] }, and TS2352 rejects the direct cast to Record<AssetSymbol, ...>. This is the build failure that takes down the Test job. Type the record explicitly before filling it, or cast through unknown.
| export interface ReportPortfolioState { | ||
| readonly timestamp: SimulationTimestamp; | ||
| readonly holdings: Readonly<Record<AssetSymbol, FixedPointDecimal>>; | ||
| readonly holdings: Readonly<Partial<Record<AssetSymbol, FixedPointDecimal>>>; |
There was a problem hiding this comment.
@Fi-zzy widening holdings to Partial<...> is fine, but the tsc step never got past the build error above, so consumers that index a single asset (for example holdings[asset].toStroops()) have not been typechecked. Please confirm pnpm typecheck passes after the cast is fixed.
collinsezedike
left a comment
There was a problem hiding this comment.
@Fi-zzy the Typecheck job fails on the cast in regime-scenarios.ts and the Format job fails on feeds.ts, regime-scenarios.ts, and scenario-backtest.test.ts. Run pnpm format to fix the formatting.
| > = { USDC: [], EURC: [] }; | ||
| > = Object.fromEntries( | ||
| KNOWN_ASSET_SYMBOLS.map((supported) => [supported, []]) | ||
| ) as Record<AssetSymbol, Array<{ timestamp: number; price: FixedPointDecimal }>>; |
There was a problem hiding this comment.
Object.fromEntries(...) is inferred as { [k: string]: never[] }, so this cast to Record<AssetSymbol, ...> is rejected with TS2352. Type the map callback so each entry is [AssetSymbol, Array<{ timestamp: number; price: FixedPointDecimal }>], or build the record with an explicit accumulator.
collinsezedike
left a comment
There was a problem hiding this comment.
@Fi-zzy the Typecheck job fails on the cast in regime-scenarios.ts and the Format job fails on feeds.ts, regime-scenarios.ts, and scenario-backtest.test.ts. Run pnpm format to fix the formatting.
| > = { USDC: [], EURC: [] }; | ||
| > = Object.fromEntries( | ||
| KNOWN_ASSET_SYMBOLS.map((supported) => [supported, []]) | ||
| ) as Record<AssetSymbol, Array<{ timestamp: number; price: FixedPointDecimal }>>; |
There was a problem hiding this comment.
Object.fromEntries(...) is inferred as { [k: string]: never[] }, so this cast to Record<AssetSymbol, ...> is rejected with TS2352. Type the map callback so each entry is [AssetSymbol, Array<{ timestamp: number; price: FixedPointDecimal }>], or build the record with an explicit accumulator.
1bb73f6 to
14ba22e
Compare
collinsezedike
left a comment
There was a problem hiding this comment.
Thank you for the updates, @Fi-zzy. The rebased branch resolves the cast and formatting failures, and the widened asset union is covered by XLM tests. All required checks pass; this is ready to merge. Please consider picking up another open issue.
|
Thank you for your contribution, @Fi-zzy; if you have a moment, starring the repository would help the project reach more people. |
Summary
This is a source-level TypeScript change that accepts more asset symbols; existing USDC/EURC behavior and vault deposit asset constraints are unchanged.
Closes #1022
Verification: tests, typecheck, and lint pass for
@meridian/strategies; the branch is rebased onto currentmain.