Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(connector-fabric): add WatchBlocks endpoint #2119

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

outSH
Copy link
Contributor

@outSH outSH commented Jul 15, 2022

  • Add new endpoint for monitoring new blocks. Response type is determined from input argument.
    Monitor endpoints are stored in connector.runningWatchBlocksMonitors.
  • Add FabricApiClient which supports new socketio endpoint.
  • Add functional test to check the new endpoint.
  • Upgrade fabric-sdk-node to solve test hand issue.
  • Loose up ISocketApiClient to allow interface monitor options.
  • Update readme with WatchBlocks usage.

Regarding fabric-sdk-node upgrade: with sdk2.3, the test used to hang occasionally.
I've ensured that all resources are being cleaned-up, and investigated the problem. The issue was
infinite recurse dns resolution (of fabric peer address) in grpc-js ResolvingLoadBalancer.
Could not find exact cause of this strange behaviour, the code there is pretty complex
and didn't want to dig to deep, but I've noticed that recently there were some fixes
in the functions I suspected so I've updated the fabric-sdk, which uses newer grpc-js,
and the problem went away, so I assume this was in fact bug in gprc-js. Run this test on repeat for
hours and didn't notice any error.

Closes: #2118

Signed-off-by: Michal Bajer michal.bajer@fujitsu.com

@outSH
Copy link
Contributor Author

outSH commented Jul 19, 2022

OK, it seems that fabric SDK upgrade is not as trivial as I wanted it to be because it breaks all the custom identity providers feature tests:

  • run-transaction-with-identities.test.ts
  • run-transaction-with-ws-ids.test

