refactor(billing): migrate from boolean wallet flag to version-based system #2432
Conversation
📝 WalkthroughWalkthroughRemoves legacy old-wallet support and the DB column; strips Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Service / Controller
participant ManagedSigner as ManagedSignerService
participant TxManager as TxManagerService
participant Balances as BalancesService
participant Chain as Blockchain
Note over Client,ManagedSigner: Request to execute derived transaction (no old-wallet flag)
Client->>ManagedSigner: executeDerivedTx(walletId, messages)
ManagedSigner->>TxManager: signAndBroadcastWithDerivedWallet(derivationIndex, messages, options?)
TxManager->>TxManager: resolve WalletResources (v1|v2) using WalletOptions
TxManager->>Balances: getFreshLimits({ address })
Balances-->>TxManager: limits
TxManager->>Chain: sign & broadcast batched tx
Chain-->>TxManager: tx result
TxManager-->>ManagedSigner: broadcast result
ManagedSigner-->>Client: result / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (38)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (16)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Files:
**/*.spec.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Files:
🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-10-15T16:39:55.348ZApplied to files:
📚 Learning: 2025-12-29T12:12:23.112ZApplied to files:
📚 Learning: 2025-10-31T11:26:42.138ZApplied to files:
📚 Learning: 2025-06-03T15:06:34.211ZApplied to files:
🧬 Code graph analysis (7)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
apps/api/test/services/test-wallet.service.ts (1)
apps/api/src/billing/services/balances/balances.service.ts (2)
apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.ts (3)
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (2)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (2)
apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts (1)
⏰ 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). (3)
🔇 Additional comments (35)
Comment |
ygrishajev
force-pushed
the
feature/wallet
branch
from
019eecd to
8375c26
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/billing/config/env.config.ts (1)
6-6: Remove the obsolete dotenv.config call for non-existent env file.Line 6 references
env/.env.funding-wallet-index, which no longer exists. This file should be removed as dead code.
🧹 Nitpick comments (4)
apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.ts (1)
10-16: Consider migrating to thesetupfunction pattern.Per coding guidelines, tests should use a
setupfunction at the bottom of the describe block instead ofbeforeEach. This avoids shared state and improves test isolation.🔎 Suggested refactor
- let service: CachedBalanceService; - let balancesService: jest.Mocked<BalancesService>; - - beforeEach(() => { - balancesService = { - getFreshLimits: jest.fn() - } as unknown as jest.Mocked<BalancesService>; - - service = new CachedBalanceService(balancesService); - }); // At the bottom of the describe block: + function setup() { + const balancesService = { + getFreshLimits: jest.fn() + } as unknown as jest.Mocked<BalancesService>; + + const service = new CachedBalanceService(balancesService); + + return { service, balancesService }; + }apps/api/src/billing/services/tx-manager/tx-manager.service.ts (3)
103-114: API inconsistency:getDerivedWalletAddressaccepts wallet version butsignAndBroadcastWithDerivedWalletdoes not.
getDerivedWalletAddressacceptsWalletOptionsto specify a wallet version, butsignAndBroadcastWithDerivedWalletalways uses the default version (v2). If code retrieves an address withwalletVersion: "v1"and later tries to sign/broadcast for the same derivation index, it would use a different wallet (v2).Consider either:
- Add
WalletOptionsparameter tosignAndBroadcastWithDerivedWalletand#getClient, or- Remove
WalletOptionsfromgetDerivedWalletAddressif derived wallets are always v2Option 1: Add WalletOptions to signAndBroadcastWithDerivedWallet
- async signAndBroadcastWithDerivedWallet(derivationIndex: number, messages: readonly EncodeObject[], options?: SignAndBroadcastOptions) { - const { client, address } = await this.#getClient(derivationIndex); + async signAndBroadcastWithDerivedWallet(derivationIndex: number, messages: readonly EncodeObject[], options?: SignAndBroadcastOptions & WalletOptions) { + const { client, address } = await this.#getClient(derivationIndex, options);- async #getClient(derivationIndex: number): Promise<CachedClient> { - const wallet = this.getDerivedWallet(derivationIndex); + async #getClient(derivationIndex: number, options?: WalletOptions): Promise<CachedClient> { + const wallet = this.getDerivedWallet(derivationIndex, options);Also applies to: 116-118
120-133: Potential race condition in client caching.Based on learnings, the pattern of checking a cached value and then performing an async operation to set it is susceptible to race conditions. Multiple concurrent calls to
#getClientwith the samederivationIndexcan all pass thehas()check before any of them sets the value, leading to duplicateBatchSigningClientServiceinstantiation.While this doesn't cause correctness issues (clients are functionally equivalent and one eventually wins), it's worth considering synchronization for efficiency if high concurrency is expected.
Potential fix using promise caching
- readonly #clientsByAddress: Map<string, CachedClient> = new Map(); + readonly #clientsByAddress: Map<string, CachedClient> = new Map(); + readonly #clientCreationPromises: Map<string, Promise<CachedClient>> = new Map(); - async #getClient(derivationIndex: number): Promise<CachedClient> { - const wallet = this.getDerivedWallet(derivationIndex); - const address = await wallet.getFirstAddress(); - - if (!this.#clientsByAddress.has(address)) { - this.logger.debug({ event: "DERIVED_SIGNING_CLIENT_CREATE", address }); - this.#clientsByAddress.set(address, { - address, - client: this.batchSigningClientServiceFactory(wallet) - }); - } - - return this.#clientsByAddress.get(address)!; - } + async #getClient(derivationIndex: number): Promise<CachedClient> { + const wallet = this.getDerivedWallet(derivationIndex); + const address = await wallet.getFirstAddress(); + + if (this.#clientsByAddress.has(address)) { + return this.#clientsByAddress.get(address)!; + } + + if (!this.#clientCreationPromises.has(address)) { + const creationPromise = Promise.resolve().then(() => { + this.logger.debug({ event: "DERIVED_SIGNING_CLIENT_CREATE", address }); + const cachedClient = { + address, + client: this.batchSigningClientServiceFactory(wallet) + }; + this.#clientsByAddress.set(address, cachedClient); + this.#clientCreationPromises.delete(address); + return cachedClient; + }); + this.#clientCreationPromises.set(address, creationPromise); + } + + return this.#clientCreationPromises.get(address)!; + }
93-101: Design is intentional: funding wallet operations default to v2 only.Both
signAndBroadcastWithFundingWalletandgetFundingWalletAddressuse v2 by default with no runtime option to specify a wallet version. This differs from derived wallet methods, which acceptWalletOptionsto select between v1 and v2.The v1 infrastructure remains available (via config fallback like
FUNDING_WALLET_MNEMONIC_V1), but the runtime APIs are v2-only. No call sites attempt to access v1 funding wallet operations, suggesting this migration is intentional. If v1 funding wallet support is needed for backward compatibility, consider adding aWalletOptionsparameter to these methods to align with the derived wallet API design.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
apps/api/drizzle/0024_new_nova.sqlapps/api/drizzle/meta/0024_snapshot.jsonapps/api/drizzle/meta/_journal.jsonapps/api/env/.env.funding-wallet-indexapps/api/src/billing/config/env.config.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.tsapps/api/src/billing/repositories/usage/usage.repository.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/billing/repositories/wallet-settings/wallet-settings.repository.tsapps/api/src/billing/services/balances/balances.service.tsapps/api/src/billing/services/managed-signer/managed-signer.service.spec.tsapps/api/src/billing/services/managed-signer/managed-signer.service.tsapps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.tsapps/api/src/billing/services/provider-cleanup/provider-cleanup.service.tsapps/api/src/billing/services/refill/refill.service.spec.tsapps/api/src/billing/services/refill/refill.service.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-balance-reload-check/wallet-balance-reload-check.handler.spec.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.spec.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.tsapps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.tsapps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.tsapps/api/src/provider/controllers/jwt-token/jwt-token.controller.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.tsapps/api/test/seeders/auto-top-up-deployment.seeder.tsapps/api/test/seeders/user-wallet.seeder.tsapps/api/test/services/local.config.jsapps/api/test/services/test-wallet.service.tsapps/api/test/setup-global-functional.ts
💤 Files with no reviewable changes (4)
- apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.ts
- apps/api/test/seeders/auto-top-up-deployment.seeder.ts
- apps/api/test/services/local.config.js
- apps/api/env/.env.funding-wallet-index
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/billing/repositories/wallet-settings/wallet-settings.repository.tsapps/api/test/seeders/user-wallet.seeder.tsapps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.tsapps/api/src/billing/repositories/usage/usage.repository.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.tsapps/api/src/billing/config/env.config.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.tsapps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.tsapps/api/src/billing/services/managed-signer/managed-signer.service.spec.tsapps/api/src/billing/services/refill/refill.service.spec.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.tsapps/api/src/billing/services/provider-cleanup/provider-cleanup.service.tsapps/api/src/provider/controllers/jwt-token/jwt-token.controller.tsapps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/billing/services/refill/refill.service.tsapps/api/src/billing/services/managed-signer/managed-signer.service.tsapps/api/test/setup-global-functional.tsapps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.tsapps/api/src/billing/services/tx-manager/tx-manager.service.spec.tsapps/api/src/billing/services/balances/balances.service.tsapps/api/src/billing/services/tx-manager/tx-manager.service.tsapps/api/test/services/test-wallet.service.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under testUse
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.
**/*.spec.{ts,tsx}: Use<Subject>.namein the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references
Use either a method name or a condition starting with 'when' for nested suite descriptions in tests
Use present simple, 3rd person singular for test descriptions without prepending 'should'
Files:
apps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.tsapps/api/src/billing/services/managed-signer/managed-signer.service.spec.tsapps/api/src/billing/services/refill/refill.service.spec.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
📚 Learning: 2025-11-28T09:17:28.832Z
Learnt from: ygrishajev
Repo: akash-network/console PR: 2274
File: apps/api/drizzle/0023_sad_adam_warlock.sql:1-1
Timestamp: 2025-11-28T09:17:28.832Z
Learning: In the Akash Network console billing system, the `user_wallets.user_id` column is required (NOT NULL). All user wallets must be associated with a user ID, and there are no anonymous wallets without a user_id in production.
Applied to files:
apps/api/src/billing/repositories/wallet-settings/wallet-settings.repository.ts
📚 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/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.tsapps/api/src/billing/services/refill/refill.service.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/test/setup-global-functional.tsapps/api/test/services/test-wallet.service.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/repositories/usage/usage.repository.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.spec.tsapps/api/src/billing/services/tx-manager/tx-manager.service.ts
📚 Learning: 2025-11-25T17:45:52.965Z
Learnt from: CR
Repo: akash-network/console PR: 0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-11-25T17:45:52.965Z
Learning: Applies to **/*.spec.{ts,tsx} : Use `setup` function instead of `beforeEach` in test files. The `setup` function must be at the bottom of the root `describe` block, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.
Applied to files:
apps/api/test/setup-global-functional.ts
🧬 Code graph analysis (5)
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/wallet-balance-reload-check/wallet-balance-reload-check.handler.ts (2)
apps/api/src/core/types/require.type.ts (1)
Require(1-3)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(18-22)
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(38-52)
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (1)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(9-57)
apps/api/test/services/test-wallet.service.ts (1)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(9-57)
⏰ 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). (3)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (54)
apps/api/test/services/test-wallet.service.ts (2)
51-53: LGTM! Wallet index removal is consistent.The removal of the
walletIndexparameter frominit()and the updated call toprepareWalletsalign with the PR objectives to simplify wallet initialization.
62-71: LGTM! Test wallet creation simplified.The
prepareWalletsmethod andWalletconstruction have been correctly updated to remove wallet index handling. Test wallets will now use default derivation paths, which is appropriate for functional test scenarios.apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
39-39: LGTM! Test expectations updated correctly.The test expectations have been properly updated to match the new
createAndAuthorizeTrialSpendingsignature that no longer includes theuseOldWalletparameter.Also applies to: 84-84
apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts (1)
208-208: LGTM! Test expectation aligned with refactored signature.The test has been correctly updated to expect
signAndBroadcastWithDerivedWalletto be called without theuseOldWalletparameter, consistent with the service refactoring.apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts (1)
67-83: LGTM! Repository updated to remove isOldWallet.The query has been correctly updated to no longer select the
isOldWalletfield, aligning with the schema changes that removed this column from theuser_walletstable.apps/api/test/setup-global-functional.ts (1)
5-7: LGTM! Test setup updated correctly.The call to
TestWalletService.init()has been correctly updated to remove thewalletIndexargument, aligning with the refactored service signature.apps/api/src/billing/config/env.config.ts (1)
12-15: LGTM! Version-based mnemonic configuration added.The new optional mnemonic fields (
FUNDING_WALLET_MNEMONIC_V1/V2andDERIVATION_WALLET_MNEMONIC_V1/V2) properly support the version-based wallet system described in the PR objectives.apps/api/drizzle/0024_new_nova.sql (1)
1-1: LGTM! Safe migration to remove is_old_wallet column.The migration correctly uses
IF EXISTSto safely drop theis_old_walletcolumn from theuser_walletstable, aligning with the PR's goal to remove the boolean wallet flag.apps/api/src/provider/controllers/jwt-token/jwt-token.controller.ts (1)
25-29: LGTM! Controller updated to remove isOldWallet parameter.The call to
generateJwtTokenhas been correctly updated to remove theisOldWalletparameter while retaining thettlfield, aligning with the service signature changes.apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts (1)
21-21: LGTM!The test expectation correctly reflects the updated
getDerivedWalletsignature that no longer requires the boolean flag parameter.apps/api/src/billing/repositories/wallet-settings/wallet-settings.repository.ts (1)
48-64: LGTM!The removal of
isOldWalletfrom the wallet relation columns is correct and aligns with the migration to a version-based wallet system.apps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.ts (1)
31-35: LGTM!The test expectation correctly reflects the simplified service signature without the
isOldWalletparameter.apps/api/drizzle/meta/_journal.json (1)
173-179: LGTM!The migration journal entry is properly formatted and correctly positioned after the previous migration.
apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.ts (1)
310-341: LGTM!The wallet object construction has been correctly updated to remove the
isOldWalletfield, keeping only theaddressfield as required by the new wallet data structure.apps/api/test/seeders/user-wallet.seeder.ts (1)
6-28: LGTM!The seeder has been correctly updated to remove the
isOldWalletfield, aligning with the schema changes.apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (2)
47-70: LGTM!The
getOrCreatemethod has been correctly updated to remove theisOldWalletfield from the insert payload.
72-82: LGTM!The
createmethod has been correctly updated to remove theisOldWalletfield from the insert payload, now only includinguserIdandaddress.apps/api/src/billing/repositories/usage/usage.repository.ts (1)
125-150: LGTM!The migration from boolean flag to version-based system is correctly implemented:
- Explicitly specifies
walletVersion: "v1"to retrieve the previous wallet address for backward compatibility- Variable renamed from
oldAddresstoprevAddressfor clearer semantics- Logic preserved: resolves both current and previous addresses for wallets created before the migration date
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
98-109: LGTM!The
getBalancesmethod correctly removes theisOldWalletflag from thegetFullBalanceMemoizedcall, aligning with the PR's migration to a version-based wallet system. The method now only passescurrentAddress, which is consistent with the updatedBalancesServiceAPI.apps/api/src/billing/services/refill/refill.service.spec.ts (4)
29-32: LGTM!Test expectation correctly updated to match the new
authorizeSpendingsignature that accepts a single options object withaddressandlimitsproperties, removing the legacyisOldWalletparameter.
51-54: LGTM!Consistent update for the "create new wallet" scenario - test expectation aligns with the new single-object signature.
76-83: LGTM!Race condition test correctly updated with both
authorizeSpendingcall expectations using the new signature format.
90-118: LGTM!The
setupfunction follows coding guidelines: positioned at the bottom of the describe block, creates and returns the object under test, usesjest-mock-extended'smockfunction, and avoids shared state.apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.ts (2)
23-23: LGTM!The
InitializedWallettype is correctly simplified to only require theaddressfield, removing the now-unnecessaryisOldWalletproperty from the wallet version migration.
96-96: LGTM!The
getFullBalanceInFiatcall is correctly updated to pass onlywallet.address, consistent with the updatedBalancesServiceAPI.apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (2)
28-28: LGTM!The
createAndAuthorizeTrialSpendingcall correctly removes the explicituseOldWallet: falseparameter. Per the PR objectives, wallet version now defaults to V2 internally, making this parameter unnecessary for new wallet creation.
55-55: LGTM!Consistent update for
createWallet- removes theuseOldWallet: falseparameter, relying on the new default V2 behavior.apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.ts (1)
32-32: LGTM!Test expectation correctly updated to match the new
getFreshLimitssignature that only accepts{ address }.apps/api/src/deployment/services/cached-balance/cached-balance.service.ts (1)
27-36: Potential race condition on cache initialization.The pattern of checking
!cachedBalanceand then performing an asyncgetFreshLimitscall is race-condition unsafe. Multiple concurrent calls toget()with the same address can all pass the!cachedBalancecheck before any of them sets the value, leading to duplicate async operations and potential inconsistent state.Based on learnings, this pattern requires proper synchronization (e.g., caching the promise instead of the result, or using a mutex).
🔎 Suggested fix using promise caching
@singleton() export class CachedBalanceService { - private readonly balanceCache = new Map<string, CachedBalance>(); + private readonly balanceCache = new Map<string, Promise<CachedBalance>>(); constructor(private readonly balancesService: BalancesService) {} public async get(address: string): Promise<CachedBalance> { - let cachedBalance = this.balanceCache.get(address); - if (!cachedBalance) { - const limits = await this.balancesService.getFreshLimits({ address }); - cachedBalance = new CachedBalance(limits.deployment); - this.balanceCache.set(address, cachedBalance); + let cachedPromise = this.balanceCache.get(address); + if (!cachedPromise) { + cachedPromise = this.balancesService.getFreshLimits({ address }) + .then(limits => new CachedBalance(limits.deployment)); + this.balanceCache.set(address, cachedPromise); } - return cachedBalance; + return cachedPromise; } }Note: This race condition may be pre-existing and not introduced by this PR. If so, per learnings, consider creating a separate issue to track the fix rather than expanding the scope of this PR.
apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts (3)
77-95: LGTM!Test expectation correctly updated for
executeDerivedTx- now expects only(walletId, [messages])without the trailingisOldWalletboolean. TheMsgAccountDepositstructure with nesteddepositobject containingamountandsourcesaligns with the updated chain SDK types.
280-315: LGTM!Batched deposit test correctly verifies that multiple deployments for the same owner are processed in a single transaction. The expectation properly reflects the new
executeDerivedTxsignature without theisOldWalletflag.
529-571: LGTM!The
setupfunction is correctly positioned at the bottom of the describe block, creates all necessary mocks usingjest-mock-extended, and returns the service under test along with its dependencies. This follows the coding guidelines.apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts (2)
65-65: LGTM!The
executeDerivedTxcall correctly removes theisOldWalletparameter, using the new simplified signature(wallet.id, messages).
69-76: LGTM!Both the
authorizeSpendingcall and the retryexecuteDerivedTxcall are correctly updated to use the new signatures without theisOldWalletflag. The error recovery flow for "not allowed to pay fees" is preserved correctly.apps/api/drizzle/meta/0024_snapshot.json (1)
7-98: LGTM!The
user_walletstable snapshot correctly reflects the removal of theis_old_walletcolumn, aligning with the PR's migration from boolean wallet flags to version-based system.apps/api/src/billing/services/refill/refill.service.ts (2)
51-57: LGTM!The
authorizeSpendingcall is correctly updated to use a single options object, removing the redundantisOldWalletflag.
72-75: LGTM!Consistent with the refill method above—clean options object pattern without the wallet version flag.
apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts (2)
87-90: LGTM!The
cachedBalanceService.get(address)call correctly removes theisOldWalletparameter, aligning with the updated service signature.
150-153: LGTM!The
executeDerivedTxcall is correctly simplified to(walletId, messages), removing the obsoleteisOldWalletflag.apps/api/src/billing/services/provider-cleanup/provider-cleanup.service.ts (2)
57-61: LGTM!The
executeDerivedTxcall correctly uses the simplified signature without theisOldWalletflag.
64-72: LGTM!The error recovery path correctly updates both
authorizeSpending(with options object) and the retryexecuteDerivedTxcall to use the new signatures.apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts (2)
18-22: LGTM!The
GenerateJwtTokenParamstype correctly removes theuseOldWalletproperty, simplifying the parameter interface.
33-34: LGTM!Both
generateJwtTokenandgetJwtTokenare correctly updated to remove theuseOldWalletparameter, with the derived wallet now obtained viagetDerivedWallet(walletId)directly.Also applies to: 53-54
apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts (1)
236-249: LGTM!The test correctly implements the new
walletResourcespattern with separatev1andv2configurations, aligning with the productionTxManagerServiceconstructor changes. Thesetupfunction follows the coding guidelines—positioned at the bottom of the describe block, creating the object under test with proper mocks.apps/api/src/billing/services/managed-signer/managed-signer.service.ts (3)
46-55: LGTM!The
executeDerivedTxmethod is correctly simplified to accept onlywalletIndexandmessages. The funding wallet address is now obtained viagetFundingWalletAddress()without version flags, and the transaction is signed with the streamlinedsignAndBroadcastWithDerivedWalletcall.
57-63: LGTM!The
executeFundingTxmethod correctly uses the simplifiedsignAndBroadcastWithFundingWallet(messages)signature.
134-134: LGTM!Internal call site correctly updated to use the new
executeDerivedTx(userWallet.id, messages)signature.apps/api/src/billing/services/balances/balances.service.ts (4)
58-61: LGTM!The
getFreshLimitssignature correctly narrows the required property to justaddressusingPick<UserWalletOutput, "address">, removing theisOldWalletdependency.
63-72: LGTM!The
retrieveAndCalcFeeLimitmethod correctly usesgetFundingWalletAddress()without version parameters, simplifying the funding wallet resolution.
74-83: LGTM!Consistent with the fee limit method—
retrieveDeploymentLimitcorrectly uses the parameterlessgetFundingWalletAddress().
103-108: LGTM!All balance retrieval methods (
getFullBalanceMemoized,getFullBalance,getFullBalanceInFiatMemoized,getFullBalanceInFiat) are correctly simplified to accept onlyaddress, with internal calls updated to use{ address }options objects.Also applies to: 119-125
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (2)
38-52: Clean refactoring of method signatures.The removal of
useOldWalletparameters across all methods is consistent and aligns with the new wallet versioning approach inTxManagerService. The methods now rely on the default wallet version (v2), simplifying the API surface while maintaining backward compatibility through the fallback mechanism inTxManagerService.Also applies to: 54-59, 61-83
85-101: LGTM!The private methods
authorizeFeeSpendingandauthorizeDeploymentSpending, along withrevokeAll, have been correctly updated to work with the new wallet version system without explicit version parameters.Also applies to: 103-133
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (1)
21-62: Well-structured wallet versioning system.The type definitions and factory registration are clean. The fallback chain (
FUNDING_WALLET_MNEMONIC_V1→OLD_MASTER_WALLET_MNEMONICfor v1,FUNDING_WALLET_MNEMONIC_V2→FUNDING_WALLET_MNEMONICfor v2) ensures backward compatibility as stated in the PR objectives. UsinginstancePerContainerCachingFactorycorrectly ensures wallet resources are created once per container.
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/billing/services/tx-manager/tx-manager.service.ts (1)
120-133: Race condition: Multiple concurrent calls may create duplicate clients.The
#getClientmethod checks if a client exists in the cache and creates one if not:if (!this.#clientsByAddress.has(address)) { this.logger.debug({ event: "DERIVED_SIGNING_CLIENT_CREATE", address }); this.#clientsByAddress.set(address, { ... }); } return this.#clientsByAddress.get(address)!;Multiple concurrent calls for the same
derivationIndexcan all pass theif (!this.#clientsByAddress.has(address))check before any of them sets the value, leading to duplicate client creation and potential race conditions.Based on learnings, this pattern without proper synchronization is unsafe. Consider using a mutex or atomic promise caching to ensure only one client is created per address.
🧹 Nitpick comments (2)
apps/api/src/billing/repositories/usage/usage.repository.ts (1)
147-147: Consider updating the error event name for consistency.The error event name
FAILED_TO_DERIVE_OLD_WALLET_ADDRESSuses "OLD" terminology, while the codebase has migrated to version-based naming (prevAddress,v1). For consistency, consider renaming to something likeFAILED_TO_DERIVE_V1_WALLET_ADDRESSorFAILED_TO_DERIVE_PREV_WALLET_ADDRESS.🔎 Proposed fix
- this.logger.error({ event: "FAILED_TO_DERIVE_OLD_WALLET_ADDRESS", address, walletId: userWallet.id, error }); + this.logger.error({ event: "FAILED_TO_DERIVE_V1_WALLET_ADDRESS", address, walletId: userWallet.id, error });apps/api/src/billing/services/tx-manager/tx-manager.service.ts (1)
38-47: Consider handling optional index in wallet factories more explicitly.The derived wallet factories use
walletIndex ?? 0when creating wallets:const v1DerivedWalletFactory: WalletFactory = (walletIndex?: number) => { return new Wallet(config.get("DERIVATION_WALLET_MNEMONIC_V1") ?? config.get("OLD_MASTER_WALLET_MNEMONIC"), walletIndex ?? 0); };Since
WalletFactorydeclares the parameter as optional (walletIndex?), and theWalletconstructor accepts an optionalindex, consider whether passing0explicitly is the intended behavior when no index is provided, or ifundefinedshould be passed to allow the Wallet class to handle the default behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
apps/api/drizzle/0024_new_nova.sqlapps/api/drizzle/meta/0024_snapshot.jsonapps/api/drizzle/meta/_journal.jsonapps/api/env/.env.funding-wallet-indexapps/api/src/billing/config/env.config.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.tsapps/api/src/billing/repositories/usage/usage.repository.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/billing/repositories/wallet-settings/wallet-settings.repository.tsapps/api/src/billing/services/balances/balances.service.tsapps/api/src/billing/services/managed-signer/managed-signer.service.spec.tsapps/api/src/billing/services/managed-signer/managed-signer.service.tsapps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.tsapps/api/src/billing/services/provider-cleanup/provider-cleanup.service.tsapps/api/src/billing/services/refill/refill.service.spec.tsapps/api/src/billing/services/refill/refill.service.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-balance-reload-check/wallet-balance-reload-check.handler.spec.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.spec.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.tsapps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.tsapps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.tsapps/api/src/provider/controllers/jwt-token/jwt-token.controller.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.tsapps/api/test/seeders/auto-top-up-deployment.seeder.tsapps/api/test/seeders/user-wallet.seeder.tsapps/api/test/services/local.config.jsapps/api/test/services/test-wallet.service.tsapps/api/test/setup-global-functional.ts
💤 Files with no reviewable changes (4)
- apps/api/env/.env.funding-wallet-index
- apps/api/test/seeders/auto-top-up-deployment.seeder.ts
- apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.ts
- apps/api/test/services/local.config.js
🚧 Files skipped from review as they are similar to previous changes (16)
- apps/api/src/billing/repositories/wallet-settings/wallet-settings.repository.ts
- apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts
- apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts
- apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
- apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts
- apps/api/src/billing/services/refill/refill.service.spec.ts
- apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts
- apps/api/src/provider/controllers/jwt-token/jwt-token.controller.ts
- apps/api/drizzle/0024_new_nova.sql
- apps/api/src/deployment/services/cached-balance/cached-balance.service.ts
- apps/api/src/billing/config/env.config.ts
- apps/api/src/billing/services/provider-cleanup/provider-cleanup.service.ts
- apps/api/src/billing/services/refill/refill.service.ts
- apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.ts
- apps/api/test/services/test-wallet.service.ts
- apps/api/drizzle/meta/0024_snapshot.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/api/test/setup-global-functional.tsapps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.tsapps/api/src/billing/services/balances/balances.service.tsapps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.tsapps/api/src/billing/repositories/usage/usage.repository.tsapps/api/src/billing/services/managed-signer/managed-signer.service.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.tsapps/api/src/billing/services/tx-manager/tx-manager.service.spec.tsapps/api/test/seeders/user-wallet.seeder.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.tsapps/api/src/billing/services/tx-manager/tx-manager.service.tsapps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under testUse
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.
**/*.spec.{ts,tsx}: Use<Subject>.namein the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references
Use either a method name or a condition starting with 'when' for nested suite descriptions in tests
Use present simple, 3rd person singular for test descriptions without prepending 'should'
Files:
apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.tsapps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.tsapps/api/src/billing/services/tx-manager/tx-manager.service.spec.tsapps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts
🧠 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/test/setup-global-functional.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.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/repositories/usage/usage.repository.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/billing/services/tx-manager/tx-manager.service.ts
🧬 Code graph analysis (5)
apps/api/test/setup-global-functional.ts (1)
apps/api/test/services/test-wallet.service.ts (1)
TestWalletService(21-171)
apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.ts (3)
apps/api/src/core/types/require.type.ts (1)
Require(1-3)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(18-22)apps/deploy-web/src/utils/apiUtils.ts (1)
balance(44-46)
apps/api/src/billing/services/balances/balances.service.ts (2)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(18-22)apps/api/src/billing/http-schemas/balance.schema.ts (1)
GetBalancesResponseOutput(17-17)
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/tx-manager/tx-manager.service.ts (5)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(9-57)apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (2)
BatchSigningClientService(57-360)SignAndBroadcastOptions(24-34)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
singleton(33-164)apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
singleton(26-134)apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.ts (3)
singleton(35-222)resources(167-205)resources(207-217)
⏰ 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). (2)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (37)
apps/api/drizzle/meta/_journal.json (1)
173-179: LGTM! Migration journal entry is correctly formatted.The new migration entry follows the established pattern with the correct sequential index (24), consistent version ("7"), and valid timestamp. This entry tracks the migration that removes the
is_old_walletcolumn fromuser_wallets, aligning with the PR's objective to migrate from a boolean flag to a version-based wallet system.apps/api/src/billing/repositories/usage/usage.repository.ts (1)
138-145: LGTM! Migration to version-based wallet system is correctly implemented.The updated call to
getDerivedWalletAddresswith{ walletVersion: "v1" }aligns with the PR's migration from boolean flags to explicit version-based configuration. The variable rename toprevAddressand its consistent usage throughout the conditional logic properly handle the address resolution for wallets created before the migration date.apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (1)
28-28: New WalletInitializerService correctly defaults to V2 wallets.The
createAndAuthorizeTrialSpendingandcreateWalletmethods both rely on the default V2 wallet version behavior fromTxManagerService. With no wallet version parameter specified, the underlyinggetDerivedWalletAddressmethod defaults to V2 as intended by the migration strategy.apps/api/test/seeders/user-wallet.seeder.ts (1)
26-27: LGTM: Seeder updated to reflect removal ofisOldWallet.The removal of
isOldWalletfrom the seededUserWalletOutputcorrectly aligns with the type changes across the codebase. The seeder now produces wallet objects consistent with the new schema.apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.ts (2)
23-23: LGTM: Type simplified to address-only requirement.The
InitializedWallettype correctly narrows to require only theaddressfield, removing the deprecatedisOldWalletdependency.
96-96: LGTM: Balance retrieval simplified.The call to
getFullBalanceInFiatnow correctly passes onlywallet.address, aligning with the updated service signature.apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.ts (1)
338-339: LGTM: Test fixture updated to match simplified wallet shape.The wallet object in the test setup correctly reflects the removal of
isOldWallet, aligning with the updatedInitializedWallettype in the handler.apps/api/test/setup-global-functional.ts (1)
6-6: LGTM: Test wallet initialization updated.The call to
init()without arguments correctly reflects the removal ofFUNDING_WALLET_INDEXfrom the configuration and the updatedTestWalletService.init()signature.apps/api/src/billing/services/balances/balances.service.ts (5)
58-61: LGTM: Signature simplified to address-only.The
getFreshLimitsmethod signature correctly drops theisOldWalletrequirement, accepting only the wallet address. This simplifies the API while maintaining functionality.
63-72: LGTM: Fee limit retrieval simplified.The method correctly:
- Accepts only
addressin the parameter- Calls
getFundingWalletAddress()without arguments (aligning with the updated signature)- Maintains the same fee allowance calculation logic
74-83: LGTM: Deployment limit retrieval simplified.The method correctly:
- Accepts only
addressin the parameter- Calls
getFundingWalletAddress()without arguments- Maintains the same deployment grant validation logic
102-117: LGTM: Balance retrieval methods simplified.Both
getFullBalanceMemoizedandgetFullBalancecorrectly:
- Accept only
addressas a parameter- Pass
{ address }togetFreshLimits- Maintain memoization behavior with simplified cache keys
- Preserve the balance calculation logic
119-132: LGTM: Fiat balance conversion simplified.The fiat balance methods correctly:
- Accept only
addressas a parameter- Delegate to the simplified
getFullBalancesignature- Maintain the USD conversion logic
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
108-108: LGTM: Balance retrieval call simplified.The call to
getFullBalanceMemoizedcorrectly passes only thecurrentAddress, aligning with the simplified service signature that no longer requiresisOldWallet.apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (5)
38-52: LGTM: Trial wallet creation simplified.The
createAndAuthorizeTrialSpendingmethod correctly:
- Removes the
useOldWalletparameter from its signature- Calls
getDerivedWalletAddresswith onlyaddressIndex- Passes simplified parameters to
authorizeSpendingThis aligns with the version-based wallet system where the version is determined by configuration rather than per-call flags.
54-59: LGTM: Wallet creation simplified.The
createWalletmethod correctly removes theuseOldWalletparameter and callsgetDerivedWalletAddresswith only theaddressIndex.
61-83: LGTM: Spending authorization simplified.The
authorizeSpendingmethod and its helper calls correctly:
- Remove the
useOldWalletparameter- Call
getFundingWalletAddress()without arguments- Pass simplified parameters to authorization helper methods
The authorization logic flow remains intact while adopting the cleaner API.
85-101: LGTM: Authorization helper methods simplified.Both
authorizeFeeSpendingandauthorizeDeploymentSpendingcorrectly:
- Remove the
useOldWalletparameter from their signatures- Call
executeFundingTxwithout the wallet version flag
103-133: LGTM: Grant revocation simplified.The
revokeAllmethod correctly:
- Removes the
useOldWalletparameter from its signature- Calls
getFundingWalletAddress()without arguments- Calls
executeFundingTxwithout the wallet version flagThe revocation logic remains complete and handles both fee allowances and deployment grants.
apps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.ts (1)
31-35: LGTM: Test expectation updated for simplified JWT token generation.The test correctly expects
generateJwtTokento be called without theisOldWalletparameter, aligning with the updated service signature that removes this flag.apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts (1)
208-208: LGTM!The test expectation correctly reflects the removal of the
useOldWalletboolean parameter from thesignAndBroadcastWithDerivedWalletmethod signature.apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts (2)
65-65: LGTM!The call to
executeDerivedTxcorrectly uses the new two-argument signature, consistent with the removal of theisOldWalletflag.
69-76: Verify the updatedauthorizeSpendingsignature across the codebase.The
authorizeSpendingmethod is now called with an object containingaddressandlimits. Ensure that all call sites ofauthorizeSpendinghave been updated to match this new signature.apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts (2)
77-95: LGTM!The test expectation correctly validates the new batched deposit structure, where
executeDerivedTxis called with the wallet ID and an array containing the deposit message objects. The structure properly includes all required fields:signer,id(withscopeandxid), anddeposit(withamountandsources).
280-315: LGTM!The test properly validates that multiple deployments sharing the same wallet are batched into a single
executeDerivedTxcall with an array of deposit messages, rather than separate calls. This demonstrates the correct batching behavior.apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts (2)
33-34: LGTM!The
generateJwtTokenmethod signature has been correctly simplified by removing theuseOldWalletparameter, and the call togetJwtTokennow only passes thewalletId.
53-54: LGTM!The
getJwtTokenmethod signature has been simplified to only acceptwalletId, and the internal call togetDerivedWallethas been updated accordingly. The memoization decorator remains in place, ensuring the caching behavior is preserved.apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts (1)
236-249: LGTM!The test setup correctly implements the new
walletResources-based structure, providing bothv1andv2configurations with their respective master wallets, signing clients, and derived wallet factories. The constructor call properly passes the new dependencies.apps/api/src/billing/services/managed-signer/managed-signer.service.ts (3)
46-55: LGTM!The
executeDerivedTxmethod has been correctly refactored to remove theuseOldWalletparameter. The granter address is now obtained viagetFundingWalletAddress()without requiring a flag, and the call tosignAndBroadcastWithDerivedWalletuses the simplified signature. Error handling remains intact.
57-63: LGTM!The
executeFundingTxmethod has been correctly simplified to remove theuseOldWalletparameter. The implementation now directly callssignAndBroadcastWithFundingWalletwith just the messages, relying on the internal wallet version resolution.
134-134: LGTM!The call to
executeDerivedTxcorrectly uses the new two-argument signature, consistent with the method's updated definition.apps/api/src/billing/services/tx-manager/tx-manager.service.ts (6)
21-29: LGTM!The new type definitions clearly structure the wallet version system. The
WalletVersionunion type restricts values to"v1"or"v2", andWalletResourcesproperly maps each version to its configuration containing the master wallet, signing client, and derived wallet factory.
87-91: LGTM!The
#getWalletResourceshelper method cleanly encapsulates the wallet version resolution logic, defaulting to v2 and returning the appropriate version configuration.
93-101: LGTM!The methods
signAndBroadcastWithFundingWalletandgetFundingWalletAddresscorrectly use the new wallet resources system via the#getWalletResourceshelper to access the master wallet and signing client.
103-114: LGTM!The
signAndBroadcastWithDerivedWalletmethod properly handles client lifecycle management, cleaning up cached clients when there are no pending transactions. The try-finally pattern ensures cleanup occurs even if the transaction fails.
135-138: LGTM!The
getDerivedWalletmethod correctly retrieves the derived wallet factory from the wallet resources based on the provided options (or default version) and invokes it with the given index.
33-62: Backward compatibility of mnemonic fallbacks is correctly implemented.The wallet resource initialization properly handles configuration with optional
*_V1and*_V2keys that fall back to required base keys. The environment schema definesFUNDING_WALLET_MNEMONIC,OLD_MASTER_WALLET_MNEMONIC, andDERIVATION_WALLET_MNEMONICas required, whileFUNDING_WALLET_MNEMONIC_V1/V2andDERIVATION_WALLET_MNEMONIC_V1/V2are optional. Existing deployments without the versioned keys will continue to work using the base keys.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2432 +/- ##
==========================================
- Coverage 51.27% 50.92% -0.35%
==========================================
Files 1072 1062 -10
Lines 29404 29013 -391
Branches 6490 6433 -57
==========================================
- Hits 15077 14775 -302
+ Misses 14027 13959 -68
+ Partials 300 279 -21
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
ygrishajev
force-pushed
the
feature/wallet
branch
from
8375c26 to
6a1ce52
Compare
ygrishajev
force-pushed
the
feature/wallet
branch
2 times, most recently
from
426eae6 to
10474e4
Compare
…system Replace the `isOldWallet` boolean flag with a version-based wallet system (`v1` | `v2`) to support gradual migration and improve maintainability. Changes: - Remove `isOldWallet` column from `user_wallets` table - Remove `FUNDING_WALLET_INDEX` config (index always 0 for funding wallets) - Add optional V1/V2 wallet mnemonic config keys for gradual migration: - `FUNDING_WALLET_MNEMONIC_V1` / `FUNDING_WALLET_MNEMONIC_V2` - `DERIVATION_WALLET_MNEMONIC_V1` / `DERIVATION_WALLET_MNEMONIC_V2` - Refactor `TxManagerService` to use `WalletResources` provider with version-based access instead of separate injection tokens - Update all services to use `WalletOptions` with `walletVersion` instead of `useOldWallet` boolean parameter - Default to V2 (new system) while maintaining V1 backward compatibility - Update all tests and test utilities to match new structure Backward compatibility: - V1 wallets fall back to `OLD_MASTER_WALLET_MNEMONIC` if V1 keys not set - V2 wallets fall back to `FUNDING_WALLET_MNEMONIC` / `DERIVATION_WALLET_MNEMONIC` - Existing deployments continue working without config changes closes #2298
ygrishajev
force-pushed
the
feature/wallet
branch
from
10474e4 to
98a9123
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/billing/config/env.config.ts (1)
6-6: Remove the obsolete dotenv config path.The file
env/.env.funding-wallet-indexdoes not exist, and no references toFUNDING_WALLET_INDEXremain in the codebase after its removal. The dotenv config on line 6 should be deleted since it serves no purpose.apps/api/src/billing/services/tx-manager/tx-manager.service.ts (1)
120-133: Potential race condition in client caching.The pattern in
#getClientchecks the cache and then performs an async operation without synchronization:if (!this.#clientsByAddress.has(address)) { // Multiple concurrent calls can pass this check this.#clientsByAddress.set(address, { address, client: this.batchSigningClientServiceFactory(wallet) }); }Multiple concurrent calls with the same
derivationIndexcan all pass thehascheck before any sets the value, leading to duplicate client creation. While not catastrophic (the factory is synchronous), this wastes resources and could cause issues if the factory were more expensive or had side effects.🔎 Consider using atomic promise caching
async #getClient(derivationIndex: number): Promise<CachedClient> { const wallet = this.getDerivedWallet(derivationIndex); const address = await wallet.getFirstAddress(); if (!this.#clientsByAddress.has(address)) { this.logger.debug({ event: "DERIVED_SIGNING_CLIENT_CREATE", address }); - this.#clientsByAddress.set(address, { - address, - client: this.batchSigningClientServiceFactory(wallet) - }); + const cachedClient = { + address, + client: this.batchSigningClientServiceFactory(wallet) + }; + // Re-check after creation to avoid overwriting + if (!this.#clientsByAddress.has(address)) { + this.#clientsByAddress.set(address, cachedClient); + } } return this.#clientsByAddress.get(address)!; }Alternatively, use a mutex library or ensure single-threaded access to this method.
Based on learnings, this caching pattern without proper synchronization is race condition unsafe.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
apps/api/drizzle/0024_new_nova.sqlapps/api/drizzle/meta/0024_snapshot.jsonapps/api/drizzle/meta/_journal.jsonapps/api/env/.env.funding-wallet-indexapps/api/src/billing/config/env.config.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.tsapps/api/src/billing/repositories/usage/usage.repository.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/billing/repositories/wallet-settings/wallet-settings.repository.tsapps/api/src/billing/services/balances/balances.service.tsapps/api/src/billing/services/managed-signer/managed-signer.service.spec.tsapps/api/src/billing/services/managed-signer/managed-signer.service.tsapps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.tsapps/api/src/billing/services/provider-cleanup/provider-cleanup.service.tsapps/api/src/billing/services/refill/refill.service.spec.tsapps/api/src/billing/services/refill/refill.service.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-balance-reload-check/wallet-balance-reload-check.handler.spec.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.spec.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.tsapps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.tsapps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.tsapps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.tsapps/api/src/provider/controllers/jwt-token/jwt-token.controller.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.tsapps/api/test/seeders/auto-top-up-deployment.seeder.tsapps/api/test/seeders/user-wallet.seeder.tsapps/api/test/services/local.config.jsapps/api/test/services/test-wallet.service.tsapps/api/test/setup-global-functional.ts
💤 Files with no reviewable changes (4)
- apps/api/test/seeders/auto-top-up-deployment.seeder.ts
- apps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.ts
- apps/api/test/services/local.config.js
- apps/api/env/.env.funding-wallet-index
✅ Files skipped from review due to trivial changes (1)
- apps/api/drizzle/meta/0024_snapshot.json
🚧 Files skipped from review as they are similar to previous changes (16)
- apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts
- apps/api/drizzle/0024_new_nova.sql
- apps/api/src/deployment/services/cached-balance/cached-balance.service.spec.ts
- apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts
- apps/api/test/seeders/user-wallet.seeder.ts
- apps/api/src/billing/services/refill/refill.service.ts
- apps/api/src/billing/repositories/wallet-settings/wallet-settings.repository.ts
- apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.spec.ts
- apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts
- apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts
- apps/api/src/billing/controllers/wallet/wallet.controller.ts
- apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts
- apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts
- apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts
- apps/api/src/billing/services/refill/refill.service.spec.ts
- apps/api/src/provider/controllers/jwt-token/jwt-token.controller.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany. Always define the proper TypeScript types.
Never use deprecated methods from libraries.
Don't add unnecessary comments to the code.
Files:
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/provider/controllers/jwt-token/jwt-token.controller.tsapps/api/test/services/test-wallet.service.tsapps/api/src/billing/config/env.config.tsapps/api/src/billing/services/managed-signer/managed-signer.service.tsapps/api/src/billing/repositories/usage/usage.repository.tsapps/api/src/billing/services/provider-cleanup/provider-cleanup.service.tsapps/api/test/setup-global-functional.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.tsapps/api/src/billing/services/balances/balances.service.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.tsapps/api/src/billing/services/tx-manager/tx-manager.service.tsapps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under testUse
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.
**/*.spec.{ts,tsx}: Use<Subject>.namein the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references
Use either a method name or a condition starting with 'when' for nested suite descriptions in tests
Use present simple, 3rd person singular for test descriptions without prepending 'should'
Files:
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: ygrishajev
Repo: akash-network/console PR: 2432
File: apps/api/src/billing/services/tx-manager/tx-manager.service.ts:116-118
Timestamp: 2025-12-29T12:12:23.112Z
Learning: In the Akash Console codebase (akash-network/console), v1 wallets are only used for read-only purposes such as retrieving addresses. All transaction signing operations use v2 wallets exclusively. Therefore, methods like `getDerivedWalletAddress` need to accept `WalletOptions` to support both v1 and v2 address lookups, while transaction signing methods like `signAndBroadcastWithDerivedWallet` correctly default to v2 without requiring a wallet version parameter.
📚 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/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/test/services/test-wallet.service.tsapps/api/test/setup-global-functional.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts
📚 Learning: 2025-12-29T12:12:23.112Z
Learnt from: ygrishajev
Repo: akash-network/console PR: 2432
File: apps/api/src/billing/services/tx-manager/tx-manager.service.ts:116-118
Timestamp: 2025-12-29T12:12:23.112Z
Learning: In the Akash Console codebase (akash-network/console), v1 wallets are only used for read-only purposes such as retrieving addresses. All transaction signing operations use v2 wallets exclusively. Therefore, methods like `getDerivedWalletAddress` need to accept `WalletOptions` to support both v1 and v2 address lookups, while transaction signing methods like `signAndBroadcastWithDerivedWallet` correctly default to v2 without requiring a wallet version parameter.
Applied to files:
apps/api/test/services/test-wallet.service.tsapps/api/src/billing/services/managed-signer/managed-signer.service.tsapps/api/src/billing/repositories/usage/usage.repository.tsapps/api/src/billing/services/provider-cleanup/provider-cleanup.service.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.tsapps/api/src/billing/services/balances/balances.service.tsapps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.tsapps/api/src/billing/services/tx-manager/tx-manager.service.tsapps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.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/repositories/usage/usage.repository.tsapps/api/src/deployment/services/cached-balance/cached-balance.service.tsapps/api/src/billing/services/tx-manager/tx-manager.service.ts
📚 Learning: 2025-06-03T15:06:34.211Z
Learnt from: baktun14
Repo: akash-network/console PR: 1428
File: apps/api/src/deployment/controllers/deployment/deployment.controller.ts:0-0
Timestamp: 2025-06-03T15:06:34.211Z
Learning: The `getByOwnerAndDseq` method in `apps/api/src/deployment/controllers/deployment/deployment.controller.ts` is intentionally public without the `Protected` decorator because it serves public blockchain data from an indexer, following the pattern of public blockchain APIs.
Applied to files:
apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts
🧬 Code graph analysis (7)
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(38-52)
apps/api/test/services/test-wallet.service.ts (1)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(9-57)
apps/api/src/billing/services/balances/balances.service.ts (2)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(18-22)apps/api/src/billing/http-schemas/balance.schema.ts (1)
GetBalancesResponseOutput(17-17)
apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.ts (3)
apps/api/src/core/types/require.type.ts (1)
Require(1-3)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(18-22)apps/deploy-web/src/utils/apiUtils.ts (1)
balance(44-46)
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (2)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(9-57)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)
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)
⏰ 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). (3)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (35)
apps/api/drizzle/meta/_journal.json (1)
172-179: LGTM! Standard migration journal entry.The new migration entry is correctly formatted and follows the established pattern. The migration index increments properly from 23 to 24, and all required fields (version, timestamp, tag, breakpoints) are present and consistent with previous entries.
apps/api/test/services/test-wallet.service.ts (2)
51-55: LGTM! Simplified initialization aligns with PR objectives.The removal of the
walletIndexparameter is consistent with the PR objective to eliminateFUNDING_WALLET_INDEXand default to index 0. TheWalletclass constructor handles the optional index parameter correctly.
62-71: LGTM! Wallet instantiation correctly uses default index.The removal of the
walletIndexparameter from both the method signature and theWalletinstantiation is correct. Wallets will now use the default HD path (index 0), which aligns with the PR's goal of standardizing on a single wallet index for testing.apps/api/test/setup-global-functional.ts (1)
6-6: LGTM! Consistent with test wallet service refactoring.The removal of the
FUNDING_WALLET_INDEXargument correctly reflects the updatedTestWalletService.init()signature and aligns with the PR objective to eliminate wallet index configuration.apps/api/src/billing/config/env.config.ts (1)
12-15: LGTM: Version-based mnemonic fields added correctly.The optional versioned mnemonic fields align with the PR objectives and enable backward compatibility by allowing fallback to the original required fields (
OLD_MASTER_WALLET_MNEMONIC,FUNDING_WALLET_MNEMONIC,DERIVATION_WALLET_MNEMONIC).apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts (2)
18-23: LGTM! Type definition correctly updated.The removal of
isOldWalletfrom theAutoTopUpDeploymenttype aligns with the schema migration and PR objectives to move to a version-based wallet system.
67-84: Query correctly updated to excludeisOldWallet.The
findAutoTopUpDeploymentsByOwnermethod's select projection exactly matches the updatedAutoTopUpDeploymenttype definition (id, walletId, dseq, address). No references toisOldWalletremain in the codebase, and all consumers—including the test seeder, draining deployment service, and top-up service—are compatible with the type structure. The changes are consistent with the schema migration.apps/api/src/billing/repositories/usage/usage.repository.ts (1)
139-145: LGTM! Correct use of version-based wallet lookup for historical compatibility.The refactor from
oldAddresswith a boolean flag toprevAddresswith{ walletVersion: "v1" }correctly implements version-based wallet addressing. The v1 wallet is appropriately used for read-only address lookup to aggregate historical billing data for wallets created before the migration date.Based on learnings, v1 wallets are only used for read-only purposes like address retrieval, which is exactly what's happening here.
apps/api/src/billing/services/provider-cleanup/provider-cleanup.service.ts (1)
59-71: LGTM! Correctly updated to version-based wallet system.The removal of the
useOldWalletparameter from bothexecuteDerivedTxandauthorizeSpendingcalls is correct. Transaction signing operations now default to v2 wallets, and theauthorizeSpendingcall has been properly updated to use the new single-object parameter structure.Based on learnings, transaction signing operations use v2 wallets exclusively, so removing the wallet version parameter from these calls is appropriate.
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (3)
46-55: LGTM! Transaction signing correctly defaults to v2 wallets.The removal of the
useOldWalletparameter fromexecuteDerivedTxis correct. The method now callsgetFundingWalletAddress()andsignAndBroadcastWithDerivedWalletwithout wallet version parameters, correctly defaulting to v2 for all transaction signing operations.Based on learnings, all transaction signing operations use v2 wallets exclusively, making this parameter removal appropriate.
57-63: LGTM! Funding transaction signing correctly simplified.The removal of the
useOldWalletparameter fromexecuteFundingTxis correct. The method now defaults to v2 for funding wallet operations.
127-127: LGTM! Call site correctly updated.The call to
executeDerivedTxhas been correctly updated to pass onlywalletIdandmessages, matching the new method signature.apps/api/src/billing/services/tx-manager/tx-manager.service.ts (4)
33-62: LGTM! Well-architected wallet resource factory with proper backward compatibility.The
WALLET_RESOURCESfactory correctly centralizes wallet management using a version-based system. Key strengths:
- Config fallback chain maintains backward compatibility (v1 →
OLD_MASTER_WALLET_MNEMONIC, v2 →FUNDING_WALLET_MNEMONIC)- Each version has isolated signing clients with descriptive logging contexts
instancePerContainerCachingFactoryensures proper singleton behavior- Version-specific derived wallet factories enable flexible wallet creation
77-91: LGTM! Clean version selection helper.The
#getWalletResourceshelper provides a clean abstraction for selecting wallet resources by version, correctly defaulting to v2.
93-101: LGTM! Funding wallet operations correctly use v2 by default.Both methods correctly use the wallet resources system and default to v2 wallets for funding operations.
103-118: LGTM! Correct asymmetric API design for version handling.The intentional asymmetry where:
signAndBroadcastWithDerivedWalletdoes NOT acceptWalletOptions(always uses v2 for signing)getDerivedWalletAddressDOES acceptWalletOptions(can read v1 or v2 addresses)is correct and aligns with the wallet migration strategy.
Based on learnings, v1 wallets are read-only (address lookup), while all transaction signing uses v2 wallets exclusively.
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (5)
38-52: LGTM! Trial wallet creation correctly defaults to v2.The method now creates trial wallets using v2 by default, which is correct since trials are only created for new wallets. The removal of the
useOldWalletparameter simplifies the API while maintaining correct behavior.
54-59: LGTM! Wallet creation correctly defaults to v2.The simplified parameter structure correctly defaults to v2 for new wallet creation.
61-83: LGTM! Cleaner API with single parameter object.The refactored
authorizeSpendingmethod uses a cleaner single-parameter object design and correctly defaults to v2 for funding wallet operations.
85-101: LGTM! Internal authorization methods correctly updated.Both private authorization methods have been correctly updated to remove
useOldWalletparameters and default to v2 for funding transactions.
103-133: LGTM! Revocation flow correctly simplified.The
revokeAllmethod has been correctly simplified to remove theuseOldWalletparameter, defaulting to v2 for all funding wallet operations.apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.ts (2)
28-28: LGTM! Call site correctly updated.The removal of
useOldWallet: falsecorrectly matches the updatedcreateAndAuthorizeTrialSpendingsignature, which now defaults to v2.
55-55: LGTM! Call site correctly updated.The removal of
useOldWallet: falsecorrectly matches the updatedcreateWalletsignature, which now defaults to v2.apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (2)
39-39: LGTM! Test expectation correctly updated.The test expectation correctly verifies the new signature with only
addressIndexparameter.
84-84: LGTM! Test expectation correctly updated.The test expectation correctly verifies the new signature with only
addressIndexparameter.apps/api/src/provider/controllers/jwt-token/jwt-token.controller.ts (1)
25-29: LGTM! Call site correctly updated.The removal of the
useOldWalletparameter correctly matches the updatedgenerateJwtTokensignature, which now defaults to v2 for token generation.apps/api/src/billing/services/wallet-balance-reload-check/wallet-balance-reload-check.handler.ts (2)
23-23: LGTM! Type definition correctly simplified.The removal of
isOldWalletfromInitializedWalletaligns with the migration to version-based wallet handling inTxManagerService. The handler now correctly requires only the wallet address for balance operations.
96-96: LGTM! Balance retrieval correctly updated.The call to
getFullBalanceInFiatnow correctly passes only the wallet address, matching the simplified signature of the balance service method.apps/api/src/deployment/services/cached-balance/cached-balance.service.ts (1)
27-36: LGTM! Cache service correctly simplified.The removal of the
isOldWalletparameter from both the method signature and the internalgetFreshLimitscall aligns with the simplified balance service API. The cache logic remains sound with address-only keys.apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts (2)
33-50: LGTM! JWT token generation correctly simplified.The removal of
useOldWalletfrom the JWT token generation flow is appropriate. Based on learnings, all transaction signing operations (including provider JWT token signing) use v2 wallets exclusively, making this parameter unnecessary.
52-59: LGTM! Private method correctly updated.The simplified signature and call to
getDerivedWallet(walletId)correctly default to v2 wallet behavior. The memoization remains functional with the simpler cache key.apps/api/src/billing/services/balances/balances.service.ts (4)
58-61: LGTM! Public API correctly simplified.The
getFreshLimitssignature now requires only the wallet address, which is sufficient for querying limits. This aligns with the version-agnostic balance operations.
63-83: LGTM! Limit retrieval methods correctly updated.Both
retrieveAndCalcFeeLimitandretrieveDeploymentLimitnow correctly callgetFundingWalletAddress()without parameters. The wallet version handling is now appropriately abstracted withinTxManagerService.
102-117: LGTM! Balance methods correctly simplified.Both the memoized and non-memoized
getFullBalancemethods now use simplified signatures with address-only parameters. The memoization cache key is appropriately simplified, and the balance calculation logic remains intact.
119-132: LGTM! Fiat balance methods correctly simplified.The fiat balance methods mirror the simplifications made to the token balance methods. The memoization, delegation chain, and conversion logic all remain correct with the simplified signatures.
Replace the
isOldWalletboolean flag with a version-based wallet system (v1|v2) to support gradual migration and improve maintainability.Changes:
isOldWalletcolumn fromuser_walletstableFUNDING_WALLET_INDEXconfig (index always 0 for funding wallets)FUNDING_WALLET_MNEMONIC_V1/FUNDING_WALLET_MNEMONIC_V2DERIVATION_WALLET_MNEMONIC_V1/DERIVATION_WALLET_MNEMONIC_V2TxManagerServiceto useWalletResourcesprovider with version-based access instead of separate injection tokensWalletOptionswithwalletVersioninstead ofuseOldWalletboolean parameterBackward compatibility:
OLD_MASTER_WALLET_MNEMONICif V1 keys not setFUNDING_WALLET_MNEMONIC/DERIVATION_WALLET_MNEMONICcloses #2298
Summary by CodeRabbit
Refactor
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.