refactor(billing): require wallet address in public wallet API responses#3521
Open
baktun14 wants to merge 3 commits into
Open
refactor(billing): require wallet address in public wallet API responses#3521baktun14 wants to merge 3 commits into
baktun14 wants to merge 3 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughWallet initialization now guarantees initialized public wallet data, frontend hooks handle wallet presence independently of address presence, and API schemas document non-null wallet fields, a new trial-provisioning conflict response, and optional transaction idempotency keys. ChangesWallet initialization and API contract
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3521 +/- ##
==========================================
- Coverage 74.18% 73.20% -0.98%
==========================================
Files 1142 1052 -90
Lines 29721 27371 -2350
Branches 7447 6987 -460
==========================================
- Hits 22049 20038 -2011
+ Misses 6771 6462 -309
+ Partials 901 871 -30
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse type-only imports for wallet aliases.
UserWalletOutput,UserWalletPublicOutput, andWalletInitializedare type-only symbols, whileUserWalletRepositoryis a runtime dependency. Split or annotate these imports to satisfy the shared TypeScript import rule and avoid runtime import issues.
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts#L6-L6: mark the wallet aliases as type-only while keepingUserWalletRepositoryas a value import.apps/api/src/billing/services/wallet-reader/wallet-reader.service.ts#L6-L6: apply the same type-only import treatment.As per coding guidelines, TypeScript imports must use consistent type imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts` at line 6, Update the imports in apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts#L6-L6 and apps/api/src/billing/services/wallet-reader/wallet-reader.service.ts#L6-L6 so UserWalletOutput, UserWalletPublicOutput, and WalletInitialized use type-only imports, while UserWalletRepository remains a runtime import.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts`:
- Around line 87-94: Update `#ensureWalletVia` to perform the wallet address
mutation through the authorization-scoped repository argument by replacing
this.userWalletRepository.updateById with repository.updateById, while
preserving the existing update parameters and returned wallet behavior.
In `@apps/api/src/billing/services/wallet-reader/wallet-reader.service.ts`:
- Around line 22-24: Update the wallet filter in the wallet-reader mapping flow
to validate that address is present and non-empty before asserting
WalletInitialized; reject undefined, null, and empty or whitespace-only values
while preserving the activatedAt requirement and existing toPublic mapping.
In
`@apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts`:
- Around line 19-20: Change the WalletInitialized import in the deployment
reader service to a type-only import, while keeping WalletReaderService as a
regular import.
In `@apps/api/swagger/openapi.json`:
- Line 1790: Remove the generated static default value for dynamic endDate at
apps/api/swagger/openapi.json lines 1790-1790 and 1916-1916, omitting the
default property entirely at both sites so the OpenAPI schema does not contain a
date that becomes stale.
- Line 5: Update the OpenAPI server URL in the generator configuration to the
intended public API URL instead of localhost, then regenerate the specification
so Swagger UI and generated clients use the deployed server.
---
Nitpick comments:
In
`@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts`:
- Line 6: Update the imports in
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts#L6-L6
and apps/api/src/billing/services/wallet-reader/wallet-reader.service.ts#L6-L6
so UserWalletOutput, UserWalletPublicOutput, and WalletInitialized use type-only
imports, while UserWalletRepository remains a runtime import.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cee7dad8-2ed7-4893-a71b-d7722cb62820
⛔ Files ignored due to path filters (1)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
apps/api/src/billing/http-schemas/wallet.schema.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/billing/services/refill/refill.service.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/billing/services/wallet-reader/wallet-reader.service.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.spec.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.spec.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/src/deployment/services/lease/lease.service.spec.tsapps/api/swagger/openapi.jsonapps/api/test/seeders/user-wallet.seeder.tsapps/deploy-web/src/hooks/useEnsureTrialStarted.spec.tsapps/deploy-web/src/hooks/useEnsureTrialStarted.tsapps/deploy-web/src/hooks/useManagedWallet.spec.tsxapps/deploy-web/src/hooks/useManagedWallet.tspackages/console-api-types/src/schema.d.ts
💤 Files with no reviewable changes (1)
- apps/deploy-web/src/hooks/useEnsureTrialStarted.spec.ts
Comment on lines
+87
to
+94
| async #ensureWalletVia(repository: UserWalletRepository, userId: string): Promise<WalletInitialized> { | ||
| const { wallet } = await repository.getOrCreate({ userId }); | ||
|
|
||
| if (wallet.address) return wallet; | ||
| if (wallet.address) return { ...wallet, address: wallet.address }; | ||
|
|
||
| const { address } = await this.walletManager.createWallet({ addressIndex: wallet.id }); | ||
| return await this.userWalletRepository.updateById(wallet.id, { address }, { returning: true }); | ||
| const updatedWallet = await this.userWalletRepository.updateById(wallet.id, { address }, { returning: true }); | ||
| return { ...updatedWallet, address }; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Preserve the CASL-scoped repository for the address update.
#ensureWalletVia reads through repository—which is authorization-scoped during trial initialization—but mutates through this.userWalletRepository. Use repository.updateById(...) so the entire get/check/mutate sequence remains authorization-scoped.
Proposed fix
- const updatedWallet = await this.userWalletRepository.updateById(wallet.id, { address }, { returning: true });
+ const updatedWallet = await repository.updateById(wallet.id, { address }, { returning: true });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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async #ensureWalletVia(repository: UserWalletRepository, userId: string): Promise<WalletInitialized> { | |
| const { wallet } = await repository.getOrCreate({ userId }); | |
| if (wallet.address) return wallet; | |
| if (wallet.address) return { ...wallet, address: wallet.address }; | |
| const { address } = await this.walletManager.createWallet({ addressIndex: wallet.id }); | |
| return await this.userWalletRepository.updateById(wallet.id, { address }, { returning: true }); | |
| const updatedWallet = await this.userWalletRepository.updateById(wallet.id, { address }, { returning: true }); | |
| return { ...updatedWallet, address }; | |
| async `#ensureWalletVia`(repository: UserWalletRepository, userId: string): Promise<WalletInitialized> { | |
| const { wallet } = await repository.getOrCreate({ userId }); | |
| if (wallet.address) return { ...wallet, address: wallet.address }; | |
| const { address } = await this.walletManager.createWallet({ addressIndex: wallet.id }); | |
| const updatedWallet = await repository.updateById(wallet.id, { address }, { returning: true }); | |
| return { ...updatedWallet, address }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts`
around lines 87 - 94, Update `#ensureWalletVia` to perform the wallet address
mutation through the authorization-scoped repository argument by replacing
this.userWalletRepository.updateById with repository.updateById, while
preserving the existing update parameters and returned wallet behavior.
Sources: Path instructions, Learnings
Comment on lines
+22
to
+24
| return wallets | ||
| .filter((wallet): wallet is WalletInitialized => wallet.activatedAt !== null && wallet.address !== null) | ||
| .map(wallet => this.userWalletRepository.toPublic(wallet)); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the actual address value before asserting WalletInitialized.
address !== null allows undefined and empty strings, while the type predicate claims a guaranteed initialized wallet. That can emit invalid API data and pass an unusable owner address downstream.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return wallets | |
| .filter((wallet): wallet is WalletInitialized => wallet.activatedAt !== null && wallet.address !== null) | |
| .map(wallet => this.userWalletRepository.toPublic(wallet)); | |
| return wallets | |
| .filter( | |
| (wallet): wallet is WalletInitialized => | |
| wallet.activatedAt != null && typeof wallet.address === "string" && wallet.address.length > 0 | |
| ) | |
| .map(wallet => this.userWalletRepository.toPublic(wallet)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/billing/services/wallet-reader/wallet-reader.service.ts` around
lines 22 - 24, Update the wallet filter in the wallet-reader mapping flow to
validate that address is present and non-empty before asserting
WalletInitialized; reject undefined, null, and empty or whitespace-only values
while preserving the activatedAt requirement and existing toPublic mapping.
Comment on lines
+19
to
+20
| import { WalletInitialized } from "@src/billing/repositories"; | ||
| import { WalletReaderService } from "@src/billing/services/wallet-reader/wallet-reader.service"; |
Contributor
There was a problem hiding this comment.
📐 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 WalletInitialized. WalletInitialized is only used in a type position here, so import type keeps this aligned with the repo's consistent-type-imports rule.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts`
around lines 19 - 20, Change the WalletInitialized import in the deployment
reader service to a type-only import, while keeping WalletReaderService as a
regular import.
Source: Coding guidelines
| "type": "string", | ||
| "format": "date", | ||
| "default": "2026-07-20", | ||
| "default": "2026-07-24", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove time-dependent OpenAPI defaults. These endpoints state that endDate defaults to the current UTC day, but the committed July 24, 2026 value becomes false on July 25, 2026.
apps/api/swagger/openapi.json#L1790-L1790: omit the generated static default for the dynamicendDate.apps/api/swagger/openapi.json#L1916-L1916: omit the generated static default for the dynamicendDate.
📍 Affects 1 file
apps/api/swagger/openapi.json#L1790-L1790(this comment)apps/api/swagger/openapi.json#L1916-L1916
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/swagger/openapi.json` at line 1790, Remove the generated static
default value for dynamic endDate at apps/api/swagger/openapi.json lines
1790-1790 and 1916-1916, omitting the default property entirely at both sites so
the OpenAPI schema does not contain a date that becomes stale.
- merge duplicated @src/billing/repositories import in deployment-writer - flatten managed-wallet storage-sync effect to an early return + ternary - revert accidental openapi regeneration artifacts (localhost server url, today's date defaults, dropped trailing newline)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Closes CON-745
CON-744 (#3512, #3513, deployed) guarantees that every wallet returned by the public API has an address:
GET /v1/walletsonly returns activated wallets (activation happens strictly after address derivation), andPOST /v1/start-trialresponds only after the address is derived. The loosenullablecontract now only hides bugs and forces null-tolerant handling in API consumers.What
apps/api — declare
address(plusid,userId,createdAt, which are DB-non-null already) as required in the wallet response schema:WalletOutputSchema: drop.nullable()on the four fields;createdAtswitchesz.coerce.date()→z.date()because coercion makes the OpenAPI generator'sisNullable()probe succeed onnull(new Date(null)is valid), which kept a phantomnullable: truein the spec. Response schemas are compile-time/docs only — nothing parses them at runtime.toPublicnow requiresWalletInitialized(moved to the repository fromwallet-reader.service.ts),WalletReaderService.getWalletsfilters with a type predicate (which also silently excludes any pathological activated-but-addressless legacy row instead of 500ing), andWalletInitializerService.#ensureWalletViareturns the narrowed type its JSDoc already promised.createInitializedUserWalletseeder for fixtures that mock the narrowed contract.Generated artifacts — docs snapshot,
swagger/openapi.json, and@akashnetwork/console-api-typesregenerated. Note: the checked-in spec was stale (predates #3513 and the idempotent top-up work), so the regen also drops the long-removed202from/v1/start-trial, adds its409, and picks upidempotencyKeyon checkout — incidental drift, not introduced by this PR.apps/deploy-web — drop the null-address tolerance that CON-744 shipped as a temporary bridge:
useManagedWalletand remove its now-impossible null-address specs.useEnsureTrialStarted:isWalletReadyis now simply "a wallet came back from the API" (!!wallet).Verified end-to-end against a locally booted API: an activated wallet returns a non-null address, a pre-activation wallet is excluded from
GET /v1/wallets, and the served OpenAPI doc marks all four fields required (only the intentionally nullable 3DS fields remain nullable).Summary by CodeRabbit
idempotencyKeysupport for Stripe transaction confirmation requests.409response for/v1/start-trialwhen trial provisioning is already in progress.id,userId,address, andcreatedAtwhere applicable.