When using 2.3.0-snapshot.69 or later (including 2.5 I've initially used) there's following error during runtime:
TypeError: Channel credentials must be a ChannelCredentials object

When using LTS 2.2 (which also solves my test), the fabric connector doesn't compile, because CryptoSuite ` in LTS is still sync instead of async.

I couldn't make our identity sign() function sync in anyway, and I have no idea how to fix the error in recent fabric-sdk, so my proposal is to:

  • Disable my test for now, but leave it in repo.
  • Keep the current fabric-sdk version.
  • Add tasks to upgrade the fabric-sdk, and to restore my test that is flaky at the moment.

@petermetz @jagpreetsinghsasan any thoughts?

@petermetz
Copy link
Member

OK, it seems that fabric SDK upgrade is not as trivial as I wanted it to be because it breaks all the custom identity providers feature tests:

  • run-transaction-with-identities.test.ts
  • run-transaction-with-ws-ids.test

When using 2.3.0-snapshot.69 or later (including 2.5 I've initially used) there's following error during runtime: TypeError: Channel credentials must be a ChannelCredentials object

When using LTS 2.2 (which also solves my test), the fabric connector doesn't compile, because CryptoSuite ` in LTS is still sync instead of async.

I couldn't make our identity sign() function sync in anyway, and I have no idea how to fix the error in recent fabric-sdk, so my proposal is to:

  • Disable my test for now, but leave it in repo.
  • Keep the current fabric-sdk version.
  • Add tasks to upgrade the fabric-sdk, and to restore my test that is flaky at the moment.

@petermetz @jagpreetsinghsasan any thoughts?

@outSH TypeError: Channel credentials must be a ChannelCredentials object means that the grpc versions are different between your client and the server so you could fix this by upgrading the fabric dependency versions consistently across the packages. I'm also fine if you open an issue for doing that and for now just disable the flaky test (just make sure to leave a note in the issue that the test has to be re-enabled as part of the acceptance criteria)

@outSH
Copy link
Contributor Author

outSH commented Jul 22, 2022

@outSH TypeError: Channel credentials must be a ChannelCredentials object means that the grpc versions are different between your client and the server so you could fix this by upgrading the fabric dependency versions consistently across the packages. I'm also fine if you open an issue for doing that and for now just disable the flaky test (just make sure to leave a note in the issue that the test has to be re-enabled as part of the acceptance criteria)

I read the same thing on the internet, but I've updated the test ledger to latest 2.4, to match fabric-sdk from this package and the problem remains (and only for identity related stuff). Or do you have different setup in mind??

@outSH outSH force-pushed the merge_fabric_connectors_pr branch from 7bdb18b to e936960 Compare July 22, 2022 11:39
Copy link
Contributor

@izuru0 izuru0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jagpreetsinghsasan
Copy link
Contributor

@outSH can you re-request review from @petermetz if the requested changes were addressed?

@outSH outSH force-pushed the merge_fabric_connectors_pr branch 2 times, most recently from 69dd303 to 1c3dbf2 Compare December 6, 2022 16:57
@outSH outSH requested review from petermetz and removed request for sandeepnRES, jagpreetsinghsasan and takeutak December 6, 2022 19:48
@outSH
Copy link
Contributor Author

outSH commented Dec 6, 2022

I read the same thing on the internet, but I've updated the test ledger to latest 2.4, to match fabric-sdk from this package and the problem remains (and only for identity related stuff). Or do you have different setup in mind??

@petermetz Funny thing, I've just needed to update fabric SDK on test ledger as well, it somehow interfered with these problematic tests. All works now, ready to merge :)

Copy link
Member

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the same thing on the internet, but I've updated the test ledger to latest 2.4, to match fabric-sdk from this package and the problem remains (and only for identity related stuff). Or do you have different setup in mind??

@petermetz Funny thing, I've just needed to update fabric SDK on test ledger as well, it somehow interfered with these problematic tests. All works now, ready to merge :)

@outSH Sorry for the slow response! Very glad to hear it figured itself out somehow! Please resolve the merge conflicts and then pass it back for review!

@outSH
Copy link
Contributor Author

outSH commented Dec 15, 2022

@outSH Sorry for the slow response! Very glad to hear it figured itself out somehow! Please resolve the merge conflicts and then pass it back for review!
Done

@petermetz
Copy link
Member

Suggested edit:

diff --git a/packages/cactus-plugin-ledger-connector-fabric/src/main/typescript/watch-blocks/watch-blocks-v1-endpoint.ts b/packages/cactus-plugin-ledger-connector-fabric/src/main/typescript/watch-blocks/watch-blocks-v1-endpoint.ts
index 82bac1f0..15f91a5b 100644
--- a/packages/cactus-plugin-ledger-connector-fabric/src/main/typescript/watch-blocks/watch-blocks-v1-endpoint.ts
+++ b/packages/cactus-plugin-ledger-connector-fabric/src/main/typescript/watch-blocks/watch-blocks-v1-endpoint.ts
@@ -19,6 +19,7 @@ import {
 
 import safeStringify from "fast-safe-stringify";
 import sanitizeHtml from "sanitize-html";
+import { RuntimeError } from "run-time-error";
 
 /**
  * WatchBlocksV1Endpoint configuration.
@@ -254,11 +255,9 @@ export class WatchBlocksV1Endpoint {
         listenerType = "full";
         break;
       default:
-        // Will not compile if any type was not handled by above switch.
-        const unknownType: never = type;
-        throw new Error(
-          `Unknown block listen type - '${unknownType}'. Check name and connector version.`,
-        );
+        const validTypes = Object.keys(WatchBlocksListenerTypeV1).join(";");
+        const errorMessage = `Unknown block listen type '${type}'. Check name and connector version. Accepted listener types for WatchBlocksListenerTypeV1 are: [${validTypes}]`;
+        throw new RuntimeError(errorMessage);
     }
 
     return { listener, listenerType };
diff --git a/packages/cactus-plugin-ledger-connector-fabric/src/test/typescript/integration/fabric-v2-2-x/fabric-watch-blocks-v1-endpoint.test.ts b/packages/cactus-plugin-ledger-connector-fabric/src/test/typescript/integration/fabric-v2-2-x/fabric-watch-blocks-v1-endpoint.test.ts
index bb713579..92941212 100644
--- a/packages/cactus-plugin-ledger-connector-fabric/src/test/typescript/integration/fabric-v2-2-x/fabric-watch-blocks-v1-endpoint.test.ts
+++ b/packages/cactus-plugin-ledger-connector-fabric/src/test/typescript/integration/fabric-v2-2-x/fabric-watch-blocks-v1-endpoint.test.ts
@@ -400,4 +400,31 @@ describe("watchBlocksV1 of fabric connector tests", () => {
 
     await monitorPromise;
   });
+
+  test("Invalid WatchBlocksListenerTypeV1 value gets knocked down", async () => {
+    const monitorPromise = testWatchBlock(
+      "CactusTransactionsTest",
+      "Some_INVALID_WatchBlocksListenerTypeV1" as WatchBlocksListenerTypeV1,
+      () => undefined, // will never reach this because it is meant to error out
+    );
+
+    try {
+      await monitorPromise;
+    } catch (ex: unknown) {
+      // Execution never reaches this point - I'm assuming because the
+      // testWatchBlock method somehow does not fulfil it's obligation of
+      // either succeeding or throwing (it seems to get stuck idling forever
+      // when I debug this in VSCode)
+      expect(ex).toBeTruthy();
+      expect(ex).toBe(Error);
+      expect(ex).toHaveProperty("message");
+      expect((ex as Error).message).toContainAllValues([
+        WatchBlocksListenerTypeV1.CactusTransactions,
+        WatchBlocksListenerTypeV1.Filtered,
+        WatchBlocksListenerTypeV1.Full,
+        WatchBlocksListenerTypeV1.Private,
+        ...Object.keys(WatchBlocksListenerTypeV1),
+      ]);
+    }
+  });
 });

Copy link
Member

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@outSH Please see #2119 (comment) and then pass it back for review after you decided how to proceed.

- Add new endpoint for monitoring new blocks. Response type is determined from input argument.
  Monitor endpoints are stored in connector.runningWatchBlocksMonitors.
- Add `FabricApiClient` which supports new socketio endpoint.
- Add functional test to check the new endpoint.
- Upgrade fabric-sdk-node to solve test hand issue.
- Loose up `ISocketApiClient` to allow interface monitor options.
- Update readme with WatchBlocks usage.

Regarding fabric-sdk-node upgrade: with sdk2.3, the test used to hang occasionally.
I've ensured that all resources are being cleaned-up, and investigated the problem. The issue was
infinite recurse dns resolution (of fabric peer address) in grpc-js `ResolvingLoadBalancer`.
Could not find exact cause of this strange behaviour, the code there is pretty complex
and didn't want to dig to deep, but I've noticed that recently there were some fixes
in the functions I suspected so I've updated the fabric-sdk, which uses newer grpc-js,
and the problem went away, so I assume this was in fact bug in gprc-js. Run this test on repeat for
hours and didn't notice any error.

Closes: hyperledger#2118

Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
Copy link
Member

@petermetz petermetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@outSH LGTM, thank you!

@petermetz petermetz merged commit 6c62de4 into hyperledger:main Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(connector-fabric): add WatchBlocks endpoint
4 participants