Add monitoring and send sync/async functionality #1966

StepniowskiP · 2022-04-12T09:16:30Z

Add monitoring and sending async/sync requests to ledger via SocketIO.

Add monitoring and sending async/sync requests to ledger via SocketIO. Closes: hyperledger-cacti#1941 Signed-off-by: stepniowskip <[email protected]>

izuru0

LGTM

petermetz

@StepniowskiP The two tests below are being skipped by the CI right now, did you run these manually on your machine and did they pass?

packages/cactus-plugin-ledger-connector-iroha/src/test/typescript/integration/iroha-iroha-transfer-example.test.ts
packages/cactus-plugin-ledger-connector-iroha/src/test/typescript/integration/run-transaction-endpoint-v1.test.ts

petermetz · 2022-04-19T18:12:47Z

packages/cactus-core-api/src/main/typescript/plugin/ledger-connector/i-socket-api-client.ts

    args: any,
+    method?: Record<string, unknown>,


@StepniowskiP Seems like a breaking change. Could you please refactor it to be backwards compatible with 1.0.0?

Sure, I have refactored method parameter to one before my changes

@StepniowskiP Is it just me or the parameters are still out of order (e.g. not backwards compatible)?
This is the diff I'm looking at:

petermetz · 2022-04-19T18:12:56Z

packages/cactus-core-api/src/main/typescript/plugin/ledger-connector/i-socket-api-client.ts

    args: any,
+    method?: Record<string, unknown>,


@StepniowskiP Seems like a breaking change. Could you please refactor it to be backwards compatible with 1.0.0?

Sure, I have refactored method parameter to one before my changes

petermetz · 2022-04-19T18:16:22Z

packages/cactus-plugin-ledger-connector-iroha/src/main/typescript/transaction.ts

