feat(billing): splits master wallets and consolidates them in a single place#2254
feat(billing): splits master wallets and consolidates them in a single place#2254ygrishajev merged 2 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReplace direct master-wallet and signing-client wiring with a new TxManagerService centralizing funding and derived wallet signing; rename MASTER_WALLET_MNEMONIC → FUNDING_WALLET_MNEMONIC and add DERIVATION_WALLET_MNEMONIC; remove dedupe signing client and ChainErrorService dependency; update DI, method names, providers, and tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ManagedSigner as ManagedSignerService
participant TxMgr as TxManagerService
participant DerivedClient as DerivedSigningClient
participant FundingClient as FundingSigningClient
participant Chain
Client->>ManagedSigner: executeDerivedDecodedTxByUserId(userId, messages)
ManagedSigner->>TxMgr: getDerivedWalletAddress(index)
TxMgr-->>ManagedSigner: derivedAddress
ManagedSigner->>TxMgr: signAndBroadcastWithDerivedWallet(index, messages)
alt no cached client
TxMgr->>DerivedClient: create client for derivedAddress
Note right of DerivedClient `#E6FFEA`: DERIVED_SIGNING_CLIENT_CREATE
DerivedClient-->>TxMgr: client ready
end
DerivedClient->>Chain: signAndBroadcast(messages)
Chain-->>DerivedClient: txResult
DerivedClient-->>TxMgr: txResult
alt no pending txs after result
Note right of TxMgr `#FFF4E6`: cleanup cached client & log
end
TxMgr-->>ManagedSigner: txResult
ManagedSigner-->>Client: txResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
Comment |
ygrishajev
force-pushed
the
feature/billing-master-wallets
branch
from
103de10 to
e4b64bf
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "node": "24.11.1", | ||
| "npm": "^11.2.0" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🔄 Carefully review the package-lock.json diff
Resolve the comment if everything is ok
+ node_modules/@types/glob-to-regexp 0.4.4
+ node_modules/fetch-mock 12.6.0
+ node_modules/regexparam 3.0.0 
❌ 4 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: 1
🧹 Nitpick comments (9)
apps/api/env/.env.unit.test (1)
3-4: Unit-test env updated correctly; dotenv key-order warning is purely cosmeticThe new
FUNDING_WALLET_MNEMONICandDERIVATION_WALLET_MNEMONICentries line up with the updatedenvSchema, so unit tests should parse config fine.The dotenv‑linter “UnorderedKey” warnings are non-functional; if you want a clean lint run you can reorder these two lines as suggested, but there’s no behavioral impact either way.
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
34-137: TxManager-based funding/derived flows look consistent; a couple of small cleanupsThe refactor here reads well:
createAndAuthorizeTrialSpending/createWalletnow derive addresses viatxManagerService.getDerivedWalletAddress(addressIndex), so this service no longer needs direct access to mnemonics or wallet construction.authorizeSpending,authorizeFeeSpending,authorizeDeploymentSpending, andrevokeAllconsistently use the funding wallet (getFundingWalletAddress) as the granter and execute viamanagedSignerService.executeFundingTx(...), which matches the new split‑wallet model.Two minor follow-ups you might consider:
PREFIXandHD_PATHare now unused in this class; if they’re not planned for future use, dropping them will keep the service focused.- In
authorizeFeeSpending/revokeAllyou checkhasFeeAllowance(any allowance) rather thanhasValidFeeAllowance. If the intent is specifically “revoke only active / non‑expired allowances before re‑granting”, you may want to switch to the latter; if you do want to revoke even expired ones, current behavior is fine but perhaps worth an inline comment.apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts (1)
47-47: executeDerivedTx migration in top-up tests is coherentThe setup stub and all expectations now consistently use
executeDerivedTx(walletId, msgs), with unchanged payloads and call counts across success, dry-run, empty, and error scenarios, so the behavioral coverage is preserved. If you later align terminology from “master wallet” to “funding wallet” in logs and test descriptions, that’s probably best handled in a focused follow-up issue rather than this PR. Based on learningsAlso applies to: 108-110, 210-210, 237-238, 245-246, 277-278, 379-380, 407-408, 442-443, 464-465
apps/api/src/billing/services/chain-error/chain-error.service.spec.ts (1)
8-8: TxManagerService injection into ChainErrorService tests is soundThe tests now construct
ChainErrorServicewith a mockedTxManagerService, andgetFundingWalletAddressis safely stubbed, so existing toAppError scenarios remain valid under the new dependency. If you want stronger coupling to the funding-wallet behavior, you could optionally assert thattxManagerService.getFundingWalletAddressis called in the insufficient-funds cases.Also applies to: 175-179, 183-184, 191-196
apps/api/src/billing/services/financial-stats/financial-stats.service.ts (1)
11-19: Funding-wallet-based implementation of getMasterWalletBalanceUsdc looks correctSwitching
getMasterWalletBalanceUsdcto resolve the address viatxManagerService.getFundingWalletAddress()and then querying balances throughCosmosHttpServicekeeps behavior intact while removing mnemonic handling from this service. At some point you might want to rename this method to reference the “funding” wallet rather than “master” for clarity, likely in a dedicated follow-up PR. Based on learningsAlso applies to: 25-27
apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts (1)
21-23: executeDerivedDecodedTxByUserId tests align well with TxManagerService-based designThe spec now consistently targets
executeDerivedDecodedTxByUserIdand wiresTxManagerServicein via a strongly typed mock (signAndBroadcastWithDerivedWallet,getFundingWalletAddress), while preserving all behavioral coverage around allowances, feature flags, domain events, and chain-error handling. The success-path assertion againstsignAndBroadcastWithDerivedWallet(wallet.id, messages, { fee: { granter: expect.any(String) } })is especially helpful to lock in the new calling convention.Two optional nits:
- Consider importing
createAkashAddressvia the existing@test/seeders/...alias for consistency with other tests.- If you later care about the exact granter value, you could bind the generated funding address to a constant reused both in the
getFundingWalletAddressmock and in an expectation.Also applies to: 29-40, 73-74, 93-94, 120-129, 157-166, 197-207, 242-254, 265-266, 298-308, 334-340, 361-369, 397-406, 434-443, 474-483, 498-503, 530-533, 551-552
apps/api/src/billing/services/balances/balances.service.ts (1)
7-8: BalancesService now correctly resolves grants via TxManagerService funding walletSwitching both
retrieveAndCalcFeeLimitandretrieveDeploymentLimitto usetxManagerService.getFundingWalletAddress()keeps all grant lookups aligned with the new funding wallet source of truth and preserves prior logic around fee/deployment allowances. As a small clean-up, you could maketxManagerServiceprivate readonlyin the constructor for consistency with other injected dependencies since it’s never reassigned.Also applies to: 13-19, 55-75
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (2)
19-28: Validate FUNDING_WALLET_MNEMONIC presence and funding wallet indexEagerly constructing
fundingWalletandfundingSigningClientin field initializers is fine, but it does couple TxManagerService resolution to a validFUNDING_WALLET_MNEMONICand the hard‑coded derivation index1. It would be safer to mirror the explicit guard you already have forDERIVATION_WALLET_MNEMONIC(e.g., throw a clear error if the funding mnemonic is empty) and to double‑check that index1indeed matches the on‑chain funding account you provisioned; otherwise funding and grants will silently point at the wrong account.Also applies to: 31-38, 44-47, 80-87
48-59: Client cache and cleanup logic looks good; consider explicit client disposal on evictionThe per‑address cache with lazy creation in
getClientand cleanup insignAndBroadcastWithDerivedWalletwhen!client.hasPendingTransactionsis a solid approach and should avoid unbounded growth. As a non‑blocking improvement, when you delete an entry fromclientsByAddress, consider also exposing and calling a lightweight “dispose”/“disconnect” onBatchSigningClientServiceso any underlying RPC resources are explicitly released rather than relying on GC alone. Given this would affect other callers of the signing client, it’s probably best tracked as a follow‑up issue rather than in this PR. Based on learningsAlso applies to: 65-78
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
apps/api/env/.env.unit.test(1 hunks)apps/api/package.json(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/lib/batch-signing-client/batch-signing-client.service.spec.ts(1 hunks)apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts(1 hunks)apps/api/src/billing/providers/signing-client.provider.ts(0 hunks)apps/api/src/billing/providers/wallet.provider.ts(1 hunks)apps/api/src/billing/services/balances/balances.service.ts(5 hunks)apps/api/src/billing/services/chain-error/chain-error.service.spec.ts(2 hunks)apps/api/src/billing/services/chain-error/chain-error.service.ts(3 hunks)apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts(0 hunks)apps/api/src/billing/services/financial-stats/financial-stats.service.ts(1 hunks)apps/api/src/billing/services/index.ts(0 hunks)apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts(20 hunks)apps/api/src/billing/services/managed-signer/managed-signer.service.ts(3 hunks)apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts(5 hunks)apps/api/src/billing/services/provider-cleanup/provider-cleanup.service.ts(2 hunks)apps/api/src/billing/services/tx-manager/tx-manager.service.ts(1 hunks)apps/api/src/billing/types/wallet.type.ts(0 hunks)apps/api/src/certificate/services/certificate/certificate.service.spec.ts(4 hunks)apps/api/src/certificate/services/certificate/certificate.service.ts(1 hunks)apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts(1 hunks)apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts(3 hunks)apps/api/src/deployment/services/lease/lease.service.ts(1 hunks)apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts(2 hunks)apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts(9 hunks)apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts(1 hunks)apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts(5 hunks)apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts(3 hunks)apps/api/test/custom-jest-environment.ts(2 hunks)apps/api/test/functional/deployments.spec.ts(4 hunks)apps/api/test/functional/sign-and-broadcast-tx.spec.ts(2 hunks)apps/api/test/functional/stale-anonymous-users-cleanup.spec.ts(3 hunks)apps/api/test/functional/start-trial.spec.ts(2 hunks)apps/api/test/services/test-wallet.service.ts(1 hunks)apps/api/test/setup-global-functional.ts(1 hunks)
💤 Files with no reviewable changes (4)
- apps/api/src/billing/services/index.ts
- apps/api/src/billing/types/wallet.type.ts
- apps/api/src/billing/providers/signing-client.provider.ts
- apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts
🧰 Additional context used
🧠 Learnings (3)
📓 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-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/test/custom-jest-environment.tsapps/api/test/services/test-wallet.service.tsapps/api/test/functional/stale-anonymous-users-cleanup.spec.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/test/functional/start-trial.spec.tsapps/api/src/billing/services/chain-error/chain-error.service.spec.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.tsapps/api/src/certificate/services/certificate/certificate.service.spec.tsapps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts
📚 Learning: 2025-05-28T20:42:58.200Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 1372
File: apps/api/src/dashboard/services/stats/stats.service.ts:0-0
Timestamp: 2025-05-28T20:42:58.200Z
Learning: The user successfully implemented CosmosHttpService with retry logic using axiosRetry, exponential backoff, and proper error handling for Cosmos API calls, replacing direct axios usage in StatsService.
Applied to files:
apps/api/src/billing/services/financial-stats/financial-stats.service.ts
🧬 Code graph analysis (12)
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
apps/api/src/billing/http-schemas/tx.schema.ts (1)
SignTxResponseOutput(34-34)
apps/api/test/custom-jest-environment.ts (3)
apps/api/test/setup-global-functional.ts (1)
localConfig(4-8)apps/api/test/services/test-wallet.service.ts (1)
TestWalletService(21-165)apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (5)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (2)
BatchSigningClientService(57-360)SignAndBroadcastOptions(24-34)apps/api/src/billing/lib/signing-stargate-client-factory/signing-stargate-client.factory.ts (1)
createSigningStargateClient(57-61)apps/api/src/billing/providers/wallet.provider.ts (1)
WALLET_FACTORY(7-7)apps/api/src/billing/lib/wallet/wallet.ts (2)
walletFactory(68-68)WalletFactory(67-67)apps/api/src/billing/providers/type-registry.provider.ts (1)
TYPE_REGISTRY(34-34)
apps/api/src/billing/services/financial-stats/financial-stats.service.ts (2)
apps/api/src/billing/services/balances/balances.service.ts (1)
singleton(11-106)apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
singleton(26-138)
apps/api/test/services/test-wallet.service.ts (1)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)
apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts (1)
apps/api/test/services/stub.ts (1)
stub(2-2)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (3)
packages/http-sdk/src/authz/authz-http.service.ts (1)
AuthzHttpService(56-218)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-16)
apps/api/src/billing/services/chain-error/chain-error.service.spec.ts (1)
packages/http-sdk/src/balance/balance-http.service.ts (1)
BalanceHttpService(20-29)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (1)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
BatchSigningClientService(57-360)
apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts (1)
apps/api/test/seeders/akash-address.seeder.ts (1)
createAkashAddress(4-12)
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (3)
packages/http-sdk/src/lease/lease-http.service.ts (2)
LeaseHttpService(72-96)hasLeases(89-95)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(17-21)apps/api/src/billing/events/trial-deployment-lease-created.ts (1)
TrialDeploymentLeaseCreated(4-21)
🪛 dotenv-linter (4.0.0)
apps/api/env/.env.unit.test
[warning] 3-3: [UnorderedKey] The FUNDING_WALLET_MNEMONIC key should go before the POSTGRES_DB_URI key
(UnorderedKey)
[warning] 4-4: [UnorderedKey] The DERIVATION_WALLET_MNEMONIC key should go before the FUNDING_WALLET_MNEMONIC 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). (5)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
🔇 Additional comments (37)
apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts (1)
54-54: No functional changes; note pre-existing type-safety concern.The field reordering in the SELECT projection (line 54) is cosmetic and doesn't affect logic. However, the adjacent BUGALERT comment (line 67) flags a pre-existing issue: the LEFT JOINs (lines 58-59) can produce null values for
walletId, but theAutoTopUpDeploymenttype expectswalletId: number(line 21). The unsafe type cast on line 68 compounds this risk.Verify whether the LEFT JOINs should be INNER JOINs based on your business requirements, and align the type definition and cast accordingly.
apps/api/src/deployment/services/lease/lease.service.ts (1)
28-41: Switch to derived signer for lease txs looks correctUsing
executeDerivedDecodedTxByUserId(wallet.userId, leaseMessages)here cleanly aligns lease creation with the derived-wallet flow introduced in this PR, without changing the message construction or manifest send logic.apps/api/src/billing/config/env.config.ts (1)
5-8: New funding/derivation mnemonics are now required env varsMaking
FUNDING_WALLET_MNEMONICandDERIVATION_WALLET_MNEMONICrequired is consistent with the split‑wallet design, but it means any environment that parses this schema (local dev, tests, CI, deployed envs) must define both, or startup will fail atenvSchema.parse.Please double‑check:
- All
.envvariants used by the API (including functional/e2e test env files), and- Any deployment/runtime config (Helm, Doppler, etc.)
are updated to provide these two vars and no longer rely on
MASTER_WALLET_MNEMONIC.apps/api/src/billing/providers/wallet.provider.ts (1)
1-10: Simplified wallet DI registration looks goodRegistering
WALLET_FACTORYdirectly withwalletFactoryviacontainer.register(... useValue: walletFactory)cleanly preserves the factory injection without the old master‑wallet wiring. No issues from a DI or typing standpoint.apps/api/package.json (1)
86-86: Let me verify if there are other test files importing fetch-mock to confirm the scope:Based on verification, the grep search found fetch-mock used only in the test file
./apps/api/test/functional/sign-and-broadcast-tx.spec.ts, and web search confirms that devDependencies should include testing frameworks and mock APIs used in projects. The recommendation to move fetch-mock to devDependencies is correct.Move
fetch-mockto devDependencies inapps/api/package.jsonfetch-mock is a test-mocking library used only in
test/functional/sign-and-broadcast-tx.spec.ts. Following npm best practices, testing utilities should be listed underdevDependenciesrather than productiondependenciesto keep the runtime install surface minimal.apps/api/test/services/test-wallet.service.ts (1)
57-85: ****The wallet index concern does not reflect an actual issue. Verification shows the mnemonics cached from
new Wallet(undefined, 1)wallets are rehydrated intx-manager.service.tsline 19 withthis.walletFactory(...FUNDING_WALLET_MNEMONIC, 1), which explicitly passes the matchingindex=1. The index consistency is already maintained throughout the codebase, and no problematic rehydrations without the index parameter were found.Likely an incorrect or invalid review comment.
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
114-114: LGTM! Method name updated to reflect derived wallet signing.The change from
executeEncodedTxByUserIdtoexecuteDerivedEncodedTxByUserIdcorrectly aligns with the new TxManagerService-based architecture.apps/api/test/functional/deployments.spec.ts (1)
90-90: LGTM! Test spies updated to match the new method signatures.All occurrences of
executeDerivedDecodedTxByUserIdspy definitions are correctly aligned with the production code changes.Also applies to: 662-662, 758-758, 793-793
apps/api/src/certificate/services/certificate/certificate.service.ts (1)
34-34: LGTM! Certificate signing updated to use derived wallet flow.The change from
executeDecodedTxByUserIdtoexecuteDerivedDecodedTxByUserIdis consistent with the architectural refactor.apps/api/test/setup-global-functional.ts (1)
5-5: LGTM! Environment variable check updated correctly.The condition now checks for
FUNDING_WALLET_MNEMONICinstead ofMASTER_WALLET_MNEMONIC, aligning with the new wallet architecture.apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts (1)
65-65: LGTM! Deployment cleanup updated to use derived transactions.Both calls to
executeDerivedTx(main path and retry path) are correctly updated fromexecuteManagedTx, maintaining the error handling logic.Also applies to: 76-76
apps/api/src/billing/services/provider-cleanup/provider-cleanup.service.ts (1)
59-59: LGTM! Provider cleanup updated to use derived transactions.Both calls to
executeDerivedTx(primary and retry paths) are correctly updated fromexecuteManagedTx, preserving the dryRun gating and error handling logic.Also applies to: 71-71
apps/api/test/custom-jest-environment.ts (1)
18-28: Inconsistent wallet initialization strategy confirmed.The review comment's concern is valid. FUNDING_WALLET_MNEMONIC uses
TestWalletServicefor consistent test wallets, while DERIVATION_WALLET_MNEMONIC creates a fresh random wallet vianew Wallet().getMnemonic().In production, TxManagerService.getDerivedWallet(index) reads DERIVATION_WALLET_MNEMONIC to derive child wallets at specific indices. Each test file receiving a different seed mnemonic means derived wallets are not reproducible across test runs or test files.
This inconsistency may be intentional (derivation wallets need per-test isolation while funding wallet requires consistency), but it warrants explicit verification to ensure test expectations are met.
apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts (1)
149-152: LGTM! Method rename aligns with the new derived wallet architecture.The change from
executeManagedTxtoexecuteDerivedTxis consistent with the broader refactoring to separate funding and derived wallets. The method signature and parameters remain unchanged.apps/api/test/functional/start-trial.spec.ts (2)
9-9: LGTM! Import updated for new wallet architecture.
50-56: LGTM! Test correctly updated to use TxManagerService.The funding wallet address is now obtained via
TxManagerService.getFundingWalletAddress()instead of the previousMANAGED_MASTER_WALLET.getFirstAddress()pattern, which aligns with the PR's separation of funding and derived wallets.apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (3)
60-60: LGTM! Deployment creation now uses derived wallet signing.
92-92: LGTM! Deposit transactions now use derived wallet signing.
127-127: LGTM! Deployment updates now use derived wallet signing.All three transaction paths (create, deposit, update) consistently use
executeDerivedDecodedTxByUserId, which aligns with the new wallet architecture.apps/api/src/billing/services/chain-error/chain-error.service.ts (3)
7-7: LGTM! Import updated for new wallet architecture.
72-76: LGTM! Constructor updated to use TxManagerService.The dependency injection correctly shifts from the previous
WallettoTxManagerService, which provides a centralized interface for wallet operations.
95-105: LGTM! Funding wallet address correctly retrieved via TxManagerService.The method now uses
txManagerService.getFundingWalletAddress()to obtain the master/funding wallet address, which is semantically clearer than the previouswallet.getFirstAddress()approach.apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts (2)
28-31: LGTM! Constructor simplified with TxManagerService.Dependencies reduced from
(jwtModule, billingConfigService, walletFactory)to(jwtModule, txManagerService), consolidating wallet management concerns in a single service.
52-59: LGTM! Wallet derivation now centralized via TxManagerService.The change from
walletFactory(masterWalletMnemonic, walletId)totxManagerService.getDerivedWallet(walletId)simplifies the interface and removes the need for direct mnemonic handling.apps/api/test/functional/stale-anonymous-users-cleanup.spec.ts (3)
7-7: LGTM! Import updated for new wallet architecture.
22-27: LGTM! Test setup correctly uses TxManagerService.The funding wallet address is obtained in
beforeAllviatxManagerService.getFundingWalletAddress(), replacing the previousMANAGED_MASTER_WALLET.getFirstAddress()pattern.
70-82: LGTM! All assertions updated to use funding wallet address.All authz service calls consistently use
fundingMasterWalletinstead of the previousmasterAddress, verifying that grants are properly revoked during cleanup.apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts (4)
7-7: LGTM! Import updated for new service dependency.
16-32: LGTM! Test correctly verifies TxManagerService usage.The test expectations are updated to verify
txManagerService.getDerivedWalletis called with the correctwalletId, replacing the previous wallet factory verification.
34-42: LGTM! Memoization test correctly updated.The test verifies that
getDerivedWalletis called only once when generating multiple tokens, confirming the memoization behavior is preserved.
92-117: LGTM! Test setup correctly mocks TxManagerService.The mock setup replaces
walletFactoryandbillingConfigServicewithtxManagerService, and the return values are updated to include the new dependencies.apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (2)
185-191: Error handling change verified - callers properly handle raw errors.The raw error propagation is appropriately handled by all callers:
managed-signer.service.tscatches errors fromsignAndBroadcastWithDerivedWallet(line 43) andsignAndBroadcastWithFundingWallet(line 53), transforming them viachainErrorService.toAppError()at lines 47 and 55 respectivelytx-manager.service.tspasses errors through without transformation, which is correct by design- Tests validate this error propagation model with
Promise.allSettledand rawErrorexpectationsThe architectural improvement—moving error transformation from the library layer to the service boundary—is sound and maintains proper error handling throughout the call chain.
149-159: ChainErrorService dependency removal verified as safe.Verification confirms
ChainErrorServicewas never used inBatchSigningClientService—zero references found in the file. The service'ssignAndBroadcast()method throws raw errors directly to callers without any error transformation, and callers (e.g.,tx-manager.service.ts) handle them naturally via try-finally patterns. The removal is a valid code cleanup that eliminates unused dependency injection with no behavioral impact.apps/api/src/certificate/services/certificate/certificate.service.spec.ts (1)
33-34: executeDerivedDecodedTxByUserId wiring in certificate tests looks correctThe mock, type annotation, and expectation are consistently updated to
executeDerivedDecodedTxByUserId, and the argument/return shapes match the existing usage, so behavior remains unchanged.Also applies to: 42-42, 81-82, 95-95
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (1)
138-185: Spec setup correctly updated for new BatchSigningClientService signatureThe test helper now matches the constructor (config, wallet, registry, createClientWithSigner) and still wires mocks as expected; the remaining tests exercise the key batch, error, and retry flows, so this change looks good.
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (2)
13-14: TxManagerService‑based execute*Tx wrappers are wired correctly
executeDerivedTxandexecuteFundingTxnow cleanly delegate signing/broadcasting to TxManagerService while still usingChainErrorService.toAppErrorto normalize chain errors, and the fee‑granter being the funding wallet for derived transactions matches the new split‑wallet model. This keeps ManagedSignerService focused on auth/balance checks and domain events while centralizing chain interactions in TxManagerService.Also applies to: 26-38, 40-57
59-80: Confirm mapping between UserWallet and derived wallet index used in executeDerivedTxIn
executeDerivedDecodedTxByUserId/executeDecodedTxByUserWallet, the actual send now goes through:const tx = await this.executeDerivedTx(userWallet.id, messages);TxManagerService interprets that argument as the derivation index. Please double‑check that
userWallet.idis indeed the same index you used when creating the derived wallet (e.g., viacreateWallet({ addressIndex })), and not just the database primary key. If there is a dedicated “derivation index”/“address index” field on the wallet record, that field should be passed here instead to ensure we sign with the correct derived account that holds the allowances. The rest of the flow (trial lease event emission andrefreshUserWalletLimits) looks consistent with the previous behaviour.Also applies to: 115-144
ygrishajev
force-pushed
the
feature/billing-master-wallets
branch
from
e4b64bf to
67b9ddd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (2)
65-87: Funding wallet as granter and funding‑tx execution pathThe new flow in
authorizeSpending/authorizeFeeSpending/authorizeDeploymentSpendingandrevokeAlllooks coherent:
- Granter is now
fundingWalletAddressfromTxManagerService, which matches the PR’s shift away from a master wallet.- All grant/revoke operations are funneled through
managedSignerService.executeFundingTx(...), ensuring the funding wallet pays fees for these authz ops.Promise.allinauthorizeSpendingcorrectly runs deployment and fee authorization in parallel, and the'deployment' in options.limitsguard keeps the union type safe.Two minor points to consider:
In
authorizeFeeSpending, you currently checkhasFeeAllowance(any allowance) rather thanhasValidFeeAllowance. If the intent is to revoke only non‑expired allowances before re‑granting, switching to the “valid” variant could avoid attempting to revoke already‑expired grants. If revoking stale grants is intentional, current behavior is fine.All these methods assume that
getFundingWalletAddress()andexecuteFundingTx(...)are aligned on which physical key is the “funding” wallet. Worth a quick check inTxManagerService/ManagedSignerServicethat there’s no mismatch in index or mnemonic source.Otherwise, the structure looks solid.
Also applies to: 89-105, 107-137
26-33: Remove now‑unused HD path / prefix constants
PREFIXandHD_PATHare no longer used after moving derivation intoTxManagerService. They can likely be dropped to reduce noise and avoid confusion about whether this class is still responsible for key derivation.- private readonly PREFIX = "akash"; - - private readonly HD_PATH = "m/44'/118'/0'/0"; -apps/api/test/functional/start-trial.spec.ts (1)
9-55: Funding wallet now resolved via TxManagerService in trial testResolving
fundingWalletAddressviaTxManagerService.getFundingWalletAddress()and using it as the granter ingetValidDepositDeploymentGrantsForGranterAndGranteekeeps the test aligned with the new funding-wallet model.As a minor style tweak, you could mirror other top-level constants by resolving
TxManagerServiceonce at the describe-scope instead of inside the test body, but this is optional.apps/api/test/functional/stale-anonymous-users-cleanup.spec.ts (1)
7-82: Stale-anonymous cleanup test correctly switched to funding wallet via TxManagerServiceUsing
txManagerService.getFundingWalletAddress()inbeforeAlland passingfundingMasterWalletinto the variousAuthzHttpServicechecks keeps the expectations consistent with the new funding-wallet model.If you want to tighten readability, consider renaming
fundingMasterWalletto something likefundingWalletAddress, but this is purely cosmetic.apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts (1)
7-59: VerifyTxManagerService.getDerivedWalletsync/async semantics in JWT token flowIn
getJwtToken, you currently do:const wallet = this.txManagerService.getDerivedWallet(walletId); const jwtTokenManager = new this.jwtModule.JwtTokenManager(wallet); const address = await wallet.getFirstAddress();This assumes
getDerivedWallet(walletId)is synchronous and returns a wallet object (withgetFirstAddress()), not aPromise. IfgetDerivedWalletis actuallyasyncand returns aPromise<Wallet>, this would pass a Promise intoJwtTokenManagerand only work by accident (or fail at runtime/type-check).Please double-check the
TxManagerService.getDerivedWalletsignature and adjust accordingly. For an async API, this method should look like:- private async getJwtToken(walletId: number): Promise<JwtTokenWithAddress> { - const wallet = this.txManagerService.getDerivedWallet(walletId); + private async getJwtToken(walletId: number): Promise<JwtTokenWithAddress> { + const wallet = await this.txManagerService.getDerivedWallet(walletId); const jwtTokenManager = new this.jwtModule.JwtTokenManager(wallet); const address = await wallet.getFirstAddress();apps/api/src/billing/services/balances/balances.service.ts (1)
7-8: Centralizing funding wallet usage via TxManagerService and caching full balance is consistent and safe
- Injecting
TxManagerServicein the constructor and usinggetFundingWalletAddress()in bothretrieveAndCalcFeeLimitandretrieveDeploymentLimitcorrectly centralizes the funding wallet source, matching the broader TxManager refactor.- The existing authz calls now use the funding wallet as granter while keeping user wallet as grantee, which preserves the original authorization model.
- Adding
@Memoize({ ttlInSeconds: averageBlockTime })ongetFullBalanceis a reasonable optimization: it avoids redundant network calls while staying aligned with on-chain update cadence.- Minor nit: you could mark
txManagerServiceasprivate readonlyfor consistency with other injected services, but current code is functionally fine as-is.Also applies to: 16-19, 55-58, 66-69, 94-95
apps/api/env/.env.unit.test (1)
3-4: Env mnemonic keys match new funding/derivation model; consider addressing dotenv-linter ordering warningsSwitching to
FUNDING_WALLET_MNEMONICand addingDERIVATION_WALLET_MNEMONICis consistent with the TxManagerService-based split between funding and derived wallets.
Static analysis (dotenv-linter) flags key ordering for these two entries; if your CI enforces this, you may want to reorder them as suggested, otherwise current content is fine functionally.apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts (1)
21-22: AligncreateAkashAddressimport with existing@testaliasesYou currently import
createAkashAddressvia a deep relative path while other seeders in the same file use the@test/seeders/...alias. For consistency and easier refactoring, consider switching to the alias form, e.g.@test/seeders/akash-address.seeder, and optionally inlining a simple string literal if you don't need randomness in this spec.Also applies to: 530-533
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (2)
33-41: Consider tightening return types for signing methods
signAndBroadcastWithFundingWallet/signAndBroadcastWithDerivedWalletcurrently rely on the underlying client’s implicit return type (Promise<any>), which makes callers lose static guarantees about the shape of the transaction result. IfBatchSigningClientService.signAndBroadcastis expected to return somethingIndexedTx-like, consider reflecting that in these method signatures so downstream services can rely on stronger typing.
73-80: SurfaceDERIVATION_WALLET_MNEMONICmisconfiguration earlier
getDerivedWalletthrows ifDERIVATION_WALLET_MNEMONICis missing, but this only shows up on the first derived call at runtime. For operator-friendliness, you may want to validate this config on service initialization (or viaBillingConfigServicevalidation) so misconfiguration fails fast at startup instead of at the first transaction. This is a cross-cutting concern and might be better tracked as a follow‑up issue rather than expanding this PR.apps/api/src/billing/services/managed-signer/managed-signer.service.ts (2)
119-143: Result normalization looks correct; consider tightening the typingThe pattern of picking
{ code, hash, transactionHash, rawLog }fromtxand then normalizingtransactionHashfromhashensures callers always get atransactionHash, which is good. However,resultis cast asPick<IndexedTx, "code" | "hash" | "rawLog">, even thoughtransactionHashis also being selected and used. It would be cleaner to either introduce a smallTxExecutionResulttype that explicitly includestransactionHash, or extend the pick type to match the actual shape you expect from the underlying client.
40-49: Confirm theuserWallet.id→ derivation index mapping is intentional
executeDecodedTxByUserWalletultimately callsexecuteDerivedTx(userWallet.id, messages), which then invokestxManagerService.signAndBroadcastWithDerivedWallet(walletIndex, ...). This effectively couples the DB wallet ID to the derivation index used byWalletFactory/Wallet. If this convention already existed before the refactor, keeping it is fine, but it’s subtle and long‑lived (e.g., DB IDs must stay within the derivation index range). It might be worth briefly documenting this assumption or adding a guard if there’s any risk of unexpected ID growth. Given the cross‑cutting nature, this could be a separate follow‑up issue rather than a change in this PR.Also applies to: 82-120
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
apps/api/env/.env.unit.test(1 hunks)apps/api/package.json(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/lib/batch-signing-client/batch-signing-client.service.spec.ts(1 hunks)apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts(1 hunks)apps/api/src/billing/providers/signing-client.provider.ts(1 hunks)apps/api/src/billing/providers/wallet.provider.ts(1 hunks)apps/api/src/billing/services/balances/balances.service.ts(5 hunks)apps/api/src/billing/services/chain-error/chain-error.service.spec.ts(2 hunks)apps/api/src/billing/services/chain-error/chain-error.service.ts(3 hunks)apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts(0 hunks)apps/api/src/billing/services/financial-stats/financial-stats.service.ts(1 hunks)apps/api/src/billing/services/index.ts(0 hunks)apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts(20 hunks)apps/api/src/billing/services/managed-signer/managed-signer.service.ts(3 hunks)apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts(5 hunks)apps/api/src/billing/services/provider-cleanup/provider-cleanup.service.ts(2 hunks)apps/api/src/billing/services/tx-manager/tx-manager.service.ts(1 hunks)apps/api/src/billing/types/wallet.type.ts(0 hunks)apps/api/src/certificate/services/certificate/certificate.service.spec.ts(4 hunks)apps/api/src/certificate/services/certificate/certificate.service.ts(1 hunks)apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts(1 hunks)apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts(3 hunks)apps/api/src/deployment/services/lease/lease.service.ts(1 hunks)apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts(2 hunks)apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts(9 hunks)apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts(1 hunks)apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts(5 hunks)apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts(3 hunks)apps/api/test/custom-jest-environment.ts(2 hunks)apps/api/test/functional/deployments.spec.ts(4 hunks)apps/api/test/functional/sign-and-broadcast-tx.spec.ts(0 hunks)apps/api/test/functional/stale-anonymous-users-cleanup.spec.ts(3 hunks)apps/api/test/functional/start-trial.spec.ts(2 hunks)apps/api/test/services/test-wallet.service.ts(1 hunks)apps/api/test/setup-global-functional.ts(1 hunks)
💤 Files with no reviewable changes (4)
- apps/api/test/functional/sign-and-broadcast-tx.spec.ts
- apps/api/src/billing/services/index.ts
- apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts
- apps/api/src/billing/types/wallet.type.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/api/src/billing/controllers/wallet/wallet.controller.ts
- apps/api/package.json
- apps/api/test/functional/deployments.spec.ts
- apps/api/test/custom-jest-environment.ts
- apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts
- apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts
- apps/api/test/services/test-wallet.service.ts
- apps/api/src/billing/config/env.config.ts
- apps/api/src/certificate/services/certificate/certificate.service.spec.ts
- apps/api/src/billing/services/chain-error/chain-error.service.spec.ts
- apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts
- apps/api/src/billing/providers/signing-client.provider.ts
🧰 Additional context used
🧠 Learnings (4)
📓 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-19T15:15:07.266Z
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.266Z
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/provider/services/provider-jwt-token/provider-jwt-token.service.spec.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.tsapps/api/src/billing/services/managed-signer/managed-signer.service.spec.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/provider/services/provider-jwt-token/provider-jwt-token.service.spec.tsapps/api/test/functional/start-trial.spec.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.tsapps/api/test/functional/stale-anonymous-users-cleanup.spec.tsapps/api/test/setup-global-functional.tsapps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts
📚 Learning: 2025-05-28T20:42:58.200Z
Learnt from: jzsfkzm
Repo: akash-network/console PR: 1372
File: apps/api/src/dashboard/services/stats/stats.service.ts:0-0
Timestamp: 2025-05-28T20:42:58.200Z
Learning: The user successfully implemented CosmosHttpService with retry logic using axiosRetry, exponential backoff, and proper error handling for Cosmos API calls, replacing direct axios usage in StatsService.
Applied to files:
apps/api/src/billing/services/financial-stats/financial-stats.service.ts
🧬 Code graph analysis (7)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (1)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
BatchSigningClientService(57-360)
apps/api/src/billing/services/financial-stats/financial-stats.service.ts (2)
apps/api/src/billing/services/balances/balances.service.ts (1)
singleton(11-106)apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
singleton(26-138)
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (6)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (2)
BatchSigningClientService(57-360)SignAndBroadcastOptions(24-34)apps/api/src/billing/providers/wallet.provider.ts (2)
FUNDING_WALLET(10-10)WALLET_FACTORY(18-18)apps/api/src/billing/lib/wallet/wallet.ts (3)
Wallet(7-65)walletFactory(68-68)WalletFactory(67-67)apps/api/src/billing/providers/signing-client.provider.ts (1)
FUNDING_SIGNING_CLIENT(11-11)apps/api/src/billing/providers/type-registry.provider.ts (1)
TYPE_REGISTRY(34-34)apps/api/src/billing/lib/signing-stargate-client-factory/signing-stargate-client.factory.ts (1)
createSigningStargateClient(57-61)
apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts (1)
apps/api/test/seeders/akash-address.seeder.ts (1)
createAkashAddress(4-12)
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (3)
packages/http-sdk/src/lease/lease-http.service.ts (2)
LeaseHttpService(72-96)hasLeases(89-95)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(17-21)apps/api/src/billing/events/trial-deployment-lease-created.ts (1)
TrialDeploymentLeaseCreated(4-21)
apps/api/src/billing/providers/wallet.provider.ts (1)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (3)
packages/http-sdk/src/authz/authz-http.service.ts (1)
AuthzHttpService(56-218)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)
🪛 dotenv-linter (4.0.0)
apps/api/env/.env.unit.test
[warning] 3-3: [UnorderedKey] The FUNDING_WALLET_MNEMONIC key should go before the POSTGRES_DB_URI key
(UnorderedKey)
[warning] 4-4: [UnorderedKey] The DERIVATION_WALLET_MNEMONIC key should go before the FUNDING_WALLET_MNEMONIC 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). (12)
- GitHub Check: validate (apps/provider-console) / validate-unsafe
- GitHub Check: validate / validate-app
- 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: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
185-191: Rawresult.valthrow changes error shape and typeYou now throw
result.valdirectly, which is typed asunknown. Callers may have been relying on ChainErrorService-normalized error types (e.g., specificErrorsubclasses / PaymentRequired mappings) and oninstanceof Errorchecks.If all producers of
Err(error)in this pipeline guarantee anError-like object, this is fine; otherwise, you might want to either:
- Wrap non-
Errorvalues in a genericError, or- Narrow
unknownhere before throwing to avoid surprising downstream behavior.At minimum, worth double-checking the main call sites of
signAndBroadcastfor assumptions about the thrown error type/shape.apps/api/src/billing/services/provider-cleanup/provider-cleanup.service.ts (1)
57-73: ConfirmexecuteDerivedTx(wallet.id, [message])semantics for cleanup flowThe cleanup path now uses
executeDerivedTx(wallet.id, [message])(both initially and afterauthorizeSpending). This looks consistent with the broader migration to derived wallets, but a couple of things are worth confirming:
wallet.idis indeed the right key for the derived-wallet path inManagedSignerService(and not e.g. an address or userId).- The derived wallet used here is the expected fee payer in this cleanup scenario (vs funding wallet).
If both are true in the new TxManager/ManagedSigner design, this change is good.
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
34-37: Derived address usage viaTxManagerServicelooks consistent
createAndAuthorizeTrialSpendingandcreateWalletnow rely ontxManagerService.getDerivedWalletAddress(addressIndex)and return/log that address. AssumingTxManagerServicedeterministically maps(index)to the same derivation path you previously used here, this is a clean encapsulation of the derivation logic.It would be good to sanity-check that:
- The index semantics (e.g., which index corresponds to which user) remain unchanged, and
- There’s no off‑by‑one between the derivation index here and any funding/derivation index conventions elsewhere.
Also applies to: 42-56, 58-63
apps/api/src/billing/providers/wallet.provider.ts (1)
2-16: FUNDING_WALLET registration aligns with TxManagerService, verify index and configThe FUNDING_WALLET wiring via
BillingConfigService+WALLET_FACTORYlooks good and matches the intent to centralize usage behindTxManagerService.A couple of things to confirm:
config.get("FUNDING_WALLET_MNEMONIC")is guaranteed non‑empty at startup (orBillingConfigServicewill fail fast with a clear error if missing).- The hard‑coded index
1matches the conventions inTxManagerService(i.e., funding wallet is always HD index 1, and derived wallets never collide with it).Given the “do not use directly” comment, enforcing access via
TxManagerServiceonly is a good pattern.apps/api/src/deployment/services/lease/lease.service.ts (1)
30-41: Lease signing routed through derived‑wallet pathSwitching to
executeDerivedDecodedTxByUserId(wallet.userId, leaseMessages)keeps the same inputs (userId + messages) while aligning with the new derived‑wallet‑centric signer API. As long asexecuteDerivedDecodedTxByUserIdis wired to the correct derived account for this user, this is a safe and consistent change.Looks good.
apps/api/src/certificate/services/certificate/certificate.service.ts (1)
34-40: Signer call correctly migrated to derived-tx APISwitching to
executeDerivedDecodedTxByUserId(userWallet.userId, messages)is consistent with the new derived-wallet flow; inputs and behavior remain aligned with the previous implementation.apps/api/test/setup-global-functional.ts (1)
5-7: Env gate aligned with funding wallet mnemonicUsing
localConfig.FUNDING_WALLET_MNEMONIChere keeps the previous behavior while matching the new funding-wallet naming.apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts (1)
64-77: Cleanup flow now consistently uses derived signerBoth the primary and retry paths call
executeDerivedTx(wallet.id, messages), which keeps behavior consistent while routing through the new derived-wallet signer.apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts (1)
147-153: Top-up signer call correctly switched to derived txUsing
executeDerivedTx(walletId, ownerInputs.map(i => i.message))preserves existing behavior while aligning with the derived-wallet/TxManagerService-based flow.apps/api/src/billing/services/chain-error/chain-error.service.ts (1)
7-105: Master/funding balance check correctly routed through TxManagerServiceReplacing direct wallet injection with
TxManagerServiceand usinggetFundingWalletAddress()inisMasterWalletInsufficientFundsErrorkeeps the error-mapping behavior the same while centralizing funding wallet resolution.apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts (1)
16-22: TxManagerService-based wallet resolution and memoization tests look correctThe updated tests correctly:
- Delegate wallet resolution to
txManagerService.getDerivedWallet(walletId)and assert it’s called with the expectedwalletId.- Ensure JWT creation uses the resolved
walletinstance and generated token is as expected.- Verify memoization by asserting
getDerivedWalletandJwtTokenManagerare invoked only once across repeatedgenerateJwtTokencalls.
The setup mock forTxManagerServiceand the constructor callnew ProviderJwtTokenService(jwtModule, txManagerService)are consistent with the new dependency graph, with no apparent gaps.Also applies to: 35-42, 92-95, 108-118
apps/api/src/billing/services/financial-stats/financial-stats.service.ts (1)
11-19: Funding wallet balance resolution via TxManagerService is aligned with the new architectureThe constructor and
getMasterWalletBalanceUsdcchanges cleanly switch from local/master-wallet configuration totxManagerService.getFundingWalletAddress(), making the funding wallet a shared concern.CosmosHttpServiceusage is unchanged, so the only behavioral difference is the centralized address source, which matches the TxManagerService introduction elsewhere. No issues noted.Also applies to: 25-27
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (1)
182-185: BatchSigningClientService test setup matches the new constructor signatureThe test setup now constructs
BatchSigningClientServicewith(billingConfigService, wallet, registry, createClientWithSigner), which aligns with the updated constructor (noChainErrorServiceargument). The returned{ service, client }is still sufficient for the existing batching and retry tests, so coverage remains valid for the current behavior.apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts (1)
96-214: Derived path tests look solid and aligned with the new TxManagerService flowThe updated specs around
executeDerivedDecodedTxByUserId(including feature-flag branches, successful execution, events, and chain error handling) exercise the newtxManagerService.signAndBroadcastWithDerivedWalletusage and fee-granter behavior well. The expectation onsignAndBroadcastWithDerivedWallet(wallet.id, messages, { fee: { granter: expect.any(String) } })also guards the integration withgetFundingWalletAddress.
ygrishajev
force-pushed
the
feature/billing-master-wallets
branch
from
e57bda3 to
9ff8bd5
Compare
ygrishajev
force-pushed
the
feature/billing-master-wallets
branch
from
9ff8bd5 to
2e703e9
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 (1)
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (1)
73-77: Update close() method to use derived wallet signing for consistency.The
closemethod at line 76 usesexecuteDecodedTxByUserWallet(wallet, [message]), but all other deployment operations in this service (create,deposit,ensureDeploymentIsUpToDate) useexecuteDerivedDecodedTxByUserId(wallet.userId, ...).Update line 76 to:
await this.signerService.executeDerivedDecodedTxByUserId(wallet.userId, [message]);This aligns with the established pattern and the refactoring goals across the codebase.
🧹 Nitpick comments (5)
apps/api/src/billing/services/balances/balances.service.ts (1)
7-19: TxManagerService integration for funding address looks correct; consider marking the dependency readonlyThe switch to resolving the granter via
txManagerService.getFundingWalletAddress()for both fee and deployment limits aligns with the new funding-wallet model and keeps BalancesService decoupled from wallet details. One small consistency tweak: other injected dependencies arereadonly, buttxManagerServiceis not.You can make it immutable like the others:
- private txManagerService: TxManagerService, + private readonly txManagerService: TxManagerService,Also applies to: 55-75
apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts (1)
47-48: Tests correctly track the switch toexecuteDerivedTx; “master wallet” naming can be cleaned up laterStubbing and expectations have been consistently updated to
managedSignerService.executeDerivedTx, and the message payload/assertion logic around top‑ups and error paths still matches the intended behavior.The remaining
isMasterWalletInsufficientFundsError/MASTER_WALLET_INSUFFICIENT_FUNDSnaming is now conceptually a bit off given the funding/derived split, but that’s cosmetic and cross‑cutting; it’s reasonable to keep as‑is here and, if needed, track a broader terminology cleanup in a separate issue. Based on learnings.Also applies to: 108-111, 210-211, 237-238, 245-246, 277-279, 378-379, 407-408, 442-443, 464-465
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (2)
16-48: DI setup for funding and derived wallets is coherent; tighten WalletFactory typingThe DI registrations for
FUNDING_WALLET,DERIVED_WALLET_FACTORY,FUNDING_SIGNING_CLIENT, andBATCH_SIGNING_CLIENT_FACTORYline up with howTxManagerServiceis constructed and used; this cleanly replaces the old wallet/signing‑client providers.One small type‑safety improvement:
WalletFactoryis declared with an optional index, but all usages require an index and the concrete factory implementation takes anumber. You can align the type:-export type WalletFactory = (walletIndex?: number) => Wallet; +export type WalletFactory = (walletIndex: number) => Wallet;
50-107: Client caching/cleanup for derived wallets is sound; consider passing logger context for better tracingThe per‑address cache in
#clientsByAddressplus#getClientensures only oneBatchSigningClientServiceinstance is reused per derived address, and thefinallyblock insignAndBroadcastWithDerivedWalletguarantees cleanup when there are no pending transactions. This matches the intended dedupe behavior and is well covered by the new spec.If you want more granular observability, you could propagate a context that includes the address into the derived client factory:
- this.#clientsByAddress.set(address, { - address, - client: this.batchSigningClientServiceFactory(wallet) - }); + this.#clientsByAddress.set(address, { + address, + client: this.batchSigningClientServiceFactory(wallet, `DERIVED_SIGNING_CLIENT:${address}`) + });This would make BatchSigningClientService logs easier to correlate back to specific derived wallets.
apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts (1)
21-23: TxManagerService wiring in tests looks consistent with the implementationImporting
TxManagerServicefor typing, usingcreateAkashAddressto backgetFundingWalletAddress, and passingmocks.txManagerServiceintoManagedSignerServicematches the new production constructor and keeps the mocks strongly typed viaTxManagerService["signAndBroadcastWithDerivedWallet"].As a minor follow-up, you might later add a focused unit test around
executeFundingTx(and possibly assertgetFundingWalletAddressusage) to mirror the derived-wallet coverage, but that can be deferred or tracked as a separate task.Also applies to: 490-553
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
apps/api/env/.env.unit.test(1 hunks)apps/api/package.json(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/lib/batch-signing-client/batch-signing-client.service.spec.ts(1 hunks)apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts(1 hunks)apps/api/src/billing/lib/wallet/wallet.ts(0 hunks)apps/api/src/billing/providers/index.ts(0 hunks)apps/api/src/billing/providers/signing-client.provider.ts(0 hunks)apps/api/src/billing/providers/wallet.provider.ts(0 hunks)apps/api/src/billing/services/balances/balances.service.ts(5 hunks)apps/api/src/billing/services/chain-error/chain-error.service.spec.ts(2 hunks)apps/api/src/billing/services/chain-error/chain-error.service.ts(3 hunks)apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts(0 hunks)apps/api/src/billing/services/financial-stats/financial-stats.service.ts(1 hunks)apps/api/src/billing/services/index.ts(0 hunks)apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts(20 hunks)apps/api/src/billing/services/managed-signer/managed-signer.service.ts(3 hunks)apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts(5 hunks)apps/api/src/billing/services/provider-cleanup/provider-cleanup.service.ts(2 hunks)apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts(1 hunks)apps/api/src/billing/services/tx-manager/tx-manager.service.ts(1 hunks)apps/api/src/billing/types/wallet.type.ts(0 hunks)apps/api/src/certificate/services/certificate/certificate.service.spec.ts(4 hunks)apps/api/src/certificate/services/certificate/certificate.service.ts(1 hunks)apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts(1 hunks)apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts(3 hunks)apps/api/src/deployment/services/lease/lease.service.ts(1 hunks)apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts(2 hunks)apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts(9 hunks)apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts(1 hunks)apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts(5 hunks)apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts(3 hunks)apps/api/test/custom-jest-environment.ts(2 hunks)apps/api/test/functional/deployments.spec.ts(4 hunks)apps/api/test/functional/sign-and-broadcast-tx.spec.ts(2 hunks)apps/api/test/functional/stale-anonymous-users-cleanup.spec.ts(3 hunks)apps/api/test/functional/start-trial.spec.ts(2 hunks)apps/api/test/services/test-wallet.service.ts(1 hunks)apps/api/test/setup-global-functional.ts(1 hunks)
💤 Files with no reviewable changes (7)
- apps/api/src/billing/types/wallet.type.ts
- apps/api/src/billing/lib/wallet/wallet.ts
- apps/api/src/billing/services/index.ts
- apps/api/src/billing/providers/signing-client.provider.ts
- apps/api/src/billing/providers/index.ts
- apps/api/src/billing/services/dedupe-signing-client/dedupe-signing-client.service.ts
- apps/api/src/billing/providers/wallet.provider.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- apps/api/package.json
- apps/api/test/functional/sign-and-broadcast-tx.spec.ts
- apps/api/src/billing/services/chain-error/chain-error.service.ts
- apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts
- apps/api/src/billing/services/chain-error/chain-error.service.spec.ts
- apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.ts
- apps/api/src/billing/config/env.config.ts
- apps/api/test/setup-global-functional.ts
- apps/api/src/certificate/services/certificate/certificate.service.ts
- apps/api/test/functional/start-trial.spec.ts
- apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts
- apps/api/src/billing/services/provider-cleanup/provider-cleanup.service.ts
- apps/api/test/functional/deployments.spec.ts
- apps/api/src/certificate/services/certificate/certificate.service.spec.ts
- apps/api/src/billing/services/financial-stats/financial-stats.service.ts
🧰 Additional context used
🧠 Learnings (3)
📓 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-19T15:15:07.266Z
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.266Z
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/provider/services/provider-jwt-token/provider-jwt-token.service.spec.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.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
📚 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/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.tsapps/api/test/functional/stale-anonymous-users-cleanup.spec.tsapps/api/test/services/test-wallet.service.tsapps/api/test/custom-jest-environment.tsapps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts
🧬 Code graph analysis (9)
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (4)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (2)
BatchSigningClientService(57-360)SignAndBroadcastOptions(24-34)apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)apps/api/src/billing/providers/type-registry.provider.ts (1)
TYPE_REGISTRY(34-34)apps/api/src/billing/lib/signing-stargate-client-factory/signing-stargate-client.factory.ts (1)
createSigningStargateClient(57-61)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (1)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (1)
BatchSigningClientService(57-360)
apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts (2)
apps/api/test/services/stub.ts (1)
stub(2-2)packages/logging/src/services/logger/logger.service.ts (1)
error(141-143)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (3)
packages/http-sdk/src/authz/authz-http.service.ts (1)
AuthzHttpService(56-218)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.spec.ts (3)
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)
SignAndBroadcastOptions(24-34)apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)
apps/api/test/services/test-wallet.service.ts (1)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)
apps/api/test/custom-jest-environment.ts (3)
apps/api/test/setup-global-functional.ts (1)
localConfig(4-8)apps/api/test/services/test-wallet.service.ts (1)
TestWalletService(21-165)apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)
apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts (1)
apps/api/test/seeders/akash-address.seeder.ts (1)
createAkashAddress(4-12)
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (3)
packages/http-sdk/src/lease/lease-http.service.ts (2)
LeaseHttpService(72-96)hasLeases(89-95)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(17-21)apps/api/src/billing/events/trial-deployment-lease-created.ts (1)
TrialDeploymentLeaseCreated(4-21)
🪛 dotenv-linter (4.0.0)
apps/api/env/.env.unit.test
[warning] 3-3: [UnorderedKey] The FUNDING_WALLET_MNEMONIC key should go before the POSTGRES_DB_URI key
(UnorderedKey)
[warning] 4-4: [UnorderedKey] The DERIVATION_WALLET_MNEMONIC key should go before the FUNDING_WALLET_MNEMONIC 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). (17)
- GitHub Check: validate (apps/provider-proxy) / validate-unsafe
- GitHub Check: validate (apps/notifications) / validate-unsafe
- GitHub Check: validate (apps/provider-console) / validate-unsafe
- GitHub Check: validate (apps/stats-web) / validate-unsafe
- GitHub Check: validate (apps/api) / validate-unsafe
- 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: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (21)
apps/api/src/deployment/services/lease/lease.service.ts (1)
40-40: Verification confirms the change is correct.The method
executeDerivedDecodedTxByUserIdperforms comprehensive validation including:
- Wallet fee allowance checks
- Lease provider and auditor validation
- Derived wallet signing via funding wallet fee granter pattern
- Proper permission gating ("sign" ability)
The migration is complete with no remaining old method calls, and tests are updated. Derived wallet signing is the appropriate mechanism for lease operations within this refactoring.
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (4)
24-33: LGTM! Constructor properly updated.The removal of the
masterWalletinjection aligns with the PR's objective to centralize wallet management. All necessary services for the new signing approach are correctly injected.
60-60: LGTM! Signing method updated correctly.The migration to
executeDerivedDecodedTxByUserIdproperly implements the new derived wallet signing approach.
92-92: LGTM! Deposit signing updated correctly.The migration to
executeDerivedDecodedTxByUserIdis consistent with the other updated methods.
127-127: LGTM! Update deployment signing migrated correctly.The use of
executeDerivedDecodedTxByUserIdis consistent with the other deployment operation updates.apps/api/test/services/test-wallet.service.ts (1)
65-65: Verify the hardcoded wallet index is intentional.The wallet instantiation now explicitly uses index
1for all test wallets. Note thatprepareFaucetWalletat line 114 still creates a wallet without an index, which will use the default HD path. Ensure this difference is intentional for your test setup.apps/api/test/custom-jest-environment.ts (1)
4-4: LGTM! Environment variable migration looks correct.The changes properly split the master wallet into two distinct concepts (funding and derivation wallets) with appropriate fallbacks when config values aren't provided.
Also applies to: 18-28
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (2)
149-159: LGTM! Simplified constructor signature.The removal of
ChainErrorServicedependency streamlines the service dependencies and aligns with the broader refactor to centralize wallet management inTxManagerService.
190-190: I'll examine the error handling in the caller methods and review the change in detail to verify if there's a breaking change.Now let me examine the actual code changes in batch-signing-client.service.ts and search for ChainErrorService:Based on the verification of callers, the review comment cannot be fully substantiated without examining the actual code changes and error handling patterns in the repository. Here is my rewritten review:
Verify that thrown errors won't break existing callers.
The change removes error transformation through
ChainErrorService.toError(), directly throwingresult.valinstead. Before approval, confirm: (1)result.valcontains the expected error object type that callers receive, and (2) no caller code inspects error properties or type-checks expecting a transformed error structure. Callers identified atgpu-bids-creator.service.ts:152,166,tx-manager.service.ts:65,76,closeDeployments.ts:52, andtest-wallet.service.ts:94should be reviewed for any error-specific handling.apps/api/env/.env.unit.test (1)
3-4: LGTM! Wallet mnemonic separation is properly configured.The split into
FUNDING_WALLET_MNEMONICandDERIVATION_WALLET_MNEMONICwith distinct values correctly implements the intended wallet separation architecture.apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
114-114: LGTM! Method rename aligns with derived wallet architecture.The change from
executeEncodedTxByUserIdtoexecuteDerivedEncodedTxByUserIdcorrectly reflects the new derived wallet signing flow introduced byTxManagerService.apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts (1)
28-31: LGTM! Excellent dependency consolidation.Replacing
BillingConfigServiceandWalletFactorywith a singleTxManagerServicedependency significantly simplifies the service's dependency graph while maintaining the same functionality throughgetDerivedWallet().Also applies to: 54-54
apps/api/test/functional/stale-anonymous-users-cleanup.spec.ts (1)
7-7: LGTM! Test correctly migrated to TxManagerService.The test properly adopts the new
TxManagerServicearchitecture, replacing direct master wallet address access withgetFundingWalletAddress(). All assertions are consistently updated to use the funding wallet.Also applies to: 22-27, 70-80
apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts (1)
7-7: LGTM! Test correctly updated to match service refactor.All test setup, mocks, and expectations properly reflect the migration from
WalletFactorytoTxManagerService.getDerivedWallet(). The test maintains the same behavioral coverage with the new dependency.Also applies to: 16-17, 21-21, 35-36, 40-41, 92-96, 116-117
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (1)
3-4: Funding/derived wallet split is wired correctly through TxManagerServiceUsing
txManagerService.getDerivedWalletAddressfor wallet creation andgetFundingWalletAddress+executeFundingTxfor fee/deployment grants and revocations cleanly separates derived from funding responsibilities while preserving existing behavior around limits and dry‑run handling. The transitions inauthorizeSpending,authorizeFeeSpending,authorizeDeploymentSpending, andrevokeAllall look consistent with the new model.Also applies to: 8-9, 34-40, 42-63, 65-87, 89-132
apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts (1)
12-185: TxManagerService test coverage is comprehensive and aligned with the implementationThe spec exercises all key behaviors: funding vs derived signing, client creation and reuse, cleanup with and without pending transactions, error propagation, and wallet/address resolution via the factory. The
setuphelper cleanly isolates dependencies with mocks, and the log expectations match the events emitted in the service.Also applies to: 187-236
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.spec.ts (1)
182-185: Setup helper correctly reflects updated BatchSigningClientService constructorUsing
(billingConfigService, wallet, registry, createClientWithSigner)and returning{ service, client }matches the new service signature and keeps the test helper minimal and accurate.apps/api/src/billing/services/managed-signer/managed-signer.service.spec.ts (2)
29-75: Derived-tx error handling and auth semantics are well coveredThe updated tests around
executeDerivedDecodedTxByUserIdexercise the key negative paths (null/undefined userId, missing wallet, missing user, zero fee/deployment allowance) and confirm that:
- 404/403 messages match the
assertusage in the service.userWalletRepository.accessibleByanduserRepository.findByIdare called with the expected parameters.- Chain errors from
signAndBroadcastWithDerivedWalletare translated viachainErrorService.toAppError.- When the requested userId matches
authService.currentUser.userId, the repository is not queried.These align tightly with the new TxManagerService-based implementation and should catch most regressions in the derived-tx path.
Also applies to: 172-214, 312-341, 343-371
96-170: Trial/feature-flag behavior and domain events are thoroughly testedThe scenarios around anonymous trial flags and trial wallets cover the important combinations:
- When ANONYMOUS_FREE_TRIAL is enabled,
validateLeaseProvidersandvalidateTrialLimitare invoked per message, and domain events are suppressed.- When it’s disabled, trial wallets with
MsgCreateLeasetriggerTrialDeploymentLeaseCreatedwith the correctwalletId,dseq, andisFirstLeasederived fromLeaseHttpService.hasLeases.- Lease provider validation (
validateLeaseProvidersAuditors) is asserted for both trial and non-trial wallets and also in anonymous mode.These tests closely mirror the control flow and conditionals in
executeDecodedTxByUserWallet, giving good confidence that the new TxManagerService-backed flow preserves the existing business semantics.Also applies to: 216-274, 276-310, 373-488
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (2)
13-57: TxManagerService injection and helper methods keep signing logic centralizedInjecting
TxManagerServiceand introducingexecuteDerivedTx/executeFundingTxto wrap sign/broadcast calls (withgetFundingWalletAddressfor the granter and consistentchainErrorService.toAppErrorhandling) is a clean separation of concerns and removes direct dependency on lower-level signing clients from this service.This design should make it easier to evolve signing/backoff behavior in one place without touching ManagedSignerService again.
59-61: Derived-by-user flow and tx result normalization align with the new signer abstractionRouting
executeDerivedEncodedTxByUserId→executeDerivedDecodedTxByUserId→executeDecodedTxByUserWallet→executeDerivedTx(userWallet.id, messages)keeps user lookup, validations, and event logic unchanged while delegating the actual signing/broadcast to TxManagerService.The normalization:
- Picks
{ code, hash, transactionHash, rawLog }from the returned tx.- Ensures
transactionHashis always populated, falling back tohashwhen needed.This matches how the tests assert the shape of the result and should make callers resilient to minor differences in what the underlying client returns.
Also applies to: 119-144
ygrishajev
force-pushed
the
feature/billing-master-wallets
branch
from
2e703e9 to
12a1b07
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (1)
42-48: Consider simplifying the factory resolution.As previously suggested, this registration could be simplified by directly chaining the resolution and call:
container.register(FUNDING_SIGNING_CLIENT, { useFactory: instancePerContainerCachingFactory(c => { - const factory = c.resolve<BatchSigningClientServiceFactory>(BATCH_SIGNING_CLIENT_FACTORY); - return factory(c.resolve(FUNDING_WALLET), "FUNDING_SIGNING_CLIENT"); + return c.resolve(BATCH_SIGNING_CLIENT_FACTORY)(c.resolve(FUNDING_WALLET), "FUNDING_SIGNING_CLIENT"); }) });
🧹 Nitpick comments (1)
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (1)
104-106: Consider the visibility of getDerivedWallet.This method is currently public, which exposes direct wallet instance access. If this is only used internally (by
getDerivedWalletAddress), consider making it private or protected to better encapsulate the implementation. However, if external consumers or tests need direct wallet access, the current public visibility is appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/api/src/billing/lib/wallet/wallet.ts(0 hunks)apps/api/src/billing/providers/index.ts(0 hunks)apps/api/src/billing/providers/signing-client.provider.ts(0 hunks)apps/api/src/billing/providers/wallet.provider.ts(0 hunks)apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts(1 hunks)apps/api/src/billing/services/tx-manager/tx-manager.service.ts(1 hunks)apps/api/test/functional/sign-and-broadcast-tx.spec.ts(2 hunks)
💤 Files with no reviewable changes (4)
- apps/api/src/billing/providers/signing-client.provider.ts
- apps/api/src/billing/providers/wallet.provider.ts
- apps/api/src/billing/providers/index.ts
- apps/api/src/billing/lib/wallet/wallet.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts
🧰 Additional context used
🧠 Learnings (3)
📓 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-19T15:15:07.266Z
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.266Z
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/test/functional/sign-and-broadcast-tx.spec.ts
📚 Learning: 2025-07-24T17:00:52.361Z
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.
Applied to files:
apps/api/test/functional/sign-and-broadcast-tx.spec.ts
🧬 Code graph analysis (1)
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (4)
apps/api/src/billing/lib/batch-signing-client/batch-signing-client.service.ts (2)
BatchSigningClientService(57-360)SignAndBroadcastOptions(24-34)apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-65)apps/api/src/billing/providers/type-registry.provider.ts (1)
TYPE_REGISTRY(34-34)apps/api/src/billing/lib/signing-stargate-client-factory/signing-stargate-client.factory.ts (1)
createSigningStargateClient(57-61)
⏰ 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). (6)
- 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
🔇 Additional comments (6)
apps/api/test/functional/sign-and-broadcast-tx.spec.ts (2)
4-4: Importingfetch-mockhere is appropriate for nativefetchtestsUsing
fetch-mockinstead ofnockmatches the project convention for mocking native Node 18+fetchin this test; the import is correctly scoped and used below, no extra changes needed.Based on learnings
121-128: Fetch-mock setup/teardown around RPC node calls looks correct
blockNode’sfetchMock.mockGlobal().route(RPC_NODE_ENDPOINT, { code, message })plusunblockNode’sfetchMock.hardReset()gives a clean, isolated mock of the RPC endpoint and restores globalfetchafter each use (via thetry/finallyin the test), which aligns with fetch-mock 12’s recommended pattern. Just ensure the installedfetch-mockversion supportsmockGlobalandhardReset(12.0.0+/12.2.0+).Based on learnings
apps/api/src/billing/services/tx-manager/tx-manager.service.ts (4)
64-70: LGTM!Both methods provide clean, straightforward delegation to the underlying wallet and signing client. The public API surface is well-designed.
72-83: Excellent cleanup logic in the finally block.The conditional cleanup strategy is well-designed:
- Prevents memory leaks by removing idle clients
- Preserves active clients that have pending transactions
- Logs cleanup events for observability
- Uses finally to ensure cleanup runs even on errors
This approach effectively balances memory management with performance for frequently-used wallets.
89-102: Cache management implementation is solid.The lazy client creation and caching strategy is well-implemented:
- Efficiently reuses clients for the same derivation index
- Logs client creation for debugging
- Non-null assertion on line 101 is safe within this controlled context
16-22: Funding wallet index choice is not a concern.The review comment's concern about separation is based on a misunderstanding. The FUNDING_WALLET and DERIVED_WALLET_FACTORY use different mnemonics ("FUNDING_WALLET_MNEMONIC" vs "DERIVATION_WALLET_MNEMONIC"), so they generate completely separate accounts regardless of index. The choice of index 1 for the funding wallet (versus 0) is a design decision but not related to wallet separation. Separation is already guaranteed by the mnemonic selection.
Likely an incorrect or invalid review comment.
ygrishajev
force-pushed
the
feature/billing-master-wallets
branch
from
12a1b07 to
10a2d6a
Compare
2 participants
Summary by CodeRabbit
Chores
Configuration
Tests
✏️ Tip: You can customize this high-level summary in your review settings.