Conversation
WalkthroughThis pull request introduces support for an alternate "old" master wallet execution path throughout the billing and payment system. The changes add a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
❌ 3 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
apps/provider-proxy/env/.env (1)
1-1: Verify endpoint change is intentional and environment-appropriate.The REST API node URL changed from an external endpoint (rest-akash.ecostake.com) to a sandbox endpoint (api.sandbox-2.aksh.pw). Additionally, the explicit
:443port is redundant for HTTPS (443 is the default).Verify that this sandbox endpoint is intentional for development/testing and won't inadvertently route production traffic to a sandbox. If this file represents the default configuration, confirm whether production deployments use a different env/.env file or override this value.
Minor: Consider removing the redundant
:443port for cleaner configuration:-REST_API_NODE_URL=https://api.sandbox-2.aksh.pw:443 +REST_API_NODE_URL=https://api.sandbox-2.aksh.pwapps/provider-proxy/src/config/env.config.ts (1)
5-5: Correct.optional()/.default()ordering; consider dropping redundant.optional()Reordering to
.optional().default(3040)fixes the Zod quirk where putting.optional()after.default()prevents the default from being applied when the field is omitted, soPORTwill now correctly fall back to3040when unset.(github.com)Given Zod treats
.default()as “optional on input, defined on output”, the explicit.optional()here is not strictly needed and can be removed for clarity if you want:(totaltypescript.com)- PORT: z.number({ coerce: true }).min(0).optional().default(3040) + PORT: z.number({ coerce: true }).min(0).default(3040)Please double‑check this behavior against your installed Zod version and existing config tests to ensure it matches your expectations for missing vs. explicitly
undefinedPORT values.apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
76-85: Consider centralizing defaulting ofisOldWalletfor inserts
createandgetOrCreateboth hardcodeisOldWallet: false. This is fine, but you might eventually want a single helper (or usetoInput) to keep insert behavior (includingisOldWallet) consistent across the repository.apps/api/src/deployment/services/cached-balance/cached-balance.service.ts (1)
27-33: Align cache key and concurrency with newisOldWalletdimensionYou now pass
{ address, isOldWallet }togetFreshLimits, butbalanceCacheis still keyed only byaddress. If the same address can ever be used with differentisOldWalletvalues, callers could see limits computed for the wrong funding wallet. Consider incorporating the flag into the key, e.g.this.balanceCache.get(${address}:${isOldWallet}).Also, the
if (!cachedBalance) { await this.balancesService.getFreshLimits(...) }pattern is still subject to duplicate upstream calls under concurrent cache misses; if that matters here, you could cache a pending Promise or otherwise synchronize first-use, per previous guidance on similar patterns. Based on learningsapps/api/src/billing/services/provider-cleanup/provider-cleanup.service.ts (1)
57-60: Provider cleanup correctly propagatesisOldWalletto authorization and signingUsing
wallet.isOldWallet ?? falsefor bothexecuteDerivedTxandauthorizeSpendingensures the provider cleanup flow works with the right funding/signing wallet for legacy vs new accounts, including the “not allowed to pay fees” recovery path. Looks good; at most you could factor a localconst useOldWallet = wallet.isOldWallet ?? falsefor readability.Also applies to: 65-75
apps/api/src/billing/services/refill/refill.service.ts (1)
50-59: Old-wallet routing and expiration wiring look correct; consider de-duplicating the flag.Passing
userWallet.isOldWallet ?? falseintoauthorizeSpendingin both refill paths is consistent with the new old-wallet plumbing, and theexpirationyou forward inrefillWalletFeesmatches the trial-window logic above. To reduce repetition and keep the flag in one place, you could pull this into a localconst useOldWallet = !!userWallet.isOldWallet;and reuse it in both calls.Also applies to: 74-80
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (1)
40-51: Consistent old-wallet propagation through signing paths; API remains backward compatible.The new
useOldWalletparameter is threaded correctly: you derive the granter viagetFundingWalletAddress(useOldWallet), sign via the matching derived/funding path, and select the flag fromuserWallet.isOldWallet ?? falseat the top-level call site. Defaults keep existing callers working. If you find yourself using this pattern in more places, consider hoistingconst useOldWallet = !!userWallet.isOldWallet;near the wallet fetch for slightly clearer intent.Also applies to: 56-59, 124-124
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (1)
122-135: Existing async client cache pattern still has a small race window (non-blocking).
#getClientstill does an async “check-then-create” on#clientsByAddress: two concurrent calls for the same address can both seehas(address) === falseand each create aBatchSigningClientService, with the later one winning the map slot. That’s the same pattern called out in prior learnings; it isn’t introduced by this PR but is worth tracking as a future improvement (e.g., caching a pending promise or guarding client creation with a per-address mutex) to avoid redundant client instantiation.apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
68-96: Spending authorization correctly threadsuseOldWalletand expiration through funding and RPC layers.
authorizeSpendingnow:
- Resolves the granter via
getFundingWalletAddress(useOldWallet).- Forwards the optional
expirationto both deployment and fee messages.- Delegates to
authorizeDeploymentSpending/authorizeFeeSpending, which in turn callexecuteFundingTx(..., useOldWallet).This keeps funding wallet selection and grant expiration consistent across both trial and refill flows. Small nit:
limits: limitscan be shortened to justlimitsif you touch this again, but current code is perfectly fine.Also applies to: 98-109, 111-114
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
apps/api/drizzle/0021_brave_warpath.sql(1 hunks)apps/api/drizzle/meta/0021_snapshot.json(1 hunks)apps/api/drizzle/meta/_journal.json(1 hunks)apps/api/src/billing/config/env.config.ts(1 hunks)apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.ts(1 hunks)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts(2 hunks)apps/api/src/billing/services/balances/balances.service.ts(3 hunks)apps/api/src/billing/services/managed-signer/managed-signer.service.ts(2 hunks)apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts(4 hunks)apps/api/src/billing/services/provider-cleanup/provider-cleanup.service.ts(1 hunks)apps/api/src/billing/services/refill/refill.service.ts(2 hunks)apps/api/src/billing/services/tx-manager/tx-manager.service.ts(5 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts(2 hunks)apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts(2 hunks)apps/api/src/deployment/services/cached-balance/cached-balance.service.ts(1 hunks)apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts(1 hunks)apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts(2 hunks)apps/api/src/provider/controllers/jwt-token/jwt-token.controller.ts(1 hunks)apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts(3 hunks)apps/api/src/user/services/stale-anonymous-users-cleaner/stale-anonymous-users-cleaner.service.ts(1 hunks)apps/deploy-web/src/components/get-started/GetStartedStepper.tsx(1 hunks)apps/deploy-web/src/components/onboarding/steps/FreeTrialLandingStep/FreeTrialLandingStep.tsx(1 hunks)apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.tsx(0 hunks)apps/provider-proxy/env/.env(1 hunks)apps/provider-proxy/src/config/env.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.tsx
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-07-30T10:18:24.469Z
Learnt from: baktun14
Repo: akash-network/console PR: 1750
File: apps/api/drizzle/meta/0017_snapshot.json:17-49
Timestamp: 2025-07-30T10:18:24.469Z
Learning: In the Akash Network console billing system, the `user_wallets.user_id` column can be NULL to support anonymous user wallets, allowing users to interact with the system before authentication. Multiple anonymous wallets with NULL user_id values are permitted by design.
Applied to files:
apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.tsapps/api/drizzle/0021_brave_warpath.sql
📚 Learning: 2025-11-12T16:36:02.543Z
Learnt from: baktun14
Repo: akash-network/console PR: 2203
File: apps/deploy-web/src/components/onboarding/steps/PaymentMethodContainer/PaymentMethodContainer.tsx:161-168
Timestamp: 2025-11-12T16:36:02.543Z
Learning: In apps/deploy-web/src/components/onboarding/steps/PaymentMethodContainer/PaymentMethodContainer.tsx, the organization field captured during payment method setup is internal metadata. Errors from stripe.updateCustomerOrganization should be logged to Sentry but not surfaced to users, and the flow should continue even if the organization update fails, as it's non-critical and not something users can fix.
Applied to files:
apps/deploy-web/src/components/onboarding/steps/FreeTrialLandingStep/FreeTrialLandingStep.tsxapps/deploy-web/src/components/get-started/GetStartedStepper.tsx
📚 Learning: 2025-10-15T16:39:55.348Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 2039
File: apps/deploy-web/tests/ui/change-wallets.spec.ts:4-10
Timestamp: 2025-10-15T16:39:55.348Z
Learning: In the Akash Console E2E tests using the context-with-extension fixture, the first wallet is automatically created during fixture setup via `importWalletToLeap` in `apps/deploy-web/tests/ui/fixture/wallet-setup.ts`, so tests that call `frontPage.createWallet()` are creating a second wallet to test wallet switching functionality.
Applied to files:
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/billing/services/tx-manager/tx-manager.service.ts
📚 Learning: 2025-08-12T13:52:38.708Z
Learnt from: stalniy
Repo: akash-network/console PR: 1800
File: apps/deploy-web/next.config.js:163-165
Timestamp: 2025-08-12T13:52:38.708Z
Learning: In the Akash Console project, akashnetwork/env-loader is used at the top of next.config.js files to automatically load environment variables from env/.env files into process.env. SENTRY_ORG and SENTRY_PROJECT are stored as public configuration values in apps/deploy-web/env/.env and are loaded this way, while only SENTRY_AUTH_TOKEN is handled as a GitHub secret in workflows.
Applied to files:
apps/provider-proxy/env/.env
📚 Learning: 2025-08-12T13:52:38.708Z
Learnt from: stalniy
Repo: akash-network/console PR: 1800
File: apps/deploy-web/next.config.js:163-165
Timestamp: 2025-08-12T13:52:38.708Z
Learning: In the Akash Console project, SENTRY_ORG and SENTRY_PROJECT are stored as public configuration values in apps/deploy-web/env/.env file and loaded by akashnetwork/env-loader, not as GitHub secrets. Only SENTRY_AUTH_TOKEN should be handled as a secret.
Applied to files:
apps/provider-proxy/env/.env
📚 Learning: 2025-09-25T20:34:55.117Z
Learnt from: jigar-arc10
Repo: akash-network/console PR: 1970
File: apps/provider-console/src/config/network.config.ts:17-28
Timestamp: 2025-09-25T20:34:55.117Z
Learning: In cosmos-kit integration, nodesUrl and versionUrl should point to Cosmos API endpoints (not console backend API endpoints) as cosmos-kit expects these to be Cosmos REST endpoints for proper functionality.
Applied to files:
apps/provider-proxy/env/.env
📚 Learning: 2025-10-31T11:26:42.138Z
Learnt from: stalniy
Repo: akash-network/console PR: 2138
File: apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts:379-382
Timestamp: 2025-10-31T11:26:42.138Z
Learning: In TypeScript/JavaScript, the pattern of checking a cached value and then performing an async operation to fetch it without proper synchronization is race condition unsafe:
```typescript
private async getAddress() {
if (!this.address) {
this.address = await this.wallet.getFirstAddress();
}
return this.address;
}
```
Multiple concurrent calls can all pass the `if (!this.address)` check before any of them sets the value, leading to duplicate async operations. This should be flagged as a race condition. Proper synchronization (mutex, atomic promise caching, or guaranteed single-threaded execution) is required.
Applied to files:
apps/api/src/billing/services/tx-manager/tx-manager.service.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.ts
🧬 Code graph analysis (4)
apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts (1)
apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.ts (1)
UserWallets(5-17)
apps/api/src/billing/services/balances/balances.service.ts (1)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(17-21)
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (2)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (2)
BatchSigningClientService(57-360)SignAndBroadcastOptions(24-34)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (2)
apps/api/src/billing/services/rpc-message-service/rpc-message.service.ts (1)
SpendingAuthorizationMsgOptions(16-22)apps/api/src/core/types/console.ts (1)
DryRunOptions(1-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (17)
apps/deploy-web/src/components/onboarding/steps/FreeTrialLandingStep/FreeTrialLandingStep.tsx (1)
40-40: Verify that this visibility change is intentional.The removal of the
hiddenclass makes the entire free trial landing step visible. This change appears unrelated to the PR's objective of supporting old wallet mnemonics and could potentially be a leftover debug change.Please confirm:
- Is this visibility change intentional and required for the old wallet support feature?
- If so, what is the connection between unhiding this component and the old wallet functionality?
- Are there any other components or logic that control when this step should be shown?
apps/deploy-web/src/components/get-started/GetStartedStepper.tsx (1)
85-86: Verify PR scope - mismatch between summary and changes.The PR summary describes adding database columns, environment configuration, and service layer changes for old wallet support, but this file contains only minor text reformatting. If this is the complete changeset, the summary is inconsistent. If other files exist, they should be included for comprehensive review.
apps/api/drizzle/meta/0021_snapshot.json (1)
50-56: LGTM - Snapshot correctly reflects schema changes.The snapshot accurately captures the new
is_old_walletcolumn definition with nullable boolean type and default value oftrue, consistent with the schema definition.apps/api/drizzle/0021_brave_warpath.sql (1)
1-1: Migration is safe but verify default value intent.The migration safely adds the column with a default value, ensuring existing rows are populated. However, the default of
truemeans all existing wallets will be marked as "old wallets" - ensure this aligns with the intended behavior as noted in the schema review.apps/api/drizzle/meta/_journal.json (1)
151-158: LGTM - Journal entry properly records migration.The journal entry correctly documents the new migration with appropriate metadata following the established pattern.
apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts (2)
24-24: Type definition correctly allows null.The
isOldWallet: boolean | nulltype accurately reflects that the field can be null due to the LEFT JOIN withUserWallets. This is consistent with the existing pattern wherewalletIdcan also be null (as noted in the BUGALERT comment on line 69).
56-57: LGTM - Query correctly retrieves isOldWallet field.The query appropriately selects the new
isOldWalletfield fromUserWalletsand includes it in the result set.apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.ts (1)
14-14: Clarify the default value oftruefor isOldWallet—schema default conflicts with actual wallet creation logic.The
isOldWalletfield defaults totruein the schema, but wallet creation code inuser-wallet.repository.ts(lines 58, 80) explicitly overrides this withfalsefor all new wallets. This mismatch is confusing and creates risk:
- Direct database operations or future code changes could inadvertently rely on the schema default
- The ?? false fallback pattern throughout the codebase suggests awareness of this inconsistency
- No migration strategy found for existing wallets or legacy wallet handling
- The naming "isOldWallet" is unclear about the actual derivation/signing logic it controls
Either change the schema default to
falseto match actual behavior, or document clearly why the defensivetruedefault exists and ensure all wallet creation paths override it explicitly.apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
55-63: Explicitly settingisOldWallet: falseon insert looks correctNew wallets should not be marked as legacy, so overriding the DB default here is appropriate and consistent with the new column semantics.
apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts (1)
85-88: Consistent propagation ofisOldWalletthrough top‑up flowUsing
deployment.isOldWallet ?? falsefor both cached balance retrieval andexecuteDerivedTxkeeps funding/signing behavior aligned for legacy vs new wallets. This looks correct as long as all deployments for a givenwalletId/address share the sameisOldWalletvalue; if that invariant could be broken, it’d be worth enforcing or asserting it.Also applies to: 149-153
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
28-29: Initializer correctly defaults new wallets to non‑legacy pathPassing
useOldWallet: falseinto bothcreateAndAuthorizeTrialSpendingandcreateWalletmakes it explicit that wallets created via the initializer are backed by the new mnemonic. That seems intentional; if there’s a future need to initialize against the old mnemonic based on config or migration state, this would be the place to parameterize it.Also applies to: 55-56
apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts (1)
18-23: EnsureMemoizekeys includeuseOldWalletfor JWT token cachingPlumbing
useOldWalletthroughgenerateJwtTokenintogetJwtTokenand thengetDerivedWalletlooks good and keeps behavior unchanged when the flag is omitted.Because
getJwtTokenis memoized, please double‑check that theMemoizehelper incorporates all arguments (includinguseOldWallet) into its cache key. If it only keys onwalletId, old/new wallet variants could unintentionally share the same cached derived wallet/JwtTokenManager instance.Also applies to: 34-40, 58-61
apps/api/src/user/services/stale-anonymous-users-cleaner/stale-anonymous-users-cleaner.service.ts (1)
39-43: Revocation flow now correctly distinguishes legacy vs new walletsPassing
wallet.isOldWallet ?? falseintomanagedUserWalletService.revokeAllkeeps stale‑user cleanup aligned with the new legacy wallet handling, so revocations can use the appropriate funding/signing path. Looks consistent with the rest of the PR’s changes.apps/api/src/billing/services/tx-manager/tx-manager.service.ts (2)
90-98: Old vs new wallet selection in funding/derived flows looks coherent.
useOldWalletcleanly switches betweenoldMasterSigningClient/fundingSigningClientandoldWalletFactory/walletFactory, andgetDerivedWalletAddressandsignAndBroadcastWithDerivedWalletboth route through the samegetDerivedWallet/#getClientpath, so address resolution and signing are consistent. This aligns with howManagedSignerServiceand provider JWT generation are now calling into TxManagerService.Also applies to: 100-107, 118-120, 137-139
24-30: Schema enforcement prevents the misconfiguration risk; no action needed.The concern about
OLD_MASTER_WALLET_MNEMONICsilently generating a random wallet is unfounded. The Zod schema explicitly requires it as a non-optional string (z.string()), and it's validated at module load time inenv.config.tsviaenvSchema.parse(process.env). If the env var is missing, the application fails at startup with a clear validation error. If validation passes,config.get()is guaranteed to return a non-undefined value. Additionally, theWalletconstructor only generates a random wallet if the mnemonic is strictlyundefined(not just falsy), so even an empty string would fail when deriving the wallet, not silently succeed.The DI wiring and the uses of
config.get("OLD_MASTER_WALLET_MNEMONIC")at lines 28, 43 (and env.config.ts:11) are safe and correctly mirror the existingFUNDING_WALLET_MNEMONICpattern.apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (2)
42-56: Trial wallet creation now respects old-wallet selection; wiring looks good.
createAndAuthorizeTrialSpendingandcreateWalletnow take{ addressIndex, useOldWallet }and consistently pass the flag down togetDerivedWalletAddress, so derived addresses for legacy wallets will be computed from the old mnemonic while new wallets continue using the default path. This keeps the call surface small and backward compatible thanks to the defaulteduseOldWallet = false.Also applies to: 61-62
116-141: Revocation flow now supports old wallets without breaking existing callers.Adding
useOldWalletas a trailing, defaulted parameter torevokeAlllets you pick the correct funding wallet for granter/grantee checks and for the eventualexecuteFundingTxcall, while existing three-argument call sites continue to behave as before. The dry-run behavior is preserved and now correctly aligned with whichever wallet family the grantee belongs to.
| export const envSchema = z.object({ | ||
| FUNDING_WALLET_MNEMONIC: z.string(), | ||
| FUNDING_WALLET_INDEX: z.number({ coerce: true }), | ||
| OLD_MASTER_WALLET_MNEMONIC: z.string(), |
There was a problem hiding this comment.
🧩 Analysis chain
Verify deployment strategy for required environment variable.
The new OLD_MASTER_WALLET_MNEMONIC is a required field, meaning the application will fail to start if it's not provided. Ensure that:
- The environment variable is documented in setup/deployment guides
- A migration path exists for existing deployments
- Consider whether this should be optional with a default or feature-flag during the transition period
I'll verify the concerns raised in the review comment by examining the codebase and checking for deployment documentation and migration strategies.
🏁 Script executed:
#!/bin/bash
# Inspect the env.config.ts file to understand the full context
echo "=== Examining env.config.ts ==="
cat apps/api/src/billing/config/env.config.ts
echo -e "\n=== Checking for migration scripts or guides ==="
find . -type f \( -name "*migration*" -o -name "*deploy*" -o -name "*setup*" \) \
\( -name "*.md" -o -name "*.txt" -o -name "*.sh" \) | head -20
echo -e "\n=== Searching for OLD_MASTER_WALLET_MNEMONIC references ==="
rg -n "OLD_MASTER_WALLET_MNEMONIC" --type ts --type js
echo -e "\n=== Looking for .env documentation ==="
find . -type f -name ".env*" -o -name "*.env.example" -o -name "*.env.template" | head -10
echo -e "\n=== Checking for deployment or environment documentation ==="
find . -type f -name "*.md" | xargs grep -l "environment\|deployment\|setup" 2>/dev/null | head -10Length of output: 2981
🏁 Script executed:
#!/bin/bash
# Check .env.sample and related template files for documentation
echo "=== Checking .env.sample ==="
cat apps/api/env/.env.sample
echo -e "\n=== Checking .env.local.sample ==="
cat apps/api/env/.env.local.sample
echo -e "\n=== Checking UPGRADE.md for migration instructions ==="
cat apps/api/UPGRADE.md 2>/dev/null || echo "UPGRADE.md not found or empty"
echo -e "\n=== Checking apps/api/README.md for setup guidance ==="
grep -A 5 -B 5 "environment\|OLD_MASTER\|MNEMONIC" apps/api/README.md 2>/dev/null | head -30
echo -e "\n=== Checking for git context or comments near the variable ==="
rg -B 5 -A 2 "OLD_MASTER_WALLET_MNEMONIC" apps/api/src/billing/config/env.config.tsLength of output: 4112
🏁 Script executed:
#!/bin/bash
# Check for conditional logic or fallbacks around OLD_MASTER_WALLET_MNEMONIC
echo "=== Checking tx-manager.service.ts for conditional logic ==="
cat apps/api/src/billing/services/tx-manager/tx-manager.service.ts | head -60
echo -e "\n=== Looking for how envConfig is parsed and handled ==="
rg -n "envSchema\|envConfig" apps/api/src/billing/config/env.config.ts -A 2
echo -e "\n=== Checking if FUNDING_WALLET_MNEMONIC and DERIVATION_WALLET_MNEMONIC are in samples ==="
grep "FUNDING_WALLET\|DERIVATION_WALLET\|OLD_MASTER" apps/api/env/.env.sample
echo -e "\n=== Checking git log for recent changes to env.config.ts ==="
git log --oneline -20 -- apps/api/src/billing/config/env.config.ts 2>/dev/null || echo "Git history not available"Length of output: 4518
Add OLD_MASTER_WALLET_MNEMONIC to environment documentation and provide migration guidance.
The required environment variable is missing from .env.sample and .env.local.sample, and has no deployment migration documentation. Existing deployments will fail on startup:
- Add to
apps/api/env/.env.sample: IncludeOLD_MASTER_WALLET_MNEMONIC=alongside other wallet variables - Add to
apps/api/env/.env.local.sample: Document with example value - Create migration guide: Add section to
apps/api/README.mdor createUPGRADE.mdexplaining the transition for existing deployments - Consider optional transition: Evaluate making this optional with a feature flag or default during the transition period, given the "OLD_MASTER" naming suggests a legacy/migration context
🤖 Prompt for AI Agents
In apps/api/src/billing/config/env.config.ts around line 11, the new required
env var OLD_MASTER_WALLET_MNEMONIC is not documented for deployments; update
apps/api/env/.env.sample to include OLD_MASTER_WALLET_MNEMONIC= alongside other
wallet vars, update apps/api/env/.env.local.sample with an example/mocked
mnemonic value, and add a migration section to apps/api/README.md or create
UPGRADE.md describing how to populate this variable for existing deployments and
suggested rollout options (make it optional behind a feature flag or provide a
default migration path) so existing services won’t fail on startup.
|
|
||
| try { | ||
| await this.managedSignerService.executeDerivedTx(wallet.id, messages); | ||
| await this.managedSignerService.executeDerivedTx(wallet.id, messages, wallet.isOldWallet ?? false); |
There was a problem hiding this comment.
🧩 Analysis chain
Inconsistent default handling - same issue as jwt-token controller.
Using wallet.isOldWallet ?? false defaults to new wallet behavior when the value is nullish, but the database schema defaults is_old_wallet to true. This creates the same inconsistency noted in the jwt-token controller.
Verify whether the fallback should be true to match the schema default, or if false is intentional for this specific flow.
I'll verify this review comment by examining the codebase to understand the inconsistency. Let me search for the schema definition, the related jwt-token controller issue, and usage patterns.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the schema/entity definition for is_old_wallet field
echo "=== Searching for is_old_wallet in schema/entity/migration files ==="
fd -e ts -e js -e sql | xargs grep -l "is_old_wallet\|isOldWallet" | head -20
Length of output: 1048
🏁 Script executed:
#!/bin/bash
# Search for the file mentioned in the review
echo "=== Finding the stale-managed-deployments-cleaner.service.ts file ==="
find . -name "stale-managed-deployments-cleaner.service.ts" -type fLength of output: 183
🏁 Script executed:
#!/bin/bash
# Search for jwt-token controller to compare handling
echo "=== Searching for jwt-token controller ==="
find . -name "*jwt*token*controller*" -type fLength of output: 187
🏁 Script executed:
#!/bin/bash
# Search for all isOldWallet usage in codebase
echo "=== All isOldWallet usages ==="
rg -n "isOldWallet" --type ts -C 2Length of output: 14246
Now let me examine the deployment-setting repository and understand how isOldWallet is fetched to clarify the context:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check deployment-setting repository to see if isOldWallet can actually be null
echo "=== Examining deployment-setting repository ==="
cat apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts | head -70
Length of output: 2587
🏁 Script executed:
#!/bin/bash
# Check the user-wallet schema more carefully
echo "=== Examining user-wallet schema and how new wallets are created ==="
cat apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.tsLength of output: 928
🏁 Script executed:
#!/bin/bash
# Check if there's any database migration that might change the default
echo "=== Check migration file for is_old_wallet ==="
cat apps/api/drizzle/0021_brave_warpath.sqlLength of output: 122
Inconsistent default handling across codebase - issue confirmed and broader than initially scoped.
The review comment is accurate. The database schema defaults is_old_wallet to true (confirmed in schema and migration 0021), but the codebase consistently uses ?? false to handle nullish values across 8+ locations. This creates an inconsistency: when isOldWallet is null (which can occur from LEFT JOINs in deployment-setting repository line 57, typed as boolean | null), the code treats it as false (new wallet) instead of true (old wallet/legacy).
Additionally, new wallets are explicitly created with isOldWallet: false (user-wallet.repository.ts lines 58, 80), establishing an intentional distinction from the schema default. The inconsistency affects not just line 65, but the entire codebase (jwt-token controller, provider-cleanup service, balances service, refill service, etc.).
Action required: Either change all ?? false to ?? true to match the schema default semantic, or document why the schema default should not apply to nullish values. This is a systemic issue, not isolated to this file.
| await this.managedUserWalletService.authorizeSpending( | ||
| { | ||
| address: wallet.address!, | ||
| limits: { | ||
| fees: this.config.FEE_ALLOWANCE_REFILL_AMOUNT | ||
| } | ||
| }, | ||
| wallet.isOldWallet ?? false | ||
| ); |
There was a problem hiding this comment.
Same inconsistent default in error recovery path.
The error recovery path also uses wallet.isOldWallet ?? false, propagating the same default mismatch. Ensure consistency across all usages.
🤖 Prompt for AI Agents
In
apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts
around lines 69 to 77, the error recovery path uses the inconsistent default
expression `wallet.isOldWallet ?? false`; replace that with a consistent boolean
conversion used elsewhere (e.g. `Boolean(wallet.isOldWallet)` or
`!!wallet.isOldWallet`) so the flag is always an explicit true/false value and
matches the other usages across the codebase.
| ); | ||
|
|
||
| await this.managedSignerService.executeDerivedTx(wallet.id, messages); | ||
| await this.managedSignerService.executeDerivedTx(wallet.id, messages, wallet.isOldWallet ?? false); |
There was a problem hiding this comment.
Same inconsistent default in retry logic.
The retry after authorization also uses the inconsistent ?? false fallback.
🤖 Prompt for AI Agents
In
apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts
around line 79, the retry call uses the inconsistent fallback
`wallet.isOldWallet ?? false`; change this to match the same default/fallback
used in the earlier authorization call (i.e. remove the `?? false` and use the
identical expression or explicit default used previously) so both calls use the
same boolean resolution for wallet.isOldWallet.
| ttl: payload.ttl, | ||
| useOldWallet: wallet.isOldWallet ?? false |
There was a problem hiding this comment.
🧩 Analysis chain
Inconsistent default handling between schema and application code.
The code uses wallet.isOldWallet ?? false, defaulting to false when the value is nullish. However, the database schema defaults is_old_wallet to true. This creates an inconsistency:
- Database: defaults to
true(old wallet behavior) - Application code: defaults to
false(new wallet behavior) when null/undefined
This mismatch could lead to unexpected behavior. Ensure the nullish coalescing fallback aligns with the database default, or clarify why they should differ.
Consider:
- useOldWallet: wallet.isOldWallet ?? false
+ useOldWallet: wallet.isOldWallet ?? trueOr make the schema default false if new wallet behavior should be the fallback.
🏁 Script executed:
#!/bin/bash
# Find database schema and migration files related to is_old_wallet
# Search for schema definitions (TypeORM, Prisma, Sequelize, etc.)
echo "=== Searching for is_old_wallet in schema definitions ==="
fd -e ts -e js -e sql -e entity | xargs rg -l "is_old_wallet|isOldWallet" 2>/dev/null | head -20
echo ""
echo "=== Searching in migrations directory ==="
fd -type d -name migrations -o -name migration 2>/dev/null
echo ""
echo "=== Direct search for is_old_wallet with context ==="
rg -i "is_old_wallet" -A 5 -B 5 --type-list | head -5
rg -i "is_old_wallet" -A 5 -B 5 -g '!node_modules' 2>/dev/null | head -80
echo ""
echo "=== Search for wallet entity/schema files ==="
fd -i wallet -e entity.ts -e entity.js -o -name "*wallet*.ts" | grep -E "(entity|schema|model)" | head -20Length of output: 3886
🏁 Script executed:
#!/bin/bash
# Look for TypeORM entity definition or Prisma schema for wallet
echo "=== Searching for Wallet entity or model definition ==="
rg -i "class.*wallet|@Entity.*wallet" -A 20 --type-list 2>/dev/null | head -5
rg -i "class.*wallet|@Entity.*wallet" -A 20 -g '!node_modules' 2>/dev/null | head -100
echo ""
echo "=== Check for Prisma schema ==="
fd schema.prisma -o prisma.schema 2>/dev/null | xargs cat 2>/dev/null
echo ""
echo "=== Search for wallet table/entity files directly ==="
fd -i "wallet" --type f | grep -v node_modules | grep -E "\.(ts|js|sql)$"Length of output: 12217
🏁 Script executed:
#!/bin/bash
# Examine the specific file mentioned in the review comment
echo "=== Reading the target controller file ==="
cat "apps/api/src/provider/controllers/jwt-token/jwt-token.controller.ts" 2>/dev/null | head -50
echo ""
echo "=== Search for wallet model/entity usage in the same file ==="
rg -i "wallet|isOldWallet" "apps/api/src/provider/controllers/jwt-token/jwt-token.controller.ts" -B 3 -A 3 2>/dev/nullLength of output: 2975
🏁 Script executed:
#!/bin/bash
# Find all wallet-related entity definitions
echo "=== Search for @Column definitions with is_old_wallet ==="
rg "@Column.*is_old_wallet|@Column.*isOldWallet" -B 5 -A 2 -g '!node_modules' 2>/dev/null
echo ""
echo "=== Broader search for wallet entity in src directory ==="
fd -path "*/src/*" -name "*wallet*" -type f | head -20
echo ""
echo "=== Search for database entities ==="
fd -path "*/src/*" -name "*.entity.ts" -o -name "*.model.ts" | xargs rg -l "wallet" -i 2>/dev/nullLength of output: 23899
🏁 Script executed:
#!/bin/bash
# Check if is_old_wallet has NOT NULL constraint
echo "=== Check column definition for NOT NULL constraint ==="
cat "apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.ts" 2>/dev/null
echo ""
echo "=== Check migration for NOT NULL ==="
cat "apps/api/drizzle/0021_brave_warpath.sql" 2>/dev/null
echo ""
echo "=== Check drizzle snapshot metadata ==="
rg -A 10 '"is_old_wallet"' "apps/api/drizzle/meta/0021_snapshot.json" 2>/dev/nullLength of output: 1403
🏁 Script executed:
#!/bin/bash
# Search for other uses of isOldWallet in the codebase
echo "=== All usages of isOldWallet ==="
rg -n "isOldWallet|is_old_wallet" -g '!node_modules' -g '!.git' -B 2 -A 2 2>/dev/null | head -120Length of output: 13032
Fix inconsistency between database default (true) and application default (false) for isOldWallet.
The database schema sets is_old_wallet to true by default and allows null values, but the application code uses wallet.isOldWallet ?? false, defaulting to false. This mismatch creates a semantic inconsistency: if the column ever becomes null, the application treats it as a new wallet instead of the old wallet behavior the schema intended.
This pattern appears systematically across 8+ files (stale-anonymous-users-cleaner.service.ts, top-up-managed-deployments.service.ts, stale-managed-deployments-cleaner.service.ts, balances.service.ts, refill.service.ts, provider-cleanup.service.ts, managed-signer.service.ts) and affects critical operations like balance caching and transaction signing.
Update line 29 in the controller:
- useOldWallet: wallet.isOldWallet ?? false
+ useOldWallet: wallet.isOldWallet ?? trueAnd apply the same fix throughout the codebase to align application defaults with schema intent.
📝 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.
| ttl: payload.ttl, | |
| useOldWallet: wallet.isOldWallet ?? false | |
| ttl: payload.ttl, | |
| useOldWallet: wallet.isOldWallet ?? true |
🤖 Prompt for AI Agents
In apps/api/src/provider/controllers/jwt-token/jwt-token.controller.ts around
lines 28-29, the code defaults isOldWallet to false (wallet.isOldWallet ??
false) while the DB default is true; change the application default to true (use
wallet.isOldWallet ?? true) so null/undefined matches the schema intent; then
find and update the same pattern across the other listed files
(stale-anonymous-users-cleaner.service.ts,
top-up-managed-deployments.service.ts,
stale-managed-deployments-cleaner.service.ts, balances.service.ts,
refill.service.ts, provider-cleanup.service.ts, managed-signer.service.ts and
any other occurrences of "isOldWallet ?? false") to default to true, run tests
and linting to ensure no regressions.
apps/deploy-web/src/components/get-started/GetStartedStepper.tsx
Outdated
Show resolved
Hide resolved
| async getFullBalance(address: string): Promise<GetBalancesResponseOutput> { | ||
| const [balanceData, deploymentEscrowBalance] = await Promise.all([this.getFreshLimits({ address }), this.calculateDeploymentEscrowBalance(address)]); | ||
| const [balanceData, deploymentEscrowBalance] = await Promise.all([ | ||
| this.getFreshLimits({ address, isOldWallet: false }), |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/src/billing/services/balances/balances.service.ts (1)
50-68: Funding wallet selection now respectsisOldWallet; consider small API tidy‑upUsing
userWallet.isOldWallet ?? falsewhen callingtxManagerService.getFundingWalletAddressin bothretrieveAndCalcFeeLimitandretrieveDeploymentLimitcorrectly routes fee/deployment limits through the appropriate legacy/new funding wallet. That fixes the earlier mismatch between limits and wallet type.Two small optional cleanups:
- Both methods duplicate the
getFundingWalletAddress(userWallet.isOldWallet ?? false)call; a tiny helper (or precomputedfundingWalletAddress) would reduce the chance of these drifting apart later.- If
retrieveDeploymentLimitis not intended to be called from outside this service, keeping itprivatewould keep the public surface smaller.Neither is blocking, but they’d tighten this area up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/billing/controllers/wallet/wallet.controller.ts(1 hunks)apps/api/src/billing/services/balances/balances.service.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-31T11:26:42.138Z
Learnt from: stalniy
Repo: akash-network/console PR: 2138
File: apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts:379-382
Timestamp: 2025-10-31T11:26:42.138Z
Learning: In TypeScript/JavaScript, the pattern of checking a cached value and then performing an async operation to fetch it without proper synchronization is race condition unsafe:
```typescript
private async getAddress() {
if (!this.address) {
this.address = await this.wallet.getFirstAddress();
}
return this.address;
}
```
Multiple concurrent calls can all pass the `if (!this.address)` check before any of them sets the value, leading to duplicate async operations. This should be flagged as a race condition. Proper synchronization (mutex, atomic promise caching, or guaranteed single-threaded execution) is required.
Applied to files:
apps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/billing/services/balances/balances.service.ts
🧬 Code graph analysis (2)
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
apps/api/src/auth/services/auth.service.ts (4)
ability(25-27)ability(29-31)currentUser(12-14)currentUser(16-23)
apps/api/src/billing/services/balances/balances.service.ts (1)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(17-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
98-111: PropagatingisOldWalletinto balance lookup fixes the legacy‑wallet issue; confirm override semanticsThe new
isOldWalletflag, sourced fromuserWallet.isOldWallet ?? falsewhen noaddressis provided, correctly threads legacy/new wallet selection intogetFullBalance. One thing to double‑check: when an explicitaddressis passed,isOldWalletalways staysfalse. If callers can pass old‑wallet addresses via this override, those balances will be computed using the “new” funding wallet path. If the override is only ever used for new wallets or non‑user addresses, this is fine—otherwise consider looking up the wallet by address here (or exposing anisOldWalletquery param) to keep behavior consistent.apps/api/src/billing/services/balances/balances.service.ts (1)
95-99:getFullBalancenow takesisOldWallet, resolving the legacy‑wallet limit bug—verify callers and memoizationExtending
getFullBalanceto(address, isOldWallet = false)and passing{ address, isOldWallet }intogetFreshLimitsaligns the balance endpoint with the legacy/new wallet logic you added elsewhere and addresses the earlier misreporting concern.Please just confirm:
- All external callers that work with legacy wallets now pass
isOldWallet: true(e.g., controller, cached balance service, any background jobs).@Memoize’s keying takes bothaddressandisOldWalletinto account, so legacy and new wallets don’t share a cached entry for the same address.If both hold, this path looks solid.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/deploy-web/src/components/user/payment/PaymentForm.tsx (1)
30-31: Optionally trim coupon before checking emptinessRight now
disabledCoupononly checks!coupon, so a whitespace‑only string will enable the Claim button. If you want slightly stricter UX, consider trimming before the emptiness check (or doing it in the parent handler) so only non‑blank codes enable the button.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.ts(0 hunks)apps/deploy-web/src/components/user/payment/PaymentForm.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-18T07:44:29.079Z
Learnt from: baktun14
Repo: akash-network/console PR: 1933
File: apps/deploy-web/src/pages/payment.tsx:92-116
Timestamp: 2025-09-18T07:44:29.079Z
Learning: In the payment system, discount calculation is not applied during payment processing. Instead, when users claim coupons/discounts, the discount amount is applied directly to their account balance. Therefore, the payment flow should charge parseFloat(amount) as entered, not a discounted amount.
Applied to files:
apps/deploy-web/src/components/user/payment/PaymentForm.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/deploy-web/src/components/user/payment/PaymentForm.tsx (1)
30-31: Disable logic for Pay and coupon actions looks correct and consistent with payment semanticsThe
disabledanddisabledCouponconditions are well chosen: they gate Pay on a positive numeric amount, a selected payment method, no validation error, and not being mid‑processing, and gate coupon claims on a non‑empty code and not being mid‑processing/claim. This aligns with the payment flow expectation that we always chargeparseFloat(amount)as entered and treat coupons as separate balance credits rather than discounts at charge time. Based on learnings
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
apps/api/env/.env.unit.test (1)
3-3: Reorder key to satisfy linting rules.The dotenv-linter flags
OLD_MASTER_WALLET_MNEMONICas out of order—it should appear beforePOSTGRES_DB_URI(line 2). Consider moving the variable to maintain consistent key ordering across environment files.Apply this diff to reorder the key:
CHAIN_INDEXER_POSTGRES_DB_URI=postgres://postgres:password@localhost:5432/console-akash-sandbox +OLD_MASTER_WALLET_MNEMONIC="motion isolate mother convince snack twenty tumble boost elbow bundle modify balcony" POSTGRES_DB_URI=postgres://postgres:password@localhost:5432/console-users -OLD_MASTER_WALLET_MNEMONIC="motion isolate mother convince snack twenty tumble boost elbow bundle modify balcony" FUNDING_WALLET_MNEMONIC="motion isolate mother convince snack twenty tumble boost elbow bundle modify balcony"apps/api/env/.env.sample (1)
7-7: Reorder key to satisfy linting rules.The dotenv-linter flags
OLD_MASTER_WALLET_MNEMONICas out of order—it should appear beforePOSTGRES_DB_URI(line 2). Consider moving the variable to maintain consistent key ordering across environment files.Apply this diff to reorder the key:
# Secrets POSTGRES_DB_URI= CHAIN_INDEXER_POSTGRES_DB_URI= GITHUB_PAT= ANONYMOUS_USER_TOKEN_SECRET= HealthChecks_SyncAKTMarketData= +OLD_MASTER_WALLET_MNEMONIC= FUNDING_WALLET_MNEMONIC= DERIVATION_WALLET_MNEMONIC= SECRET_TOKEN= STRIPE_SECRET_KEY= STRIPE_PRODUCT_ID= -OLD_MASTER_WALLET_MNEMONIC=apps/api/test/seeders/user-wallet.seeder.ts (1)
7-28: MakeisOldWalletparameterizable for test flexibility.The
isOldWalletfield is hardcoded tofalse, preventing tests from easily creating "old wallet" scenarios. For comprehensive test coverage of both wallet types, consider accepting it as an optional parameter like the other fields.Apply this diff to make the field parameterizable:
static create({ id = faker.number.int({ min: 0, max: 1000 }), userId = faker.string.uuid(), address = createAkashAddress(), deploymentAllowance = faker.number.float({ min: 0, max: 1000000 }), feeAllowance = faker.number.float({ min: 0, max: 1000000 }), isTrialing = faker.helpers.arrayElement([true, false]), + isOldWallet = false, createdAt = faker.date.past(), updatedAt = faker.date.past() }: Partial<UserWalletOutput> = {}): UserWalletOutput { return { id, userId, address, deploymentAllowance, feeAllowance, isTrialing, creditAmount: deploymentAllowance, createdAt, updatedAt, - isOldWallet: false + isOldWallet }; }apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts (1)
206-206: Consider adding test coverage for the old wallet path.The test correctly verifies the call with
useOldWalletset tofalse. However, there's no test case that exercises the behavior when this flag istrue(the old wallet path). Given the PR's objective to support old mnemonics, it would be valuable to add a test that verifies the system works correctly with the old wallet flag enabled.Consider adding a test case similar to this:
it("executes transaction successfully with old wallet flag", async () => { const wallet = UserWalletSeeder.create({ userId: "user-123", feeAllowance: 100, deploymentAllowance: 100, isOldWallet: true // if this field exists in the wallet model }); const user = UserSeeder.create({ userId: "user-123" }); const messages: EncodeObject[] = [ { typeUrl: MsgCreateLease.$type, value: MsgCreateLease.fromPartial({ bidId: { dseq: Long.fromNumber(123) } }) } ]; const txResult = { code: 0, hash: "tx-hash-123", rawLog: "success", transactionHash: "tx-hash-123" }; const { service, txManagerService, balancesService } = setup({ findOneByUserId: jest.fn().mockResolvedValue(wallet), findById: jest.fn().mockResolvedValue(user), signAndBroadcastWithDerivedWallet: jest.fn().mockResolvedValue(txResult), refreshUserWalletLimits: jest.fn().mockResolvedValue(undefined) }); const result = await service.executeDerivedDecodedTxByUserId("user-123", messages); expect(txManagerService.signAndBroadcastWithDerivedWallet).toHaveBeenCalledWith( wallet.id, messages, { fee: { granter: expect.any(String) } }, true // old wallet flag ); expect(result).toEqual({ code: 0, hash: "tx-hash-123", transactionHash: "tx-hash-123", rawLog: "success" }); });apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts (1)
282-321: Consider adding test coverage for the old wallet path.Similar to the first test case, this test correctly verifies the behavior with
useOldWalletset tofalse. However, there's no test that exercises the scenario where wallets use the old derivation path (useOldWallet: true). This is particularly important for the batch operation scenario where multiple deposits are consolidated into a single transaction.Consider adding a test case like:
it("should top up draining deployments for the same owner using old wallet in the same tx", async () => { const owner = createAkashAddress(); const walletId = faker.number.int({ min: 1000000, max: 9999999 }); const deployments = [ AutoTopUpDeploymentSeeder.create({ address: owner, walletId, isOldWallet: true }), AutoTopUpDeploymentSeeder.create({ address: owner, walletId, isOldWallet: true }) ]; // ... rest of test setup ... await service.topUpDeployments({ concurrency: 10, dryRun: false }); expect(managedSignerService.executeDerivedTx).toHaveBeenCalledTimes(1); expect(managedSignerService.executeDerivedTx).toHaveBeenCalledWith( walletId, [ /* deposit messages */ ], true // old wallet flag ); });apps/api/src/billing/services/refill/refill.service.spec.ts (1)
28-35: ConfirmisOldWallet/ old-wallet semantics and avoid over-hardcodingfalsein specsThe updates correctly reflect the new
authorizeSpending(payload, flag)signature and ensure the second argument is being passed. Two follow-ups to consider:
- With
UserWalletnow carrying an old/new wallet flag, please double-check that always expectingfalsehere matches the intended behavior ofRefillService.topUpWalletfor both existing and newly initialized wallets. If the service is supposed to honor a per-walletisOldWalletflag, these expectations might eventually want to derive the boolean from the wallet fixture (or add separate coverage for thetruepath) instead of hard-codingfalse.- In the race-condition test, you assert that
authorizeSpendingwas called with each of the two payloads, but not that it was called exactly twice. If you care about guarding against duplicate authorizations, consider addingexpect(managedUserWalletService.authorizeSpending).toHaveBeenCalledTimes(2)alongside the existingtoHaveBeenCalledWithchecks.Also applies to: 53-60, 82-95
apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts (1)
201-204: Consider adding explicit tests for the old‑wallet pathYou’ve introduced
oldMasterWallet,oldMasterSigningClient, andoldWalletFactoryand exposed them fromsetup, but none of the specs currently assert how TxManagerService behaves when the “old wallet” path is used (e.g.,useOldWallet === true).For better coverage of the new behavior:
- Add tests that call:
signAndBroadcastWithFundingWallet(messages, true)and assertoldMasterSigningClient.signAndBroadcastis used (and the regular funding client is not).getFundingWalletAddress(true)and assertoldMasterWallet.getFirstAddressis called.- Any derived‑wallet flows that should switch to
oldWalletFactorywhenuseOldWalletis set.- Optionally make
oldWalletFactoryreturn a distinct wallet mock fromwalletFactoryso assertions can clearly distinguish which factory was used.This will ensure the new “old wallet” wiring is actually exercised and will catch regressions if routing logic changes later.
Also applies to: 213-216, 226-229, 250-255
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/api/env/.env.sample(1 hunks)apps/api/env/.env.unit.test(1 hunks)apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts(1 hunks)apps/api/src/billing/services/refill/refill.service.spec.ts(3 hunks)apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts(3 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(2 hunks)apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.ts(1 hunks)apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts(2 hunks)apps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.ts(1 hunks)apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts(1 hunks)apps/api/test/seeders/auto-top-up-deployment.seeder.ts(1 hunks)apps/api/test/seeders/user-wallet.seeder.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-15T16:39:55.348Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 2039
File: apps/deploy-web/tests/ui/change-wallets.spec.ts:4-10
Timestamp: 2025-10-15T16:39:55.348Z
Learning: In the Akash Console E2E tests using the context-with-extension fixture, the first wallet is automatically created during fixture setup via `importWalletToLeap` in `apps/deploy-web/tests/ui/fixture/wallet-setup.ts`, so tests that call `frontPage.createWallet()` are creating a second wallet to test wallet switching functionality.
Applied to files:
apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/test/seeders/auto-top-up-deployment.seeder.tsapps/api/src/billing/services/refill/refill.service.spec.tsapps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts
📚 Learning: 2025-10-31T11:26:42.138Z
Learnt from: stalniy
Repo: akash-network/console PR: 2138
File: apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts:379-382
Timestamp: 2025-10-31T11:26:42.138Z
Learning: In TypeScript/JavaScript, the pattern of checking a cached value and then performing an async operation to fetch it without proper synchronization is race condition unsafe:
```typescript
private async getAddress() {
if (!this.address) {
this.address = await this.wallet.getFirstAddress();
}
return this.address;
}
```
Multiple concurrent calls can all pass the `if (!this.address)` check before any of them sets the value, leading to duplicate async operations. This should be flagged as a race condition. Proper synchronization (mutex, atomic promise caching, or guaranteed single-threaded execution) is required.
Applied to files:
apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.ts
🧬 Code graph analysis (2)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
createAndAuthorizeTrialSpending(42-59)
apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts (3)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)apps/api/test/seeders/akash-address.seeder.ts (1)
createAkashAddress(4-12)apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
BatchSigningClientService(57-360)
🪛 dotenv-linter (4.0.0)
apps/api/env/.env.unit.test
[warning] 3-3: [UnorderedKey] The OLD_MASTER_WALLET_MNEMONIC key should go before the POSTGRES_DB_URI key
(UnorderedKey)
apps/api/env/.env.sample
[warning] 7-7: [UnorderedKey] The OLD_MASTER_WALLET_MNEMONIC key should go before the POSTGRES_DB_URI key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
39-39: LGTM! Test expectations correctly updated for new method signature.The addition of
useOldWallet: falseto the test assertions correctly reflects the updatedcreateAndAuthorizeTrialSpendingmethod signature. Usingfalsefor new trial wallets is appropriate since trials should use the new wallet derivation path.Also applies to: 84-84
apps/api/test/seeders/auto-top-up-deployment.seeder.ts (2)
18-20: LGTM: Delegation pattern is correct.The
createManymethod correctly delegates tocreatewith the same overrides, ensuring consistent behavior.
13-13: The test seeder default is correct as written.The production schema defines
is_old_walletwith a default oftrue, but this is a baseline for existing records. When new wallets are created in production, the code explicitly setsisOldWallet: false. The test seeder correctly matches the actual production behavior—not the unused schema default—by defaulting tofalse. No changes needed.Likely an incorrect or invalid review comment.
apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts (1)
110-132: Deposit structure in test is correct and matches implementation.The nested
sourcesfield within thedepositobject is the correct structure. The implementation inrpc-message.service.ts(lines 178-191 and 204-217) uses the same nested structure:deposit { amount: {...}, sources: [Source.grant] }. This is also consistent across other parts of the codebase (deploy-web and functional tests).Likely an incorrect or invalid review comment.
apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts (1)
236-245: Constructor wiring of old‑wallet dependencies looks correctThe TxManagerService instantiation matches the updated constructor signature and passes the new
oldMasterWallet,oldMasterSigningClient, andoldWalletFactoryin a consistent order. No issues from the DI/wiring perspective.apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.ts (1)
29-43: Fix race condition in cache check pattern and add test coverage for old wallet scenario.The method signature and parameter handling are correct—
isOldWalletdefaults tofalseand is properly passed togetFreshLimits. However, the caching logic at lines 28-33 has a race condition: multiple concurrent calls can all pass theif (!cachedBalance)check before the first one sets the cache, resulting in duplicategetFreshLimitscalls. Additionally, there's no test coverage for theisOldWallet: truescenario—all five existing tests use the default value or omit the parameter.Recommended fixes:
- Synchronize cache access using a Map of promises or mutex to ensure only one
getFreshLimitscall per address even under concurrent load- Add a test case for
isOldWallet: trueto verify both wallet types are properly supportedLikely an incorrect or invalid review comment.
| ttl: payload.ttl, | ||
| useOldWallet: false | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the source of useOldWallet and add test coverage for old wallet scenario.
The test hardcodes useOldWallet: false in the expectation, but doesn't verify:
- Where the controller obtains this value (presumably from
wallet.is_old_walletin the database) - Whether the controller correctly maps the database field to the service parameter
- The behavior when a user has an old wallet (
is_old_wallet: true)
This creates a gap between the test and the actual implementation flow: the test doesn't validate that the controller properly reads is_old_wallet from the wallet retrieved from the repository and passes it to the service.
Additionally, the AI summary states the database column defaults to true, but the test asserts false, suggesting a potential inconsistency.
Consider:
- Updating the test setup to explicitly set
is_old_walleton the mock wallet - Adding assertions to verify the controller reads the value from the wallet
- Adding a test case for when
wallet.is_old_walletistrueto ensure proper handling of old wallets
it("creates JWT token successfully when user has wallet", async () => {
const user = UserSeeder.create();
const { controller, authService, userWalletRepository, providerJwtTokenService, jwtToken, wallet } = setup({ user });
authService.currentUser = user;
userWalletRepository.accessibleBy.mockReturnThis();
userWalletRepository.findOneByUserId.mockResolvedValue(wallet);
providerJwtTokenService.generateJwtToken.mockResolvedValue(Ok(jwtToken));
const payload = createPayload();
const result = await controller.createJwtToken(payload);
expect(result.unwrap()).toEqual({ token: jwtToken });
expect(userWalletRepository.accessibleBy).toHaveBeenCalledWith(authService.ability, "sign");
expect(userWalletRepository.findOneByUserId).toHaveBeenCalledWith(user.id);
expect(providerJwtTokenService.generateJwtToken).toHaveBeenCalledWith({
walletId: wallet.id,
leases: payload.leases,
ttl: payload.ttl,
- useOldWallet: false
+ useOldWallet: wallet.isOldWallet ?? false
});
});
+
+ it("creates JWT token with old wallet when user has old wallet", async () => {
+ const user = UserSeeder.create();
+ const { controller, authService, userWalletRepository, providerJwtTokenService, jwtToken } = setup({ user });
+ const oldWallet = UserWalletSeeder.create({ userId: user.id, isOldWallet: true });
+
+ authService.currentUser = user;
+ userWalletRepository.accessibleBy.mockReturnThis();
+ userWalletRepository.findOneByUserId.mockResolvedValue(oldWallet);
+ providerJwtTokenService.generateJwtToken.mockResolvedValue(Ok(jwtToken));
+
+ const payload = createPayload();
+ const result = await controller.createJwtToken(payload);
+
+ expect(result.unwrap()).toEqual({ token: jwtToken });
+ expect(providerJwtTokenService.generateJwtToken).toHaveBeenCalledWith({
+ walletId: oldWallet.id,
+ leases: payload.leases,
+ ttl: payload.ttl,
+ useOldWallet: true
+ });
+ });Run this script to verify how the controller maps is_old_wallet from the database:
🏁 Script executed:
#!/bin/bash
# Description: Verify the controller implementation reads is_old_wallet from wallet
# Find the jwt-token controller implementation
ast-grep --pattern $'async createJwtToken($$$) {
$$$
}'
# Check UserWallet entity/model definition for is_old_wallet field
rg -nP 'class UserWallet|interface UserWallet' --type=ts -A20
# Check if UserWalletSeeder supports isOldWallet parameter
ast-grep --pattern 'class UserWalletSeeder {
$$$
create($$$) {
$$$
}
$$$
}'Length of output: 5289
I need to gather more information about the controller implementation and the UserWalletOutput interface definition.The web search returned generic TypeScript interface information. I need to search for code-specific information about the jwt-token controller and UserWalletOutput directly in the codebase.
🏁 Script executed:
#!/bin/bash
# Description: Find UserWalletOutput interface definition and jwt-token controller implementation
# Find UserWalletOutput interface definition
rg -nP 'interface UserWalletOutput|type UserWalletOutput' --type=ts -A10 -B2
# Find jwt-token controller implementation
rg -nP 'async createJwtToken' --type=ts -A15 -B2
# Find what fields UserWallet entity has
rg -nP 'is_old_wallet|isOldWallet' --type=ts -C3Length of output: 32192
I need to view the actual test file to see the current state and verify the exact code at lines 34-36.
🏁 Script executed:
#!/bin/bash
# Description: View the actual test file at the specified lines
cat apps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.ts | head -50 | tail -30Length of output: 1287
Update UserWalletSeeder to accept isOldWallet parameter before adding old wallet test case.
The test currently expects useOldWallet: false, which is correct given that UserWalletSeeder.create() hardcodes isOldWallet: false. However, the controller correctly reads wallet.isOldWallet ?? false, so the test should verify both scenarios. The suggested fix cannot be implemented as written because UserWalletSeeder.create() does not accept an isOldWallet parameter (see apps/api/test/seeders/user-wallet.seeder.ts lines 7–16).
First, update the seeder to expose this parameter:
static create({
id = faker.number.int({ min: 0, max: 1000 }),
userId = faker.string.uuid(),
address = createAkashAddress(),
deploymentAllowance = faker.number.float({ min: 0, max: 1000000 }),
feeAllowance = faker.number.float({ min: 0, max: 1000000 }),
isTrialing = faker.helpers.arrayElement([true, false]),
+ isOldWallet = false,
createdAt = faker.date.past(),
updatedAt = faker.date.past()
}: Partial<UserWalletOutput> = {}): UserWalletOutput {
return {
id,
userId,
address,
deploymentAllowance,
feeAllowance,
isTrialing,
creditAmount: deploymentAllowance,
createdAt,
updatedAt,
+ isOldWalletThen add the old wallet test case to verify the controller passes the wallet's isOldWallet value correctly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.ts
around lines 34–36 and in apps/api/test/seeders/user-wallet.seeder.ts around
lines 7–16, the test assumes UserWalletSeeder.create() cannot set isOldWallet;
update the seeder to accept an optional isOldWallet boolean parameter (default
false) and set the created wallet's isOldWallet accordingly, then update
existing tests to call UserWalletSeeder.create(..., isOldWallet:false) and add a
new test case that creates a wallet with isOldWallet:true and asserts the
controller passes wallet.isOldWallet (or wallet.isOldWallet ?? false) into the
token generation payload so both old and non-old wallet flows are covered.
|
|
||
| expect(result.unwrap()).toEqual(jwtTokenValue); | ||
| expect(txManagerService.getDerivedWallet).toHaveBeenCalledWith(walletId); | ||
| expect(txManagerService.getDerivedWallet).toHaveBeenCalledWith(walletId, false); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the default value and add test coverage for the old wallet path.
The test asserts getDerivedWallet is called with false for the useOldWallet parameter, but the AI summary indicates the database column is_old_wallet defaults to true. This creates ambiguity about the intended default behavior:
- Should the service default to
false(new wallet) when no value is provided, overriding the database default? - Or should the database default of
true(old wallet) be respected?
Additionally, there's no test coverage for the useOldWallet: true scenario, which is the primary purpose of this PR.
Consider:
- Adding a test case that explicitly verifies behavior when
useOldWallet: trueto ensure the old wallet path works correctly - Clarifying the relationship between the database default and the service-level default
- Documenting the intended default behavior in comments or test names
it("should generate a JWT token successfully", async () => {
const { providerJwtTokenService, walletId, accessRules, jwtTokenValue, jwtToken, txManagerService, jwtModule, address, wallet } = setup();
const result = await providerJwtTokenService.generateJwtToken({ walletId, leases: accessRules });
expect(result.unwrap()).toEqual(jwtTokenValue);
expect(txManagerService.getDerivedWallet).toHaveBeenCalledWith(walletId, false);
expect(jwtModule.JwtTokenManager).toHaveBeenCalledWith(wallet);
expect(jwtToken.generateToken).toHaveBeenCalledWith({
version: "v1",
exp: expect.any(Number),
nbf: expect.any(Number),
iat: expect.any(Number),
iss: address,
jti: expect.any(String),
leases: accessRules
});
});
+
+ it("should generate a JWT token with old wallet when useOldWallet is true", async () => {
+ const { providerJwtTokenService, walletId, accessRules, jwtTokenValue, jwtToken, txManagerService, jwtModule, address, wallet } = setup();
+
+ const result = await providerJwtTokenService.generateJwtToken({ walletId, leases: accessRules, useOldWallet: true });
+
+ expect(result.unwrap()).toEqual(jwtTokenValue);
+ expect(txManagerService.getDerivedWallet).toHaveBeenCalledWith(walletId, true);
+ expect(jwtModule.JwtTokenManager).toHaveBeenCalledWith(wallet);
+ });Run this script to verify how useOldWallet defaults are handled in the implementation:
🏁 Script executed:
#!/bin/bash
# Description: Check how useOldWallet defaults are set in the service implementation
# Search for the generateJwtToken implementation and default parameter values
ast-grep --pattern $'generateJwtToken($$$) {
$$$
}'
# Search for where is_old_wallet is used in repositories
rg -nP 'is_old_wallet|isOldWallet|useOldWallet' --type=ts -A3 -B3Length of output: 46514
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if there's a test for useOldWallet: true in the spec file
cat apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts
Length of output: 4112
Add test coverage for the useOldWallet: true scenario.
The test correctly asserts getDerivedWallet is called with false because the service defaults to new wallets (useOldWallet = false). The database schema default of true applies only to existing wallets for backward compatibility—new wallets are explicitly created with isOldWallet: false in the repository.
However, this PR adds support for useOldWallet: true, and there is no test case verifying that path works correctly. Add a test case to ensure the old wallet scenario functions as intended:
it("should generate a JWT token successfully", async () => {
const { providerJwtTokenService, walletId, accessRules, jwtTokenValue, jwtToken, txManagerService, jwtModule, address, wallet } = setup();
const result = await providerJwtTokenService.generateJwtToken({ walletId, leases: accessRules });
expect(result.unwrap()).toEqual(jwtTokenValue);
expect(txManagerService.getDerivedWallet).toHaveBeenCalledWith(walletId, false);
expect(jwtModule.JwtTokenManager).toHaveBeenCalledWith(wallet);
expect(jwtToken.generateToken).toHaveBeenCalledWith({
version: "v1",
exp: expect.any(Number),
nbf: expect.any(Number),
iat: expect.any(Number),
iss: address,
jti: expect.any(String),
leases: accessRules
});
});
+
+ it("should generate a JWT token with old wallet when useOldWallet is true", async () => {
+ const { providerJwtTokenService, walletId, accessRules, jwtTokenValue, jwtToken, txManagerService, jwtModule, address, wallet } = setup();
+
+ const result = await providerJwtTokenService.generateJwtToken({ walletId, leases: accessRules, useOldWallet: true });
+
+ expect(result.unwrap()).toEqual(jwtTokenValue);
+ expect(txManagerService.getDerivedWallet).toHaveBeenCalledWith(walletId, true);
+ expect(jwtModule.JwtTokenManager).toHaveBeenCalledWith(wallet);
+ expect(jwtToken.generateToken).toHaveBeenCalledWith({
+ version: "v1",
+ exp: expect.any(Number),
+ nbf: expect.any(Number),
+ iat: expect.any(Number),
+ iss: address,
+ jti: expect.any(String),
+ leases: accessRules
+ });
+ });📝 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.
| expect(txManagerService.getDerivedWallet).toHaveBeenCalledWith(walletId, false); | |
| expect(txManagerService.getDerivedWallet).toHaveBeenCalledWith(walletId, false); | |
| }); | |
| it("should generate a JWT token with old wallet when useOldWallet is true", async () => { | |
| const { providerJwtTokenService, walletId, accessRules, jwtTokenValue, jwtToken, txManagerService, jwtModule, address, wallet } = setup(); | |
| const result = await providerJwtTokenService.generateJwtToken({ walletId, leases: accessRules, useOldWallet: true }); | |
| expect(result.unwrap()).toEqual(jwtTokenValue); | |
| expect(txManagerService.getDerivedWallet).toHaveBeenCalledWith(walletId, true); | |
| expect(jwtModule.JwtTokenManager).toHaveBeenCalledWith(wallet); | |
| expect(jwtToken.generateToken).toHaveBeenCalledWith({ | |
| version: "v1", | |
| exp: expect.any(Number), | |
| nbf: expect.any(Number), | |
| iat: expect.any(Number), | |
| iss: address, | |
| jti: expect.any(String), | |
| leases: accessRules | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.spec.tsx (1)
8-19: Verify test coverage after removing "no managed wallet" test case.The enriched summary indicates a test asserting no rendering when the user has no managed wallet was removed. Additionally, given that this PR introduces old wallet support (
isOldWalletflag), consider whether this component's behavior with old vs. new wallets should be tested here.Please confirm:
- The removed test case is no longer relevant due to changed component behavior
- Whether old wallet functionality impacts this button's behavior and requires test coverage
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.spec.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-15T16:39:55.348Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 2039
File: apps/deploy-web/tests/ui/change-wallets.spec.ts:4-10
Timestamp: 2025-10-15T16:39:55.348Z
Learning: In the Akash Console E2E tests using the context-with-extension fixture, the first wallet is automatically created during fixture setup via `importWalletToLeap` in `apps/deploy-web/tests/ui/fixture/wallet-setup.ts`, so tests that call `frontPage.createWallet()` are creating a second wallet to test wallet switching functionality.
Applied to files:
apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.spec.tsx
📚 Learning: 2025-07-11T10:46:43.711Z
Learnt from: stalniy
Repo: akash-network/console PR: 1660
File: apps/deploy-web/src/components/alerts/DeploymentAlertsContainer/DeploymentAlertsContainer.spec.tsx:54-56
Timestamp: 2025-07-11T10:46:43.711Z
Learning: In apps/{deploy-web,provider-console}/**/*.spec.tsx files: Use `getBy` methods instead of `queryBy` methods when testing element presence with `toBeInTheDocument()` because `getBy` throws an error and shows DOM state when element is not found, providing better debugging information than `queryBy` which returns null.
Applied to files:
apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.spec.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.spec.tsx (2)
9-13: LGTM! Test correctly verifies enabled state.The test appropriately checks that the button is enabled when the blockchain is available. The use of
getByTextfollows best practices for better error messages during test failures. Based on learnings
15-19: LGTM! Test correctly verifies disabled state.The test properly validates that the button is disabled when the blockchain is unavailable, providing good coverage of the component's behavior under different network conditions. Based on learnings
…per.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Maxime Beauchamp <15185355+baktun14@users.noreply.github.com>
baktun14
force-pushed
the
feat/support-old-wallet
branch
from
7036e3c to
0933de0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts (1)
19-25: Type safety inconsistency with nullable fields.The
isOldWalletfield is correctly typed asboolean | nullto account for the LEFT JOIN on lines 60-61, which can result in null values when no UserWallets record exists. However,walletId(line 21) andaddress(line 23) are not marked as nullable despite being sourced from the same LEFT JOIN and having the same null risk (as acknowledged in the BUGALERT comment on line 69).Consider updating the type definition for consistency:
export type AutoTopUpDeployment = { id: string; - walletId: number; + walletId: number | null; dseq: string; - address: string; + address: string | null; isOldWallet: boolean | null; };Alternatively, change the LEFT JOINs to INNER JOINs if deployments without associated wallets should not be included in auto-top-up processing.
♻️ Duplicate comments (5)
apps/api/src/provider/controllers/jwt-token/jwt-token.controller.ts (1)
28-29: AlignuseOldWalletdefault with DB semantics (still defaults tofalsehere).This still uses
wallet.isOldWallet ?? false, while earlier analysis of theuser-walletschema/migration showedis_old_walletdefaulting totrueand allowing null. If that schema is unchanged, null/undefined would be interpreted as “new wallet” in the app but “old wallet” by DB default, which is inconsistent and may route requests through the wrong master wallet path.Either:
- Change the application default to match the DB default:
- useOldWallet: wallet.isOldWallet ?? false + useOldWallet: wallet.isOldWallet ?? trueor
- Update the DB default to
falseand ensure all records are backfilled, if the intended default behavior is the new wallet.Please confirm which default is canonical and adjust code/schema (and related usages of
isOldWallet ?? false) accordingly.#!/bin/bash # Verify current default/nullability of is_old_wallet echo "=== user-wallet schema ===" cat apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.ts 2>/dev/null | rg -n "is_old_wallet|isOldWallet" -n -C2 || true echo echo "=== migration 0021 (or latest touching is_old_wallet) ===" rg -n "is_old_wallet" apps/api/drizzle -n -C3 2>/dev/null || trueapps/api/src/billing/config/env.config.ts (1)
11-11: Documentation and migration guidance still needed.The addition of the required
OLD_MASTER_WALLET_MNEMONICenvironment variable is technically correct, but as noted in the previous review, this will cause existing deployments to fail on startup without proper documentation and migration guidance.apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts (1)
65-80: Old-wallet flag propagation in cleanup flow is consistent with updated APIsThe additions to pass
wallet.isOldWallet ?? falseinto bothexecuteDerivedTxandauthorizeSpending, and then again on retry, correctly follow the updated method signatures and keep the initial and recovery paths consistent.The earlier review already raised the question of whether
wallet.isOldWallet ?? falseshould instead align with the schema default oftrue; that same systemic concern applies here, but the plumbing itself is sound.apps/api/src/billing/services/managed-signer/managed-signer.service.ts (1)
40-51: Managed signer correctly threads useOldWallet into tx-manager callsThe new
useOldWalletparameter onexecuteDerivedTxandexecuteFundingTxis wired appropriately intogetFundingWalletAddress,signAndBroadcastWithDerivedWallet, andsignAndBroadcastWithFundingWallet, keeping behavior unchanged when omitted. TheexecuteDecodedTxByUserWalletcall now passinguserWallet.isOldWallet ?? falseis consistent with the rest of the codebase, though it shares the same previously noted concern about the?? falsefallback versus the schema default.Also applies to: 56-62, 124-124
apps/api/src/billing/services/refill/refill.service.ts (1)
50-59: authorizeSpending wiring looks correct, but defaulting pattern matches prior systemic concernPassing
{ address, limits, expiration }/{ address, limits }as the first argument anduserWallet.isOldWallet ?? falseas the second aligns with the updatedManagedUserWalletService.authorizeSpendingsignature and correctly propagates the old-wallet flag.However, this introduces another usage of the same
isOldWallet ?? falsefallback that was already flagged elsewhere as inconsistent with the DB/schema default ofis_old_wallet = true. The broader decision (aligning code defaults with schema or documenting why they differ) should cover this call site as well.Also applies to: 74-80
🧹 Nitpick comments (6)
apps/provider-proxy/src/config/env.config.ts (1)
5-5: Simplify by removing redundant.optional()call.The new order is better than the old, but
.optional()is redundant here since.default()already makes the field optional. This change also appears unrelated to the PR's main objective of supporting old wallet mnemonics.Apply this diff to use the idiomatic Zod pattern:
- PORT: z.number({ coerce: true }).min(0).optional().default(3040) + PORT: z.number({ coerce: true }).min(0).default(3040)apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts (1)
110-132: Deposit payload shape and newexecuteDerivedTxflag look correctThe expectation now matches the nested
deposit: { amount: { denom, amount }, sources: [Source.grant] }format and passes the new boolean flag asfalse, which aligns with using the non‑old wallet path for these deployments. If you find yourself adding more tests building the same MsgAccountDeposit, consider a small helper factory to reduce duplication, but it’s not strictly necessary here.apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts (1)
206-206: Consider adding test coverage for the old wallet path.The test correctly passes
falsefor the newuseOldWalletparameter, but there's no test case covering thetruescenario. To ensure the old wallet execution path works correctly, add a test that validates behavior whenuseOldWalletistrue.Would you like me to generate a test case for the old wallet scenario?
apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts (1)
201-259: Test setup correctly wires new old-wallet dependencies; consider adding explicit old-path coverageThe added mocks for
oldMasterWallet,oldMasterSigningClient, andoldWalletFactory, and the updatedTxManagerServiceconstructor call, look consistent with the new service signature and keep existing tests focused on the primary (non-old) path working.As a follow-up (not blocking), it may be worth adding at least one test that exercises a
useOldWalletflow to prevent regressions in the old-wallet-specific wiring.apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
116-146:revokeAllcorrectly switches funding wallet; consider avoiding positional boolean
revokeAllnow derives the granter address and executes the revoke transaction based onuseOldWallet, which keeps revocation behavior aligned with the same wallet used for granting. Signature is backward compatible since the new flag is last with a default.To reduce the chance of misordered arguments as this API evolves, consider moving to an options object (e.g.
{ grantee, reason, dryRunOptions, useOldWallet }) instead of adding more positional parameters in the future. This is optional but would improve readability.apps/api/src/billing/services/tx-manager/tx-manager.service.ts (1)
122-135: Cached client creation remains race-prone under concurrent calls (low impact)
#getClientlazily creates and cachesBatchSigningClientServiceinstances in#clientsByAddressusing:
- derive wallet →
getFirstAddress()if (!this.#clientsByAddress.has(address))then create andsetUnder concurrent calls for the same derived address, multiple clients can still be created before the first
setwins, similar in spirit to the unsynchronized async caching pattern we discussed earlier. This mainly affects efficiency and cache behavior (extra clients, slightly unpredictable retention), not correctness.If you want strict single-client-per-address semantics, consider synchronizing client creation (e.g. caching a
Promise<CachedClient>per address or using a simple per-address mutex) so only one factory call runs per address at a time. Based on learnings, this would make the caching more robust under concurrency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
apps/api/drizzle/0021_brave_warpath.sql(1 hunks)apps/api/drizzle/meta/0021_snapshot.json(1 hunks)apps/api/drizzle/meta/_journal.json(1 hunks)apps/api/env/.env.sample(1 hunks)apps/api/env/.env.unit.test(1 hunks)apps/api/src/billing/config/env.config.ts(1 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.ts(1 hunks)apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.ts(1 hunks)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts(2 hunks)apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.ts(0 hunks)apps/api/src/billing/services/balances/balances.service.ts(3 hunks)apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts(1 hunks)apps/api/src/billing/services/managed-signer/managed-signer.service.ts(2 hunks)apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts(4 hunks)apps/api/src/billing/services/provider-cleanup/provider-cleanup.service.ts(1 hunks)apps/api/src/billing/services/refill/refill.service.spec.ts(3 hunks)apps/api/src/billing/services/refill/refill.service.ts(2 hunks)apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts(3 hunks)apps/api/src/billing/services/tx-manager/tx-manager.service.ts(5 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(2 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts(2 hunks)apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts(2 hunks)apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.ts(1 hunks)apps/api/src/deployment/services/cached-balance/cached-balance.service.ts(1 hunks)apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.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(2 hunks)apps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.ts(1 hunks)apps/api/src/provider/controllers/jwt-token/jwt-token.controller.ts(1 hunks)apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts(1 hunks)apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts(3 hunks)apps/api/test/custom-jest-environment.ts(1 hunks)apps/api/test/seeders/auto-top-up-deployment.seeder.ts(1 hunks)apps/api/test/seeders/user-wallet.seeder.ts(1 hunks)apps/api/test/services/local.config.js(1 hunks)apps/deploy-web/src/components/get-started/GetStartedStepper.tsx(1 hunks)apps/deploy-web/src/components/layout/TopBanner.tsx(3 hunks)apps/deploy-web/src/components/onboarding/steps/FreeTrialLandingStep/FreeTrialLandingStep.tsx(1 hunks)apps/deploy-web/src/components/user/payment/PaymentForm.tsx(1 hunks)apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.spec.tsx(1 hunks)apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.tsx(0 hunks)apps/deploy-web/src/hooks/useHasCreditCardBanner.ts(1 hunks)apps/provider-proxy/env/.env(1 hunks)apps/provider-proxy/src/config/env.config.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.tsx
- apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.ts
✅ Files skipped from review due to trivial changes (1)
- apps/api/drizzle/meta/0021_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (19)
- apps/api/drizzle/meta/_journal.json
- apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts
- apps/provider-proxy/env/.env
- apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.ts
- apps/deploy-web/src/components/layout/TopBanner.tsx
- apps/deploy-web/src/components/user/payment/PaymentForm.tsx
- apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts
- apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts
- apps/api/test/seeders/user-wallet.seeder.ts
- apps/api/src/billing/services/balances/balances.service.ts
- apps/api/drizzle/0021_brave_warpath.sql
- apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.ts
- apps/deploy-web/src/components/wallet/ConnectManagedWalletButton.spec.tsx
- apps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.ts
- apps/api/src/billing/services/refill/refill.service.spec.ts
- apps/api/test/seeders/auto-top-up-deployment.seeder.ts
- apps/deploy-web/src/components/get-started/GetStartedStepper.tsx
- apps/api/src/deployment/services/cached-balance/cached-balance.service.ts
- apps/api/src/billing/services/provider-cleanup/provider-cleanup.service.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-15T16:39:55.348Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 2039
File: apps/deploy-web/tests/ui/change-wallets.spec.ts:4-10
Timestamp: 2025-10-15T16:39:55.348Z
Learning: In the Akash Console E2E tests using the context-with-extension fixture, the first wallet is automatically created during fixture setup via `importWalletToLeap` in `apps/deploy-web/tests/ui/fixture/wallet-setup.ts`, so tests that call `frontPage.createWallet()` are creating a second wallet to test wallet switching functionality.
Applied to files:
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/test/custom-jest-environment.tsapps/api/src/billing/services/tx-manager/tx-manager.service.spec.tsapps/api/src/billing/services/tx-manager/tx-manager.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
📚 Learning: 2025-10-31T11:26:42.138Z
Learnt from: stalniy
Repo: akash-network/console PR: 2138
File: apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts:379-382
Timestamp: 2025-10-31T11:26:42.138Z
Learning: In TypeScript/JavaScript, the pattern of checking a cached value and then performing an async operation to fetch it without proper synchronization is race condition unsafe:
```typescript
private async getAddress() {
if (!this.address) {
this.address = await this.wallet.getFirstAddress();
}
return this.address;
}
```
Multiple concurrent calls can all pass the `if (!this.address)` check before any of them sets the value, leading to duplicate async operations. This should be flagged as a race condition. Proper synchronization (mutex, atomic promise caching, or guaranteed single-threaded execution) is required.
Applied to files:
apps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/billing/services/tx-manager/tx-manager.service.ts
📚 Learning: 2025-11-19T15:15:07.283Z
Learnt from: ygrishajev
Repo: akash-network/console PR: 2254
File: apps/api/test/functional/sign-and-broadcast-tx.spec.ts:4-4
Timestamp: 2025-11-19T15:15:07.283Z
Learning: In the Akash Network Console project, when tests use native Node.js fetch (available in Node 18+), fetch-mock should be used for HTTP mocking instead of nock, as nock does not support intercepting native fetch calls. This applies to apps/api/test/functional/sign-and-broadcast-tx.spec.ts and any other tests using native fetch.
Applied to files:
apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts
📚 Learning: 2025-11-12T16:36:02.543Z
Learnt from: baktun14
Repo: akash-network/console PR: 2203
File: apps/deploy-web/src/components/onboarding/steps/PaymentMethodContainer/PaymentMethodContainer.tsx:161-168
Timestamp: 2025-11-12T16:36:02.543Z
Learning: In apps/deploy-web/src/components/onboarding/steps/PaymentMethodContainer/PaymentMethodContainer.tsx, the organization field captured during payment method setup is internal metadata. Errors from stripe.updateCustomerOrganization should be logged to Sentry but not surfaced to users, and the flow should continue even if the organization update fails, as it's non-critical and not something users can fix.
Applied to files:
apps/deploy-web/src/components/onboarding/steps/FreeTrialLandingStep/FreeTrialLandingStep.tsx
🧬 Code graph analysis (5)
apps/api/test/custom-jest-environment.ts (3)
apps/api/test/services/local.config.js (1)
localConfig(4-4)apps/api/test/setup-global-functional.ts (1)
localConfig(4-8)apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)
apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts (1)
apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.ts (1)
UserWallets(5-17)
apps/api/test/services/local.config.js (1)
apps/api/test/setup-global-functional.ts (1)
localConfig(4-8)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (2)
apps/api/src/billing/services/rpc-message-service/rpc-message.service.ts (1)
SpendingAuthorizationMsgOptions(16-22)apps/api/src/core/types/console.ts (1)
DryRunOptions(1-3)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
createAndAuthorizeTrialSpending(42-59)
🪛 dotenv-linter (4.0.0)
apps/api/env/.env.sample
[warning] 7-7: [UnorderedKey] The OLD_MASTER_WALLET_MNEMONIC key should go before the POSTGRES_DB_URI key
(UnorderedKey)
apps/api/env/.env.unit.test
[warning] 3-3: [UnorderedKey] The OLD_MASTER_WALLET_MNEMONIC key should go before the POSTGRES_DB_URI key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: validate (apps/api) / validate-unsafe
- GitHub Check: validate (apps/provider-proxy) / validate-unsafe
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
apps/deploy-web/src/hooks/useHasCreditCardBanner.ts (1)
34-34: LGTM! Correctly enables banner visibility.This change properly returns the banner visibility state instead of a constant false conjunction, allowing the credit card banner to display when the computed conditions are met.
apps/deploy-web/src/components/onboarding/steps/FreeTrialLandingStep/FreeTrialLandingStep.tsx (1)
40-40: Verify that making this component visible aligns with the intended onboarding flow.The removal of the
hiddenclass makes the FreeTrialLandingStep component visible by default. While this aligns with the PR objectives mentioning that "frontend UI components are made always-visible," please confirm:
- This visibility change is intentional and necessary for the payment/form flow fixes
- The onboarding flow has been tested to ensure this component displays at the correct step
- This doesn't cause the component to appear unexpectedly in other parts of the application
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (2)
39-39: LGTM: Test expectation correctly updated.The test expectation now matches the updated
createAndAuthorizeTrialSpendingsignature withuseOldWallet: false, consistent with the wallet-initializer service always using the new wallet path.
84-84: LGTM: Error path test expectation correctly updated.The test expectation properly verifies that
createAndAuthorizeTrialSpendingis called with the updated signature before the error is thrown, ensuring consistent parameter usage across both success and error paths.apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts (1)
282-321: Combined multi‑deployment tx expectation matches updated APIThe single‑tx expectation for two deployments correctly builds two MsgAccountDeposit messages with the updated nested
depositstructure and includes the new trailingfalseargument toexecuteDerivedTx. This keeps the behavior explicit for the default (non‑old) wallet path while validating the batched message layout.apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts (1)
51-64: LGTM! Changes align with PR objectives.The addition of
isOldWallet: UserWallets.isOldWalletto the query projection correctly implements support for the old wallet execution path as described in the PR objectives. The field is properly sourced from the UserWallets schema and integrates well with the broader changes across the codebase.Note: The pre-existing LEFT JOIN issue mentioned in the BUGALERT comment on line 69 applies equally to this new field, but that's not introduced by this change.
apps/api/env/.env.unit.test (1)
3-3: LGTM!The addition of
OLD_MASTER_WALLET_MNEMONICcorrectly provides test configuration for the old wallet path. The static analysis warning about key ordering is a minor linting issue that can be addressed optionally for consistency but doesn't affect functionality.apps/api/env/.env.sample (1)
7-7: LGTM!The rename from
MASTER_WALLET_MNEMONICtoOLD_MASTER_WALLET_MNEMONICcorrectly reflects the new environment schema. Note that existing deployments will need to update their environment configuration accordingly (documentation concerns were already raised in the review ofenv.config.ts).apps/api/test/services/local.config.js (1)
22-23: LGTM!The addition of
OLD_MASTER_WALLET_MNEMONICto the local configuration correctly follows the established pattern for wallet mnemonics and ensures test environments have access to the old wallet configuration.apps/api/test/custom-jest-environment.ts (1)
30-34: LGTM!The setup logic for
OLD_MASTER_WALLET_MNEMONICcorrectly mirrors the existing patterns for other wallet mnemonics, ensuring consistent test environment initialization.apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
58-58: No issues found. The migration strategy is sound.The
is_old_walletcolumn was added in migration 0021 withDEFAULT true, which automatically marks all existing wallets as old. New wallets are explicitly set toisOldWallet: falsein the code (lines 58, 79-80). This is a valid implicit migration strategy that distinguishes legacy wallets from newly created ones.apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
98-111: Asymmetry in wallet type handling requires clarification.The implementation has an asymmetry: when fetching balances for the current user,
isOldWalletis retrieved from the user's wallet record and passed to the balance service. However, when an address is provided directly as a query parameter,isOldWalletis hardcoded tofalsewith no way to specify the wallet type.The query schema allows any valid address to be passed ("Optional wallet address to fetch balances for instead of the current user"), but there's no mechanism to indicate whether that address is an old or new wallet. Since
isOldWalletaffects balance calculations ingetFreshLimits, this could produce incorrect results if an old wallet address is queried directly.Verify that:
- The design intentionally restricts direct address queries to new wallets only, or
- A mechanism should be added to query the
UserWallettable to determine wallet type before calculating balances, or- A query parameter should allow clients to specify wallet type for direct addresses.
apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts (1)
18-23: useOldWallet flag is correctly threaded through JWT token derivationThe new
useOldWalletparameter is plumbed cleanly fromGenerateJwtTokenParamsintogenerateJwtTokenand down totxManagerService.getDerivedWallet, preserving existing behavior when omitted and enabling the old-wallet path when explicitly set totrue. Just ensure any futuregetJwtTokencall sites also pass the boolean explicitly so memoization keys distinguish old vs new wallet derivations.Also applies to: 34-40, 59-60
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (2)
42-66: Trial wallet and generic wallet creation correctly honoruseOldWallet
createAndAuthorizeTrialSpendingandcreateWalletnow consistently passuseOldWalletthrough togetDerivedWalletAddress, and the flag defaults tofalse, so existing call sites remain compatible. Behavior looks correct for both new and old wallet paths.
68-114: Old-wallet path is correctly threaded through spending authorization
authorizeSpending,authorizeFeeSpending, andauthorizeDeploymentSpendingall now derive the funding address and execute funding transactions via theuseOldWalletflag, keeping fee and deployment grants in sync with the selected wallet. The existing'deployment' in options.limitsguard remains valid and no new edge cases are introduced by this change.apps/api/src/billing/services/tx-manager/tx-manager.service.ts (1)
77-87:useOldWalletrouting between funding and derived wallets is consistentThe constructor wiring and the methods
signAndBroadcastWithFundingWallet,getFundingWalletAddress,signAndBroadcastWithDerivedWallet,getDerivedWalletAddress, andgetDerivedWalletconsistently choose between the new and old wallet/signing-client paths based onuseOldWallet, defaulting tofalseso existing callers keep current behavior. The selection logic and address derivation look coherent for both wallet families.Also applies to: 90-98, 100-120, 137-140
| const OLD_MASTER_WALLET: InjectionToken<Wallet> = Symbol("OLD_MASTER_WALLET"); | ||
| container.register(OLD_MASTER_WALLET, { | ||
| useFactory: instancePerContainerCachingFactory(c => { | ||
| const config = c.resolve(BillingConfigService); | ||
| return new Wallet(config.get("OLD_MASTER_WALLET_MNEMONIC"), 0); | ||
| }) | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Check whether OLD_MASTER_WALLET_MNEMONIC is now a hard requirement for service startup
OLD_MASTER_WALLET, OLD_DERIVED_WALLET_FACTORY, and OLD_MASTER_SIGNING_CLIENT are always registered and injected into TxManagerService. That means BillingConfigService.get("OLD_MASTER_WALLET_MNEMONIC") will be called during DI resolution, even if useOldWallet is never used at runtime.
If BillingConfigService.get throws or Wallet rejects an undefined mnemonic, this change will make OLD_MASTER_WALLET_MNEMONIC effectively mandatory and could break existing environments that don’t configure an old mnemonic. If the old wallet is meant to be optional, consider guarding registration or injection behind config presence, or deferring resolution until useOldWallet === true is actually requested.
Also applies to: 40-46, 65-71
I need to examine the codebase to verify this concern. Let me start by looking at the actual service file and the configuration service.
🏁 Script executed:
#!/bin/bash
# Examine the TxManagerService file to see the full context of the issue
cat apps/api/src/billing/services/tx-manager/tx-manager.service.tsLength of output: 6251
🏁 Script executed:
#!/bin/bash
# Find and examine BillingConfigService to understand how .get() behaves
fd -t f "billing.*config" --type f | head -20Length of output: 70
🏁 Script executed:
#!/bin/bash
# Search for BillingConfigService definition and get() method
rg -n "class BillingConfigService|\.get\(" --type=ts -A 5 | head -100Length of output: 7355
🏁 Script executed:
#!/bin/bash
# Search for any guards or try-catch around OLD_MASTER_WALLET_MNEMONIC
rg -n "OLD_MASTER_WALLET_MNEMONIC" --type=ts -B 3 -A 3Length of output: 2887
I need to examine the BillingConfigService and confirm the schema change history.
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine BillingConfigService to see how .get() behaves
cat apps/api/src/billing/services/billing-config/billing-config.service.ts
Length of output: 398
🏁 Script executed:
#!/bin/bash
# Check the Wallet class constructor to see if it throws on undefined mnemonic
find . -path ./node_modules -prune -o -type f -name "wallet.ts" -o -name "wallet.js" | grep -v node_modules | head -5Length of output: 85
🏁 Script executed:
#!/bin/bash
# Look at actual wallet implementation
rg -n "class Wallet|constructor\(" --type=ts -A 10 apps/api/src/billing/lib/wallet/Length of output: 1848
I need to check the ConfigService parent implementation to understand how it validates and retrieves config values.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine ConfigService in core
fd -t f "config.service.ts" | grep core | head -5
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Examine ConfigService implementation
rg -n "class ConfigService|get\(" --type=ts -A 8 packages/core/src/services/config/ | head -80Length of output: 74
🏁 Script executed:
#!/bin/bash
# Check the full ConfigService class
cat packages/core/src/services/config/config.service.tsLength of output: 83
---
🏁 Script executed:
```shell
#!/bin/bash
# Find ConfigService definition by searching imports
rg -n "class ConfigService" --type=ts -B 2 -A 15
Length of output: 3095
🏁 Script executed:
#!/bin/bash
# Search for where ConfigService is defined
rg -n "export class ConfigService" --type=ts -A 20Length of output: 3234
🏁 Script executed:
#!/bin/bash
# Check the import path from BillingConfigService
grep -n "from.*ConfigService" apps/api/src/billing/services/billing-config/billing-config.service.tsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find the actual ConfigService file
find . -name "*.ts" -type f -exec grep -l "export class ConfigService" {} \; | grep -v node_modules | head -5Length of output: 112
---
🏁 Script executed:
```shell
#!/bin/bash
# Check env.config.ts to see the full schema
cat apps/api/src/billing/config/env.config.ts
Length of output: 1717
🏁 Script executed:
#!/bin/bash
# Look for documentation or .env example files
find . -name ".env*" -o -name "*.env.example" | grep -v node_modules | head -10Length of output: 321
🏁 Script executed:
#!/bin/bash
# Check if there's a README or documentation about environment setup
find . -name "README*" -o -name "SETUP*" | grep -i billing | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any environment variable documentation or initialization
rg -n "OLD_MASTER_WALLET" --type=ts --type=md --type=json -B 2 -A 2 | head -50Length of output: 3862
Verify that OLD_MASTER_WALLET_MNEMONIC is now a hard startup requirement that breaks existing deployments
Confirmed. The OLD_MASTER_WALLET_MNEMONIC configuration is now effectively mandatory and will break startup for existing environments:
-
Schema is required: In
env.config.tsline 11, the field isz.string()without.optional(), making it required by Zod validation. -
Validation occurs at startup:
ConfigServiceconstructor (line 15 inconfig.service.ts) callsenvSchema.parse(process.env), which throws immediately if any required field is missing. -
DI registration is unconditional: All three tokens (
OLD_MASTER_WALLET,OLD_DERIVED_WALLET_FACTORY,OLD_MASTER_SIGNING_CLIENT) are registered unconditionally at module load time, before the container is even instantiated. -
Injection is unconditional:
TxManagerServiceconstructor (lines 78–88) injects all three old wallet dependencies without guards. -
Backward compatibility broken: The test setup (
custom-jest-environment.tslines 30–34) conditionally generates a fallback mnemonic if missing, suggesting this feature was designed to be optional. TheuseOldWalletflag throughout the service methods confirms the intent to gate this functionality at runtime, not startup.
Impact: Any existing deployment without OLD_MASTER_WALLET_MNEMONIC configured will fail immediately at service initialization, before any code path even checks the useOldWallet flag. To support optional old wallet functionality, either guard registration/injection behind config presence or defer wallet creation until actually needed.
🤖 Prompt for AI Agents
In apps/api/src/billing/services/tx-manager/tx-manager.service.ts around lines
24–30, the OLD_MASTER_WALLET token is unconditionally registered using a config
value that is required in the env schema, causing startup failures when the
mnemonic is absent; make the old-wallet feature optional by either (A) updating
the env schema to mark OLD_MASTER_WALLET_MNEMONIC optional so missing values
don't break startup, or (B) conditionally register the OLD_MASTER_WALLET/related
tokens only when the config contains a mnemonic (e.g., check config.has or
config.get and skip registration if missing), and change the token
types/injections and TxManagerService constructor to accept the wallet tokens as
optional (Wallet | undefined) and handle the absent case at runtime behind the
existing useOldWallet guards.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.