refactor: migrates another part of console-api to strict types#1488
refactor: migrates another part of console-api to strict types#1488
Conversation
WalkthroughThis update introduces explicit TypeScript typing, non-null assertions, and schema refinements across deployment, billing, and provider modules. It enhances type safety, clarifies nullable fields, and standardizes object properties. Internal logic remains largely unchanged, with improvements focused on code correctness, error handling, and test consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant Service
participant Repository
participant Cache
Controller->>Service: executeDecodedTxByUserId(userId, messages)
Service->>Repository: Execute transaction
Repository-->>Service: Transaction result (code, hash, rawLog)
Service-->>Controller: { code, hash, transactionHash, rawLog }
Controller->>Service: get(address)
Service->>Cache: Check for cached balance
alt Cache miss
Service->>Repository: Fetch fresh limits
Service->>Cache: Store new CachedBalance
end
Cache-->>Service: Return CachedBalance
Service-->>Controller: CachedBalance
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (19)
🚧 Files skipped from review as they are similar to previous changes (19)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
stalniy
force-pushed
the
refactor/console-api-strict-5
branch
from
ec68ac5 to
bd3360d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (4)
apps/api/src/deployment/services/lease/lease.service.ts (1)
22-27: 🛠️ Refactor suggestionNon-null assertion hides missing-address edge case
wallet.address!compiles but will still throw at runtime if the address is actuallyundefined(e.g. wallet not fully onboarded). Prefer an explicit guard with a domain-specific error:- owner: wallet.address!, + if (!wallet.address) { + throw new Error("WALLET_ADDRESS_MISSING"); + } + owner: wallet.address,apps/api/src/deployment/controllers/deployment/deployment.controller.ts (1)
100-108: 🛠️ Refactor suggestionSame unchecked address issue repeated
Replicate the explicit address check here too to keep behaviour consistent and avoid accidental
undefinedlook-ups.apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (1)
160-170: 🛠️ Refactor suggestionID concatenation is collision-prone
idis built by simply concatenatingdseq,gseq, andoseqwithout a delimiter.
1-11-1and11-1-1both evaluate to the same string"1111", which may break look-ups, caching, or React key re-use.-id: lease.lease.lease_id.dseq + lease.lease.lease_id.gseq + lease.lease.lease_id.oseq, +id: `${lease.lease.lease_id.dseq}-${lease.lease.lease_id.gseq}-${lease.lease.lease_id.oseq}`,apps/api/src/billing/services/managed-signer/managed-signer.service.ts (1)
84-94:⚠️ Potential issueType cast drops
transactionHash; risk of runtime undefined
pick(...).as Pick<IndexedTx, ...>discardstransactionHashfrom the type system, yet you rely on it afterwards.
Iftxlackshash, the fallback branch assumestransactionHashis present but TS cannot help you.-const result = pick(tx, ["code", "hash", "transactionHash", "rawLog"]) as Pick< - IndexedTx, - "code" | "hash" | "rawLog" ->; +const result = pick(tx, ["code", "hash", "rawLog"]) as Pick< + IndexedTx, + "code" | "hash" | "rawLog" +> & { transactionHash?: string };Even better—avoid the cast completely:
const { code, hash, rawLog } = tx; return { code, hash, rawLog, transactionHash: hash };
♻️ Duplicate comments (1)
apps/api/src/deployment/services/lease/lease.service.ts (1)
39-41: Same non-null assertion – please apply the guard here too
🧹 Nitpick comments (7)
apps/api/src/deployment/services/top-up-custodial-balance/top-up-custodial-balance.service.ts (1)
1-7: Optional: defaultfeesBalanceto zero to simplify branchingIf
feesBalancelogically behaves like a numeric counter, consider initialising it to0in theBalancesproducer and drop thetypeof === "number"guard. That keeps mutation paths uniform.apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts (1)
8-9: Type-unsafe double cast – tighten the mock instead
billingConfig as unknown as BillingConfigServicedefeats the purpose of strict mode. HaveMockConfigServiceimplementBillingConfigService(or a narrowed interface) and skip the cast:-import type { BillingConfigService } from "@src/billing/services/billing-config/billing-config.service"; ... -billingConfig as unknown as BillingConfigService, +billingConfig,apps/api/test/functional/deployments.spec.ts (1)
80-85: DuplicatedhashvstransactionHash– consider converging on one fieldTests now expect both properties with identical values. Unless there is a compelling API reason, keeping a single canonical property reduces noise and risk of drift.
Also applies to: 505-510
apps/api/src/deployment/services/cached-balance/cached-balance.service.ts (1)
28-35: Possible race condition on first fetchTwo concurrent
get()calls for the sameaddresswith an empty cache may both fetch limits and overwrite each other. To avoid duplicate RPC hits consider caching aPromise<CachedBalance>or usingMap#has/Map#setguard with a mutex.-let cachedBalance = this.balanceCache.get(address); -if (!cachedBalance) { - const limits = await this.balancesService.getFreshLimits({ address }); - cachedBalance = new CachedBalance(limits.deployment); - this.balanceCache.set(address, cachedBalance); -} +if (!this.balanceCache.has(address)) { + const limits = await this.balancesService.getFreshLimits({ address }); + this.balanceCache.set(address, new CachedBalance(limits.deployment)); +} +const cachedBalance = this.balanceCache.get(address)!;apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (1)
164-169: Leaking extra provider fields
{ ...provider, address: provider.owner }spreads every column returned bygetProviderList()(uptime, ip, etc.).
ListWithResourcesResponseSchemaallows onlyaddressandhostUri.
The current implementation implicitly relies on Zod’s non-strict behaviour; once.strict()is enabled (or OpenAPI client generation is used), this will fail validation.Either:
- Limit the spread to required keys, e.g.
{ address: provider.owner, hostUri: provider.hostUri }, or- Mark the schema as non-strict explicitly to avoid accidental breakage.
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (1)
54-63: Return-type definition duplicates informationYou manually expanded the shape instead of extracting it into a reusable type alias.
This inflates the signature and invites drift.-async executeDecodedTxByUserId( - userId: UserWalletOutput["userId"], - messages: EncodeObject[] -): Promise<{ - code: number; - hash: string; - transactionHash: string; - rawLog: string; -}> { +type TxExecutionResult = { + code: number; + hash: string; + transactionHash: string; + rawLog: string; +}; + +async executeDecodedTxByUserId( + userId: UserWalletOutput["userId"], + messages: EncodeObject[] +): Promise<TxExecutionResult> {apps/api/src/deployment/lib/top-up-summarizer/top-up-summarizer.ts (1)
25-32: Ensureset()handlesundefinedgracefully
set()still requiresvalue: number.
When callers intentionally skip setting a height, they must now passundefined, which is disallowed by the signature.Consider:
-set(param: keyof Pick<TopUpSummary, "startBlockHeight" | "endBlockHeight">, value: number) { +set(param: keyof Pick<TopUpSummary, "startBlockHeight" | "endBlockHeight">, value: number | undefined) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/api/src/billing/services/managed-signer/managed-signer.service.ts(4 hunks)apps/api/src/deployment/controllers/deployment/deployment.controller.ts(3 hunks)apps/api/src/deployment/http-schemas/deployment.schema.ts(3 hunks)apps/api/src/deployment/lib/top-up-summarizer/top-up-summarizer.ts(2 hunks)apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts(1 hunks)apps/api/src/deployment/services/cached-balance/cached-balance.service.ts(1 hunks)apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts(2 hunks)apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts(4 hunks)apps/api/src/deployment/services/draining-deployment/draining-deployment.service.spec.ts(1 hunks)apps/api/src/deployment/services/draining-deployment/draining-deployment.service.ts(1 hunks)apps/api/src/deployment/services/lease/lease.service.ts(2 hunks)apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts(2 hunks)apps/api/src/deployment/services/top-up-custodial-balance/top-up-custodial-balance.service.ts(1 hunks)apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts(2 hunks)apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts(3 hunks)apps/api/src/provider/services/provider/provider.service.ts(2 hunks)apps/api/src/routes/v1/gpu.ts(3 hunks)apps/api/test/functional/deployments.spec.ts(10 hunks)apps/api/tsconfig.strict.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
apps/api/src/deployment/controllers/deployment/deployment.controller.ts (1)
apps/indexer/drizzle/schema.ts (1)
deployment(180-203)
apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts (1)
packages/logging/src/servicies/logger/logger.service.ts (1)
error(107-109)
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (3)
apps/indexer/drizzle/schema.ts (1)
deployment(180-203)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(16-20)apps/api/src/deployment/http-schemas/deployment.schema.ts (2)
GetDeploymentResponse(252-252)UpdateDeploymentRequest(259-259)
apps/api/src/provider/services/provider/provider.service.ts (1)
apps/api/src/types/provider.ts (1)
ProviderList(1-94)
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (1)
apps/indexer/drizzle/schema.ts (1)
provider(205-237)
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (1)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(16-20)
🔇 Additional comments (16)
apps/api/tsconfig.strict.json (1)
32-34: Included additional source folders in strict compile – looks goodExpanding strict-mode coverage to
provider,certificate, anddeploymentcode is exactly what we want for the gradual migration. No functional concerns spotted here.apps/api/src/deployment/services/top-up-custodial-balance/top-up-custodial-balance.service.ts (1)
19-21: Good defensive guard for optionalfeesBalanceThe extra type check prevents
NaNsubtraction whenfeesBalanceis undefined. No further issues.apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts (2)
108-116: Preferunknownoveranyin catch blocksUsing
error: anydisables type-safety. Capture asunknownand narrow only when necessary:-} catch (error: any) { +} catch (error: unknown) { + const err = error as Error;or introduce a helper to normalise errors.
[ suggest_essential_refactor ]
129-130:filterdoes not narrow the element type – use a type-guard
messageInputs.filter(x => !!x)returns(CollectedMessage | undefined)[], conflicting with the declared return typeCollectedMessage[]. Provide an explicit type-guard to preserve the narrower array type:-return messageInputs.filter(x => !!x); +return messageInputs.filter( + (input): input is CollectedMessage => input !== undefined +);This avoids the need for later non-null assertions.
[ raise_critical_issue ]apps/api/src/deployment/services/draining-deployment/draining-deployment.service.ts (1)
75-76: Repeated non-null assertions hide real nullability
userWallet.address!presumes the repository never stores wallets without an address.
Expose this guarantee in the type instead of suppressing the compiler:
- Make
addressnon-optional inUserWalletOutput(preferred), or- Guard once and early:
if (!userWallet?.address) { return 0; } const deploymentSetting = await this.leaseRepository.findOneByDseqAndOwner( dseq, userWallet.address );This removes the need for
!and prevents runtimeundefineddereferencing.
[ suggest_essential_refactor ]apps/api/src/deployment/services/draining-deployment/draining-deployment.service.spec.ts (1)
210-214: LGTM – test now mirrors service contractChanging the mock to
undefinedcorrectly follows the service’sif (!userWallet)branch.
[ approve_code_changes ]apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (2)
50-58: Centraliseownervariable to avoid multiple!assertions
wallet.address!is sprinkled throughout the class, repeating noise and risking missed checks. Capture once:public async create(wallet: UserWalletOutput, input: CreateDeploymentRequest["data"]) { - let sdl = input.sdl; + const owner = wallet.address; + assert(owner, 400, "Wallet without address"); + let sdl = input.sdl;Then use
ownereverywhere (ownerinstead ofwallet.address!).
Benefits: readability, single assertion, and compile-time safety.[ suggest_optional_refactor ]
90-93: Redundant network callAfter
deposit, you retrieve the deployment again:await this.signerService.executeDecodedTxByUserId(...); return await this.deploymentReaderService.findByOwnerAndDseq(owner, dseq);If the RPC succeeds the local
deploymentobject is already available; consider updating itsbalancefield locally or returning the transaction result to save one query.
[ suggest_optional_refactor ]apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts (2)
52-57: Same narrowing issue as above
wallet.address!occurs three times; apply the same centralised guard pattern to improve safety and readability.
[ suggest_optional_refactor ]
67-76: Type-safe error handlingUse
unknownforerrorand narrow. Also, the string checkerror.message.includes("not allowed to pay fees")will throw iferroris not anError. Guard first:} catch (error: unknown) { if (error instanceof Error && error.message.includes("not allowed to pay fees")) {[ suggest_essential_refactor ]
apps/api/src/deployment/controllers/deployment/deployment.controller.ts (1)
133-136: 👍 Explicit 404 for missing deploymentThe new null-check using
assert(deployment, 404 …)makes the intent clear and avoids leaking internal errors.apps/api/src/routes/v1/gpu.ts (1)
64-66: LGTM – switch toundefinedaligns with query replacement logicInitialising
provider_address/provider_hosturiasundefinedmatches the later?? nullcoercion and removes double-null semantics.apps/api/src/provider/services/provider/provider.service.ts (1)
15-16: Return type annotation adds clarityImporting
ProviderListand annotatinggetProviderList’s return type strengthens the public contract and will surface mismatches at compile time.Also applies to: 102-102
apps/api/src/deployment/http-schemas/deployment.schema.ts (2)
221-233: Consider marking nested properties optional whenprovideris nullableWhen
providerisnull, everything is fine, but when it’s defined the inner fields are still required.
If any of them becomes nullable in the DB (e.g. provider goes offline and loseshostUri), schema validation will fail.Recommend mirroring the DB constraints (
hostUriisNOT NULLnow, but worth keeping an eye on).
No action required if that invariant is guaranteed.
244-245: Typo fix LGTM
txHashrename aligns the schema with actual field names used elsewhere.apps/api/src/deployment/lib/top-up-summarizer/top-up-summarizer.ts (1)
9-12: Optional properties align with actual usageMaking heights optional removes the need for sentinel values.
No issues spotted.
apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts
Show resolved
Hide resolved
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts
Show resolved
Hide resolved
stalniy
force-pushed
the
refactor/console-api-strict-5
branch
2 times, most recently
from
a97b6bc to
81776cf
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (66.66%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1488 +/- ##
==========================================
- Coverage 40.09% 40.08% -0.01%
==========================================
Files 870 870
Lines 21016 21023 +7
Branches 3857 3863 +6
==========================================
+ Hits 8427 8428 +1
- Misses 12410 12414 +4
- Partials 179 181 +2
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
|
this PR is about refactoring to strict types without introducing any new logic. That's why coverage report failure will be ignored |
stalniy
force-pushed
the
refactor/console-api-strict-5
branch
from
81776cf to
ca49adf
Compare
2 participants
Why
to improve project stability
What
migrates another part of console-api to strict types
Summary by CodeRabbit
Bug Fixes
transactionHashand provideraddress.Refactor
Tests
Chores