-
Notifications
You must be signed in to change notification settings - Fork 88
refactor(billing): require wallet address in public wallet API responses #3521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,14 +3,12 @@ import assert from "http-assert"; | |||||||||||||||||||
| import { Lifecycle, scoped } from "tsyringe"; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import { AuthService } from "@src/auth/services/auth.service"; | ||||||||||||||||||||
| import { UserWalletOutput, UserWalletPublicOutput, UserWalletRepository } from "@src/billing/repositories"; | ||||||||||||||||||||
| import { UserWalletOutput, UserWalletPublicOutput, UserWalletRepository, WalletInitialized } from "@src/billing/repositories"; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| export interface GetWalletOptions { | ||||||||||||||||||||
| userId: string; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| export type WalletInitialized = Omit<UserWalletOutput, "address"> & { address: string }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @scoped(Lifecycle.ResolutionScoped) | ||||||||||||||||||||
| export class WalletReaderService { | ||||||||||||||||||||
| constructor( | ||||||||||||||||||||
|
|
@@ -21,7 +19,9 @@ export class WalletReaderService { | |||||||||||||||||||
| async getWallets(query: GetWalletOptions): Promise<UserWalletPublicOutput[]> { | ||||||||||||||||||||
| const wallets = await this.userWalletRepository.accessibleBy(this.authService.ability, "read").find(query); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return wallets.filter(wallet => wallet.activatedAt).map(wallet => this.userWalletRepository.toPublic(wallet)); | ||||||||||||||||||||
| return wallets | ||||||||||||||||||||
| .filter((wallet): wallet is WalletInitialized => wallet.activatedAt !== null && wallet.address !== null) | ||||||||||||||||||||
| .map(wallet => this.userWalletRepository.toPublic(wallet)); | ||||||||||||||||||||
|
Comment on lines
+22
to
+24
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Validate the actual address value before asserting
Proposed fix return wallets
- .filter((wallet): wallet is WalletInitialized => wallet.activatedAt !== null && wallet.address !== null)
+ .filter(
+ (wallet): wallet is WalletInitialized =>
+ wallet.activatedAt != null && typeof wallet.address === "string" && wallet.address.length > 0
+ )
.map(wallet => this.userWalletRepository.toPublic(wallet));📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| async getWalletByUserId(userId: string): Promise<WalletInitialized>; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,8 @@ import { InternalServerError } from "http-errors"; | |
| import { Op } from "sequelize"; | ||
| import { singleton } from "tsyringe"; | ||
|
|
||
| import { WalletInitialized, WalletReaderService } from "@src/billing/services/wallet-reader/wallet-reader.service"; | ||
| import { WalletInitialized } from "@src/billing/repositories"; | ||
| import { WalletReaderService } from "@src/billing/services/wallet-reader/wallet-reader.service"; | ||
|
Comment on lines
+19
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n --glob 'eslint.config.*' --glob '.eslintrc*' 'consistent-type-imports' .
ast-grep run --lang ts \
--pattern 'import { WalletInitialized } from "`@src/billing/repositories`"' \
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsRepository: akash-network/console Length of output: 305 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n## CLAUDE.md\n'
fd -a '^CLAUDE\.md$' .
printf '\n## ESLint config files\n'
fd -a 'eslint\.config\..*|\.eslintrc.*' .
printf '\n## Target file outline\n'
ast-grep outline apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts --view expanded || true
printf '\n## Target file lines\n'
cat -n apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts | sed -n '1,140p'Repository: akash-network/console Length of output: 8765 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n## Search for type-import rule\n'
rg -n --hidden --glob '!**/node_modules/**' 'consistent-type-imports|import type|type imports|typescript-eslint' .
printf '\n## Search for WalletInitialized usage\n'
rg -n --hidden --glob '!**/node_modules/**' 'WalletInitialized' apps/api/src
printf '\n## Package / lint context\n'
fd -a 'package\.json$' . | head -n 20Repository: akash-network/console Length of output: 50378 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n## apps/api/eslint.config.mjs\n'
cat -n apps/api/eslint.config.mjs | sed -n '1,220p'
printf '\n## root eslint.config.mjs\n'
cat -n eslint.config.mjs | sed -n '1,220p'
printf '\n## shared TS config\n'
cat -n packages/dev-config/eslint/typescript.mjs | sed -n '1,120p'Repository: akash-network/console Length of output: 3656 Use a type-only import for 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| import { Memoize } from "@src/caching/helpers"; | ||
| import { LoggerService } from "@src/core"; | ||
| import { GetDeploymentResponse, ListDeploymentsItem } from "@src/deployment/http-schemas/deployment.schema"; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Preserve the CASL-scoped repository for the address update.
#ensureWalletViareads throughrepository—which is authorization-scoped during trial initialization—but mutates throughthis.userWalletRepository. Userepository.updateById(...)so the entire get/check/mutate sequence remains authorization-scoped.Proposed fix
Based on learnings, user-scoped billing mutations should remain behind the CASL-protected repository boundary; as per path instructions, get/check/mutate authorization gaps must be reviewed explicitly.
📝 Committable suggestion
🤖 Prompt for AI Agents
Sources: Path instructions, Learnings