fix: ensure managed wallet deployment close doesnot depend on authService#1932
fix: ensure managed wallet deployment close doesnot depend on authService#1932
Conversation
WalkthroughAdds a wallet-aware signer method executeDecodedTxByUserWallet, makes executeDecodedTxByUserId delegate to it, updates deployment close to call the wallet-based method, and applies wallet-owner-aware user resolution and gating for anonymous trial and lease-created events. Changes
Sequence Diagram(s)sequenceDiagram
participant DeploymentWriterService as DeploymentWriterService
participant ManagedSignerService as ManagedSignerService
participant UserRepo as UserRepository
participant Chain as Chain RPC
DeploymentWriterService->>ManagedSignerService: executeDecodedTxByUserWallet(wallet, [message])
alt ANONYMOUS_FREE_TRIAL enabled
ManagedSignerService->>ManagedSignerService: determine user context (walletOwner or fetch by wallet.userId)
ManagedSignerService->>UserRepo: fetch user (if needed)
UserRepo-->>ManagedSignerService: user
ManagedSignerService->>ManagedSignerService: run trial/provider/lease validations with user
end
ManagedSignerService->>Chain: broadcast tx (messages)
Chain-->>ManagedSignerService: { code, hash, transactionHash, rawLog }
opt hasCreateTrialLeaseMessage
ManagedSignerService->>ManagedSignerService: emit lease-created domain event
end
ManagedSignerService-->>DeploymentWriterService: tx result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
![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/services/managed-signer/managed-signer.service.ts (1)
143-145: Disallowanyin catch per repo TS guidelinesUse
unknownand pass it through. This complies with “never use any” and keeps error typing safe.Apply this diff:
- } catch (error: any) { + } catch (error: unknown) { throw await this.chainErrorService.toAppError(error, messages); }
🧹 Nitpick comments (3)
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (3)
100-103: Status code consistency for missing wallet ownerConsider returning 404 instead of 500 when the user tied to the wallet is not found, to align with other “not found” asserts in this service.
Apply this diff:
- assert(user, 500, "User for wallet not found"); + assert(user, 404, "User for wallet not found");
133-142: Make tx result assembly type-safe and resilient (hash vs transactionHash)Current casting can produce an object typed to have transactionHash when it may be absent. Prefer explicit normalization and a hard assert if neither hash is present.
Apply this diff:
- const result = pick(tx, ["code", "hash", "transactionHash", "rawLog"]) as Pick<IndexedTx, "code" | "hash" | "rawLog">; - - if (result.hash) { - return { - ...result, - transactionHash: result.hash - }; - } - - return result as Pick<IndexedTx, "code" | "hash" | "rawLog"> & { transactionHash: string }; + const picked = pick(tx, ["code", "hash", "transactionHash", "rawLog"]) as { + code: number; + hash?: string; + transactionHash?: string; + rawLog: string; + }; + const normalizedHash = picked.hash ?? picked.transactionHash; + assert(normalizedHash, 500, "Tx hash missing"); + return { + code: picked.code, + hash: normalizedHash!, + rawLog: picked.rawLog, + transactionHash: picked.transactionHash ?? normalizedHash! + };
92-99: Optional: early guard to ensure messages are owned by the walletRight now we rely on chain-level signature checks. A fast-fail assert that message owner fields (where applicable) match userWallet.address would improve defense-in-depth and error clarity.
Would you like a small helper that inspects known Akash message types (*.MsgCloseDeployment, *.MsgUpdateDeployment, *.MsgDepositDeployment, *.MsgCreateDeployment, *.MsgCreateLease) and asserts owner matches wallet?
Also applies to: 113-121
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/billing/services/managed-signer/managed-signer.service.ts(4 hunks)apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/src/billing/services/managed-signer/managed-signer.service.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/src/billing/services/managed-signer/managed-signer.service.ts
🧬 Code graph analysis (1)
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (2)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(16-20)apps/api/src/user/repositories/user/user.repository.ts (1)
UserOutput(11-13)
⏰ 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 (4)
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (1)
71-71: Decouples close path from auth context — LGTMSwitching to executeDecodedTxByUserWallet removes the authService dependency, unblocking CloseDeploymentJob usage in workers.
Please confirm that all job/worker call sites now route through this wallet-based path.
apps/api/src/billing/services/managed-signer/managed-signer.service.ts (3)
79-80: Good delegation to wallet-based signerexecuteDecodedTxByUserId now delegates to the wallet variant with an explicit walletOwner. This preserves existing API while enabling worker-safe execution.
82-91: New wallet-based signer API fits the background-job use casePublic executeDecodedTxByUserWallet removes reliance on authService. Solid direction for worker contexts.
82-91: No unsafe call sites found — wallets are retrieved from trusted repos/internal services.executeDecodedTxByUserWallet is only invoked from internal services/controllers that pass UserWallet objects obtained via UserWalletRepository (accessibleBy/findById) — e.g. deployment-writer.service.ts (close), lease.service.ts (createLeasesAndSendManifest), provider-cleanup.service.ts, stale-managed-deployments-cleaner.service.ts — tests use seeded wallets. No direct use of request-provided wallet data detected.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1932 +/- ##
==========================================
- Coverage 44.68% 44.31% -0.38%
==========================================
Files 979 969 -10
Lines 27586 27241 -345
Branches 7134 7094 -40
==========================================
- Hits 12327 12071 -256
+ Misses 14160 13991 -169
- Partials 1099 1179 +80
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
stalniy
force-pushed
the
fix/close-deployment-trial
branch
from
e8d991b to
b10f1fc
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/test/functional/deployments.spec.ts (1)
587-588: Update tests: spy on executeDecodedTxByUserWallet for close flowClose() now calls signerService.executeDecodedTxByUserWallet (apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts:71). Change the jest.spyOn in apps/api/test/functional/deployments.spec.ts (lines 587–588 and 683–684) from executeDecodedTxByUserId to executeDecodedTxByUserWallet. Keep executeDecodedTxByUserId mocks for flows that still use it (deployment-writer.service.ts:60/89/124; apps/api/src/deployment/services/lease/lease.service.ts:30; apps/api/src/certificate/services/certificate.service.ts:34).
🧹 Nitpick comments (5)
apps/api/test/functional/deployments.spec.ts (5)
520-521: Switch to wallet-based signer for close: good; assert the call to prevent regressionsThis aligns the DELETE flow with the wallet-based API. Add an assertion that the wallet path is invoked, so future refactors don’t silently drift back to userId.
Apply this diff:
- jest.spyOn(signerService, "executeDecodedTxByUserWallet").mockResolvedValueOnce(mockTxResult); + const execWalletSpy = jest.spyOn(signerService, "executeDecodedTxByUserWallet").mockResolvedValueOnce(mockTxResult); @@ expect(response.status).toBe(200); const result = await response.json(); expect(result).toEqual({ data: { success: true } }); + expect(execWalletSpy).toHaveBeenCalledTimes(1); + expect(execWalletSpy).toHaveBeenCalledWith( + expect.objectContaining({ address: wallets[0].address }), + expect.any(Array), + expect.anything() + );
507-535: Add a failure-path test for signer non-zero codeWe should cover the case where
executeDecodedTxByUserWalletreturns a non‑zero code (or throws), and assert the endpoint responds with the expected error status/body.I can draft the additional test if you confirm the expected HTTP status for a failed sign (e.g., 4xx vs 5xx) in the close flow.
172-174: Remove duplicate nock stub for active leases listThe same persistent handler is registered twice; drop the duplicate to avoid confusion.
- nock(apiNodeUrl).persist().get(`/akash/market/${betaTypeVersionMarket}/leases/list?filters.owner=${address}&filters.state=active`).reply(200, { leases }); - nock(apiNodeUrl).persist().get(`/akash/market/${betaTypeVersionMarket}/leases/list?filters.owner=${address}&filters.state=active`).reply(200, { leases }); + nock(apiNodeUrl) + .persist() + .get(`/akash/market/${betaTypeVersionMarket}/leases/list?filters.owner=${address}&filters.state=active`) + .reply(200, { leases });
836-844: Assert status code in this GET-by-address testTiny hardening: verify 200 before parsing.
const response = await app.request(`/v1/addresses/${wallets[0].address}/deployments/0/1`, { method: "GET", headers: new Headers({ "Content-Type": "application/json", "x-api-key": userApiKeySecret }) }); + expect(response.status).toBe(200); const result = await response.json();
45-89: Optional: prefer per-test setup() over beforeEach for spec filesGuideline suggests a
setupfunction at the bottom of the rootdescribe. This suite is functional and fine, but consider migrating incrementally to reduce shared state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/billing/services/managed-signer/managed-signer.service.ts(4 hunks)apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts(1 hunks)apps/api/test/functional/deployments.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts
- apps/api/src/billing/services/managed-signer/managed-signer.service.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/test/functional/deployments.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/test/functional/deployments.spec.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/test/functional/deployments.spec.ts
⏰ 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
2 participants
Why
Otherwise we can't use it in background jobs because it requires authenticated user. CloseDeploymentJob failed with:
Summary by CodeRabbit
Bug Fixes
Refactor