Merge pull request #948 from golemfactory/feature/JST-940/todos-in-co…

…debase

Fix TODOs left in the codebase

src/activity/exe-script-executor.test.ts

@@ -1,4 +1,3 @@
-// TODO: Implement these tests as they survive separation from old Activity entity
 import { Activity } from "./activity";
 import { _, anything, imock, instance, mock, verify, when } from "@johanblumenberg/ts-mockito";
 import { Capture, Deploy, DownloadFile, Run, Script, Start, Terminate, UploadFile } from "./script";

src/activity/work/batch.spec.ts

@@ -49,7 +49,8 @@ describe("Batch", () => {
       it("should accept shell command", async () => {
         expect(batch.run("rm -rf")).toBe(batch);
         expect(batch["script"]["commands"][0]).toBeInstanceOf(Run);
-        // TODO: check if constructed script is correct.
+        expect(batch["script"]["commands"][0]["args"]["entry_point"]).toBe("/bin/sh");
+        expect(batch["script"]["commands"][0]["args"]["args"]).toStrictEqual(["-c", "rm -rf"]);
       });
 
       it("should accept executable", async () => {

src/experimental/deployment/builder.ts

@@ -50,21 +50,15 @@ export class GolemDeploymentBuilder {
 
   getDeployment(): Deployment {
     validateDeployment(this.components);
-    const deployment = new Deployment(
-      this.components,
-      {
-        logger: this.glm.services.logger,
-        yagna: this.glm.services.yagna,
-        payment: this.glm.payment,
-        market: this.glm.market,
-        activity: this.glm.activity,
-        network: this.glm.network,
-        lease: this.glm.lease,
-      },
-      {
-        dataTransferProtocol: this.glm.options.dataTransferProtocol ?? "gftp",
-      },
-    );
+    const deployment = new Deployment(this.components, {
+      logger: this.glm.services.logger,
+      yagna: this.glm.services.yagna,
+      payment: this.glm.payment,
+      market: this.glm.market,
+      activity: this.glm.activity,
+      network: this.glm.network,
+      lease: this.glm.lease,
+    });
 
     this.reset();
 

src/experimental/deployment/deployment.ts

@@ -3,14 +3,12 @@ import { defaultLogger, Logger, YagnaApi } from "../../shared/utils";
 import { EventEmitter } from "eventemitter3";
 import { ActivityModule } from "../../activity";
 import { Network, NetworkModule, NetworkOptions } from "../../network";
-import { GftpStorageProvider, StorageProvider, WebSocketBrowserStorageProvider } from "../../shared/storage";
 import { validateDeployment } from "./validate-deployment";
 import { DraftOfferProposalPool, MarketModule } from "../../market";
 import { PaymentModule } from "../../payment";
 import { CreateLeaseProcessPoolOptions } from "./builder";
 import { Subscription } from "rxjs";
 import { LeaseProcessPool } from "../../lease-process";
-import { DataTransferProtocol } from "../../shared/types";
 import { LeaseModule } from "../../lease-process/lease.module";
 
 export enum DeploymentState {
@@ -50,10 +48,6 @@ export type DeploymentComponents = {
   networks: { name: string; options: NetworkOptions }[];
 };
 
-export interface DeploymentOptions {
-  dataTransferProtocol?: DataTransferProtocol;
-}
-
 /**
  * @experimental This feature is experimental!!!
  */
@@ -77,7 +71,6 @@ export class Deployment {
   >();
 
   private readonly networks = new Map<string, Network>();
-  private readonly dataTransferProtocol: StorageProvider;
 
   private readonly modules: {
     market: MarketModule;
@@ -98,7 +91,6 @@ export class Deployment {
       network: NetworkModule;
       lease: LeaseModule;
     },
-    options: DeploymentOptions,
   ) {
     validateDeployment(components);
 
@@ -109,29 +101,14 @@ export class Deployment {
 
     this.modules = modules;
 
-    this.dataTransferProtocol = this.getStorageProvider(options.dataTransferProtocol);
-
     this.abortController.signal.addEventListener("abort", () => {
       this.logger.info("Abort signal received");
       this.stop().catch((e) => {
         this.logger.error("stop() error on abort", { error: e });
-        // TODO: should the error be sent to event listener?
       });
     });
   }
 
-  private getStorageProvider(protocol: DataTransferProtocol | StorageProvider = "gftp"): StorageProvider {
-    if (protocol === "gftp") {
-      return new GftpStorageProvider();
-    }
-
-    if (protocol === "ws") {
-      return new WebSocketBrowserStorageProvider(this.yagnaApi, {});
-    }
-
-    return protocol;
-  }
-
   getState(): DeploymentState {
     return this.state;
   }
@@ -145,20 +122,25 @@ export class Deployment {
       throw new GolemUserError(`Cannot start backend, expected backend state INITIAL, current state is ${this.state}`);
     }
 
-    await this.dataTransferProtocol.init();
-
     for (const network of this.components.networks) {
       const networkInstance = await this.modules.network.createNetwork(network.options);
       this.networks.set(network.name, networkInstance);
     }
 
-    // TODO: Derive this from deployment spec
+    // Allocation is re-used for all demands so the expiration date should
+    // be the equal to the longest expiration date of all demands
+    const longestExpiration =
+      Math.max(...this.components.leaseProcessPools.map((pool) => pool.options.market.rentHours)) * 3600;
+    const totalBudget = this.components.leaseProcessPools.reduce(
+      (acc, pool) => acc + this.modules.market.estimateBudget(pool.options),
+      0,
+    );
+
     const allocation = await this.modules.payment.createAllocation({
-      budget: 1,
-      expirationSec: 30 * 60, // 30 minutes
+      budget: totalBudget,
+      expirationSec: longestExpiration,
     });
 
-    // TODO: pass dataTransferProtocol to pool
     for (const pool of this.components.leaseProcessPools) {
       const network = pool.options?.deployment?.network
         ? this.networks.get(pool.options?.deployment.network)
@@ -209,8 +191,6 @@ export class Deployment {
     try {
       this.abortController.abort();
 
-      this.dataTransferProtocol.close();
-
       const stopPools = Array.from(this.pools.values()).map((pool) =>
         Promise.allSettled([pool.proposalSubscription.unsubscribe(), pool.leaseProcessPool.drainAndClear()]),
       );

src/market/agreement/agreement.ts

@@ -3,6 +3,18 @@ import { MarketApi } from "ya-ts-client";
 import { Demand, OfferProposal } from "../index";
 import { InvoiceFilter } from "../../payment/agreement_payment_process";
 
+/**
+ * * `Proposal` - newly created by a Requestor (draft based on Proposal)
+ * * `Pending` - confirmed by a Requestor and send to Provider for approval
+ * * `Cancelled` by a Requestor
+ * * `Rejected` by a Provider
+ * * `Approved` by both sides
+ * * `Expired` - not approved, rejected nor cancelled within validity period
+ * * `Terminated` - finished after approval.
+ *
+ */
+export type AgreementState = "Proposal" | "Pending" | "Cancelled" | "Rejected" | "Approved" | "Expired" | "Terminated";
+
 export interface ProviderInfo {
   name: string;
   id: string;
@@ -51,8 +63,7 @@ export interface IAgreementApi {
    */
   proposeAgreement(proposal: OfferProposal): Promise<Agreement>;
 
-  // TODO: Detach return type from ya-ts-client!
-  getAgreementState(id: string): Promise<MarketApi.AgreementDTO["state"]>;
+  getAgreementState(id: string): Promise<AgreementState>;
 
   confirmAgreement(agreement: Agreement): Promise<Agreement>;
 

src/market/agreement/index.ts

@@ -1,3 +1,3 @@
-export { Agreement, LegacyAgreementServiceOptions, ProviderInfo } from "./agreement";
+export { Agreement, LegacyAgreementServiceOptions, ProviderInfo, AgreementState } from "./agreement";
 export { AgreementCandidate, AgreementSelector } from "./selector";
 export { AgreementApiConfig } from "./config";

src/market/demand/directors/workload-demand-director-config.ts

@@ -17,7 +17,7 @@ export class WorkloadDemandDirectorConfig {
   readonly manifestSig?: string;
   readonly manifestSigAlgorithm?: string;
   readonly manifestCert?: string;
-
+  readonly useHttps?: boolean = false;
   readonly imageHash?: string;
   readonly imageTag?: string;
   readonly imageUrl?: string;

src/market/demand/directors/workload-demand-director.ts

@@ -47,8 +47,7 @@ export class WorkloadDemandDirector implements IDemandDirector {
   private async resolveTaskPackageUrl(): Promise<string> {
     const repoUrl = EnvUtils.getRepoUrl();
 
-    //TODO : in future this should be passed probably through config
-    const isHttps = false;
+    const useHttps = this.config.useHttps;
 
     const isDev = EnvUtils.isDevMode();
 
@@ -65,7 +64,7 @@ export class WorkloadDemandDirector implements IDemandDirector {
 
       const data = await response.json();
 
-      const imageUrl = isHttps ? data.https : data.http;
+      const imageUrl = useHttps ? data.https : data.http;
       hash = data.sha3;
 
       return `hash:sha3:${hash}:${imageUrl}`;

src/market/demand/options.ts

@@ -54,6 +54,13 @@ type ImageDemandOptions = {
 
   /**  finds package by registry tag  */
   imageTag?: string;
+
+  /**
+   * Force the image download url that will be passed to the provider to use HTTPS.
+   * This option is only relevant when you use `imageHash` or `imageTag` options.
+   * Default is false
+   */
+  useHttps?: boolean;
 };
 
 export type WorkloadDemandDirectorConfigOptions = RuntimeDemandOptions &

src/shared/yagna/adapters/activity-api-adapter.ts

@@ -17,13 +17,16 @@ export class ActivityApiAdapter implements IActivityApi {
 
   async createActivity(agreement: Agreement): Promise<Activity> {
     try {
-      // TODO: Use options
-      // @ts-expect-error: FIXME #yagna ts types
-      const { activityId } = await this.control.createActivity({
+      const activityOrError = await this.control.createActivity({
         agreementId: agreement.id,
       });
 
-      return this.activityRepo.getById(activityId);
+      if (typeof activityOrError !== "object" || !("activityId" in activityOrError)) {
+        // will be caught in the catch block and converted to GolemWorkError
+        throw new Error(activityOrError);
+      }
+
+      return this.activityRepo.getById(activityOrError.activityId);
     } catch (error) {
       const message = getMessageFromApiError(error);
       throw new GolemWorkError(

src/shared/yagna/adapters/agreement-api-adapter.ts

@@ -1,4 +1,4 @@
-import { Agreement, IAgreementApi, IAgreementRepository } from "../../../market/agreement/agreement";
+import { Agreement, AgreementState, IAgreementApi, IAgreementRepository } from "../../../market/agreement/agreement";
 import { MarketApi } from "ya-ts-client";
 import { GolemMarketError, MarketErrorCode, OfferProposal } from "../../../market";
 import { withTimeout } from "../../utils/timeout";
@@ -64,14 +64,6 @@ export class AgreementApiAdapter implements IAgreementApi {
         demandId: proposal.demand.id,
       });
 
-      // TODO - do we need it?
-      // this.events.emit("agreementCreated", {
-      //   id: agreementId,
-      //   provider: "todo",
-      //   validTo: data?.validTo,
-      //   proposalId: proposal.id,
-      // });
-
       return this.repository.getById(agreementId);
     } catch (error) {
       const message = getMessageFromApiError(error);
@@ -104,7 +96,7 @@ export class AgreementApiAdapter implements IAgreementApi {
     return this.repository.getById(id);
   }
 
-  async getAgreementState(id: string): Promise<MarketApi.AgreementDTO["state"]> {
+  async getAgreementState(id: string): Promise<AgreementState> {
     const entry = await this.repository.getById(id);
     return entry.getState();
   }
@@ -119,7 +111,6 @@ export class AgreementApiAdapter implements IAgreementApi {
       }
 
       await withTimeout(
-        // TODO: Make a fix in ya-ts-client typings so that's going to be specifically {message:string}
         this.api.terminateAgreement(current.id, {
           message: reason,
         }),

src/shared/yagna/adapters/network-api-adapter.ts

@@ -13,7 +13,7 @@ export class NetworkApiAdapter implements INetworkApi {
   async createNetwork(options: { id: string; ip: string; mask?: string; gateway?: string }): Promise<Network> {
     try {
       const { id, ip, mask, gateway } = await this.yagnaApi.net.createNetwork(options);
-      // @ts-expect-error TODO: Can we create a network without an id or is this just a bug in ya-clinet spec?
+      // @ts-expect-error TODO: Remove when this PR is merged: https://github.com/golemfactory/ya-client/pull/179
       return new Network(id, ip, mask, gateway);
     } catch (error) {
       const message = getMessageFromApiError(error);

tests/e2e/express.spec.ts

@@ -33,7 +33,6 @@ describe("Express", function () {
             imageTag: "severyn/espeak:latest",
           },
         },
-        // TODO: This should be optional
         market: {
           maxAgreements: 1,
           rentHours: 1,