feat(auth): implements managed wallet API JWT auth#1766
feat(auth): implements managed wallet API JWT auth#1766ygrishajev merged 26 commits intoakash-network:mainfrom
Conversation
WalkthroughRefactors deployment/lease flows to resolve wallets by userId, adds JWT-based provider authentication alongside mTLS (ProviderJwtTokenService and DI wiring), updates provider-proxy/HTTP SDK to accept unified auth, makes certificate fields optional, adjusts caching keys, removes legacy examples/tests, and adds test infra and DI tokens. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as API (Controller)
participant DWS as DeploymentWriter / LeaseService
participant WRS as WalletReaderService
participant PS as ProviderService
participant PJ as ProviderJwtTokenService
participant PX as ProviderProxy
participant Prov as Provider
Client->>API: create/update/lease (includes userId)
API->>DWS: call action with userId
DWS->>WRS: getWalletByUserId(userId)
WRS-->>DWS: WalletInitialized (includes address)
DWS->>PS: sendManifest({ provider, dseq, manifest, auth })
alt auth is mTLS (certificate provided)
PS->>PX: request (mtls auth)
else auth via walletId/JWT
PS->>PJ: generateJwtToken({ walletId, leases })
PJ-->>PS: jwtToken
PS->>PX: request (jwt auth)
end
PX->>Prov: forward request
Prov-->>PX: response
PX-->>PS: result
PS-->>DWS: success/error
DWS-->>API: result
API-->>Client: response
sequenceDiagram
autonumber
actor Client
participant API as Deployment Reader
participant WRS as WalletReaderService
participant PS as ProviderService
participant PX as ProviderProxy
participant Prov as Provider
Client->>API: GET /deployments (user)
API->>WRS: getWalletByUserId(userId)
WRS-->>API: WalletInitialized
API->>PS: getLeaseStatus(..., auth)
PS->>PX: request GET /lease/.../status (auth)
PX->>Prov: forward
Prov-->>PX: lease status
PX-->>PS: data
PS-->>API: status
API-->>Client: deployment + leases
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🧹 Nitpick comments (4)
apps/api/package.json (1)
45-45: Consider pinning to a specific version for production stability.While the wildcard version may be acceptable for internal packages, consider using a specific version to ensure predictable builds and deployments.
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
19-19: Consider renaming for clarity.The
findWalletBymock function name could be more descriptive to indicate it's mocking thefindOneBymethod ofUserWalletRepository.Consider this naming improvement:
- const findWalletBy = jest.fn().mockImplementation(async () => newWallet); + const findOneBy = jest.fn().mockImplementation(async () => newWallet);And update the setup call accordingly:
- findWalletBy, + findOneBy,Also applies to: 26-26
apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts (1)
82-89: Consider consistent mocking approach.The JWT module mocking mixes manual
jest.fn()with thejest-mock-extendedapproach used elsewhere. While functional, consider consistency.The current approach works well for this scenario since you're mocking static methods. The mix of approaches is acceptable here.
apps/api/src/provider/services/provider/provider.service.ts (1)
18-18: Address the dependency cycle detected by static analysis.Static analysis has identified a dependency cycle involving billing services, deployment services, and the JWT token service. This can lead to circular dependency issues and reduced maintainability.
Based on the retrieved learning about handling cross-cutting concerns, consider creating a separate GitHub issue to address this architectural concern rather than expanding the current PR scope.
The cycle path is:
@src/billing/services→managed-signer.service→trial-validation.service→deployment-reader.serviceConsider extracting shared concerns or restructuring the dependency graph to eliminate the cycle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/package.json(1 hunks)apps/api/src/billing/lib/wallet/wallet.ts(1 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(3 hunks)apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts(3 hunks)apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts(2 hunks)apps/api/src/deployment/services/lease/lease.service.ts(1 hunks)apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts(1 hunks)apps/api/src/provider/services/jwt-token/jwt-token.service.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/billing/lib/wallet/wallet.tsapps/api/src/deployment/services/lease/lease.service.tsapps/api/src/provider/services/jwt-token/jwt-token.service.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/src/provider/services/jwt-token/jwt-token.service.spec.tsapps/api/src/provider/services/provider/provider.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/billing/lib/wallet/wallet.tsapps/api/src/deployment/services/lease/lease.service.tsapps/api/src/provider/services/jwt-token/jwt-token.service.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/src/provider/services/jwt-token/jwt-token.service.spec.tsapps/api/src/provider/services/provider/provider.service.ts
**/*.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/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: baktun14
PR: akash-network/console#1312
File: packages/jwt/src/jwt-token.ts:0-0
Timestamp: 2025-05-13T17:35:25.875Z
Learning: In the Akash JWT implementation, cryptographic signature verification is handled by the provider node code rather than in the JWT package itself. The JWT package only handles token creation, decoding, and basic validation (schema and time constraints).
Learnt from: baktun14
PR: akash-network/console#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: in the akash jwt implementation, cryptographic signature verification is handled by the provider nod...
Learnt from: baktun14
PR: akash-network/console#1312
File: packages/jwt/src/jwt-token.ts:0-0
Timestamp: 2025-05-13T17:35:25.875Z
Learning: In the Akash JWT implementation, cryptographic signature verification is handled by the provider node code rather than in the JWT package itself. The JWT package only handles token creation, decoding, and basic validation (schema and time constraints).
Applied to files:
apps/api/package.jsonapps/api/src/provider/services/jwt-token/jwt-token.service.tsapps/api/src/provider/services/jwt-token/jwt-token.service.spec.tsapps/api/src/provider/services/provider/provider.service.ts
📚 Learning: the `getbyowneranddseq` method in `apps/api/src/deployment/controllers/deployment/deployment.control...
Learnt from: baktun14
PR: akash-network/console#1428
File: apps/api/src/deployment/controllers/deployment/deployment.controller.ts:0-0
Timestamp: 2025-06-03T15:06:34.211Z
Learning: The `getByOwnerAndDseq` method in `apps/api/src/deployment/controllers/deployment/deployment.controller.ts` is intentionally public without the `@Protected` decorator because it serves public blockchain data from an indexer, following the pattern of public blockchain APIs.
Applied to files:
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts
📚 Learning: applies to **/*.spec.{ts,tsx} : don't use `jest.mock()` to mock dependencies in test files. instead,...
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-07-21T08:24:24.269Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use `jest.mock()` to mock dependencies in test files. Instead, use `jest-mock-extended` to create mocks and pass mocks as dependencies to the service under test.
Applied to files:
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
📚 Learning: applies to apps/{deploy-web,provider-console}/**/*.spec.tsx : use `queryby` methods instead of `getb...
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/query-by-in-tests.mdc:0-0
Timestamp: 2025-07-21T08:24:27.953Z
Learning: Applies to apps/{deploy-web,provider-console}/**/*.spec.tsx : Use `queryBy` methods instead of `getBy` methods in test expectations in `.spec.tsx` files
Applied to files:
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
📚 Learning: in apps/{deploy-web,provider-console}/**/*.spec.tsx files: use `getby` methods instead of `queryby` ...
Learnt from: stalniy
PR: akash-network/console#1660
File: apps/deploy-web/src/components/alerts/DeploymentAlertsContainer/DeploymentAlertsContainer.spec.tsx:54-56
Timestamp: 2025-07-11T10:46:43.711Z
Learning: In apps/{deploy-web,provider-console}/**/*.spec.tsx files: Use `getBy` methods instead of `queryBy` methods when testing element presence with `toBeInTheDocument()` because `getBy` throws an error and shows DOM state when element is not found, providing better debugging information than `queryBy` which returns null.
Applied to files:
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
📚 Learning: applies to **/*.spec.{ts,tsx} : `setup` function creates an object under test and returns it...
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : `setup` function creates an object under test and returns it
Applied to files:
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
📚 Learning: applies to **/*.spec.{ts,tsx} : use `setup` function instead of `beforeeach` in test files...
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Use `setup` function instead of `beforeEach` in test files
Applied to files:
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
📚 Learning: the getprovidershosturibyattributes method in apps/api/src/provider/repositories/provider/provider.r...
Learnt from: stalniy
PR: akash-network/console#1436
File: apps/api/src/provider/repositories/provider/provider.repository.ts:79-90
Timestamp: 2025-06-08T03:07:13.871Z
Learning: The getProvidersHostUriByAttributes method in apps/api/src/provider/repositories/provider/provider.repository.ts already has comprehensive test coverage in provider.repository.spec.ts, including tests for complex HAVING clause logic with COUNT(*) FILTER (WHERE ...) syntax, signature conditions (anyOf/allOf), and glob pattern matching.
Applied to files:
apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
🧬 Code Graph Analysis (5)
apps/api/src/deployment/services/lease/lease.service.ts (1)
apps/indexer/drizzle/schema.ts (1)
lease(110-178)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (2)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (2)
createWallet(59-69)createAndAuthorizeTrialSpending(44-57)apps/deploy-web/src/services/auth/auth.service.ts (1)
AuthService(5-21)
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (2)
apps/indexer/drizzle/schema.ts (3)
deployment(180-203)lease(110-178)provider(205-237)apps/api/src/deployment/http-schemas/deployment.schema.ts (1)
GetDeploymentResponse(252-252)
apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts (2)
apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-53)apps/api/src/billing/providers/config.provider.ts (1)
BillingConfig(9-9)
apps/api/src/provider/services/provider/provider.service.ts (4)
apps/api/src/billing/providers/config.provider.ts (2)
InjectBillingConfig(11-11)BillingConfig(9-9)apps/indexer/drizzle/schema.ts (1)
provider(205-237)apps/api/src/provider/services/provider/provider-proxy.service.ts (1)
ProviderIdentity(7-10)apps/api/src/deployment/http-schemas/lease.schema.ts (1)
LeaseStatusResponse(53-53)
🪛 GitHub Check: validate / validate-app
apps/api/src/provider/services/jwt-token/jwt-token.service.ts
[failure] 6-6:
Dependency cycle via "@src/billing/services/managed-signer/managed-signer.service:6=>../trial-validation/trial-validation.service:20=>@src/deployment/services/deployment-reader/deployment-reader.service:9=>@src/provider/services/provider/provider.service:12"
apps/api/src/provider/services/provider/provider.service.ts
[failure] 18-18:
Dependency cycle via "@src/billing/services:6=>@src/billing/services/managed-signer/managed-signer.service:6=>../trial-validation/trial-validation.service:20=>@src/deployment/services/deployment-reader/deployment-reader.service:9"
⏰ 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). (1)
- GitHub Check: test-build
🔇 Additional comments (17)
apps/api/src/billing/lib/wallet/wallet.ts (1)
50-52: LGTM! Clean method addition for JWT token support.The
getInstance()method provides a clean way to access the resolved wallet instance, which is needed for JWT token generation in the new authentication flow.apps/api/src/deployment/services/lease/lease.service.ts (1)
33-33: LGTM! Proper migration from certificate to JWT authentication.The change correctly replaces certificate-based authentication with wallet ID, which will be used to generate JWT tokens in the provider service.
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (1)
59-59: LGTM! Proper migration to wallet ID for JWT authentication.The change correctly replaces certificate-based authentication with wallet ID, which aligns with the JWT migration objectives.
apps/api/src/provider/services/jwt-token/jwt-token.service.ts (2)
14-31: LGTM! Well-structured JWT token generation logic.The JWT token generation implementation is solid:
- Proper use of the Akash wallet for signing
- Correct JWT claims structure with appropriate timing
- Good use of UUID for jti (JWT ID)
- Follows the established patterns from retrieved learnings
1-32: Ignore incorrect circular dependency warningAfter tracing the import graph through the following files, there is no cycle back to
managed-signer.serviceortrial-validation.service:•
managed-signer.service.tsdoes not importtrial-validation.service.ts.
•trial-validation.service.tsis only referenced by its own module (no upstream imports).
• The real chain isdeployment-reader.service.ts→provider.service.ts→jwt-token.service.ts→ billing providers/lib, with no return path.No refactoring is required. Please dismiss this warning.
Likely an incorrect or invalid review comment.
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (4)
5-5: LGTM!The import addition for
JwtTokenServicecorrectly supports the JWT authentication migration described in the PR objectives.
98-112: LGTM!The
UserWalletRepositorymock updates provide comprehensive coverage with proper method mocking, including the chaining pattern foraccessibleByand correct value transformation intoPublic.
119-124: LGTM!The
JwtTokenServicemock registration properly supports the JWT authentication migration with appropriate method stubbing forgenerateJwtToken.
132-132: LGTM!The
SetupInputinterface extension with the optionalfindWalletByproperty maintains consistency with the mock structure updates.apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts (3)
58-102: LGTM!The
setupfunction follows coding guidelines correctly: positioned at the bottom of the describe block, uses inline type definition for parameters, returns objects without explicit return type annotation, and avoids shared state.
21-32: LGTM!The test cases for basic JWT token generation and different wallet IDs provide good coverage with proper assertions and mock verification.
Also applies to: 47-55
91-94: LGTM!Container registration and cleanup are handled correctly, following established patterns.
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (2)
104-104: LGTM!The method call correctly passes
wallet.idinstead of certificate parameters, aligning with the JWT authentication migration.
128-128: LGTM!The method signature and implementation changes correctly transition from certificate-based to JWT token-based authentication by:
- Accepting
walletIdparameter instead of certificate options- Passing the
walletIdtoproviderService.sendManifestThis aligns with the broader JWT authentication migration described in the PR objectives.
Also applies to: 131-131
apps/api/src/provider/services/provider/provider.service.ts (3)
13-13: LGTM!The import additions for
ProviderIdentity,ProviderProxyService, andJwtTokenServicecorrectly support the JWT authentication migration. The constructor parameter updates properly inject the required dependencies.Also applies to: 18-18, 32-33
38-38: LGTM!The method signature changes correctly transition from certificate-based to wallet ID-based authentication:
sendManifestandgetLeaseStatusnow acceptwalletIdparameter- Private
sendManifestToProvidermethod updated accordinglyThis supports the JWT token generation approach described in the PR objectives.
Also applies to: 82-82, 56-56
62-65: LGTM!The JWT token implementation correctly:
- Generates JWT tokens using the
JwtTokenServicewith wallet ID- Uses standard Bearer token format in Authorization header
- Sets appropriate Content-Type for JSON requests
This properly implements the JWT authentication described in the PR objectives.
Also applies to: 95-98
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts
Show resolved
Hide resolved
apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
Outdated
Show resolved
Hide resolved
apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
Outdated
Show resolved
Hide resolved
|
This is great, thanks @jzsfkzm. A few things missing to consider this issue closed:
|
jzsfkzm
force-pushed
the
features/1038-jwt-not-cert
branch
from
75d3545 to
8401bd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
apps/api/examples/lease-flow.ts(9 hunks)apps/api/package.json(1 hunks)apps/api/src/billing/lib/wallet/wallet.ts(1 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(3 hunks)apps/api/src/deployment/http-schemas/deployment.schema.ts(1 hunks)apps/api/src/deployment/http-schemas/lease.schema.ts(0 hunks)apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts(5 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/provider/services/jwt-token/jwt-token.service.spec.ts(1 hunks)apps/api/src/provider/services/jwt-token/jwt-token.service.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.ts(5 hunks)apps/api/test/functional/deployments.spec.ts(7 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/deployment/http-schemas/lease.schema.ts
✅ Files skipped from review due to trivial changes (1)
- apps/api/examples/lease-flow.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/api/package.json
- apps/api/src/billing/lib/wallet/wallet.ts
- apps/api/src/deployment/services/lease/lease.service.ts
- apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
- apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
- apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/http-schemas/deployment.schema.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/test/functional/deployments.spec.tsapps/api/src/provider/services/jwt-token/jwt-token.service.tsapps/api/src/provider/services/provider/provider.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/http-schemas/deployment.schema.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/test/functional/deployments.spec.tsapps/api/src/provider/services/jwt-token/jwt-token.service.tsapps/api/src/provider/services/provider/provider.service.ts
**/*.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
🧠 Learnings (9)
📓 Common learnings
Learnt from: baktun14
PR: akash-network/console#1312
File: packages/jwt/src/jwt-token.ts:0-0
Timestamp: 2025-05-13T17:35:25.875Z
Learning: In the Akash JWT implementation, cryptographic signature verification is handled by the provider node code rather than in the JWT package itself. The JWT package only handles token creation, decoding, and basic validation (schema and time constraints).
Learnt from: baktun14
PR: akash-network/console#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: the `getbyowneranddseq` method in `apps/api/src/deployment/controllers/deployment/deployment.control...
Learnt from: baktun14
PR: akash-network/console#1428
File: apps/api/src/deployment/controllers/deployment/deployment.controller.ts:0-0
Timestamp: 2025-06-03T15:06:34.211Z
Learning: The `getByOwnerAndDseq` method in `apps/api/src/deployment/controllers/deployment/deployment.controller.ts` is intentionally public without the `@Protected` decorator because it serves public blockchain data from an indexer, following the pattern of public blockchain APIs.
Applied to files:
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/test/functional/deployments.spec.ts
📚 Learning: applies to apps/{deploy-web,provider-console}/**/*.spec.tsx : use `queryby` methods instead of `getb...
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/query-by-in-tests.mdc:0-0
Timestamp: 2025-07-21T08:24:27.953Z
Learning: Applies to apps/{deploy-web,provider-console}/**/*.spec.tsx : Use `queryBy` methods instead of `getBy` methods in test expectations in `.spec.tsx` files
Applied to files:
apps/api/test/functional/deployments.spec.ts
📚 Learning: applies to **/*.spec.{ts,tsx} : use `setup` function instead of `beforeeach` in test files...
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Use `setup` function instead of `beforeEach` in test files
Applied to files:
apps/api/test/functional/deployments.spec.ts
📚 Learning: applies to **/*.spec.{ts,tsx} : don't use `jest.mock()` to mock dependencies in test files. instead,...
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-07-21T08:24:24.269Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use `jest.mock()` to mock dependencies in test files. Instead, use `jest-mock-extended` to create mocks and pass mocks as dependencies to the service under test.
Applied to files:
apps/api/test/functional/deployments.spec.ts
📚 Learning: in apps/{deploy-web,provider-console}/**/*.spec.tsx files: use `getby` methods instead of `queryby` ...
Learnt from: stalniy
PR: akash-network/console#1660
File: apps/deploy-web/src/components/alerts/DeploymentAlertsContainer/DeploymentAlertsContainer.spec.tsx:54-56
Timestamp: 2025-07-11T10:46:43.711Z
Learning: In apps/{deploy-web,provider-console}/**/*.spec.tsx files: Use `getBy` methods instead of `queryBy` methods when testing element presence with `toBeInTheDocument()` because `getBy` throws an error and shows DOM state when element is not found, providing better debugging information than `queryBy` which returns null.
Applied to files:
apps/api/test/functional/deployments.spec.ts
📚 Learning: applies to **/*.spec.{ts,tsx} : don't use shared state in `setup` function...
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use shared state in `setup` function
Applied to files:
apps/api/test/functional/deployments.spec.ts
📚 Learning: applies to **/*.spec.{ts,tsx} : `setup` function creates an object under test and returns it...
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : `setup` function creates an object under test and returns it
Applied to files:
apps/api/test/functional/deployments.spec.ts
📚 Learning: in the akash jwt implementation, cryptographic signature verification is handled by the provider nod...
Learnt from: baktun14
PR: akash-network/console#1312
File: packages/jwt/src/jwt-token.ts:0-0
Timestamp: 2025-05-13T17:35:25.875Z
Learning: In the Akash JWT implementation, cryptographic signature verification is handled by the provider node code rather than in the JWT package itself. The JWT package only handles token creation, decoding, and basic validation (schema and time constraints).
Applied to files:
apps/api/src/provider/services/jwt-token/jwt-token.service.tsapps/api/src/provider/services/provider/provider.service.ts
🧬 Code Graph Analysis (4)
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (2)
apps/api/src/deployment/http-schemas/deployment.schema.ts (1)
GetDeploymentResponse(248-248)apps/indexer/drizzle/schema.ts (2)
lease(110-178)deployment(180-203)
apps/api/test/functional/deployments.spec.ts (1)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(16-20)
apps/api/src/provider/services/jwt-token/jwt-token.service.ts (2)
apps/api/src/provider/services/provider/provider.service.ts (1)
singleton(21-231)apps/api/src/billing/providers/config.provider.ts (2)
InjectBillingConfig(11-11)BillingConfig(9-9)
apps/api/src/provider/services/provider/provider.service.ts (4)
apps/api/src/billing/providers/config.provider.ts (2)
InjectBillingConfig(11-11)BillingConfig(9-9)apps/indexer/drizzle/schema.ts (1)
provider(205-237)apps/api/src/provider/services/provider/provider-proxy.service.ts (1)
ProviderIdentity(7-10)apps/api/src/deployment/http-schemas/lease.schema.ts (1)
LeaseStatusResponse(49-49)
🪛 GitHub Check: validate / validate-app
apps/api/src/provider/services/jwt-token/jwt-token.service.ts
[failure] 6-6:
Dependency cycle via "@src/billing/services/managed-signer/managed-signer.service:6=>../trial-validation/trial-validation.service:20=>@src/deployment/services/deployment-reader/deployment-reader.service:9=>@src/provider/services/provider/provider.service:12"
apps/api/src/provider/services/provider/provider.service.ts
[failure] 18-18:
Dependency cycle via "@src/billing/services:6=>@src/billing/services/managed-signer/managed-signer.service:6=>../trial-validation/trial-validation.service:20=>@src/deployment/services/deployment-reader/deployment-reader.service:9"
⏰ 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). (15)
- GitHub Check: validate (apps/indexer) / validate-unsafe
- GitHub Check: validate (apps/api) / validate-unsafe
- GitHub Check: validate (apps/provider-proxy) / validate-unsafe
- 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: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (13)
apps/api/src/deployment/http-schemas/deployment.schema.ts (1)
102-106: LGTM! Certificate fields correctly removed.The simplification of
UpdateDeploymentRequestSchemato only require thesdlfield aligns perfectly with the migration from certificate-based to JWT-based authentication. The removal of the nested certificate object withcertPemandkeyPemfields is consistent with the broader refactor.apps/api/src/provider/services/jwt-token/jwt-token.service.ts (2)
10-12: LGTM! Proper singleton service structure.The service is correctly implemented as a singleton with appropriate dependency injection for the billing configuration.
14-31: JWT token generation implementation looks correct.The token generation follows proper JWT standards with appropriate claims structure for Akash network authentication. The use of ES256K signing with secp256k1 curve and the claims structure (iss, exp, nbf, iat, jti, version, leases) align with Akash JWT specifications.
apps/api/test/functional/deployments.spec.ts (3)
43-43: Good addition for wallet-based testing.The
allWalletsarray properly aggregates user wallets across tests to support the new wallet-based authentication flow.
71-73: Proper wallet repository mocking.The mock implementation correctly returns wallets by address, which aligns with how
DeploymentReaderServicenow fetches wallets by owner address instead of using certificates.
698-699: Certificate fields correctly removed from test payloads.The test request bodies now only include the
sdlfield, properly reflecting the removal of certificate requirements from the deployment update API. This maintains test coverage while adapting to the JWT-based authentication model.Also applies to: 725-726, 746-747, 770-771
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (4)
10-10: Proper dependency injection for wallet repository.The addition of
UserWalletRepositorydependency correctly enables wallet lookup functionality needed for the JWT-based authentication flow.Also applies to: 25-25
28-29: Clean method signature simplification.Removing the optional certificate parameter and using the helper method to fetch wallet information simplifies the method signature while maintaining functionality through the JWT-based approach.
48-48: Wallet ID correctly used for provider authentication.The switch from certificate-based to wallet ID-based provider authentication is properly implemented, using the wallet's ID to generate JWT tokens for lease status requests.
288-295: Well-implemented helper method with proper error handling.The
getWalletByAddresshelper method includes appropriate null checking and error handling, addressing the concern from previous reviews about potential null access.apps/api/src/provider/services/provider/provider.service.ts (3)
18-18: JWT token service properly integrated.The addition of
JwtTokenServicedependency enables JWT-based authentication for provider communications, replacing the previous certificate-based approach.Also applies to: 33-33
38-38: Method signatures correctly updated for wallet-based authentication.Both
sendManifestandgetLeaseStatusmethods now acceptwalletIdparameters instead of certificate options, which aligns with the JWT-based authentication model.Also applies to: 82-82
62-65: Proper JWT Bearer token authentication implementation.The HTTP headers correctly use the
Authorization: Bearer <token>format with JWT tokens generated from wallet IDs. TheContent-Type: application/jsonheader is appropriately set for JSON payloads.Also applies to: 95-98
jzsfkzm
force-pushed
the
features/1038-jwt-not-cert
branch
from
8401bd7 to
879a472
Compare
|
Also, I haven't done any benchmarks on jwt generation, but if it takes some time to generate a new one every time, we could keep a cache per address for like a few minutes. |
jzsfkzm
force-pushed
the
features/1038-jwt-not-cert
branch
2 times, most recently
from
464301b to
a4cba21
Compare
jzsfkzm
force-pushed
the
features/1038-jwt-not-cert
branch
from
a4cba21 to
9deffa0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
apps/api/src/provider/services/provider/provider.service.ts (2)
74-80: Do not useany; improve error handling with type guards.This violates the TS guideline forbidding
any. Also, usingerr.messageon an unknown shape is unsafe and can misreport Axios error payloads.Apply:
+import { isAxiosError } from "axios"; @@ - } catch (err: any) { - if (err.message?.includes("no lease for deployment") && i < this.MANIFEST_SEND_MAX_RETRIES) { + } catch (err: unknown) { + const message = + isAxiosError(err) + ? (typeof err.response?.data === "string" ? err.response.data : err.message) + : (err instanceof Error ? err.message : String(err)); + if (message?.includes("no lease for deployment") && i < this.MANIFEST_SEND_MAX_RETRIES) { await delay(this.MANIFEST_SEND_RETRY_DELAY); continue; } - throw new Error(err?.response?.data || err); + throw new Error(message); }
31-47: Ensurelease-flow.tsuses JWT instead of API keys
The example atapps/api/examples/lease-flow.tsno longer references certificates—which is good—but it still authenticates with an API key (x-api-key) rather than a JWT. To complete the migration:
- File to update:
apps/api/examples/lease-flow.ts- In each call that sets
replace with something like:headers: { "x-api-key": apiKey }headers: { Authorization: `Bearer ${jwtToken}` }- Add a step at the top of the script to obtain or load
JWT_TOKEN(e.g. call your auth endpoint or read from env)- Remove the
API_KEYenv var references and related setupThis will close the issue by fully adopting JWT flows in the example.
♻️ Duplicate comments (1)
apps/api/src/provider/services/jwt-token/jwt-token.service.ts (1)
11-12: TTL increased to 30s — good balance for network latency and retries.This addresses earlier concerns about too-short tokens. Keep the ability to override via the method param.
🧹 Nitpick comments (4)
apps/api/src/provider/services/jwt-token/jwt-token.service.ts (2)
28-41: Account for small clock skew and guard TTL.To reduce “token not yet valid” edge cases, offset nbf by a few seconds and ensure
ttlis at least 1.Apply:
- const now = Math.floor(Date.now() / 1000); + const now = Math.floor(Date.now() / 1000); + const skew = 5; // seconds const token = await jwtToken.createToken({ iss: address, - exp: now + ttl, - nbf: now, + exp: now + Math.max(1, ttl), + nbf: now - skew, iat: now, jti: uuid.v4(), version: "v1", leases });
18-22: Consider typingleasesas JWTTokenPayload['leases'] (post jwt types update).Once the improved types are released in the jwt package, aligning to the payload type clarifies intent and reduces mismatch risk.
apps/api/src/provider/services/provider/provider.service.ts (2)
31-33: Avoid string-based JSON mutation for manifest.Regex replace on a JSON string is fragile. Prefer parsing + transforming the object, then
JSON.stringify.If out of scope, add a TODO to replace this with a structured transformation.
84-99: Consistent parameter shape: prefer object params forgetLeaseStatus.
sendManifestuses an object param;getLeaseStatusstill uses positional args. For consistency and future extensibility, consider switching to an object param (tracked as a separate issue to avoid widening PR scope).Would you like me to open an issue proposing this API shape refactor?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
apps/api/examples/lease-flow.ts(9 hunks)apps/api/package.json(1 hunks)apps/api/src/billing/lib/wallet/wallet.ts(1 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(3 hunks)apps/api/src/deployment/http-schemas/deployment.schema.ts(1 hunks)apps/api/src/deployment/http-schemas/lease.schema.ts(0 hunks)apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts(5 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/provider/services/jwt-token/jwt-token.service.spec.ts(1 hunks)apps/api/src/provider/services/jwt-token/jwt-token.service.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.spec.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.ts(3 hunks)apps/api/test/functional/deployments.spec.ts(7 hunks)packages/http-sdk/src/provider/provider-http.service.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/deployment/http-schemas/lease.schema.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/api/package.json
- apps/api/src/billing/lib/wallet/wallet.ts
- apps/api/examples/lease-flow.ts
- apps/api/src/deployment/http-schemas/deployment.schema.ts
- apps/api/src/deployment/services/lease/lease.service.ts
- apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
- packages/http-sdk/src/provider/provider-http.service.ts
- apps/api/test/functional/deployments.spec.ts
- apps/api/src/provider/services/provider/provider.service.spec.ts
- apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts
- apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts
- apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
🧰 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/provider/services/jwt-token/jwt-token.service.tsapps/api/src/provider/services/provider/provider.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/provider/services/jwt-token/jwt-token.service.tsapps/api/src/provider/services/provider/provider.service.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: baktun14
PR: akash-network/console#1312
File: packages/jwt/src/jwt-token.ts:0-0
Timestamp: 2025-05-13T17:35:25.875Z
Learning: In the Akash JWT implementation, cryptographic signature verification is handled by the provider node code rather than in the JWT package itself. The JWT package only handles token creation, decoding, and basic validation (schema and time constraints).
Learnt from: baktun14
PR: akash-network/console#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-05-13T17:35:25.875Z
Learnt from: baktun14
PR: akash-network/console#1312
File: packages/jwt/src/jwt-token.ts:0-0
Timestamp: 2025-05-13T17:35:25.875Z
Learning: In the Akash JWT implementation, cryptographic signature verification is handled by the provider node code rather than in the JWT package itself. The JWT package only handles token creation, decoding, and basic validation (schema and time constraints).
Applied to files:
apps/api/src/provider/services/jwt-token/jwt-token.service.tsapps/api/src/provider/services/provider/provider.service.ts
📚 Learning: 2025-07-24T17:00:52.361Z
Learnt from: baktun14
PR: akash-network/console#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/src/provider/services/jwt-token/jwt-token.service.ts
📚 Learning: 2025-06-08T03:07:13.871Z
Learnt from: stalniy
PR: akash-network/console#1436
File: apps/api/src/provider/repositories/provider/provider.repository.ts:79-90
Timestamp: 2025-06-08T03:07:13.871Z
Learning: The getProvidersHostUriByAttributes method in apps/api/src/provider/repositories/provider/provider.repository.ts already has comprehensive test coverage in provider.repository.spec.ts, including tests for complex HAVING clause logic with COUNT(*) FILTER (WHERE ...) syntax, signature conditions (anyOf/allOf), and glob pattern matching.
Applied to files:
apps/api/src/provider/services/provider/provider.service.ts
📚 Learning: 2025-06-03T15:06:34.211Z
Learnt from: baktun14
PR: akash-network/console#1428
File: apps/api/src/deployment/controllers/deployment/deployment.controller.ts:0-0
Timestamp: 2025-06-03T15:06:34.211Z
Learning: The `getByOwnerAndDseq` method in `apps/api/src/deployment/controllers/deployment/deployment.controller.ts` is intentionally public without the `Protected` decorator because it serves public blockchain data from an indexer, following the pattern of public blockchain APIs.
Applied to files:
apps/api/src/provider/services/provider/provider.service.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). (14)
- GitHub Check: validate (apps/provider-proxy) / 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: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (2)
apps/api/src/provider/services/jwt-token/jwt-token.service.ts (1)
45-52: Memoized signer/token scaffolding is appropriate.Caching the signer/JwtToken per wallet for 5 minutes reduces overhead without violating short token TTL constraints.
apps/api/src/provider/services/provider/provider.service.ts (1)
62-70: JWT scoping looks correct for send-manifest.Granular, provider-scoped permission for "send-manifest" aligns with the intended least-privilege approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/test/functional/lease-flow.spec.ts (1)
132-147: Skip certificate creation whenincludeCertificateis false
runLifecycle()always calls/v1/certificates, even when the test variant is supposed to run without a certificate. This adds ~3 network calls and blurs the intention of the “no-certificate” scenario.- // 3. Create certificate - const certResponse = await app.request("/v1/certificates", { … }); - … + let certPem: string | undefined; + let encryptedKey: string | undefined; + if (includeCertificate) { + const certResponse = await app.request("/v1/certificates", { … }); + ({ certPem, encryptedKey } = (await certResponse.json()).data); + }This keeps the happy-path fast and makes the two variants semantically distinct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/test/functional/deployments.spec.ts(7 hunks)apps/api/test/functional/lease-flow.spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/test/functional/deployments.spec.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/lease-flow.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/lease-flow.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/lease-flow.spec.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: baktun14
PR: akash-network/console#1312
File: packages/jwt/src/jwt-token.ts:0-0
Timestamp: 2025-05-13T17:35:25.875Z
Learning: In the Akash JWT implementation, cryptographic signature verification is handled by the provider node code rather than in the JWT package itself. The JWT package only handles token creation, decoding, and basic validation (schema and time constraints).
Learnt from: baktun14
PR: akash-network/console#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-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Use `setup` function instead of `beforeEach` in test files
Applied to files:
apps/api/test/functional/lease-flow.spec.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use shared state in `setup` function
Applied to files:
apps/api/test/functional/lease-flow.spec.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : `setup` function should accept a single parameter with inline type definition
Applied to files:
apps/api/test/functional/lease-flow.spec.ts
📚 Learning: 2025-07-21T08:24:27.953Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/query-by-in-tests.mdc:0-0
Timestamp: 2025-07-21T08:24:27.953Z
Learning: Applies to apps/{deploy-web,provider-console}/**/*.spec.tsx : Use `queryBy` methods instead of `getBy` methods in test expectations in `.spec.tsx` files
Applied to files:
apps/api/test/functional/lease-flow.spec.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : `setup` function creates an object under test and returns it
Applied to files:
apps/api/test/functional/lease-flow.spec.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : `setup` function must be at the bottom of the root `describe` block in test files
Applied to files:
apps/api/test/functional/lease-flow.spec.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't specify return type of `setup` function
Applied to files:
apps/api/test/functional/lease-flow.spec.ts
📚 Learning: 2025-07-21T08:24:24.269Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-07-21T08:24:24.269Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use `jest.mock()` to mock dependencies in test files. Instead, use `jest-mock-extended` to create mocks and pass mocks as dependencies to the service under test.
Applied to files:
apps/api/test/functional/lease-flow.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). (12)
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (1)
apps/api/test/functional/lease-flow.spec.ts (1)
117-130: Prefertest.each& migrate away frombeforeEach/shared stateThe new parameterisation works, but:
- Jest already provides
test.each/describe.each, which yields cleaner output and eliminates the manual loop.- Project guidelines for
*.spec.tsrequire using asetup()helper instead ofbeforeEachand shared top-level variables. Migrating to that pattern here will bring the file in line with the rest of the suite and avoid hidden state between runs.Refactoring now keeps the file future-proof and consistent.
⛔ Skipped due to learnings
Learnt from: CR PR: akash-network/console#0 File: .cursor/rules/setup-instead-of-before-each.mdc:0-0 Timestamp: 2025-07-21T08:25:07.474Z Learning: Applies to **/*.spec.{ts,tsx} : Use `setup` function instead of `beforeEach` in test filesLearnt from: CR PR: akash-network/console#0 File: .cursor/rules/setup-instead-of-before-each.mdc:0-0 Timestamp: 2025-07-21T08:25:07.474Z Learning: Applies to **/*.spec.{ts,tsx} : Don't use shared state in `setup` functionLearnt from: CR PR: akash-network/console#0 File: .cursor/rules/setup-instead-of-before-each.mdc:0-0 Timestamp: 2025-07-21T08:25:07.474Z Learning: Applies to **/*.spec.{ts,tsx} : `setup` function must be at the bottom of the root `describe` block in test filesLearnt from: CR PR: akash-network/console#0 File: .cursor/rules/setup-instead-of-before-each.mdc:0-0 Timestamp: 2025-07-21T08:25:07.474Z Learning: Applies to **/*.spec.{ts,tsx} : `setup` function creates an object under test and returns itLearnt from: CR PR: akash-network/console#0 File: .cursor/rules/setup-instead-of-before-each.mdc:0-0 Timestamp: 2025-07-21T08:25:07.474Z Learning: Applies to **/*.spec.{ts,tsx} : `setup` function should accept a single parameter with inline type definitionLearnt from: CR PR: akash-network/console#0 File: .cursor/rules/no-jest-mock.mdc:0-0 Timestamp: 2025-07-21T08:24:24.269Z Learning: Applies to **/*.spec.{ts,tsx} : Don't use `jest.mock()` to mock dependencies in test files. Instead, use `jest-mock-extended` to create mocks and pass mocks as dependencies to the service under test.Learnt from: CR PR: akash-network/console#0 File: .cursor/rules/setup-instead-of-before-each.mdc:0-0 Timestamp: 2025-07-21T08:25:07.474Z Learning: Applies to **/*.spec.{ts,tsx} : Don't specify return type of `setup` functionLearnt from: CR PR: akash-network/console#0 File: .cursor/rules/query-by-in-tests.mdc:0-0 Timestamp: 2025-07-21T08:24:27.953Z Learning: Applies to apps/{deploy-web,provider-console}/**/*.spec.tsx : Use `queryBy` methods instead of `getBy` methods in test expectations in `.spec.tsx` files
jzsfkzm
force-pushed
the
features/1038-jwt-not-cert
branch
from
b258616 to
64c64f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (4)
apps/api/examples/lease-flow.ts (1)
256-256: Certificate fields still being sent in deployment update.The deployment update request at line 256 still includes certificate fields (
certPemandkeyPem), which contradicts the PR objective of removing certificate-based authentication.Remove the certificate fields from the update request:
- body: JSON.stringify({ data: { sdl: ymlUpdate, certificate: { certPem, keyPem: encryptedKey } } }) + body: JSON.stringify({ data: { sdl: ymlUpdate } })apps/api/test/functional/lease-flow.spec.ts (1)
256-256: Certificate still being sent in deployment update request.Similar to the example file, the deployment update request still includes certificate fields even when
includeCertificateis false, which doesn't align with the test's intention to validate certificate-less flow.Update the deployment update request to conditionally include certificate:
- body: JSON.stringify({ data: { sdl: ymlUpdate, certificate: { certPem, keyPem: encryptedKey } } }) + body: JSON.stringify({ data: { sdl: ymlUpdate, certificate: includeCertificate ? { certPem, keyPem: encryptedKey } : undefined } })apps/api/test/functional/deployments.spec.ts (1)
713-741: Legacy certificate field in payload contradicts JWT migration intent; rename and drop the field, or remove the test
- The PR’s goal is to remove certificates from endpoints. Keeping a positive test that sends certificate encourages legacy usage.
- Also, the AI summary claims the test sends only sdl, but the code includes a certificate object.
Consider renaming the test to clarify backward-compat behavior and removing the field to avoid duplication with the previous test. Apply:
- it("should update a deployment successfully with a certificate provided", async () => { + it("ignores legacy certificate field in payload (backward-compat)", async () => { @@ - body: JSON.stringify({ - data: { - sdl: yml, - certificate: { - certPem: "test-cert-pem", - keyPem: "test-key-pem" - } - } - }), + body: JSON.stringify({ + data: { + sdl: yml + } + }),Alternatively, if the API should reject legacy fields now, delete this test entirely and add a negative test asserting 400 on unknown fields (depending on schema strictness).
apps/api/src/provider/services/provider/provider.service.ts (1)
75-80: Do not useany; catch unknown and preserve error details safelyPer coding guidelines, avoid
any. Also, preserve useful context when rethrowing.Apply:
- } catch (err: any) { - if (err.message?.includes("no lease for deployment") && i < this.MANIFEST_SEND_MAX_RETRIES) { + } catch (err: unknown) { + const msg = + typeof err === "object" && err !== null && "message" in err + ? String((err as Record<string, unknown>).message) + : String(err); + if (msg.includes("no lease for deployment") && i < this.MANIFEST_SEND_MAX_RETRIES) { await delay(this.MANIFEST_SEND_RETRY_DELAY); continue; } - throw new Error(err?.response?.data || err); + const maybeResponse = (err as Record<string, unknown>)["response"]; + const maybeData = + typeof maybeResponse === "object" && maybeResponse !== null && "data" in maybeResponse + ? (maybeResponse as Record<string, unknown>)["data"] + : undefined; + throw new Error(maybeData ? JSON.stringify(maybeData) : msg); }
♻️ Duplicate comments (1)
apps/api/src/provider/services/provider/provider.service.ts (1)
11-11: Remove import from deprecated provider-proxy; define a minimal local typeThis was raised earlier and still applies. Uncouple from provider-proxy by defining a local type.
Apply:
-import { ProviderIdentity } from "@src/provider/services/provider/provider-proxy.service"; +type ProviderIdentity = { owner: string; hostUri: string };
🧹 Nitpick comments (14)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (4)
19-19: Consider renaming for clarity.The variable name
findWalletByis generic and doesn't clearly indicate it mocksfindOneBy. Consider renaming tofindOneByfor better alignment with the actual method name.- const findWalletBy = jest.fn().mockImplementation(async () => newWallet); + const findOneBy = jest.fn().mockImplementation(async () => newWallet);
26-26: Update parameter name to match the renamed mock.If the mock variable is renamed to
findOneBy, update the parameter accordingly.- findWalletBy, + findOneBy,
132-132: Update interface property name for consistency.If the mock variable is renamed to
findOneBy, update the interface property accordingly.- findWalletBy?: UserWalletRepository["findOneBy"]; + findOneBy?: UserWalletRepository["findOneBy"];
99-99: Update mock assignment to use consistent naming.If the interface property is renamed, update the mock assignment accordingly.
- findOneBy: input?.findWalletBy, + findOneBy: input?.findOneBy,apps/api/examples/lease-flow.ts (2)
5-5: Update example description to reflect JWT authentication.The comment states "Checks if API key env var is set" but this doesn't accurately describe the transition from certificate-based to JWT-based authentication, which is the main objective of this PR.
Update the comment to better reflect the new authentication approach:
- * 1. Checks if API key env var is set + * 1. Validates API key for authentication
71-71: Update script description to mention JWT authentication.The docstring should be updated to reflect that the example now demonstrates JWT-based authentication flow instead of certificate-based flow.
- * It creates a deployment, waits for bids, creates a lease, and then closes the deployment. + * It creates a deployment, waits for bids, creates a lease using JWT authentication, and then closes the deployment.apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (2)
86-95: Consider error handling for lease status fetching in bulk operations.While the individual lease status calls are wrapped in try/catch blocks in
findByOwnerAndDseq, the bulk operations inlistmethod don't have similar error handling. If one lease status fetch fails, it could cause the entire operation to fail.Add error handling for individual lease status fetches:
- leaseResults.map(async ({ leases }) => { - return await Promise.all( - leases.map(async ({ lease }) => { - return await this.providerService.getLeaseStatus(lease.lease_id.provider, lease.lease_id.dseq, lease.lease_id.gseq, lease.lease_id.oseq, wallet.id); - }) - ); - }) + leaseResults.map(async ({ leases }) => { + return await Promise.all( + leases.map(async ({ lease }) => { + try { + return await this.providerService.getLeaseStatus(lease.lease_id.provider, lease.lease_id.dseq, lease.lease_id.gseq, lease.lease_id.oseq, wallet.id); + } catch { + return null; + } + }) + ); + })
288-295: Consider caching wallet lookups to avoid redundant database queries.The
getWalletByAddressmethod is called multiple times for the same address across different operations. For better performance, consider implementing a short-lived cache for wallet lookups.Consider implementing a caching mechanism for wallet lookups to reduce database queries, especially when multiple operations are performed for the same address in quick succession. This could be done using a simple Map with TTL or a dedicated caching solution.
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (1)
126-131: Consider adding retry logic for provider manifest delivery.The
sendManifestToProvidersmethod sends manifests sequentially without retry logic. If a provider is temporarily unreachable, the manifest delivery will fail without retry.Consider adding retry logic with exponential backoff:
private async sendManifestToProviders(walletId: number, dseq: string, manifest: string, leases: GetDeploymentResponse["data"]["leases"]): Promise<void> { const leaseProviders = leases.map(lease => lease.lease_id.provider).filter((v, i, s) => s.indexOf(v) === i); for (const provider of leaseProviders) { - await this.providerService.sendManifest({ provider, dseq, manifest, walletId }); + let retries = 3; + let delay = 1000; + while (retries > 0) { + try { + await this.providerService.sendManifest({ provider, dseq, manifest, walletId }); + break; + } catch (error) { + retries--; + if (retries === 0) throw error; + await new Promise(resolve => setTimeout(resolve, delay)); + delay *= 2; + } + } } }apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts (1)
105-161: Consider using the setup function pattern as recommended.According to the coding guidelines, the
setupfunction should be at the bottom of the rootdescribeblock. Currently it's placed after the tests.Move the setup function to the bottom of the describe block:
describe("JwtTokenService", () => { const mockMnemonic = "test mnemonic phrase for testing purposes only"; const mockAddress = "akash1testaddress123456789"; describe("generateJwtToken", () => { // ... tests ... }); describe("getGranularLeases", () => { // ... tests ... }); + + function setup(): { + // ... setup implementation ... + } }); - - function setup(): { - // ... setup implementation ... - }apps/api/test/functional/deployments.spec.ts (2)
71-73: Harden findOneBy mock against undefined query inputsMinor guard to avoid accidental throws if a caller passes an empty or incompatible query.
Apply this diff:
- jest.spyOn(userWalletRepository, "findOneBy").mockImplementation(async (query: Partial<UserWalletOutput> | undefined) => { - return Promise.resolve(allWallets.find(wallet => wallet.address === query?.address)); - }); + jest.spyOn(userWalletRepository, "findOneBy").mockImplementation(async (query?: Partial<UserWalletOutput>) => { + if (!query?.address) return undefined; + return allWallets.find(wallet => wallet.address === query.address); + });
46-51: Test style deviates from repo guidelines (uses beforeEach instead of setup pattern)Per coding guidelines for .spec.ts files, prefer a setup function at the bottom of the root describe instead of beforeEach. If functional tests are exempt, ignore; otherwise, consider refactoring in a follow-up to align styles.
apps/api/src/provider/services/provider/provider.service.ts (2)
31-33: Fragile manifest string replace risks corrupting payloadReplacing "quantity" with "size" via a global regex on a serialized manifest is brittle and may produce invalid JSON or unintended replacements. If this transform is still required, prefer an AST-level transform.
Would you like me to propose a safe transform that parses and serializes the manifest, remapping only the intended fields?
49-59: Avoid re-generating JWT inside retry loopJWT creation is deterministic for the same wallet/scope and adds overhead on each retry. Generate once before looping.
Apply:
- }) { - for (let i = 1; i <= this.MANIFEST_SEND_MAX_RETRIES; i++) { - try { - const jwtToken = await this.jwtTokenService.generateJwtToken({ - walletId, - leases: this.jwtTokenService.getGranularLeases({ - provider: providerIdentity.owner, - scope: ["send-manifest"] - }) - }); - const result = await this.providerHttpService.sendManifest({ hostUri: providerIdentity.hostUri, dseq, manifest, jwtToken }); + }) { + const jwtToken = await this.jwtTokenService.generateJwtToken({ + walletId, + leases: this.jwtTokenService.getGranularLeases({ + provider: providerIdentity.owner, + scope: ["send-manifest"] + }) + }); + + for (let i = 1; i <= this.MANIFEST_SEND_MAX_RETRIES; i++) { + try { + const result = await this.providerHttpService.sendManifest({ hostUri: providerIdentity.hostUri, dseq, manifest, jwtToken });Also applies to: 62-70
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
apps/api/examples/lease-flow.ts(9 hunks)apps/api/package.json(1 hunks)apps/api/src/billing/lib/wallet/wallet.ts(1 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(3 hunks)apps/api/src/deployment/http-schemas/deployment.schema.ts(1 hunks)apps/api/src/deployment/http-schemas/lease.schema.ts(0 hunks)apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts(5 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/provider/services/jwt-token/jwt-token.service.spec.ts(1 hunks)apps/api/src/provider/services/jwt-token/jwt-token.service.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.spec.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.ts(3 hunks)apps/api/test/functional/deployments.spec.ts(7 hunks)apps/api/test/functional/lease-flow.spec.ts(3 hunks)packages/http-sdk/src/provider/provider-http.service.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/deployment/http-schemas/lease.schema.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/api/package.json
- apps/api/src/billing/lib/wallet/wallet.ts
- apps/api/src/deployment/services/lease/lease.service.ts
- apps/api/src/deployment/http-schemas/deployment.schema.ts
- packages/http-sdk/src/provider/provider-http.service.ts
- apps/api/src/provider/services/provider/provider.service.spec.ts
- apps/api/src/provider/services/jwt-token/jwt-token.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/src/provider/services/jwt-token/jwt-token.service.spec.tsapps/api/test/functional/lease-flow.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/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/src/provider/services/jwt-token/jwt-token.service.spec.tsapps/api/test/functional/lease-flow.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/examples/lease-flow.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/src/provider/services/provider/provider.service.tsapps/api/test/functional/deployments.spec.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.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/provider/services/jwt-token/jwt-token.service.spec.tsapps/api/test/functional/lease-flow.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/examples/lease-flow.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/src/provider/services/provider/provider.service.tsapps/api/test/functional/deployments.spec.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: baktun14
PR: akash-network/console#1312
File: packages/jwt/src/jwt-token.ts:0-0
Timestamp: 2025-05-13T17:35:25.875Z
Learning: In the Akash JWT implementation, cryptographic signature verification is handled by the provider node code rather than in the JWT package itself. The JWT package only handles token creation, decoding, and basic validation (schema and time constraints).
📚 Learning: 2025-07-21T08:24:24.269Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-07-21T08:24:24.269Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use `jest.mock()` to mock dependencies in test files. Instead, use `jest-mock-extended` to create mocks and pass mocks as dependencies to the service under test.
Applied to files:
apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
📚 Learning: 2025-06-03T15:06:34.211Z
Learnt from: baktun14
PR: akash-network/console#1428
File: apps/api/src/deployment/controllers/deployment/deployment.controller.ts:0-0
Timestamp: 2025-06-03T15:06:34.211Z
Learning: The `getByOwnerAndDseq` method in `apps/api/src/deployment/controllers/deployment/deployment.controller.ts` is intentionally public without the `Protected` decorator because it serves public blockchain data from an indexer, following the pattern of public blockchain APIs.
Applied to files:
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts
🧬 Code Graph Analysis (6)
apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts (2)
apps/api/src/billing/providers/config.provider.ts (1)
BillingConfig(9-9)apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-53)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
apps/api/src/billing/services/managed-user-wallet/managed-user-wallet.service.ts (2)
createWallet(59-69)createAndAuthorizeTrialSpending(44-57)
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (2)
apps/indexer/drizzle/schema.ts (3)
deployment(180-203)lease(110-178)provider(205-237)apps/api/src/deployment/http-schemas/deployment.schema.ts (1)
GetDeploymentResponse(248-248)
apps/api/src/provider/services/provider/provider.service.ts (6)
apps/api/src/provider/services/jwt-token/jwt-token.service.ts (1)
singleton(24-60)apps/api/src/deployment/services/lease/lease.service.ts (1)
singleton(10-38)apps/api/src/deployment/services/cached-balance/cached-balance.service.ts (1)
singleton(21-37)packages/http-sdk/src/provider/provider-http.service.ts (1)
ProviderHttpService(6-42)apps/api/src/provider/services/provider/provider-proxy.service.ts (1)
ProviderIdentity(7-10)apps/api/src/deployment/http-schemas/lease.schema.ts (1)
LeaseStatusResponse(49-49)
apps/api/test/functional/deployments.spec.ts (1)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(16-20)
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (2)
apps/api/src/deployment/http-schemas/deployment.schema.ts (1)
GetDeploymentResponse(248-248)apps/indexer/drizzle/schema.ts (2)
lease(110-178)deployment(180-203)
⏰ 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). (16)
- GitHub Check: validate (apps/provider-console) / validate-unsafe
- GitHub Check: validate (apps/indexer) / validate-unsafe
- GitHub Check: validate (apps/notifications) / validate-unsafe
- GitHub Check: validate (packages) / validate-unsafe
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (13)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (3)
5-5: LGTM! JWT service properly imported for dependency injection.The import is correctly added to support the new JWT-based authentication system.
98-112: Well-structured UserWalletRepository mock implementation.The mock properly implements the repository pattern with correct method signatures and the
accessibleBymethod returning the mock instance itself, which is a good pattern for chainable methods.
119-124: LGTM! JWT token service mock properly configured.The mock correctly provides the
generateJwtTokenmethod returning a mock token string, which aligns with the JWT authentication migration objectives.apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (1)
28-49: LGTM! Clean implementation of wallet-based lease status retrieval.The migration from certificate-based to wallet-based authentication is well implemented. The method now properly retrieves the wallet and uses its ID for lease status lookups, with appropriate error handling.
apps/api/test/functional/lease-flow.spec.ts (1)
117-130: Comprehensive test coverage for both certificate paths.Excellent approach to test both certificate-present and certificate-absent scenarios using parameterized tests. This ensures backward compatibility while validating the new JWT-based flow.
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (1)
94-107: LGTM! Clean transition to wallet-based manifest delivery.The update method has been successfully refactored to use wallet ID for JWT authentication instead of certificates. The manifest sending logic is properly abstracted.
apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts (2)
1-14: Well-structured test setup following coding guidelines.The test file properly uses
jest-mock-extendedfor creating mocks and avoids usingjest.mock(), adhering to the coding guidelines.
50-59: Excellent test for JWT memoization.Great test coverage for verifying that JWT token generation is properly memoized, ensuring that wallet creation and signing operations are not repeated unnecessarily.
apps/api/test/functional/deployments.spec.ts (4)
43-45: Wallet aggregation + findOneBy mock are sensible and scoped. LGTMCollecting wallets in allWallets and resolving findOneBy by address keeps tests deterministic once reset in beforeEach. This reduces duplication and mirrors the repository’s typical usage.
Also applies to: 50-51, 71-73, 117-118
694-711: PUT update without certificate looks correctThe test reflects the JWT migration by sending only sdl and asserting deployment info + leases are returned. Good coverage.
760-766: 404 path without certificate aligns with new contractUpdating the negative path to only include sdl keeps tests aligned with JWT-based flow.
785-786: Input fields trimmed down to sdl for unauthorized and invalid-sdl cases is correctThese cases no longer reference certificate fields and focus on the intended validation.
Also applies to: 809-810
apps/api/src/provider/services/provider/provider.service.ts (1)
84-99: Lease status via JWT and direct provider HTTP is correctSwitching to JWT-scoped "status" and calling provider directly matches the new architecture. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/src/provider/services/provider/provider.service.spec.ts (1)
38-38: Consider extracting granular leases creation to a helper.The
getGranularLeasesmock setup is duplicated across tests. Consider creating a test helper function to generate consistent lease configurations and reduce duplication.+ function createGranularLeases(providerAddress: string, scope: string[]): JwtTokenPayload["leases"] { + return { + access: "granular", + permissions: [{ provider: providerAddress, access: "scoped", scope }] + }; + } + function setup() { const providerHttpService = mock<ProviderHttpService>(); const providerAttributesSchemaService = mock<ProviderAttributesSchemaService>(); const auditorsService = mock<AuditorService>(); const jwtTokenService = mock<JwtTokenService>(); const service = new ProviderService(providerHttpService, providerAttributesSchemaService, auditorsService, jwtTokenService); return { service, providerHttpService, providerAttributesSchemaService, auditorsService, - jwtTokenService + jwtTokenService, + createGranularLeases }; }Also applies to: 190-190
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts(1 hunks)apps/api/src/provider/services/jwt-token/jwt-token.service.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
- apps/api/src/provider/services/jwt-token/jwt-token.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/src/provider/services/provider/provider.service.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/src/provider/services/provider/provider.service.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/src/provider/services/provider/provider.service.spec.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: baktun14
PR: akash-network/console#1312
File: packages/jwt/src/jwt-token.ts:0-0
Timestamp: 2025-05-13T17:35:25.875Z
Learning: In the Akash JWT implementation, cryptographic signature verification is handled by the provider node code rather than in the JWT package itself. The JWT package only handles token creation, decoding, and basic validation (schema and time constraints).
📚 Learning: 2025-06-08T03:07:13.871Z
Learnt from: stalniy
PR: akash-network/console#1436
File: apps/api/src/provider/repositories/provider/provider.repository.ts:79-90
Timestamp: 2025-06-08T03:07:13.871Z
Learning: The getProvidersHostUriByAttributes method in apps/api/src/provider/repositories/provider/provider.repository.ts already has comprehensive test coverage in provider.repository.spec.ts, including tests for complex HAVING clause logic with COUNT(*) FILTER (WHERE ...) syntax, signature conditions (anyOf/allOf), and glob pattern matching.
Applied to files:
apps/api/src/provider/services/provider/provider.service.spec.ts
🧬 Code Graph Analysis (1)
apps/api/src/provider/services/provider/provider.service.spec.ts (1)
packages/http-sdk/src/provider/provider-http.service.ts (1)
ProviderHttpService(6-42)
⏰ 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). (14)
- GitHub Check: validate (apps/stats-web) / validate-unsafe
- GitHub Check: validate (apps/notifications) / 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: Validate local packages
🔇 Additional comments (6)
apps/api/src/provider/services/provider/provider.service.spec.ts (6)
1-9: LGTM! Clean imports and proper TypeScript types.The imports follow best practices with type-only imports where appropriate and proper use of
jest-mock-extendedfor mocking as specified in the coding guidelines.
225-241: LGTM! Proper test setup following coding guidelines.The
setupfunction correctly:
- Uses
jest-mock-extendedfor mocking dependencies- Is placed at the bottom of the describe block
- Accepts a single parameter (none in this case, which is fine)
- Doesn't use shared state
- Returns the service and its dependencies for testing
99-125: Retry limit is defined in codeThe service uses a private constant
MANIFEST_SEND_MAX_RETRIES = 3(apps/api/src/provider/services/provider/provider.service.ts line 21), which matches the test’s expectation of three attempts. No external configuration exists for this value—any change to the retry limit must be updated here and in the corresponding spec.• Location of retry limit:
• apps/api/src/provider/services/provider/provider.service.ts, line 21
12-55: Quantity→Size Replacement Is Intentional and Implemented
Inapps/api/src/provider/services/provider/provider.service.ts, thesendManifestmethod clearly applies:const manifestWithSize = manifest.replace(/"quantity":{"val/g, '"size":{"val');This matches the test’s expectation of transforming
{"quantity":{"val":"1"}}to{"size":{"val":"1"}}. No changes required.
31-34: Scopes align with JWT schema and implementation – no changes neededVerified that both
"send-manifest"and"status"are listed in packages/jwt/src/types.ts and the generated JSON schema, and that provider.service.ts correctly uses"send-manifest"for sending manifests and"status"for lease status. Tests accordingly pass the right scopes.
57-84: Retry logic confirmed & test timeout verified
- In
apps/api/src/provider/services/provider/provider.service.ts:
MANIFEST_SEND_MAX_RETRIES = 3MANIFEST_SEND_RETRY_DELAY = 6000(ms)- Retry loop on “no lease for deployment” errors runs at most once (first call fails, waits 6 s, then succeeds), fitting comfortably within the 10 s Jest timeout.
No changes required.
jzsfkzm
force-pushed
the
features/1038-jwt-not-cert
branch
from
d64e7b2 to
3f93180
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
apps/api/test/functional/lease-flow.spec.ts (1)
256-256: Inconsistency: Certificate still included in update request.While the lease creation conditionally includes the certificate based on
includeCertificate, the update request always includes it. This seems inconsistent with the goal of supporting certificate-less flows.The update request should also respect the
includeCertificateparameter:-body: JSON.stringify({ data: { sdl: ymlUpdate, certificate: { certPem, keyPem: encryptedKey } } }) +body: JSON.stringify({ + data: { + sdl: ymlUpdate, + ...(includeCertificate && { certificate: { certPem, keyPem: encryptedKey } }) + } +})apps/api/test/functional/deployments.spec.ts (1)
713-751: Remove legacy "with certificate provided" test from deployments.spec.tsUpdateDeploymentRequestSchema no longer accepts a certificate (it only contains
sdl), so the test that sends a certificate is stale and should be removed or replaced with an explicit test for unknown-field handling at the validator layer.Files to update:
- apps/api/test/functional/deployments.spec.ts — remove the
it("should update a deployment successfully with a certificate provided", ...)test (lines ~713–751).- apps/api/src/deployment/http-schemas/deployment.schema.ts — verified:
UpdateDeploymentRequestSchemaonly includessdl.- apps/api/test/functional/lease-flow.spec.ts — contains other tests that include
certificate; review/adjust those tests for parity with the new schema.Apply:
- it("should update a deployment successfully with a certificate provided", async () => { - const { userApiKeySecret, wallets } = await mockUser(); - const dseq = "1234"; - setupDeploymentInfoMock(wallets, dseq); - - const mockTxResult = { - code: 0, - hash: "test-hash", - transactionHash: "test-hash", - rawLog: "success" - }; - - jest.spyOn(signerService, "executeDecodedTxByUserId").mockResolvedValueOnce(mockTxResult); - - const yml = fs.readFileSync(path.resolve(__dirname, "../mocks/hello-world-sdl.yml"), "utf8"); - - const response = await app.request(`/v1/deployments/${dseq}`, { - method: "PUT", - body: JSON.stringify({ - data: { - sdl: yml, - certificate: { - certPem: "test-cert-pem", - keyPem: "test-key-pem" - } - } - }), - headers: new Headers({ "Content-Type": "application/json", "x-api-key": userApiKeySecret }) - }); - - expect(response.status).toBe(200); - const result = (await response.json()) as { data: unknown }; - expect(result.data).toEqual({ - deployment: expect.any(Object), - escrow_account: expect.any(Object), - leases: expect.arrayContaining([expect.any(Object)]) - }); - });apps/api/src/provider/services/provider/provider.service.ts (1)
74-81: Preserve original error and broaden retry condition (handle provider HTTP errors)Short result of verification: rg returned no other occurrences of "no lease for deployment" and no import of provider-proxy in apps/api/src/provider/services/provider/provider.service.ts — the issue is local to this file and the suggested change is appropriate.
Please apply the fix at:
- apps/api/src/provider/services/provider/provider.service.ts — catch block (around lines 74–81)
Apply:
- } catch (err: any) { - if (err.message?.includes("no lease for deployment") && i < this.MANIFEST_SEND_MAX_RETRIES) { - await delay(this.MANIFEST_SEND_RETRY_DELAY); - continue; - } - throw new Error(err?.response?.data || err); - } + } catch (err) { + // Retry if the provider reports missing lease yet (eventual consistency) or 404 + const message = (err as any)?.message ?? ""; + const status = (err as any)?.response?.status as number | undefined; + if ((typeof message === "string" && message.includes("no lease for deployment")) || status === 404) { + if (i < this.MANIFEST_SEND_MAX_RETRIES) { + await delay(this.MANIFEST_SEND_RETRY_DELAY); + continue; + } + } + // Preserve original error to keep the stack and provider payload + throw err; + }Reason: string-matching err.message is brittle; providers commonly return HTTP errors (inspect response.status). Wrapping the original error in a new Error loses the provider payload and stack — rethrow the original error instead.
♻️ Duplicate comments (1)
apps/api/src/provider/services/provider/provider.service.ts (1)
11-12: Stop importing ProviderIdentity from deprecated provider-proxy; define a local type.We should avoid coupling to provider-proxy. The current import contradicts earlier review feedback and the migration away from provider-proxy.
Apply within this range:
-import { ProviderIdentity } from "@src/provider/services/provider/provider-proxy.service"; +// Using a local minimal identity type to avoid coupling to deprecated provider-proxyAdd this local type near the top of the file (e.g., after imports):
type ProviderIdentity = { owner: string; hostUri: string };
🧹 Nitpick comments (8)
packages/http-sdk/src/provider/provider-http.service.ts (2)
15-24: Consider adding error handling for manifest sending.The
sendManifestmethod should handle potential errors more gracefully, particularly for manifest validation or network issues.Consider wrapping the request in a try-catch to provide more context:
async sendManifest({ hostUri, dseq, manifest, jwtToken }: { hostUri: string; dseq: string; manifest: string; jwtToken: string }) { - return this.extractData( - await this.put(`/deployment/${dseq}/manifest`, { - baseURL: hostUri, - body: manifest, - headers: this.getJwtTokenHeaders(jwtToken), - timeout: 60000 - }) - ); + try { + return this.extractData( + await this.put(`/deployment/${dseq}/manifest`, { + baseURL: hostUri, + body: manifest, + headers: this.getJwtTokenHeaders(jwtToken), + timeout: 60000 + }) + ); + } catch (error) { + // Re-throw with more context for better debugging + throw new Error(`Failed to send manifest to ${hostUri}/deployment/${dseq}: ${error.message}`); + } }
36-41: Consider making JWT header generation more flexible.The current implementation hard-codes the
Content-Typeheader, which might not be appropriate for all endpoints (especially GET requests that don't have a body).Consider making the Content-Type header conditional:
-private getJwtTokenHeaders(jwtToken: string) { +private getJwtTokenHeaders(jwtToken: string, includeContentType = true) { + const headers: Record<string, string> = { + Authorization: `Bearer ${jwtToken}` + }; + + if (includeContentType) { + headers["Content-Type"] = "application/json"; + } + + return headers; +}Then update the
getLeaseStatuscall:headers: this.getJwtTokenHeaders(jwtToken, false),apps/api/examples/lease-flow.ts (1)
106-113: Consider making the provider selection configurable.The hard-coded provider address makes the example less flexible and may fail if that specific provider is unavailable.
Consider making the provider selection configurable via environment variable:
// Select a bid from a specific provider -const targetProvider = "akash175llqyjvxfle9qwt740vm46772dzaznpzgm576"; +const targetProvider = process.env.TARGET_PROVIDER || "akash175llqyjvxfle9qwt740vm46772dzaznpzgm576"; const selectedBid = bids.find(bid => bid.bid.bid_id.provider === targetProvider); if (!selectedBid) { - throw new Error(`No bid found from provider ${targetProvider}`); + // If specific provider not found, use the first available bid + console.warn(`No bid found from provider ${targetProvider}, using first available bid`); + const fallbackBid = bids[0]; + if (!fallbackBid) { + throw new Error("No bids available"); + } + return fallbackBid; }apps/api/src/provider/services/provider/provider.service.spec.ts (1)
83-84: Consider adding assertion for retry delay.The test verifies retry behavior but doesn't verify that there's an appropriate delay between retries to avoid overwhelming the provider.
Consider using jest's timer mocks to verify retry delays:
it("should retry on lease not found error with delay", async () => { jest.useFakeTimers(); const { service, jwtTokenService, providerHttpService } = setup(); // ... existing setup ... const resultPromise = service.sendManifest({ provider: providerAddress, dseq, manifest, walletId }); // Fast-forward time for first retry jest.advanceTimersByTime(2000); // or whatever the retry delay is const result = await resultPromise; expect(providerHttpService.sendManifest).toHaveBeenCalledTimes(2); expect(result).toEqual({ success: true }); jest.useRealTimers(); });apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (1)
42-59: Consider adding error logging for failed lease status retrievals.Silent failures in lease status retrieval might make debugging difficult in production.
Consider adding error logging:
} catch { + console.warn(`Failed to retrieve lease status for provider ${lease.lease_id.provider}, dseq ${lease.lease_id.dseq}`); return { lease, status: null }; }apps/api/src/provider/services/provider/provider.service.ts (3)
31-33: Avoid brittle string replacement for manifest mutation; perform a JSON-level transform.Direct regex replacement on a JSON string is error-prone and can break if formatting changes. Prefer parsing and transforming the structure, with a safe fallback if parsing fails.
Apply within this range:
- const manifestWithSize = manifest.replace(/"quantity":{"val/g, '"size":{"val'); + const manifestWithSize = transformManifestQuantitiesToSize(manifest);Add this helper in the file (outside this range):
function transformManifestQuantitiesToSize(manifestJson: string): string { try { const obj = JSON.parse(manifestJson); const walk = (node: any) => { if (node && typeof node === "object") { if ("quantity" in node && node.quantity && typeof node.quantity === "object" && "val" in node.quantity) { node.size = node.quantity; delete node.quantity; } for (const k of Object.keys(node)) walk((node as any)[k]); } }; walk(obj); return JSON.stringify(obj); } catch { // Fallback to legacy behavior if the manifest isn't strict JSON return manifestJson.replace(/"quantity":{"val/g, '"size":{"val'); } }
49-59: Consider consolidating provider identity resolution and caching.sendManifest and getLeaseStatus both call getProvider to resolve host_uri for the same provider address. Introduce a small cache (TTL) to avoid repeated network lookups under load.
If desired, I can add a memoized resolveProviderIdentity(address) that caches hostUri for a few minutes, similar to JwtTokenService.getJwtToken memoization.
84-99: Status path JWT scope and direct provider call — LGTM; consider provider identity caching.
- Using scope "status" is consistent with granular access for read-only status calls.
- Repeating getProvider lookups could be cached alongside sendManifest, as noted above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
apps/api/examples/lease-flow.ts(9 hunks)apps/api/package.json(1 hunks)apps/api/src/billing/lib/wallet/wallet.ts(1 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(3 hunks)apps/api/src/deployment/http-schemas/deployment.schema.ts(1 hunks)apps/api/src/deployment/http-schemas/lease.schema.ts(0 hunks)apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts(5 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/provider/services/jwt-token/jwt-token.service.spec.ts(1 hunks)apps/api/src/provider/services/jwt-token/jwt-token.service.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.spec.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.ts(3 hunks)apps/api/test/functional/deployments.spec.ts(7 hunks)apps/api/test/functional/lease-flow.spec.ts(3 hunks)packages/http-sdk/src/provider/provider-http.service.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/deployment/http-schemas/lease.schema.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/api/package.json
- apps/api/src/billing/lib/wallet/wallet.ts
- apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
- apps/api/src/deployment/http-schemas/deployment.schema.ts
- apps/api/src/provider/services/jwt-token/jwt-token.service.ts
- apps/api/src/deployment/services/lease/lease.service.ts
- apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts
- apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
packages/http-sdk/src/provider/provider-http.service.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/src/provider/services/provider/provider.service.spec.tsapps/api/examples/lease-flow.tsapps/api/src/provider/services/provider/provider.service.tsapps/api/test/functional/deployments.spec.tsapps/api/test/functional/lease-flow.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:
packages/http-sdk/src/provider/provider-http.service.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/src/provider/services/provider/provider.service.spec.tsapps/api/examples/lease-flow.tsapps/api/src/provider/services/provider/provider.service.tsapps/api/test/functional/deployments.spec.tsapps/api/test/functional/lease-flow.spec.ts
**/*.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/src/provider/services/provider/provider.service.spec.tsapps/api/test/functional/deployments.spec.tsapps/api/test/functional/lease-flow.spec.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: baktun14
PR: akash-network/console#1312
File: packages/jwt/src/jwt-token.ts:0-0
Timestamp: 2025-05-13T17:35:25.875Z
Learning: In the Akash JWT implementation, cryptographic signature verification is handled by the provider node code rather than in the JWT package itself. The JWT package only handles token creation, decoding, and basic validation (schema and time constraints).
📚 Learning: 2025-06-03T15:06:34.211Z
Learnt from: baktun14
PR: akash-network/console#1428
File: apps/api/src/deployment/controllers/deployment/deployment.controller.ts:0-0
Timestamp: 2025-06-03T15:06:34.211Z
Learning: The `getByOwnerAndDseq` method in `apps/api/src/deployment/controllers/deployment/deployment.controller.ts` is intentionally public without the `Protected` decorator because it serves public blockchain data from an indexer, following the pattern of public blockchain APIs.
Applied to files:
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts
📚 Learning: 2025-06-08T03:07:13.871Z
Learnt from: stalniy
PR: akash-network/console#1436
File: apps/api/src/provider/repositories/provider/provider.repository.ts:79-90
Timestamp: 2025-06-08T03:07:13.871Z
Learning: The getProvidersHostUriByAttributes method in apps/api/src/provider/repositories/provider/provider.repository.ts already has comprehensive test coverage in provider.repository.spec.ts, including tests for complex HAVING clause logic with COUNT(*) FILTER (WHERE ...) syntax, signature conditions (anyOf/allOf), and glob pattern matching.
Applied to files:
apps/api/src/provider/services/provider/provider.service.spec.ts
🧬 Code Graph Analysis (4)
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (2)
apps/api/src/deployment/http-schemas/deployment.schema.ts (1)
GetDeploymentResponse(248-248)apps/indexer/drizzle/schema.ts (2)
lease(110-178)deployment(180-203)
apps/api/src/provider/services/provider/provider.service.spec.ts (1)
packages/http-sdk/src/provider/provider-http.service.ts (1)
ProviderHttpService(6-42)
apps/api/src/provider/services/provider/provider.service.ts (6)
apps/api/src/deployment/services/lease/lease.service.ts (1)
singleton(10-38)apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (1)
singleton(22-132)apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (1)
singleton(18-296)apps/provider-proxy/src/services/ProviderService/ProviderService.ts (1)
ProviderService(7-45)packages/http-sdk/src/provider/provider-http.service.ts (1)
ProviderHttpService(6-42)apps/api/src/deployment/http-schemas/lease.schema.ts (1)
LeaseStatusResponse(49-49)
apps/api/test/functional/deployments.spec.ts (1)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(16-20)
⏰ 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/indexer) / validate-unsafe
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Validate local packages
🔇 Additional comments (17)
apps/api/examples/lease-flow.ts (4)
5-5: Documentation update looks good.The updated documentation correctly reflects the removal of certificate creation and the reliance on API key authentication.
81-100: Deployment creation step correctly updated.The deployment creation now properly uses only the SDL and deposit without any certificate data, aligning with the JWT authentication approach.
114-138: Lease creation correctly updated for JWT flow.The lease creation request properly includes the manifest and lease details without certificate data, which aligns with the JWT authentication model.
166-167: Update deployment correctly uses SDL only.The deployment update now correctly sends only the SDL without certificate data, consistent with the JWT-based approach.
apps/api/test/functional/lease-flow.spec.ts (2)
117-131: Excellent test parameterization for both certificate scenarios.The parameterized test approach effectively validates the lease lifecycle with and without certificates, ensuring backward compatibility while testing the new JWT flow.
196-201: Certificate handling is correctly conditional.The conditional certificate inclusion based on the
includeCertificateparameter properly tests both authentication paths.apps/api/src/provider/services/provider/provider.service.spec.ts (2)
225-240: Well-structured test setup using the setup pattern.The setup function correctly follows the coding guidelines by being at the bottom of the describe block, accepting inline type definitions, and not specifying a return type.
51-51: Incorrect — manifest transformation exists in code; test is validThe transformation is implemented in the service, so the test expectation is correct.
Files to note:
- apps/api/src/provider/services/provider/provider.service.ts —
const manifestWithSize = manifest.replace(/"quantity":{"val/g, '"size":{"val');- apps/deploy-web/src/services/provider-proxy/provider-proxy.service.ts —
JSON.stringify(manifest).replaceAll('"quantity":{"val', '"size":{"val');- packages/http-sdk/src/provider/provider-http.service.ts —
sendManifestsends the manifest string as the request bodyLikely an incorrect or invalid review comment.
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (2)
10-10: Good addition of UserWalletRepository dependency.The addition of UserWalletRepository to support wallet-based authentication aligns well with the JWT authentication approach.
Also applies to: 25-25
288-295: Wallet lookup error handling is properly implemented.The helper method correctly validates wallet existence and provides a clear error message when not found.
apps/api/test/functional/deployments.spec.ts (6)
43-51: Shared wallet cache looks good; reset prevents cross-test bleed.Defining allWallets and resetting it in beforeEach is a simple way to back the new findOneBy mock across tests. This avoids cross-test contamination.
117-118: Populate allWallets for cross-lookup — good call.Pushing wallets into the shared collection aligns the findOneBy mock with real behavior.
694-711: PUT without certificate path — LGTM.This validates the new contract (SDL-only) and the expected response shape. Mocks for sendManifest and getLeaseStatus keep the test isolated from provider I/O.
760-769: 404 path updated to SDL-only — LGTM.Matches the new contract and validates error handling when deployment is missing.
781-789: 401 unauthenticated path updated to SDL-only — LGTM.Consistent with JWT-only API; good coverage for auth failure behavior.
805-813: 400 invalid SDL path — LGTM.Keeps the negative case aligned with the simplified request body.
apps/api/src/provider/services/provider/provider.service.ts (1)
62-73: JWT scope usage looks correct.Granular leases with "send-manifest" scope is consistent with the new flow.
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts
Outdated
Show resolved
Hide resolved
apps/api/src/provider/services/provider/provider.service.spec.ts
Outdated
Show resolved
Hide resolved
jzsfkzm
force-pushed
the
features/1038-jwt-not-cert
branch
from
3f93180 to
167b68f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
apps/api/test/functional/lease-flow.spec.ts (1)
263-263: Deployment update always includes certificate — test doesn't exercise the certificate-absent caseThe PUT to /v1/deployments in apps/api/test/functional/lease-flow.spec.ts includes certificate unconditionally, so the "without a certificate provided" test case never exercises a certificate-free update.
Files / locations to change
- apps/api/test/functional/lease-flow.spec.ts — around lines 261–263 (the PUT /v1/deployments update call).
- (Reference) The lease creation uses a conditional certificate already (lines ~198–205); the update should mirror that.
Suggested change (replace the existing line)
Original:body: JSON.stringify({ data: { sdl: ymlUpdate, certificate: { certPem, keyPem: encryptedKey } } })Replace with:
body: JSON.stringify({ data: { sdl: ymlUpdate, certificate: includeCertificate ? { certPem, keyPem: encryptedKey } : undefined } })Please apply the change so the test covers both certificate-present and certificate-absent update flows.
apps/api/src/provider/services/provider/provider.service.ts (1)
60-82: Ensure deterministic return after retries; preserve original error context.Two improvements:
- If sendManifest returns a falsy value without throwing, the function exits without returning, yielding undefined. Return false (or throw) after exhausting retries to keep return type stable.
- Wrapping the original error with new Error(...) loses stack and can stringify objects to “[object Object]”. Prefer throwing the original error or a stringified response safely.
} catch (err: any) { if (err.message?.includes("no lease for deployment") && i < this.MANIFEST_SEND_MAX_RETRIES) { await delay(this.MANIFEST_SEND_RETRY_DELAY); continue; } - throw new Error(err?.response?.data || err); + if (err?.response?.data) { + const data = typeof err.response.data === "string" ? err.response.data : JSON.stringify(err.response.data); + throw new Error(data); + } + // Preserve original error and stack if possible + throw err instanceof Error ? err : new Error(String(err)); } } + // All retries exhausted; return a consistent boolean or consider throwing + return false;
♻️ Duplicate comments (3)
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (1)
86-95: Nested Promise.all structure impacts performance.The nested Promise.all creates suboptimal concurrency as each outer map waits for its inner Promise.all to complete. This was flagged in previous reviews and remains unaddressed.
apps/api/test/functional/deployments.spec.ts (1)
71-73: Simplify async mock; drop redundant Promise.resolve and adopt setup() per guidelines.You don’t need Promise.resolve inside an async mock. Also, as previously requested, move this into a setup() function instead of beforeEach.
Apply this diff:
- jest.spyOn(userWalletRepository, "findOneBy").mockImplementation(async (query: Partial<UserWalletOutput> | undefined) => { - return Promise.resolve(allWallets.find(wallet => wallet.address === query?.address)); - }); + jest.spyOn(userWalletRepository, "findOneBy").mockImplementation(async (query: Partial<UserWalletOutput> | undefined) => { + return allWallets.find(wallet => wallet.address === query?.address); + });apps/api/src/provider/services/provider/provider.service.ts (1)
11-12: Decouple from deprecated provider-proxy type.Define a minimal local type instead of importing ProviderIdentity from a deprecated path.
-import { ProviderIdentity } from "@src/provider/services/provider/provider-proxy.service"; +type ProviderIdentity = { owner: string; hostUri: string };
🧹 Nitpick comments (6)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
19-19: Consider using consistent mock naming.The new
findWalletBymock follows a different naming pattern than the existing mocks. Consider aligning with the existing pattern for better consistency.-const findWalletBy = jest.fn().mockImplementation(async () => newWallet); +const findOneBy = jest.fn().mockImplementation(async () => newWallet);And update the setup call:
const di = setup({ - findWalletBy, + findOneBy, findWalletByUserId, createWallet, createAndAuthorizeTrialSpending, updateWalletById });Also applies to: 25-26
apps/api/examples/lease-flow.ts (1)
106-124: Consider making provider selection more flexible.The hardcoded provider selection might make the example less useful for different environments. Consider adding this as an environment variable or selecting the first available provider.
// Select a bid from a specific provider -const targetProvider = "akash175llqyjvxfle9qwt740vm46772dzaznpzgm576"; -const selectedBid = bids.find(bid => bid.bid.bid_id.provider === targetProvider); +const targetProvider = process.env.TARGET_PROVIDER || bids[0]?.bid?.bid_id?.provider; +const selectedBid = bids.find(bid => bid.bid.bid_id.provider === targetProvider) || bids[0]; if (!selectedBid) { - throw new Error(`No bid found from provider ${targetProvider}`); + throw new Error(`No bids available`); }apps/api/test/functional/deployments.spec.ts (1)
43-51: Avoid shared mutable state; prefer setup() pattern per test guidelines.The allWallets array introduces cross-test shared state (even though reset in beforeEach). Per our spec guidelines, move this into a setup() function scoped to each test and avoid sharing state across tests.
If you want, I can provide a targeted patch converting this suite to a setup() pattern and removing the shared array.
Also applies to: 117-118
apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts (2)
2-2: Avoid deep import from package internals.Importing types from @akashnetwork/jwt/src/types couples tests to internal paths. Prefer the package root export for long-term stability.
Proposed change:
-import type { JwtTokenPayload } from "@akashnetwork/jwt/src/types"; +import type { JwtTokenPayload } from "@akashnetwork/jwt";If JwtTokenPayload isn’t exported at the package root yet, consider re-exporting it there to avoid deep imports.
23-31: Restore Date.now mock to prevent bleed across tests.The Date.now spy isn’t restored and can leak into other tests. Restore it at the end of the test.
- const now = Math.floor(Date.now() / 1000); - jest.spyOn(Date, "now").mockReturnValue(now * 1000); + const now = Math.floor(Date.now() / 1000); + const nowSpy = jest.spyOn(Date, "now").mockReturnValue(now * 1000); const result = JSON.parse(await jwtTokenService.generateJwtToken({ walletId: mockWalletId, leases })); + nowSpy.mockRestore();apps/api/src/provider/services/provider/provider.service.ts (1)
62-69: Consider token reuse to minimize signing overhead.If generateJwtToken signs on every call, cache tokens keyed by {walletId, provider, scope} until exp minus a small skew (e.g., 2–5 seconds) to reduce overhead, per earlier discussion. The JwtTokenService may already memoize; ensure reuse across ProviderService calls.
Would you like me to add a small LRU or time-based cache wrapper here keyed by walletId+provider+scope?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
apps/api/examples/lease-flow.ts(9 hunks)apps/api/package.json(1 hunks)apps/api/src/billing/lib/wallet/wallet.ts(1 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(3 hunks)apps/api/src/deployment/http-schemas/deployment.schema.ts(1 hunks)apps/api/src/deployment/http-schemas/lease.schema.ts(0 hunks)apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts(5 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/provider/services/jwt-token/jwt-token.service.spec.ts(1 hunks)apps/api/src/provider/services/jwt-token/jwt-token.service.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.spec.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.ts(3 hunks)apps/api/test/functional/deployments.spec.ts(7 hunks)apps/api/test/functional/lease-flow.spec.ts(3 hunks)packages/http-sdk/src/provider/provider-http.service.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/deployment/http-schemas/lease.schema.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/api/package.json
- apps/api/src/billing/lib/wallet/wallet.ts
- apps/api/src/deployment/services/lease/lease.service.ts
- apps/api/src/provider/services/provider/provider.service.spec.ts
- apps/api/src/deployment/http-schemas/deployment.schema.ts
- apps/api/src/provider/services/jwt-token/jwt-token.service.ts
- packages/http-sdk/src/provider/provider-http.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/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/test/functional/deployments.spec.tsapps/api/src/provider/services/jwt-token/jwt-token.service.spec.tsapps/api/test/functional/lease-flow.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/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/test/functional/deployments.spec.tsapps/api/src/provider/services/jwt-token/jwt-token.service.spec.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/examples/lease-flow.tsapps/api/src/provider/services/provider/provider.service.tsapps/api/test/functional/lease-flow.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/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/test/functional/deployments.spec.tsapps/api/src/provider/services/jwt-token/jwt-token.service.spec.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/examples/lease-flow.tsapps/api/src/provider/services/provider/provider.service.tsapps/api/test/functional/lease-flow.spec.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: baktun14
PR: akash-network/console#1312
File: packages/jwt/src/jwt-token.ts:0-0
Timestamp: 2025-05-13T17:35:25.875Z
Learning: In the Akash JWT implementation, cryptographic signature verification is handled by the provider node code rather than in the JWT package itself. The JWT package only handles token creation, decoding, and basic validation (schema and time constraints).
📚 Learning: 2025-06-03T15:06:34.211Z
Learnt from: baktun14
PR: akash-network/console#1428
File: apps/api/src/deployment/controllers/deployment/deployment.controller.ts:0-0
Timestamp: 2025-06-03T15:06:34.211Z
Learning: The `getByOwnerAndDseq` method in `apps/api/src/deployment/controllers/deployment/deployment.controller.ts` is intentionally public without the `Protected` decorator because it serves public blockchain data from an indexer, following the pattern of public blockchain APIs.
Applied to files:
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts
📚 Learning: 2025-07-21T08:24:27.953Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/query-by-in-tests.mdc:0-0
Timestamp: 2025-07-21T08:24:27.953Z
Learning: Applies to apps/{deploy-web,provider-console}/**/*.spec.tsx : Use `queryBy` methods instead of `getBy` methods in test expectations in `.spec.tsx` files
Applied to files:
apps/api/test/functional/deployments.spec.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Use `setup` function instead of `beforeEach` in test files
Applied to files:
apps/api/test/functional/deployments.spec.ts
📚 Learning: 2025-07-21T08:24:24.269Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/no-jest-mock.mdc:0-0
Timestamp: 2025-07-21T08:24:24.269Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use `jest.mock()` to mock dependencies in test files. Instead, use `jest-mock-extended` to create mocks and pass mocks as dependencies to the service under test.
Applied to files:
apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
🧬 Code Graph Analysis (4)
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (2)
apps/api/src/deployment/http-schemas/deployment.schema.ts (1)
GetDeploymentResponse(248-248)apps/indexer/drizzle/schema.ts (2)
lease(110-178)deployment(180-203)
apps/api/test/functional/deployments.spec.ts (1)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(16-20)
apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts (2)
apps/api/src/billing/providers/config.provider.ts (1)
BillingConfig(9-9)apps/api/src/billing/lib/wallet/wallet.ts (1)
Wallet(7-53)
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (2)
apps/indexer/drizzle/schema.ts (3)
deployment(180-203)lease(110-178)provider(205-237)apps/api/src/deployment/http-schemas/deployment.schema.ts (1)
GetDeploymentResponse(248-248)
⏰ 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). (15)
- GitHub Check: validate (apps/provider-console) / validate-unsafe
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (28)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (4)
5-5: LGTM! JWT service dependency added correctly.The import of JwtTokenService aligns with the PR's JWT authentication implementation.
98-112: Improved mock structure using jest-mock-extended.The refactored UserWalletRepository mock using
jest-mock-extendedprovides better type safety and cleaner mock definition compared to inline mocks.
119-124: JWT service mock properly configured.The JwtTokenService mock with predictable "mock-jwt-token" return value is appropriate for testing the wallet initializer in isolation.
132-132: Interface extension follows the pattern correctly.The SetupInput interface extension with optional
findWalletByproperty maintains consistency with the existing interface design.apps/api/examples/lease-flow.ts (4)
1-20: Documentation updated correctly to reflect JWT authentication.The comments properly document the shift from certificate-based to API key authentication, with clear requirements and updated step numbering.
71-79: Proper API key validation added upfront.The early validation ensures the script fails fast if the required API_KEY environment variable is missing.
126-137: Proper error handling for lease creation.The status check and error throwing for non-200 responses ensures the script fails appropriately when lease creation fails.
166-166: Certificate data correctly removed from update payload.The update payload now only includes SDL data, consistent with the JWT authentication approach.
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (4)
10-10: UserWalletRepository integration added correctly.The dependency injection of UserWalletRepository enables wallet-based lease status retrieval, supporting the JWT authentication flow.
Also applies to: 24-25
28-29: Method signature properly updated for JWT approach.The removal of the optional certificate parameter and return type specification aligns with the shift to wallet-based authentication.
40-61: Error handling added for lease status retrieval.The try-catch wrapper around
providerService.getLeaseStatusgracefully handles potential errors by setting status to null, preventing the entire operation from failing due to provider unavailability.
288-295: Wallet lookup implementation with proper error handling.The
getWalletByAddresshelper method correctly handles the case where no wallet is found for the given address, throwing an appropriate error message.apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (4)
95-95: Certificate handling correctly removed from update flow.The destructuring now only extracts
sdlfrom the input, removing certificate dependency as expected for JWT authentication.
104-104: Method signature updated to use wallet ID.The
sendManifestToProviderscall now useswallet.idas the first parameter, replacing certificate-based authentication with wallet-based JWT authentication.
106-106: Certificate parameter removed from deployment lookup.The final call to
deploymentReaderService.findByOwnerAndDseqcorrectly omits the certificate parameter, consistent with the new JWT approach.
126-131: Provider manifest sending updated to JWT approach.The method signature change to use
walletIdinstead of certificate and the updatedproviderService.sendManifestcall structure align with the JWT authentication implementation.apps/api/test/functional/lease-flow.spec.ts (3)
118-131: Parameterized tests for certificate scenarios.The parameterized test approach effectively covers both certificate-present and certificate-absent scenarios, ensuring backward compatibility while testing the new JWT flow.
133-297: Well-structured lifecycle test function.The
runLifecyclefunction properly encapsulates the complete lease flow with conditional certificate handling, maintaining all existing test assertions while adding flexibility for testing both scenarios.
200-205: Certificate handling correctly conditional.The conditional certificate inclusion in the lease creation payload based on the
includeCertificateparameter allows testing both the legacy certificate path and the new JWT path.apps/api/test/functional/deployments.spec.ts (5)
694-711: LGTM: SDL-only update test reflects cert removal.Good coverage for the SDL-only update path, consistent with moving to wallet/JWT-based flows.
713-727: Confirm behavior when extra “certificate” field is provided.This test implies the endpoint ignores the certificate payload. Ensure the schema/router allows unknown fields (or explicitly ignores them) so we don't inadvertently accept unused sensitive inputs.
If unknown fields are rejected by the validator, this test could become flaky. Consider adding an explicit assertion (e.g., spy on providerService.sendManifest args) to verify the certificate is not used downstream.
760-766: LGTM: 404 path updated to SDL-only payload.Matches the certificate removal and keeps negative path assertions intact.
784-786: LGTM: 401 path updated to SDL-only payload.Consistent with new request shape.
808-810: LGTM: invalid SDL path updated.Retains validation behavior post-cert removal.
apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts (1)
61-72: LGTM: unique jti coverage.Good assertion that each token gets a unique identifier even when generated in quick succession.
apps/api/src/provider/services/provider/provider.service.ts (3)
28-29: LGTM: JwtTokenService injection aligns with JWT migration.Constructor now directly depends on JwtTokenService, which is consistent with removing provider-proxy and certificate flows.
49-59: LGTM: internal sendManifest path uses JWT-scoped lease.Clear separation of concerns: identity lookup at the public method and token-scoped delivery inside the private method.
90-99: LGTM: status path uses provider-scoped JWT lease.The scope "status" and hostUri routing look correct for direct provider calls.
jzsfkzm
force-pushed
the
features/1038-jwt-not-cert
branch
from
167b68f to
74ed1a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/test/functional/lease-flow.spec.ts (2)
259-264: Update path still always sends a certificate — breaks the “without certificate” scenarioThis undermines the parameterization: the update step must also omit the certificate when includeCertificate is false.
Apply this diff to mirror the creation step’s conditional behavior:
- body: JSON.stringify({ data: { sdl: ymlUpdate, certificate: { certPem, keyPem: encryptedKey } } }) + body: JSON.stringify({ + data: { + sdl: ymlUpdate, + ...(includeCertificate ? { certificate: { certPem, keyPem: encryptedKey } } : {}) + } + })
157-157: Replace pervasive “as any” with typed response shapesCoding guideline: never use any. Define a small ApiResponse helper and assert to concrete types at call sites.
Add near the imports:
+type ApiResponse<T> = { data: T };Then, for example, update a few representative sites (apply similarly to others):
-const { certPem, encryptedKey } = ((await certResponse.json()) as any).data; +type CertificateData = { certPem: string; encryptedKey: string }; +const { certPem, encryptedKey } = + (await certResponse.json() as ApiResponse<CertificateData>).data; -const { dseq, manifest } = ((await deployResponse.json()) as any).data; +type DeploymentData = { dseq: string; manifest: unknown }; +const { dseq, manifest } = + (await deployResponse.json() as ApiResponse<DeploymentData>).data; -const leaseResult = (await leaseResponse.json()) as any; +type LeaseStatus = ReturnType<typeof LeaseStatusSeeder.create>; +type LeaseCreateData = { leases: Array<{ status: LeaseStatus }> }; +const leaseResult = await leaseResponse.json() as ApiResponse<LeaseCreateData>; -const afterDepositBalances = ((await afterDepositBalancesResponse.json()) as any).data; +type Balances = { balance: number; deployments: number; total: number }; +const afterDepositBalances = + (await afterDepositBalancesResponse.json() as ApiResponse<Balances>).data; -const updateResult = (await updateResponse.json()) as any; +type LeaseUpdateData = { leases: Array<{ status: LeaseStatus }> }; +const updateResult = await updateResponse.json() as ApiResponse<LeaseUpdateData>; -const closeResult = (await closeResponse.json()) as any; +type CloseData = { success: boolean }; +const closeResult = await closeResponse.json() as ApiResponse<CloseData>; -const finalBalances = ((await finalBalancesResponse.json()) as any).data; +const finalBalances = + (await finalBalancesResponse.json() as ApiResponse<Balances>).data;Also applies to: 171-171, 186-186, 223-224, 246-246, 266-266, 275-276, 290-291
♻️ Duplicate comments (1)
apps/api/src/provider/services/provider/provider.service.ts (1)
49-60: Decouple from deprecated provider-proxy type; define a local ProviderIdentity.Importing ProviderIdentity from provider-proxy.service keeps a dependency on a deprecated path. Define a minimal local type and use it here.
Apply:
- private async sendManifestToProvider({ + private async sendManifestToProvider({ walletId, dseq, manifest, - providerIdentity + providerIdentity }: { walletId: number; dseq: string; manifest: string; - providerIdentity: ProviderIdentity; + providerIdentity: LocalProviderIdentity; }) {Add near the top of this file (outside the selected range):
type LocalProviderIdentity = { owner: string; hostUri: string };
🧹 Nitpick comments (10)
apps/api/test/functional/lease-flow.spec.ts (5)
118-131: Prefer it.each over manual forEach for parameterized testsJest’s table-driven tests give clearer reporting and avoid subtle async pitfalls. Replace the manual loop with it.each.
- [ - { - name: "should execute complete lease lifecycle without a certificate provided", - includeCertificate: false - }, - { - name: "should execute complete lease lifecycle with a certificate provided", - includeCertificate: true - } - ].forEach(({ name, includeCertificate }) => { - it(name, async () => { - await runLifecycle(includeCertificate); - }); - }); + it.each([ + { name: "should execute complete lease lifecycle without a certificate provided", includeCertificate: false }, + { name: "should execute complete lease lifecycle with a certificate provided", includeCertificate: true } + ])("$name", async ({ includeCertificate }) => { + await runLifecycle(includeCertificate); + });
133-133: Define runLifecycle before it.each for readabilityThis works because the test body runs later, but referencing a const before its declaration can be confusing. Consider moving runLifecycle above the parameterized tests.
151-158: Only create a certificate when includeCertificate is trueThe “without a certificate provided” test currently still calls /v1/certificates. This adds needless coupling to an endpoint slated for removal and weakens the JWT-only test path.
Example refactor (outside the changed hunk):
let certPem: string | undefined; let encryptedKey: string | undefined; if (includeCertificate) { const certResponse = await app.request("/v1/certificates", { method: "POST", headers: new Headers({ "Content-Type": "application/json", "x-api-key": apiKey }) }); expect(certResponse.status).toBe(200); ({ certPem, encryptedKey } = (await certResponse.json() as { data: { certPem: string; encryptedKey: string } }).data); }
85-92: Adopt the repository’s setup() test pattern for functional specsPer guidelines for **/*.spec.ts, prefer a setup function at the bottom of the root describe instead of beforeEach + scattered spies. Encapsulate stubbing, returns, and test data in setup, and return the handles needed by the test.
Sketch:
describe("Lease Flow", () => { // ... constants, helpers ... it.each([...])("$name", async ({ includeCertificate }) => { const { apiKey, wallet, certPem, encryptedKey } = await setup({ includeCertificate }); await runLifecycle(includeCertificate, { apiKey, wallet, certPem, encryptedKey }); }); // must be at the bottom function setup({ includeCertificate }: { includeCertificate: boolean }) { // arrange: user, api key, stubs, provider mocks, optional certificate, etc. // return values for the test } });
198-214: Minor: factor manifest/lease payload building into a small helperIt’ll reduce duplication and make the certificate inclusion explicit in one place.
Example:
function buildLeaseRequest(manifest: unknown, includeCert: boolean, certPem?: string, encryptedKey?: string) { return { manifest, ...(includeCert && certPem && encryptedKey ? { certificate: { certPem, keyPem: encryptedKey } } : {}), leases: [ /* ... */ ] }; }apps/api/package.json (1)
45-46: Audit JWT Dependency and Pin VersionsThe
apps/apipackage still directly imports and usesjsonwebtoken, so it cannot be removed until those usages are migrated to@akashnetwork/jwt. Additionally, using"*"weakens reproducibility; consider tightening these ranges.
- apps/api/src/auth/services/auth-token/auth-token.service.ts
• Importsjwtfrom"jsonwebtoken"for token signing/verification.- apps/api/test/services/wallet-testing.service.ts
• Importsdecodefrom"jsonwebtoken"in tests.Recommendations:
- Keep
"jsonwebtoken"inapps/apifor now; refactor these imports to@akashnetwork/jwtbefore removing the dependency.- Replace
"*"with"workspace:*"(for monorepo installs) or a pinned semver range once@akashnetwork/jwt(and@akashnetwork/logging) are published, to improve install reproducibility.apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (2)
126-131: Parallelize manifest dispatch and aggregate errors to avoid partial-update stalls.The current loop stops at the first thrown error and sends sequentially. Parallelizing improves latency and ensures one provider’s transient error doesn’t block others.
Apply:
- private async sendManifestToProviders(walletId: number, dseq: string, manifest: string, leases: GetDeploymentResponse["data"]["leases"]): Promise<void> { - const leaseProviders = leases.map(lease => lease.lease_id.provider).filter((v, i, s) => s.indexOf(v) === i); - for (const provider of leaseProviders) { - await this.providerService.sendManifest({ provider, dseq, manifest, walletId }); - } - } + private async sendManifestToProviders( + walletId: number, + dseq: string, + manifest: string, + leases: GetDeploymentResponse["data"]["leases"] + ): Promise<void> { + const providers = Array.from(new Set(leases.map(l => l.lease_id.provider))); + if (providers.length === 0) return; + + const results = await Promise.allSettled( + providers.map(provider => this.providerService.sendManifest({ provider, dseq, manifest, walletId })) + ); + + const errors = results.filter((r): r is PromiseRejectedResult => r.status === "rejected"); + if (errors.length) { + // Bubble up a concise aggregated error to the caller (logs can capture full details). + throw new Error( + `Failed to send manifest to ${errors.length}/${providers.length} providers: ` + + errors.map(e => (e.reason?.message ?? String(e.reason))).join("; ") + ); + } + }
94-107: Deployment update path is correct; type consistency verified
- Flow aligns with PR objectives: no certificate is required in UpdateDeploymentRequest, and manifest dispatch is now keyed by walletId as intended.
- The dseq parameter is consistently a string in both the update method signature and the RPC message interface (
UpdateDeploymentMsgOptions.dseq: string), so there’s no type mismatch to address.Optional refactor for resilience:
- In sendManifestToProviders, consider dispatching manifests to providers in parallel and aggregating any errors, rather than aborting on the first failure. This would improve robustness when some providers are unreachable.
apps/api/src/provider/services/provider/provider.service.ts (2)
31-33: Avoid string-based manifest mutation; centralize normalization in SDL layer.Replacing keys via regex on a JSON string is brittle (whitespace/ordering/embedding risks) and couples transport to content shape. Prefer normalizing in SdlService (single source of truth) or parse-transform-stringify here.
Option A (preferred): move the quantity→size normalization into SdlService.getManifest() so all callers get a provider-compatible manifest.
Option B (local, safer than regex): parse and transform.
- const manifestWithSize = manifest.replace(/"quantity":{"val/g, '"size":{"val'); + // Parse → rename → stringify. Keep stable formatting if provider requires it. + const manifestWithSize = (() => { + try { + const obj = JSON.parse(manifest); + const rewrite = (node: any) => { + if (node && typeof node === "object") { + // rename "quantity" → "size" only when shaped as { quantity: { val: ... } } + if ("quantity" in node && node.quantity && typeof node.quantity === "object" && "val" in node.quantity) { + node.size = node.quantity; + delete node.quantity; + } + for (const k of Object.keys(node)) rewrite(node[k]); + } + }; + rewrite(obj); + return JSON.stringify(obj); + } catch { + // Fallback to original string if not JSON; better to fail fast than mutate blindly. + return manifest; + } + })();Please confirm providers indeed expect "size" rather than "quantity" in the manifest for beta3; if this was historically handled by the proxy, consider relocating this logic to SdlService for consistency.
62-80: Harden retry detection and error propagation.
- Relying on err.message substring is fragile. Inspect response payload/status first, then fall back to message.
- Preserve the original error as cause for diagnosability.
Apply:
- } catch (err: any) { - if (err.message?.includes("no lease for deployment") && i < this.MANIFEST_SEND_MAX_RETRIES) { - await delay(this.MANIFEST_SEND_RETRY_DELAY); - continue; - } - throw new Error(err?.response?.data || err); - } + } catch (err: any) { + const msg = + err?.response?.data?.message ?? + err?.response?.data?.error ?? + (typeof err?.response?.data === "string" ? err.response.data : undefined) ?? + err?.message ?? + String(err); + if (/no lease for deployment/i.test(msg) && i < this.MANIFEST_SEND_MAX_RETRIES) { + await delay(this.MANIFEST_SEND_RETRY_DELAY); + continue; + } + // Include cause for debugging and preserve stack of original error in logs. + throw new Error(msg, { cause: err }); + }If the provider returns a structured error code for “no lease”, consider checking that instead of message text for future-proofing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
apps/api/examples/lease-flow.ts(9 hunks)apps/api/package.json(1 hunks)apps/api/src/billing/lib/wallet/wallet.ts(1 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(2 hunks)apps/api/src/deployment/http-schemas/deployment.schema.ts(1 hunks)apps/api/src/deployment/http-schemas/lease.schema.ts(0 hunks)apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts(5 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/provider/services/jwt-token/jwt-token.service.spec.ts(1 hunks)apps/api/src/provider/services/jwt-token/jwt-token.service.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.spec.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.ts(3 hunks)apps/api/test/functional/deployments.spec.ts(7 hunks)apps/api/test/functional/lease-flow.spec.ts(3 hunks)packages/http-sdk/src/provider/provider-http.service.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/deployment/http-schemas/lease.schema.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/api/src/deployment/http-schemas/deployment.schema.ts
- apps/api/src/provider/services/provider/provider.service.spec.ts
- apps/api/src/billing/lib/wallet/wallet.ts
- apps/api/src/provider/services/jwt-token/jwt-token.service.spec.ts
- apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts
- apps/api/src/deployment/services/lease/lease.service.ts
- packages/http-sdk/src/provider/provider-http.service.ts
- apps/api/src/provider/services/jwt-token/jwt-token.service.ts
- apps/api/test/functional/deployments.spec.ts
- apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
- apps/api/examples/lease-flow.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/lease-flow.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/lease-flow.spec.tsapps/api/src/provider/services/provider/provider.service.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.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/test/functional/lease-flow.spec.tsapps/api/src/provider/services/provider/provider.service.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: baktun14
PR: akash-network/console#1312
File: packages/jwt/src/jwt-token.ts:0-0
Timestamp: 2025-05-13T17:35:25.875Z
Learning: In the Akash JWT implementation, cryptographic signature verification is handled by the provider node code rather than in the JWT package itself. The JWT package only handles token creation, decoding, and basic validation (schema and time constraints).
🧬 Code graph analysis (2)
apps/api/src/provider/services/provider/provider.service.ts (6)
apps/api/src/provider/services/jwt-token/jwt-token.service.ts (1)
singleton(24-60)apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (1)
singleton(22-132)apps/api/src/deployment/services/lease/lease.service.ts (1)
singleton(10-38)apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (1)
singleton(18-296)packages/http-sdk/src/provider/provider-http.service.ts (1)
ProviderHttpService(6-42)apps/api/src/provider/services/provider/provider-proxy.service.ts (1)
ProviderIdentity(7-10)
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (2)
apps/indexer/drizzle/schema.ts (3)
deployment(180-203)lease(110-178)provider(205-237)apps/api/src/deployment/http-schemas/deployment.schema.ts (1)
GetDeploymentResponse(248-248)
⏰ 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). (15)
- GitHub Check: validate (apps/deploy-web) / validate-unsafe
- GitHub Check: validate (apps/provider-proxy) / validate-unsafe
- GitHub Check: validate (apps/indexer) / validate-unsafe
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (2)
apps/api/test/functional/lease-flow.spec.ts (1)
200-206: Nice: certificate field is conditionally omitted from the request bodyWhen includeCertificate is false, JSON.stringify will omit undefined, aligning the test with the JWT-first flow.
apps/api/src/provider/services/provider/provider.service.ts (1)
84-99: Lease status via JWT looks sound.Scope is narrowed to "status" and token is generated per call; aligns with retrieved learning that verification happens on provider node. LGTM.
jzsfkzm
force-pushed
the
features/1038-jwt-not-cert
branch
from
74ed1a6 to
a44a793
Compare
ygrishajev
force-pushed
the
features/1038-jwt-not-cert
branch
2 times, most recently
from
a0e031c to
340abad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jwt/src/wallet-utils.ts (1)
5-9: Update interface to match implementation.The
SignArbitraryAkashWalletinterface declaressignArbitrarywith an optionalaccountIndexparameter (line 8), but the implementation (line 24) no longer accepts this parameter and uses theaccountIndexfrom the outer function scope instead. This mismatch could mislead callers.Apply this diff to align the interface with the implementation:
export interface SignArbitraryAkashWallet { pubkey: Uint8Array; address: string; - signArbitrary: (signer: string, data: string | Uint8Array, accountIndex?: number) => Promise<StdSignature>; + signArbitrary: (signer: string, data: string | Uint8Array) => Promise<StdSignature>; }
♻️ Duplicate comments (10)
apps/api/test/services/app-http.service.ts (1)
9-19: Unsafe type assertion remains unaddressed.The method still casts
result.json()toRwithout runtime validation (line 16), which was flagged in a previous review. This violates the coding guideline requiring proper TypeScript types without unsafe casts. The genericRis unconstrained, so there's no guarantee the response shape matches the expected type.As per coding guidelines
apps/api/src/caching/helpers.spec.ts (2)
228-245: Add test coverage for boolean and empty-string edge cases.The past review comment highlighting missing coverage for boolean and empty-string arguments remains valid. Current implementation in
helpers.tsuses.filter(Boolean)which drops falsy values including empty strings andfalsebooleans, potentially causing cache collisions.As noted in the previous review, add tests to verify the behavior with:
- Boolean arguments (e.g.,
truevsfalse)- Empty string arguments (e.g.,
""vs"non-empty")
248-251: Refactor setup to follow coding guidelines.The past review comment remains valid: the
setupfunction should accept a single parameter with an inline type definition and create/return the object under test, rather than only performing cleanup.As per coding guidelines, refactor to instantiate and return test objects:
- function setup() { + function setup(params: { methodArgs?: [string, number | object] } = {}) { jest.clearAllMocks(); cacheEngine.clearAllKeyInCache(); + + class TestClass { + @Memoize() + async testMethod(_arg1: string, _arg2: number | object) { + return "test"; + } + } + + const instance = new TestClass(); + return { instance, TestClass }; }Then update tests to call
const { instance, TestClass } = setup();and use the returned objects.apps/api/test/mocks/config-service.mock.ts (1)
19-22: Address the unresolved past review: strongly typegetmock and use safe property check.This implementation still has the issues flagged in the previous review:
- Line 19: Uses
=> anyas return type, violating the "Never use type any" coding guideline.- Line 20: Uses
inoperator which can incorrectly match prototype properties on the partial object.- Type safety: The cast to
ConfigOf<T>is unsafe sincevaluesisPartial<ConfigOf<T>>.Apply the previously suggested fix:
-(svc.get as unknown as jest.MockedFunction<(key: keyof ConfigOf<T>) => any>).mockImplementation(key => { - if (key in values) return (values as ConfigOf<T>)[key]; +(svc.get as unknown as jest.MockedFunction<<K extends keyof ConfigOf<T>>(key: K) => ConfigOf<T>[K]>) + .mockImplementation((key) => { + if (Object.prototype.hasOwnProperty.call(values, key)) { + return (values as ConfigOf<T>)[key]; + } throw new Error(`Missing mock for config key "${String(key)}" in ${svc.constructor?.name ?? "ConfigService"}`); });apps/api/src/provider/services/provider/provider-proxy.service.ts (1)
38-46: Restore request timeout propagation
timeoutis destructured but never passed toaxios.post, so calls likeProviderService.sendManifestandgetLeaseStatussilently fall back to Axios’ default and may hang. This regresses the earlier fix we landed for the same bug and breaks caller expectations. Please wire the timeout back into the Axios config (while keeping the existing payload intact).- const response = await axios.post(this.configService.get("PROVIDER_PROXY_URL"), { - ...params, - method: options.method || "GET", - url: providerIdentity.hostUri + url, - providerAddress: providerIdentity.owner, - network: chainNetwork - }); + const response = await axios.post( + this.configService.get("PROVIDER_PROXY_URL"), + { + ...params, + method: options.method || "GET", + url: providerIdentity.hostUri + url, + providerAddress: providerIdentity.owner, + network: chainNetwork + }, + timeout ? { timeout } : {} + );apps/api/src/deployment/services/lease/lease.service.ts (2)
34-42: Validate wallet.address before using non-null assertion.The non-null assertion operator on
wallet.address!(line 36) will bypass TypeScript's null checks. Ifwallet.addresscan be null or undefined, this will cause a runtime error when constructing lease messages.Apply this diff to add validation:
+ assert(wallet.address, 500, "Wallet address is required"); + const leaseMessages = leases.map(lease => this.rpcMessageService.getCreateLeaseMsg({ owner: wallet.address!,
58-58: Add validation for empty leases array.Accessing
leases[0].dseq(line 58) will throw a runtime error if theleasesarray is empty. Add a guard to ensure at least one lease exists.Apply this diff to add validation:
+ assert(leases.length > 0, 400, "At least one lease is required"); + return await this.deploymentReaderService.findByWalletAndDseq(wallet, leases[0].dseq);apps/api/test/functional/deployments.spec.ts (1)
40-51: Refactor to usesetup()instead ofbeforeEach.The test suite still uses
beforeEachand shared mutable state (allWallets,currentUser), which conflicts with the coding guidelines requiring asetup()function. A past review comment (lines 40-51) flagged this pattern.Per the coding guidelines:
- Use a
setup()function instead ofbeforeEachin test files.- The
setup()function must be at the bottom of the rootdescribeblock.- The
setup()function creates an object under test and returns it.- Don't use shared state in the
setup()function.As per coding guidelines.
apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts (1)
68-68: Updatesetup()signature to accept overrides.The
setup()function is declared without the mandated single-parameter signature. Per the coding guidelines, it should accept an overrides object with an inline type (e.g.,{ jwtModule?: JWTModule } = {}) and apply those overrides when constructing the test subject.As per coding guidelines.
apps/api/src/provider/services/provider/provider.service.ts (1)
75-101: Return immediately after a successful manifest request.
providerProxy.requestoften resolves to an empty string/undefined on 200/204 responses. Theif (result)guard keeps retrying even after a success, so we resend the manifest up to 3 times and still fall through withundefined. Remove the truthy check and exit as soon as the request resolves.- const result = await this.providerProxy.request(`/deployment/${options.dseq}/manifest`, { + const result = await this.providerProxy.request(`/deployment/${options.dseq}/manifest`, { method: "PUT", body: options.manifest, auth: options.auth, chainNetwork: this.chainNetwork, providerIdentity: options.providerIdentity, timeout: 60000 }); - - if (result) return result; + return result;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (51)
apps/api/examples/.env.example(0 hunks)apps/api/examples/hello-world-sdl-update.yml(0 hunks)apps/api/examples/hello-world-sdl.yml(0 hunks)apps/api/examples/lease-flow.ts(0 hunks)apps/api/package.json(2 hunks)apps/api/src/app/index.ts(1 hunks)apps/api/src/app/services/close-trial-deployment/close-trial-deployment.handler.ts(1 hunks)apps/api/src/auth/controllers/api-key/api-key.controller.ts(2 hunks)apps/api/src/auth/http-schemas/api-key.schema.ts(3 hunks)apps/api/src/auth/routes/api-keys/api-keys.router.ts(7 hunks)apps/api/src/auth/services/auth.service.ts(1 hunks)apps/api/src/bid/http-schemas/bid.schema.ts(0 hunks)apps/api/src/bid/services/bid/bid.service.ts(3 hunks)apps/api/src/billing/lib/wallet/wallet.ts(2 hunks)apps/api/src/billing/providers/wallet.provider.ts(2 hunks)apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts(2 hunks)apps/api/src/billing/services/wallet-reader/wallet-reader.service.ts(2 hunks)apps/api/src/caching/helpers.spec.ts(1 hunks)apps/api/src/caching/helpers.ts(1 hunks)apps/api/src/deployment/controllers/deployment/deployment.controller.ts(1 hunks)apps/api/src/deployment/controllers/lease/lease.controller.ts(1 hunks)apps/api/src/deployment/http-schemas/deployment.schema.ts(1 hunks)apps/api/src/deployment/http-schemas/lease.schema.ts(1 hunks)apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts(4 hunks)apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts(6 hunks)apps/api/src/deployment/services/lease/lease.service.ts(3 hunks)apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts(5 hunks)apps/api/src/provider/index.ts(1 hunks)apps/api/src/provider/providers/jwt.provider.ts(1 hunks)apps/api/src/provider/repositories/provider/provider.repository.ts(2 hunks)apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts(1 hunks)apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts(1 hunks)apps/api/src/provider/services/provider/provider-proxy.service.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.spec.ts(1 hunks)apps/api/src/provider/services/provider/provider.service.ts(4 hunks)apps/api/test/functional/deployments.spec.ts(9 hunks)apps/api/test/functional/lease-flow.spec.ts(0 hunks)apps/api/test/functional/managed-api-deployment-flow.spec.ts(1 hunks)apps/api/test/mocks/config-service.mock.ts(1 hunks)apps/api/test/services/app-http.service.ts(1 hunks)apps/api/test/services/test-wallet.service.ts(1 hunks)apps/api/test/services/wallet-testing.service.ts(2 hunks)apps/api/tsconfig.json(1 hunks)apps/deploy-web/.env.local.sample(1 hunks)apps/deploy-web/src/pages/api/auth/[...auth0].ts(1 hunks)apps/deploy-web/src/pages/api/auth/signup.ts(2 hunks)apps/deploy-web/src/services/auth/auth/rewrite-local-redirect.ts(1 hunks)packages/docker/Dockerfile.nextjs(1 hunks)packages/docker/Dockerfile.node(1 hunks)packages/http-sdk/src/provider/provider-http.service.ts(1 hunks)packages/jwt/src/wallet-utils.ts(1 hunks)
💤 Files with no reviewable changes (6)
- apps/api/examples/hello-world-sdl-update.yml
- apps/api/examples/.env.example
- apps/api/test/functional/lease-flow.spec.ts
- apps/api/examples/hello-world-sdl.yml
- apps/api/src/bid/http-schemas/bid.schema.ts
- apps/api/examples/lease-flow.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- apps/api/package.json
- apps/deploy-web/src/services/auth/auth/rewrite-local-redirect.ts
- apps/api/src/bid/services/bid/bid.service.ts
- apps/api/src/provider/index.ts
- packages/docker/Dockerfile.node
- packages/docker/Dockerfile.nextjs
- apps/api/src/deployment/http-schemas/deployment.schema.ts
- apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts
- packages/http-sdk/src/provider/provider-http.service.ts
- apps/api/src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service.spec.ts
- apps/api/src/billing/lib/wallet/wallet.ts
- apps/api/src/caching/helpers.ts
- apps/api/src/provider/services/provider/provider.service.spec.ts
- apps/api/test/services/wallet-testing.service.ts
- apps/deploy-web/.env.local.sample
- apps/api/test/functional/managed-api-deployment-flow.spec.ts
- apps/deploy-web/src/pages/api/auth/[...auth0].ts
- apps/deploy-web/src/pages/api/auth/signup.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/services/test-wallet.service.tsapps/api/src/caching/helpers.spec.tsapps/api/src/app/services/close-trial-deployment/close-trial-deployment.handler.tsapps/api/src/auth/services/auth.service.tsapps/api/src/deployment/controllers/lease/lease.controller.tsapps/api/src/deployment/http-schemas/lease.schema.tsapps/api/src/provider/services/provider/provider-proxy.service.tspackages/jwt/src/wallet-utils.tsapps/api/src/provider/providers/jwt.provider.tsapps/api/src/provider/repositories/provider/provider.repository.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.tsapps/api/src/deployment/controllers/deployment/deployment.controller.tsapps/api/src/auth/controllers/api-key/api-key.controller.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/src/billing/providers/wallet.provider.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/test/functional/deployments.spec.tsapps/api/src/auth/http-schemas/api-key.schema.tsapps/api/src/deployment/services/lease/lease.service.tsapps/api/src/billing/services/wallet-reader/wallet-reader.service.tsapps/api/src/app/index.tsapps/api/src/auth/routes/api-keys/api-keys.router.tsapps/api/test/services/app-http.service.tsapps/api/test/mocks/config-service.mock.tsapps/api/src/provider/services/provider/provider.service.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.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/services/test-wallet.service.tsapps/api/src/caching/helpers.spec.tsapps/api/src/app/services/close-trial-deployment/close-trial-deployment.handler.tsapps/api/src/auth/services/auth.service.tsapps/api/src/deployment/controllers/lease/lease.controller.tsapps/api/src/deployment/http-schemas/lease.schema.tsapps/api/src/provider/services/provider/provider-proxy.service.tspackages/jwt/src/wallet-utils.tsapps/api/src/provider/providers/jwt.provider.tsapps/api/src/provider/repositories/provider/provider.repository.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.tsapps/api/src/deployment/controllers/deployment/deployment.controller.tsapps/api/src/auth/controllers/api-key/api-key.controller.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/src/billing/providers/wallet.provider.tsapps/api/src/deployment/services/deployment-writer/deployment-writer.service.tsapps/api/test/functional/deployments.spec.tsapps/api/src/auth/http-schemas/api-key.schema.tsapps/api/src/deployment/services/lease/lease.service.tsapps/api/src/billing/services/wallet-reader/wallet-reader.service.tsapps/api/src/app/index.tsapps/api/src/auth/routes/api-keys/api-keys.router.tsapps/api/test/services/app-http.service.tsapps/api/test/mocks/config-service.mock.tsapps/api/src/provider/services/provider/provider.service.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts
**/*.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/src/caching/helpers.spec.tsapps/api/test/functional/deployments.spec.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts
🧠 Learnings (7)
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : `setup` function creates an object under test and returns it
Applied to files:
apps/api/src/caching/helpers.spec.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Use `setup` function instead of `beforeEach` in test files
Applied to files:
apps/api/src/caching/helpers.spec.tsapps/api/test/functional/deployments.spec.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : `setup` function should accept a single parameter with inline type definition
Applied to files:
apps/api/src/caching/helpers.spec.tsapps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : Don't use shared state in `setup` function
Applied to files:
apps/api/src/caching/helpers.spec.tsapps/api/test/functional/deployments.spec.ts
📚 Learning: 2025-05-13T17:35:25.875Z
Learnt from: baktun14
PR: akash-network/console#1312
File: packages/jwt/src/jwt-token.ts:0-0
Timestamp: 2025-05-13T17:35:25.875Z
Learning: In the Akash JWT implementation, cryptographic signature verification is handled by the provider node code rather than in the JWT package itself. The JWT package only handles token creation, decoding, and basic validation (schema and time constraints).
Applied to files:
apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts
📚 Learning: 2025-06-03T15:06:34.211Z
Learnt from: baktun14
PR: akash-network/console#1428
File: apps/api/src/deployment/controllers/deployment/deployment.controller.ts:0-0
Timestamp: 2025-06-03T15:06:34.211Z
Learning: The `getByOwnerAndDseq` method in `apps/api/src/deployment/controllers/deployment/deployment.controller.ts` is intentionally public without the `Protected` decorator because it serves public blockchain data from an indexer, following the pattern of public blockchain APIs.
Applied to files:
apps/api/src/deployment/controllers/deployment/deployment.controller.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts
📚 Learning: 2025-07-21T08:25:07.474Z
Learnt from: CR
PR: akash-network/console#0
File: .cursor/rules/setup-instead-of-before-each.mdc:0-0
Timestamp: 2025-07-21T08:25:07.474Z
Learning: Applies to **/*.spec.{ts,tsx} : `setup` function must be at the bottom of the root `describe` block in test files
Applied to files:
apps/api/test/functional/deployments.spec.ts
🧬 Code graph analysis (19)
apps/api/src/caching/helpers.spec.ts (1)
apps/api/src/caching/helpers.ts (2)
Memoize(21-37)cacheEngine(8-8)
apps/api/src/app/services/close-trial-deployment/close-trial-deployment.handler.ts (1)
apps/api/src/core/services/job-queue/job-queue.service.ts (1)
JOB_NAME(161-161)
apps/api/src/deployment/controllers/lease/lease.controller.ts (1)
apps/api/src/auth/services/auth.service.ts (1)
Protected(43-59)
apps/api/src/provider/services/provider/provider-proxy.service.ts (3)
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (1)
singleton(24-155)apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts (1)
singleton(25-63)apps/api/src/provider/services/provider/provider.service.ts (1)
singleton(22-189)
packages/jwt/src/wallet-utils.ts (1)
packages/jwt/src/index.ts (2)
createSignArbitraryAkashWallet(4-4)SignArbitraryAkashWallet(4-4)
apps/api/src/provider/repositories/provider/provider.repository.ts (2)
packages/database/dbSchemas/akash/index.ts (3)
ProviderAttributeSignature(10-10)ProviderSnapshotNode(5-5)ProviderSnapshotNodeGPU(7-7)apps/api/src/deployment/config/provider.config.ts (1)
AUDITOR(3-3)
apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts (5)
apps/api/src/provider/services/provider/provider.service.ts (1)
singleton(22-189)apps/api/src/provider/providers/jwt.provider.ts (2)
JWT_MODULE(6-6)JWTModule(5-5)apps/api/src/billing/providers/wallet.provider.ts (1)
WALLET_FACTORY(22-22)apps/api/src/billing/lib/wallet/wallet.ts (2)
walletFactory(60-60)WalletFactory(59-59)apps/api/src/caching/helpers.ts (1)
Memoize(21-37)
apps/api/src/deployment/controllers/deployment/deployment.controller.ts (1)
apps/api/src/auth/services/auth.service.ts (1)
Protected(43-59)
apps/api/src/auth/controllers/api-key/api-key.controller.ts (2)
apps/api/src/auth/http-schemas/api-key.schema.ts (4)
ApiKeyHiddenResponse(59-59)CreateApiKeyRequest(60-60)ApiKeyVisibleResponse(58-58)UpdateApiKeyRequest(61-61)apps/api/src/auth/services/auth.service.ts (1)
Protected(43-59)
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (2)
apps/notifications/src/interfaces/rest/services/auth/auth.service.ts (1)
userId(18-25)apps/api/src/billing/services/wallet-reader/wallet-reader.service.ts (1)
WalletInitialized(12-12)
apps/api/src/billing/providers/wallet.provider.ts (1)
apps/api/src/billing/lib/wallet/wallet.ts (2)
WalletFactory(59-59)walletFactory(60-60)
apps/api/src/deployment/services/deployment-writer/deployment-writer.service.ts (2)
apps/api/src/billing/services/wallet-reader/wallet-reader.service.ts (1)
WalletInitialized(12-12)apps/api/src/provider/services/provider/provider-proxy.service.ts (1)
ProviderMtlsAuth(11-15)
apps/api/test/functional/deployments.spec.ts (3)
apps/api/src/user/repositories/user/user.repository.ts (1)
UserOutput(11-13)apps/api/src/auth/repositories/api-key/api-key.repository.ts (1)
ApiKeyOutput(13-18)apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(16-20)
apps/api/src/deployment/services/lease/lease.service.ts (3)
apps/api/src/deployment/http-schemas/lease.schema.ts (1)
CreateLeaseRequest(53-53)apps/api/src/deployment/http-schemas/deployment.schema.ts (1)
GetDeploymentResponse(254-254)apps/notifications/src/interfaces/rest/services/auth/auth.service.ts (1)
userId(18-25)
apps/api/src/billing/services/wallet-reader/wallet-reader.service.ts (2)
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
UserWalletOutput(16-20)apps/api/src/auth/services/auth.service.ts (2)
ability(25-27)ability(29-31)
apps/api/src/auth/routes/api-keys/api-keys.router.ts (2)
apps/api/src/core/services/open-api-hono-handler/open-api-hono-handler.ts (1)
OpenApiHonoHandler(11-26)apps/api/src/auth/http-schemas/api-key.schema.ts (2)
ApiKeyHiddenResponseSchema(45-47)ApiKeyVisibleResponseSchema(41-43)
apps/api/test/mocks/config-service.mock.ts (1)
apps/api/src/core/services/config/config.service.ts (1)
ConfigService(9-22)
apps/api/src/provider/services/provider/provider.service.ts (1)
apps/api/src/provider/services/provider/provider-proxy.service.ts (3)
ProviderAuth(17-22)ProviderIdentity(6-9)ProviderMtlsAuth(11-15)
apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.spec.ts (3)
apps/api/src/billing/lib/wallet/wallet.ts (2)
walletFactory(60-60)Wallet(7-57)apps/api/src/provider/providers/jwt.provider.ts (1)
JWTModule(5-5)apps/api/test/mocks/config-service.mock.ts (1)
mockConfigService(16-25)
⏰ 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: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
🔇 Additional comments (18)
apps/api/tsconfig.json (1)
2-4: Confirm spec files still get type-checked.By excluding
src/**/*.spec.tshere, runningtsc --noEmit -p apps/api/tsconfig.jsonwill now skip all spec files. If this config underpins CI/static checks, we just lost type coverage on our Jest specs. Please confirm we have a separate tsconfig (or another path) that still type-checks the specs; otherwise, consider moving this exclusion intotsconfig.build.jsoninstead so only the build skips them.apps/api/src/caching/helpers.spec.ts (1)
208-226: LGTM! Test correctly verifies numeric arguments in cache keys.The test properly validates that both string and number arguments are included in the cache key, creating distinct keys for different numeric values. This aligns with the implementation in
helpers.ts(lines 29-30) wheretypeof argchecks include both "string" and "number".apps/api/test/services/test-wallet.service.ts (1)
18-18: LGTM—no remaining references tolease-flow.spec.tsdetected.apps/api/src/auth/controllers/api-key/api-key.controller.ts (1)
25-42: Clear response modelingNice touch splitting hidden vs. visible API key responses right at the controller boundary. This makes it explicit that the secret only ever leaves the service on creation while all other reads remain scrubbed.
apps/api/src/auth/http-schemas/api-key.schema.ts (1)
13-47: Contracts capture secret exposure correctlyIntroducing dedicated visible/hidden schemas plus the paired response wrappers gives the OpenAPI docs and consumers an unambiguous contract about when the raw apiKey can surface. Great forward step for hardening this surface.
apps/api/src/auth/routes/api-keys/api-keys.router.ts (1)
70-150: OpenAPI docs line up with the new contractsBinding the GET/PATCH routes to the hidden response schema and POST to the visible variant keeps the OpenAPI output perfectly in sync with the controller guarantees. This will save consumers a lot of guesswork.
apps/api/src/deployment/http-schemas/lease.schema.ts (1)
5-10: LGTM: Certificate field correctly made optional.The change to make the certificate field optional aligns with the PR's objective to support JWT-based authentication as an alternative to certificate-based authentication. The schema structure is correct.
apps/api/src/app/services/close-trial-deployment/close-trial-deployment.handler.ts (2)
61-73: LGTM: Address guard correctly prevents unsafe casting.The guard correctly checks for wallet address presence before proceeding with the close operation, addressing the previous review concern about unsafe type casting. The early return pattern and logging provide good observability.
75-75: LGTM: Safe wallet augmentation pattern.The spread operator pattern
{ ...wallet, address }safely satisfies theWalletInitializedtype requirement after the address guard, providing type safety without unsafe casting.apps/api/src/deployment/controllers/lease/lease.controller.ts (2)
12-15: LGTM: Controller simplified by delegating wallet resolution.Removing the UserWalletRepository dependency from the controller and delegating wallet resolution to the service layer follows proper separation of concerns and simplifies the controller.
18-24: LGTM: Clean userId propagation pattern.The userId propagation from
authService.currentUser.idto the service layer is clean and appropriate. The@Protecteddecorator ensures the current user is authenticated before accessingcurrentUser.id.apps/api/test/functional/deployments.spec.ts (2)
699-716: LGTM: Test correctly validates wallet-centric update flow.The new test correctly validates the PUT deployment flow without a certificate, aligning with the PR's objective to support JWT-based authentication as an alternative to certificates.
718-755: LGTM: Test validates backward-compatible certificate flow.The test correctly validates that the certificate-based flow continues to work when a certificate is provided, ensuring backward compatibility while supporting the new JWT-based authentication.
apps/api/src/deployment/services/lease/lease.service.ts (2)
6-6: LGTM: WalletReaderService integration follows wallet-centric pattern.The integration of
WalletReaderServiceto resolve wallet byuserIdcorrectly implements the wallet-centric authentication flow introduced in this PR. The separation of concerns is clean and appropriate.Also applies to: 23-23, 32-32
46-56: LGTM: Dual authentication path correctly implemented.The per-lease authentication derivation correctly implements support for both certificate-based and JWT-based authentication by falling back to wallet context when a certificate is not provided. This aligns with the PR's objective.
apps/api/src/provider/services/provider-jwt-token/provider-jwt-token.service.ts (3)
33-46: LGTM: JWT token generation correctly implemented.The
generateJwtTokenmethod correctly constructs JWT tokens with proper timestamp handling, version tracking, and a reasonable 30-second default TTL. The token fields (exp, nbf, iat, iss, jti, leases) are appropriately set.
48-55: LGTM: Memoization and wallet creation correctly implemented.The
@Memoizedecorator with a 5-minute TTL appropriately caches wallet creation, reducing overhead. The wallet factory and JWT token creation flow is correctly implemented, withwalletIdpassed as the HD account index to derive different signing keys per wallet.
57-62: LGTM: Granular leases helper correctly implemented.The
getGranularLeaseshelper correctly constructs the granular access structure for scoped provider permissions, enabling fine-grained JWT-based authorization.
4 participants
closes #1038
Summary by CodeRabbit
New Features
Refactor
Chores