feat(network): add API endpoints for deployment #860
Conversation
jzsfkzm
force-pushed
the
feature/api-deployments
branch
from
0d05b6a to
ace39ce
Compare
jzsfkzm
force-pushed
the
feature/api-deployments
branch
from
ace39ce to
399d40f
Compare
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (72.11%) 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 #860 +/- ##
===========================================
+ Coverage 11.33% 54.70% +43.37%
===========================================
Files 439 218 -221
Lines 11594 4994 -6600
Branches 2451 718 -1733
===========================================
+ Hits 1314 2732 +1418
+ Misses 10104 2251 -7853
+ Partials 176 11 -165
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| deposit: { | ||
| denom: "uakt", | ||
| amount: "500000" | ||
| }, |
There was a problem hiding this comment.
This will be the USDC ibc denom for managed wallets, you can use it from here
Also, the amount should be "5000000", but to test and use sandbox uakt it's probably to have parametrized.
There was a problem hiding this comment.
managed wallet denom is configured by env var https://github.com/akash-network/console/blob/main/apps/api/src/billing/config/env.config.ts#L12
| }) | ||
| }; | ||
|
|
||
| return await this.messageTransmitterService.signAndBroadcast(account.address, client, [message]); |
There was a problem hiding this comment.
We already have a sign and broadcast transaction that handles wallet for the master wallet. The gpu bot wallet mnemonic is used to another purpose.
| expect(response.status).toBe(400); | ||
| }); | ||
|
|
||
| xit("creates a deployment", async () => { |
There was a problem hiding this comment.
Was pretty unsure about that part of the code, how it should work and what should it actually do. Can we chat about it @ygrishajev?
| ) { } | ||
|
|
||
| public async findByOwnerAndDseq(owner: string, dseq: string): Promise<SingleDeploymentResponse['data']> { | ||
| const response = await axios.get<RestAkasheploymentInfoResponse>(`${apiNodeUrl}/akash/deployment/${betaTypeVersion}/deployments/info`, { |
There was a problem hiding this comment.
This will be the perfect candidates for @stalniy's new chain-sdk implemention. Technically we already have an endpoint implemented for this one, but not refactored in the new services architecture. We should abstract those http calls in the http package.
There was a problem hiding this comment.
That function is returning a somewhat different type than the one described for this endpoint, escrow_account for example is missing, or I'd rather say balance is the only thing picked from escrow_account. I'm all for using the same interface where it is possible, so happy to use that function, but should we right away?
|
|
||
| const getRoute = createRoute({ | ||
| method: "get", | ||
| path: "/v1/deployments/{dseq}", |
There was a problem hiding this comment.
dseq is not a unique identifier for a deployment. userId + dseq or address/owner + dseq is. In our case user seems to be more appropriate. So the interface should prob be something like:
| path: "/v1/deployments/{dseq}", | |
| path: "/v1/deployments/${userId}/{dseq}", |
It's not perfect so it's totally open to discussion, however user should be taken into account
There was a problem hiding this comment.
How about if it had the same interface as GET /v1/bids, with ?dseq&userId, userId being optional as it can be derived from auth, but also can be forced when you're an admin?
Maybe it should be GET /v1/x/{dseq}?userId now that I think about it. What do you think?
| import { StaleManagedDeploymentsCleanerService } from "@src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service"; | ||
| import { TopUpManagedDeploymentsService } from "@src/deployment/services/top-up-managed-deployments/top-up-managed-deployments.service"; | ||
| import { TopUpDeploymentsOptions } from "@src/deployment/types/deployments-refiller"; | ||
| import { CleanUpStaleDeploymentsParams } from "@src/deployment/types/state-deployments"; | ||
|
|
||
| @singleton() | ||
| export class TopUpDeploymentsController { | ||
| export class DeploymentController { |
There was a problem hiding this comment.
non-blocking: why do you think we should merge these? Why not having 2 specific controllers?
There was a problem hiding this comment.
Seemed logical to me as both do something with deployments, but I was unsure too, we can keep those separated, so separating.
| @@ -19,4 +28,33 @@ export class TopUpDeploymentsController { | |||
| async cleanUpStaleDeployment(options: CleanUpStaleDeploymentsParams) { | |||
| await this.staleDeploymentsCleanerService.cleanup(options); | |||
| } | |||
|
|
|||
| @Protected([{ action: "create", subject: "DeploymentSetting" }]) | |||
There was a problem hiding this comment.
blocking: this doesn't seem right. Why is it DeploymentSetting?
There was a problem hiding this comment.
What would be the proper permission here, sign::UserWallet?
| }); | ||
|
|
||
| if ('code' in response.data) { | ||
| return { |
There was a problem hiding this comment.
why do we need to resolve with this response? I assume code may be different? controller should throw appropriate http error. E.g. if deployment not found I assume this return value should be undefined and controller should throw 404. If it's smth else it should be 500 perhaps.
| try { | ||
| sdl = SDL.fromString(input.sdl, 'beta3'); | ||
| } catch (_error) { | ||
| assert(false, 400, "Invalid SDL"); |
There was a problem hiding this comment.
should the error be checked? I mean we should ensure this is a validation issue and not some code bug in which case it should be 500.
| }); | ||
|
|
||
| const message = { | ||
| typeUrl: `/akash.deployment.v1beta3.MsgCreateDeployment`, |
There was a problem hiding this comment.
refs #767