refactor: move /v1/deployment/{owner}/{dseq} to modules#1428
refactor: move /v1/deployment/{owner}/{dseq} to modules#1428baktun14 merged 1 commit intoakash-network:mainfrom
Conversation
|
""" WalkthroughA new API endpoint was introduced to fetch deployment details by owner and deployment sequence (dseq). Supporting schemas, controller, service, repository, and message handling logic were implemented or refactored to support this route. Legacy implementations and routes for similar functionality were removed. Tests were updated to cover the new endpoint and scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant Controller
participant Service
participant Repo
participant MessageService
participant MessageRepo
Client->>Router: GET /v1/deployment/{owner}/{dseq}
Router->>Controller: getByOwnerAndDseq(owner, dseq)
Controller->>Service: getDeploymentByOwnerAndDseq(owner, dseq)
Service->>Repo: findByOwnerAndDseq(owner, dseq)
Service->>MessageService: getDeploymentRelatedMessages(owner, dseq)
MessageService->>Repo: findByOwnerAndDseq(owner, dseq)
MessageService->>MessageRepo: getDeploymentRelatedMessages(deploymentId)
Service->>Service: Aggregate deployment, leases, providers, messages
Service-->>Controller: Deployment details object
Controller-->>Router: Deployment details object / 404 / 400
Router-->>Client: JSON response / error
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (14)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (11)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
apps/api/src/deployment/repositories/message/message.repository.ts (2)
36-36: Consider adding type safety checks for protobuf decoding.The type assertions
as v1beta1.MsgCreateBid | v2beta2.MsgCreateBidand similar could fail if the decoded message doesn't match the expected structure. Consider adding runtime validation or error handling.- decoded: decodeMsg(msg.type, msg.data) as v1beta1.MsgCreateBid | v2beta2.MsgCreateBid, + decoded: (() => { + try { + return decodeMsg(msg.type, msg.data) as v1beta1.MsgCreateBid | v2beta2.MsgCreateBid; + } catch (error) { + console.warn(`Failed to decode message ${msg.type}:`, error); + return null; + } + })(),Also applies to: 42-42
13-13: Consider optimizing the method parameter.The method accepts a full
Deploymentobject but only uses itsidfield. Consider accepting just the deployment ID for better performance and clearer intent.- async getDeploymentRelatedMessages(deployment: Deployment) { + async getDeploymentRelatedMessages(deploymentId: string) { const relatedMessages = await Message.findAll({ where: { - relatedDeploymentId: deployment.id, + relatedDeploymentId: deploymentId,apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (1)
176-176: Consider adding input validationWhile schema validation likely happens at the HTTP layer, consider adding validation at the service layer for defense in depth.
Add parameter validation:
public async getDeploymentByOwnerAndDseq(owner: string, dseq: string) { // Validate owner is a valid Akash address if (!owner || !/^akash1[a-z0-9]{38}$/.test(owner)) { throw new Error("Invalid owner address"); } // Validate dseq is a positive integer string if (!dseq || !/^\d+$/.test(dseq)) { throw new Error("Invalid deployment sequence"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/api/src/deployment/controllers/deployment/deployment.controller.ts(2 hunks)apps/api/src/deployment/http-schemas/deployment.schema.ts(2 hunks)apps/api/src/deployment/repositories/deployment/deployment.repository.ts(1 hunks)apps/api/src/deployment/repositories/message/message.repository.ts(1 hunks)apps/api/src/deployment/routes/deployments/deployments.router.ts(3 hunks)apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts(2 hunks)apps/api/src/deployment/services/message-service/message.service.ts(1 hunks)apps/api/src/routes/v1/deployments/byOwnerDseq.ts(0 hunks)apps/api/src/routes/v1/index.ts(0 hunks)apps/api/src/services/db/deploymentService.ts(0 hunks)apps/api/src/services/external/apiNodeService.ts(1 hunks)apps/api/test/functional/deployments.spec.ts(3 hunks)
💤 Files with no reviewable changes (3)
- apps/api/src/routes/v1/index.ts
- apps/api/src/services/db/deploymentService.ts
- apps/api/src/routes/v1/deployments/byOwnerDseq.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/api/src/deployment/controllers/deployment/deployment.controller.ts (1)
apps/api/src/deployment/http-schemas/deployment.schema.ts (1)
GetDeploymentByOwnerDseqResponse(256-256)
apps/api/src/deployment/routes/deployments/deployments.router.ts (2)
apps/api/src/deployment/http-schemas/deployment.schema.ts (2)
GetDeploymentByOwnerDseqParamsSchema(197-207)GetDeploymentByOwnerDseqResponseSchema(209-241)apps/indexer/drizzle/schema.ts (1)
deployment(180-203)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: validate-api
🔇 Additional comments (8)
apps/api/src/services/external/apiNodeService.ts (1)
18-18: LGTM!The removal of the
getDeploymentfunction aligns with the PR objective of moving deployment-related functionality to the deployment module.apps/api/test/functional/deployments.spec.ts (2)
135-138: LGTM - Well-structured mock setup for edge cases.The persistent nock mocks properly simulate 404 responses for non-existent deployment (dseq=9876) and related lease queries. This provides good test coverage for error scenarios.
Also applies to: 160-170
834-874: LGTM - Comprehensive test coverage for the new endpoint.The test block covers all essential scenarios:
- Successful retrieval with proper response structure validation
- 404 response when deployment is not found
- 400 response for invalid owner address
The test structure follows existing patterns and provides good coverage for the new functionality.
apps/api/src/deployment/repositories/message/message.repository.ts (1)
44-51:Details
❓ Verification inconclusive
Review the bid matching logic for potential edge cases.
The logic to identify accepted bids by matching
gseq,oseq, andproviderfields looks correct, but consider edge cases where these fields might be undefined or have unexpected values.Could you verify that the bid matching logic handles all edge cases properly? Specifically:
- What happens if
bidIdis undefined in lease messages?- Are there scenarios where
orderfield might be missing in bid messages?
🏁 Script executed:
#!/bin/bash # Search for similar bid matching patterns in the codebase to ensure consistency ast-grep --pattern 'l.bidId?.gseq === $_.decoded.order?.gseq'Length of output: 197
apps/api/src/deployment/services/message-service/message.service.ts (1)
6-21: LGTM - Clean service implementation with proper separation of concerns.The service follows good practices:
- Proper dependency injection
- Clear method contract returning null for not found cases
- Delegates appropriately to repository layer
- Simple and focused responsibility
The implementation looks solid and maintainable.
apps/api/src/deployment/routes/deployments/deployments.router.ts (2)
14-15: LGTM - Proper schema imports for the new endpoint.The imports for
GetDeploymentByOwnerDseqParamsSchemaandGetDeploymentByOwnerDseqResponseSchemaare correctly added to support the new route validation and documentation.
191-215: LGTM - Well-defined OpenAPI route specification.The route definition follows established patterns with:
- Proper HTTP method and path
- Clear summary and tags
- Comprehensive response definitions for 200, 400, and 404 status codes
- Correct schema references for validation and documentation
apps/api/src/deployment/http-schemas/deployment.schema.ts (1)
197-207: Well-implemented parameter validationGood use of:
- Zod schema validation with proper regex for
dseq- OpenAPI documentation with examples
- Reusable
AkashAddressSchemafor address validation
apps/api/src/deployment/repositories/deployment/deployment.repository.ts
Outdated
Show resolved
Hide resolved
| async getByOwnerAndDseq(owner: string, dseq: string): Promise<GetDeploymentByOwnerDseqResponse> { | ||
| return await this.deploymentReaderService.getDeploymentByOwnerAndDseq(owner, dseq); | ||
| } |
There was a problem hiding this comment.
Add authentication and input validation to prevent unauthorized access.
This method has critical security issues:
- Unlike all other methods in this controller, it lacks the
@Protecteddecorator, making it publicly accessible - No validation of the
owneranddseqparameters - No verification that the requester has permission to view the deployment
Based on the pattern in this controller, apply this fix:
+ @Protected([{ action: "read", subject: "Deployment" }])
async getByOwnerAndDseq(owner: string, dseq: string): Promise<GetDeploymentByOwnerDseqResponse> {
+ assert(owner, 400, "Owner is required");
+ assert(dseq, 400, "Deployment sequence is required");
+
+ const { currentUser, ability } = this.authService;
+
+ // Verify the user has access to view deployments for this owner
+ const userWallet = await this.userWalletRepository.accessibleBy(ability, "read").findOneByAddress(owner);
+ assert(userWallet && userWallet.userId === currentUser.id, 403, "Access denied");
+
return await this.deploymentReaderService.getDeploymentByOwnerAndDseq(owner, dseq);
}If this endpoint is intended to be public (which seems unlikely given the sensitive nature of deployment data), please add a comment explaining why authentication is not required.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getByOwnerAndDseq(owner: string, dseq: string): Promise<GetDeploymentByOwnerDseqResponse> { | |
| return await this.deploymentReaderService.getDeploymentByOwnerAndDseq(owner, dseq); | |
| } | |
| @Protected([{ action: "read", subject: "Deployment" }]) | |
| async getByOwnerAndDseq(owner: string, dseq: string): Promise<GetDeploymentByOwnerDseqResponse> { | |
| assert(owner, 400, "Owner is required"); | |
| assert(dseq, 400, "Deployment sequence is required"); | |
| const { currentUser, ability } = this.authService; | |
| // Verify the user has access to view deployments for this owner | |
| const userWallet = await this.userWalletRepository | |
| .accessibleBy(ability, "read") | |
| .findOneByAddress(owner); | |
| assert(userWallet && userWallet.userId === currentUser.id, 403, "Access denied"); | |
| return await this.deploymentReaderService.getDeploymentByOwnerAndDseq(owner, dseq); | |
| } |
🤖 Prompt for AI Agents
In apps/api/src/deployment/controllers/deployment/deployment.controller.ts
around lines 127 to 129, the getByOwnerAndDseq method lacks the @Protected
decorator, input validation for owner and dseq parameters, and authorization
checks to verify the requester's permission. Add the @Protected decorator to
enforce authentication, validate the owner and dseq inputs for correct format
and presence, and implement a permission check to ensure the requester is
authorized to access the deployment data. If this endpoint must remain public,
add a clear comment explaining why authentication is not required.
There was a problem hiding this comment.
We have a public api for users to query without authentication and we want to keep it this way. This information is coming from the indexer which is public data from the blockchain.
There was a problem hiding this comment.
@baktun14, thank you for the clarification! That makes perfect sense. Public blockchain data from an indexer should indeed be accessible without authentication, as it's inherently public information. I understand this endpoint is intentionally designed as a public API for querying deployment data.
✏️ Learnings added
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.090Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| public async getDeploymentByOwnerAndDseq(owner: string, dseq: string) { | ||
| let deploymentData: RestAkashDeploymentInfoResponse | null = null; | ||
| try { | ||
| deploymentData = await this.deploymentHttpService.findByOwnerAndDseq(owner, dseq); | ||
|
|
||
| if ("code" in deploymentData) { | ||
| if (deploymentData.message?.toLowerCase().includes("deployment not found")) { | ||
| return null; | ||
| } else { | ||
| throw new Error(deploymentData.message); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| if (error.response.status === 404) { | ||
| return null; | ||
| } | ||
|
|
||
| throw error; | ||
| } | ||
|
|
||
| const leasesQuery = this.leaseHttpService.list({ owner, dseq }); | ||
| const relatedMessagesQuery = this.messageService.getDeploymentRelatedMessages(owner, dseq); | ||
| const dbDeploymentQuery = Deployment.findOne({ | ||
| attributes: ["createdHeight", "closedHeight"], | ||
| where: { owner: owner, dseq: dseq }, | ||
| include: [ | ||
| { model: Block, attributes: ["datetime"], as: "createdBlock" }, | ||
| { model: Block, attributes: ["datetime"], as: "closedBlock" }, | ||
| { | ||
| model: Lease, | ||
| attributes: ["createdHeight", "closedHeight", "gseq", "oseq"], | ||
| include: [ | ||
| { model: Block, attributes: ["datetime"], as: "createdBlock" }, | ||
| { model: Block, attributes: ["datetime"], as: "closedBlock" } | ||
| ] | ||
| } | ||
| ] | ||
| }); | ||
|
|
||
| const [leasesData, relatedMessages, dbDeployment] = await Promise.all([leasesQuery, relatedMessagesQuery, dbDeploymentQuery]); | ||
|
|
||
| const providerAddresses = leasesData.leases.map(x => x.lease.lease_id.provider); | ||
| const providers = await Provider.findAll({ | ||
| where: { | ||
| owner: { | ||
| [Op.in]: providerAddresses | ||
| } | ||
| }, | ||
| include: [{ model: ProviderAttribute }] | ||
| }); | ||
| const deploymentDenom = deploymentData.escrow_account.balance.denom; | ||
|
|
||
| const leases = leasesData.leases.map(x => { | ||
| const provider = providers.find(p => p.owner === x.lease.lease_id.provider); | ||
| const group = deploymentData.groups.find(g => g.group_id.gseq === x.lease.lease_id.gseq); | ||
| const dbLease = dbDeployment?.leases.find(l => l.gseq === x.lease.lease_id.gseq && l.oseq === x.lease.lease_id.oseq); | ||
|
|
||
| return { | ||
| gseq: x.lease.lease_id.gseq, | ||
| oseq: x.lease.lease_id.oseq, | ||
| createdHeight: dbLease?.createdHeight, | ||
| createdDate: dbLease?.createdBlock?.datetime, | ||
| closedHeight: dbLease?.closedHeight, | ||
| closedDate: dbLease?.closedBlock?.datetime, | ||
| provider: { | ||
| address: provider?.owner, | ||
| hostUri: provider?.hostUri, | ||
| isDeleted: !!provider?.deletedHeight, | ||
| attributes: provider?.providerAttributes.map(attr => ({ | ||
| key: attr.key, | ||
| value: attr.value | ||
| })) | ||
| }, | ||
| status: x.lease.state, | ||
| monthlyCostUDenom: Math.round(parseFloat(x.lease.price.amount) * averageBlockCountInAMonth), | ||
| cpuUnits: group?.group_spec.resources.map(r => parseInt(r.resource.cpu.units.val) * r.count).reduce((a, b) => a + b, 0) || 0, | ||
| gpuUnits: group?.group_spec.resources.map(r => parseInt(r.resource.gpu?.units?.val) * r.count || 0).reduce((a, b) => a + b, 0) || 0, | ||
| memoryQuantity: group?.group_spec.resources.map(r => parseInt(r.resource.memory.quantity.val) * r.count).reduce((a, b) => a + b, 0) || 0, | ||
| storageQuantity: | ||
| group?.group_spec.resources | ||
| .map(r => r.resource.storage.map(s => parseInt(s.quantity.val)).reduce((a, b) => a + b, 0) * r.count) | ||
| .reduce((a, b) => a + b, 0) || 0 | ||
| }; | ||
| }); | ||
|
|
||
| return { | ||
| owner: deploymentData.deployment.deployment_id.owner, | ||
| dseq: deploymentData.deployment.deployment_id.dseq, | ||
| balance: parseFloat(deploymentData.escrow_account.balance.amount), | ||
| denom: deploymentDenom, | ||
| status: deploymentData.deployment.state, | ||
| createdHeight: dbDeployment?.createdHeight, | ||
| createdDate: dbDeployment?.createdBlock?.datetime, | ||
| closedHeight: dbDeployment?.closedHeight, | ||
| closedDate: dbDeployment?.closedBlock?.datetime, | ||
| totalMonthlyCostUDenom: leases.map(x => x.monthlyCostUDenom).reduce((a, b) => a + b, 0), | ||
| leases: leases, | ||
| events: relatedMessages || [], | ||
| other: deploymentData | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring this method for better maintainability
This method is handling multiple responsibilities and could benefit from being broken down into smaller, focused methods.
Consider extracting the following into separate private methods:
- Resource calculation logic (lines 251-257) - could be a reusable utility
- Provider mapping logic (lines 229-248)
- Lease transformation logic (lines 228-259)
Example for resource calculation:
private calculateResourceQuantities(group: any) {
return {
cpuUnits: group?.group_spec.resources.map(r => parseInt(r.resource.cpu.units.val) * r.count).reduce((a, b) => a + b, 0) || 0,
gpuUnits: group?.group_spec.resources.map(r => parseInt(r.resource.gpu?.units?.val) * r.count || 0).reduce((a, b) => a + b, 0) || 0,
memoryQuantity: group?.group_spec.resources.map(r => parseInt(r.resource.memory.quantity.val) * r.count).reduce((a, b) => a + b, 0) || 0,
storageQuantity: group?.group_spec.resources
.map(r => r.resource.storage.map(s => parseInt(s.quantity.val)).reduce((a, b) => a + b, 0) * r.count)
.reduce((a, b) => a + b, 0) || 0
};
}🤖 Prompt for AI Agents
In
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts
between lines 176 and 276, the getDeploymentByOwnerAndDseq method is handling
multiple responsibilities making it hard to maintain. Refactor by extracting the
resource calculation logic (lines 251-257) into a private method like
calculateResourceQuantities, the provider mapping logic (lines 229-248) into
another private method, and the lease transformation logic (lines 228-259) into
its own method. This will modularize the code, improve readability, and enable
reuse of these focused methods.
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts
Outdated
Show resolved
Hide resolved
a0498c4 to
8be68fd
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (72.91%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1428 +/- ##
==========================================
+ Coverage 37.07% 37.33% +0.26%
==========================================
Files 831 832 +1
Lines 19799 19845 +46
Branches 3633 3631 -2
==========================================
+ Hits 7340 7409 +69
+ Misses 12128 11709 -419
- Partials 331 727 +396
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
8be68fd to
7414305
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/api/src/deployment/controllers/deployment/deployment.controller.ts(2 hunks)apps/api/src/deployment/http-schemas/deployment.schema.ts(3 hunks)apps/api/src/deployment/repositories/deployment/deployment.repository.ts(1 hunks)apps/api/src/deployment/repositories/message/message.repository.ts(1 hunks)apps/api/src/deployment/routes/deployments/deployments.router.ts(3 hunks)apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts(2 hunks)apps/api/src/deployment/services/message-service/message.service.ts(1 hunks)apps/api/src/routes/v1/deployments/byOwnerDseq.ts(0 hunks)apps/api/src/routes/v1/index.ts(0 hunks)apps/api/src/services/db/deploymentService.ts(0 hunks)apps/api/src/services/external/apiNodeService.ts(1 hunks)apps/api/src/types/rest/akashDeploymentInfoResponse.ts(1 hunks)apps/api/test/functional/deployments.spec.ts(3 hunks)packages/http-sdk/src/deployment/deployment-http.service.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/api/src/routes/v1/index.ts
- apps/api/src/services/db/deploymentService.ts
- apps/api/src/routes/v1/deployments/byOwnerDseq.ts
✅ Files skipped from review due to trivial changes (1)
- apps/api/src/types/rest/akashDeploymentInfoResponse.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/api/src/deployment/controllers/deployment/deployment.controller.ts
- apps/api/src/deployment/repositories/deployment/deployment.repository.ts
- apps/api/test/functional/deployments.spec.ts
- apps/api/src/deployment/repositories/message/message.repository.ts
- apps/api/src/services/external/apiNodeService.ts
- apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts
- apps/api/src/deployment/services/message-service/message.service.ts
- apps/api/src/deployment/routes/deployments/deployments.router.ts
- apps/api/src/deployment/http-schemas/deployment.schema.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-deploy-web-build
- GitHub Check: validate-notifications
- GitHub Check: validate-api
- GitHub Check: Validate local packages
🔇 Additional comments (3)
packages/http-sdk/src/deployment/deployment-http.service.ts (3)
2-2: LGTM: Clean import addition.The Zod import follows proper conventions and is necessary for the schema validation implementation.
101-101: LGTM: Proper type inference from schema.The type inference follows Zod best practices and maintains type safety while enabling runtime validation.
142-151:Details
✅ Verification successful
Verify API endpoint consistency across the codebase.
The
findByOwnerAndDseqmethod implementation looks correct. However, ensure this endpoint path is consistent with the new modular structure mentioned in the PR objectives.
🏁 Script executed:
#!/bin/bash # Description: Verify API endpoint usage consistency across the codebase # Expected: Should find references to the new modular endpoint structure echo "Searching for deployment endpoint references..." rg -A 3 -B 3 "/akash/deployment/v1beta3/deployments/info" echo "Searching for any old endpoint patterns that might need updating..." rg -A 3 -B 3 "/v1/deployment"Length of output: 39869
Chain deployment info endpoint is consistent across modules.
✅ Verified that
/akash/deployment/v1beta3/deployments/infois used uniformly in:
packages/http-sdk/src/deployment/deployment-http.service.tsapps/notifications/test/functional/balance-alert.spec.tsapps/indexer/src/monitors/deploymentBalanceMonitor.tsNo stale or mismatched
/v1/deploymentpatterns remain for this chain SDK method.
7414305 to
309cb55
Compare
apps/api/src/deployment/controllers/deployment/deployment.controller.ts
Outdated
Show resolved
Hide resolved
309cb55 to
cc48cfe
Compare
refs: #1268
Summary by CodeRabbit
New Features
Bug Fixes
Tests