+    return Transaction.CLASS_NAME;
+  }
+
+  constructor(logLevel?: LogLevelDesc) {


@StepniowskiP Please use an interface to define an object that contains the options, that way adding new arguments can be done within that object instead of having to pile up new options as the Nth constructor parameter and struggle with ordering, etc.

petermetz · 2022-04-19T18:17:36Z

packages/cactus-plugin-ledger-connector-iroha/src/main/typescript/transaction.ts

+import commands from "iroha-helpers-ts/lib/commands/index";
+import queries from "iroha-helpers-ts/lib/queries";
+
+export class Transaction {


@StepniowskiP Could this be called IrohaTransactionWrapper instead? The Iroha prefix will help with distinguishing it in the logs from every other ledger's transaction logs and the Wrapper will hint at the fact that it's not the class/type that ships with the Iroha libraries that we use.

Sure, renamed to IrohaTransactionWrapper

petermetz · 2022-04-19T18:20:45Z

...us-plugin-ledger-connector-iroha/src/main/typescript/web-services/socket-session-endpoint.ts

+  public async sendSyncRequest(syncRequestData: any): Promise<void> {
+    this.log.debug(`Inside ##sendSyncRequest()`);
+
+    syncRequestData["syncRequest"] = true;


@StepniowskiP Could this input mutation be avoided by having the boolean flag as a separate parameter of the sendAsyncRequest method?

I have added flag parameter async to sendRequest function

petermetz · 2022-04-19T18:21:18Z

...us-plugin-ledger-connector-iroha/src/main/typescript/web-services/socket-session-endpoint.ts

+    clearInterval(this.monitoringInterval);
+  }
+
+  public async sendAsyncRequest(asyncRequestData: any): Promise<any> {


@StepniowskiP If it supports both sync and async would it make sense to not call it sendAsyncRequest but maybe sendRequest ?

Sure, renamed and refactored

petermetz · 2022-04-19T18:21:36Z

...us-plugin-ledger-connector-iroha/src/main/typescript/web-services/socket-session-endpoint.ts

+      const block = await this.sendAsyncRequest(syncRequestData);
+      this.log.debug(block);
+      this.socket.emit("response", block.transactionReceipt);
+    } catch (err: any) {


@StepniowskiP Can you use unknown instead of any?

petermetz · 2022-04-19T18:23:39Z

...us-plugin-ledger-connector-iroha/src/main/typescript/web-services/socket-session-endpoint.ts

+  socket: SocketIoSocket;
+}
+
+export class SocketSessionEndpoint {


@StepniowskiP Any chance that you could make the name of the class and the constructor arg interface type consistent?
SocketSessionEndpoint is a bit too generic in my opinion because literally every endpoint that uses SocketIO could be accurately be described by the same name.

Sure, renamed to iroha-socketio-endpoint

petermetz · 2022-04-19T18:27:30Z

packages/cactus-plugin-ledger-connector-iroha/src/main/json/openapi.json

@@ -206,6 +206,9 @@
            "type": "boolean",
            "nullable": false,
            "description": "Can only be set to false for an insecure grpc connection."
+          },
+          "monitorMode": {


@StepniowskiP mode implies to me an enum not a boolean, could you add a description to the spec here that explains what this does and what are the possible values (true and false) change in the behavior?
If it is a boolean then maybe call it monitorModeEnabled and if it has more possible values other than true/false then make it an enum so that later on those other possible values can be added as needed without having to break backwards compatibility.

I have added description to this parameter

fixes for iroha pr hyperledger-cacti#1966 Signed-off-by: stepniowskip <[email protected]>

petermetz · 2022-05-25T21:52:22Z

packages/cactus-core-api/src/main/typescript/plugin/ledger-connector/i-socket-api-client.ts

    args: any,
+    method?: Record<string, unknown>,


@StepniowskiP Is it just me or the parameters are still out of order (e.g. not backwards compatible)?
This is the diff I'm looking at:

outSH · 2022-05-26T11:57:56Z

Is it just me or the parameters are still out of order (e.g. not backwards compatible)?
This is the diff I'm looking at:

@petermetz Author of this PR no longer works for us, I'll take over this task. I've opened separate PR here - #2048 where I've fixed the ordering issue you mentioned. Please continue your review there, and close this PR since it'll not be handled anymore.

stepniowskip and others added 2 commits April 11, 2022 12:45

feat: monitoring, sync and async requests

24fb91d

Add monitoring and sending async/sync requests to ledger via SocketIO. Closes: hyperledger-cacti#1941 Signed-off-by: stepniowskip <[email protected]>

Merge branch 'hyperledger:main' into ts-iroha

1fd644b

StepniowskiP requested review from jonathan-m-hamilton and petermetz as code owners April 12, 2022 09:16

izuru0 approved these changes Apr 18, 2022

View reviewed changes

petermetz requested changes Apr 19, 2022

View reviewed changes

petermetz removed the request for review from jonathan-m-hamilton April 19, 2022 18:29

petermetz added enhancement New feature or request Iroha Bugs/features related to the Iroha ledger connector plugin labels Apr 19, 2022

StepniowskiP pushed a commit to StepniowskiP/cactus that referenced this pull request Apr 29, 2022

refactor: fixes for iroha pr

0e0fc62

fixes for iroha pr hyperledger-cacti#1966 Signed-off-by: stepniowskip <[email protected]>

StepniowskiP pushed a commit to StepniowskiP/cactus that referenced this pull request May 9, 2022

refactor: fixes for iroha pr

ce975bb

fixes for iroha pr hyperledger-cacti#1966 Signed-off-by: stepniowskip <[email protected]>

refactor: fixes for iroha pr

da683be

fixes for iroha pr hyperledger-cacti#1966 Signed-off-by: stepniowskip <[email protected]>

StepniowskiP requested a review from petermetz May 17, 2022 09:08

petermetz requested changes May 25, 2022

View reviewed changes

outSH mentioned this pull request May 26, 2022

feat(iroha-connector): implement validator interface #2048

Merged

petermetz closed this Jul 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add monitoring and send sync/async functionality #1966

Add monitoring and send sync/async functionality #1966

StepniowskiP commented Apr 12, 2022

izuru0 left a comment

petermetz left a comment

petermetz Apr 19, 2022

StepniowskiP May 9, 2022

petermetz May 25, 2022

petermetz Apr 19, 2022

StepniowskiP May 9, 2022

petermetz Apr 19, 2022

StepniowskiP May 9, 2022

petermetz Apr 19, 2022

StepniowskiP May 9, 2022

petermetz Apr 19, 2022

StepniowskiP May 9, 2022

petermetz Apr 19, 2022

StepniowskiP May 9, 2022

petermetz Apr 19, 2022

StepniowskiP May 9, 2022

petermetz Apr 19, 2022

StepniowskiP May 9, 2022

petermetz Apr 19, 2022

StepniowskiP May 9, 2022

petermetz May 25, 2022

outSH commented May 26, 2022

Add monitoring and send sync/async functionality #1966

Add monitoring and send sync/async functionality #1966

Conversation

StepniowskiP commented Apr 12, 2022

izuru0 left a comment

Choose a reason for hiding this comment

petermetz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

outSH commented May 26, 2022