feat(copm): add fabric COPM implementation #3546

jenniferlianne · 2024-09-18T12:29:11Z

Primary Change:

add connectRPC cactus plugin for copm primitives

Secondary Changes:

add common COPM prototypes
add common weaver protos-js directory to enable building under ci.yaml, which requires all local
dependencies to be present at build time
remove incorrect 'main' declaration in protos-js package file
add separate tsconfig file to /weaver/sdks/fabric/interoperation-node-sdk to
allow it to be built by ci.yaml
update weaver fabric network makefile 'clean' target to continue if some target files
were not generated by the make process

Signed-off-by: Jennifer Bell [email protected]

Pull Request Requirements

Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

sandeepnRES · 2024-09-19T06:34:09Z

weaver/sdks/fabric/interoperation-node-sdk/.gitignore

@@ -4,8 +4,5 @@ node_modules/
 coverage/
 package-lock.json
 old/
-protos/
-protos-js/*


Any reason for this change?
Also I see there is no protos-js in SDK, so I guess you figured out building SDK without checking in protos-js in SDK?

This was reverted. Yes, turned out not to need the one in interop to build.

sandeepnRES · 2024-09-19T06:49:35Z

packages/cacti-copm-core/src/main/proto/generated/openapi/models/asset_account_v1_pb.proto

+
+message AssetAccountV1PB {
+
+  string network = 232872497;


Sorry I don't have context about this structure, but why are these numbers so high?
Usually proto structures start with 1, 2 and so on...

Same comment for all the below proto files.

The pb files are generated from openapi, following cactus convention. The openapi desc is here: https://jenniferlianne.github.io/cacti/references/openapi/cacti-copm-core_openapi/,

sandeepnRES · 2024-09-19T06:59:02Z

packages/cacti-copm-core/src/main/proto/generated/openapi/models/view_address_v1_pb.proto

+
+message ViewAddressV1PB {
+
+  string contract_id = 512823451;


We may need more fields to this, but I'm assuming, we will keep updating this in future with implementations of these services?

Yes. For now I want to see what we can get away with. In Corda, flowpath (for instance) is very similar to a contract specification.

sandeepnRES · 2024-09-19T06:59:48Z

packages/cacti-copm-core/src/main/proto/generated/openapi/models/view_v1_pb.proto

+
+import "models/view_address_v1_pb.proto";
+
+message ViewV1PB {


View contains response to the remote query right?

No, this is part of the inbound message for the verifyState command. You can see the messages here for more context: https://jenniferlianne.github.io/cacti/references/openapi/cacti-copm-core_openapi/

petermetz

@jenniferlianne Very high quality PR overall, thank you very much for the contribution! I left a few comments with my usual nit picks but it's definitely looking great already! Congratulations!

petermetz · 2024-09-19T16:11:22Z

packages/cacti-copm-core/package.json

+    "@hyperledger/cacti-weaver-sdk-fabric": "workspace:^",
+    "@hyperledger/cactus-common": "workspace:^"
+  },


@jenniferlianne Could you please match the current pattern we have for specifying versions for packages within the monorepo depending on other packages also within the monorepo?

Alternatively, if you you think we should migrate to this syntax, I'm not against it, but then let's make that a separate PR which doesn't change anything except the version declaration syntax that we use (and then explain it over there thoroughly what the change is and why).

petermetz · 2024-09-19T16:11:47Z

packages/cacti-copm-core/package.json

+  },
+  "devDependencies": {
+    "@bufbuild/protoc-gen-es": "1.8.0",
+    "@connectrpc/connect-node": "^1.4.0",


@jenniferlianne Please make sure to pin the dependencies to exact versions everywhere.

petermetz · 2024-09-19T16:15:12Z

packages/cacti-copm-core/openapitools.json

+  "$schema": "./node_modules/@openapitools/openapi-generator-cli/config.schema.json",
+  "spaces": 2,
+  "generator-cli": {
+    "version": "7.7.0"


@jenniferlianne We currently use 6.6.0 - deviating from that version will cause race conditions in the codegen script which only pre-fetches the jar for v6.6.0 (search for "warmup" in the root package.json to see what I mean).

Similar to my other comment about the dependency declaration syntax, here it is the same idea: We can (and I would love to) upgrade to the newer versions of the Open API generator because the newer templates have fixes and additional features (usually) but if we do it it's safer to do it in a single PR dedicated to it which then upgrades the whole project, otherwise there's additional breakages due to the inconsistencies.

petermetz · 2024-09-19T16:35:37Z

...ages/cacti-plugin-copm-fabric/src/test/typescript/integration/test-copm-pledge-claim.test.ts

+  level: logLevel,
+});
+
+describe("PluginCopmFabric", () => {


@jenniferlianne I tried to run yarn configure and then this test case but it crashed with the stack trace below. Could you please take a look? In general the test cases should be operational after the configure script has executed so I'm guessing maybe some part of the build process is not integrated into configure but that's just an idea.

Here's the exact command I ran and the log it produced:

$ yarn test:jest:all packages/cacti-plugin-copm-fabric/src/test/typescript/integration/test-copm-pledge-claim.test.ts jest-haste-map: duplicate manual mock found: ConfigUtil The following files share their name; please delete one of them: * <rootDir>/examples/cactus-common-example-server/src/main/typescript/routing-interface/util/__mocks__/ConfigUtil.ts * <rootDir>/examples/cactus-common-example-server/dist/lib/main/typescript/routing-interface/util/__mocks__/ConfigUtil.js jest-haste-map: duplicate manual mock found: ConfigUtil.d The following files share their name; please delete one of them: * <rootDir>/examples/cactus-common-example-server/dist/lib/main/typescript/routing-interface/util/__mocks__/ConfigUtil.d.ts * <rootDir>/packages/cactus-cmd-socketio-server/dist/lib/main/typescript/routing-interface/util/__mocks__/ConfigUtil.d.ts jest-haste-map: duplicate manual mock found: ConfigUtil The following files share their name; please delete one of them: * <rootDir>/examples/cactus-common-example-server/dist/lib/main/typescript/routing-interface/util/__mocks__/ConfigUtil.js * <rootDir>/packages/cactus-cmd-socketio-server/dist/lib/main/typescript/routing-interface/util/__mocks__/ConfigUtil.js FAIL packages/cacti-plugin-copm-fabric/src/test/typescript/integration/test-copm-pledge-claim.test.ts ● Test suite failed to run Configuration error: Could not locate module ./build/src/Relay.js mapped as: $1. Please check your configuration for these entries: { "moduleNameMapper": { "/^(\.\.?\/.+)\.jsx?$/": "$1" }, "resolver": undefined } 13 | // likely due to that module containing multiple classes 14 | > 15 | module.exports.RelayHelper = require("./build/src/Relay.js"); | ^ 16 | module.exports.InteroperableHelper = require("./build/src/InteroperableHelper.js"); 17 | module.exports.AssetManager = require("./build/src/AssetManager.js"); 18 | module.exports.SatpAssetManager = require("./build/src/SatpAssetManager.js"); at createNoMappedModuleFoundError (node_modules/jest-resolve/build/resolver.js:759:17) at Object.<anonymous> (weaver/sdks/fabric/interoperation-node-sdk/index.js:15:30) at Object.<anonymous> (packages/cacti-copm-core/src/main/typescript/validators/common.ts:5:1) at Object.<anonymous> (packages/cacti-copm-core/src/main/typescript/validators/validate_claim_locked_asset_request.ts:5:1) at Object.<anonymous> (packages/cacti-copm-core/src/main/typescript/validators.ts:1:1) at Object.<anonymous> (packages/cacti-copm-core/src/main/typescript/public-api.ts:24:1) at Object.<anonymous> (packages/cacti-copm-core/src/main/typescript/index.ts:1:1) at Object.<anonymous> (packages/cacti-plugin-copm-fabric/src/test/typescript/integration/test-copm-pledge-claim.test.ts:18:1) Test Suites: 1 failed, 1 total Tests: 0 total Snapshots: 0 total Time: 5.807 s Ran all test suites matching /packages\/cacti-plugin-copm-fabric\/src\/test\/typescript\/integration\/test-copm-pledge-claim.test.ts/i.

More info: I have not tested the other test cases but if they suffer from the same problem then I would also like to request those to be addressed as well (if the solution is somehow unique to each test case for whatever reason)

These tests need a weaver network to run. Following the weaver pattern, the network can either run 'lock' (asset exchange) or 'pledge' (asset transfer) commands. To start a network, in the fabric directory type
make setup; make pledge-network
Then wait 10 min. More here: https://github.com/jenniferlianne/cacti/tree/copm_draft/packages/cacti-plugin-copm-fabric

Primary Change: - add connectRPC cactus plugin for copm primitives Secondary Changes: - add common COPM prototypes - add common weaver protos-js directory to enable building under ci.yaml, which requires all local dependencies to be present at build time - remove incorrect 'main' declaration in protos-js package file - add separate tsconfig file to /weaver/sdks/fabric/interoperation-node-sdk to allow it to be built by ci.yaml - update weaver fabric network makefile 'clean' target to continue if some target files were not generated by the make process Signed-off-by: Jennifer Bell <[email protected]>

jenniferlianne requested review from petermetz, takeutak, izuru0, jagpreetsinghsasan, VRamakrishna, sandeepnRES and outSH as code owners September 18, 2024 12:29

jenniferlianne force-pushed the copm_draft branch from 2342278 to a8ae134 Compare September 18, 2024 12:34

sandeepnRES reviewed Sep 19, 2024

View reviewed changes

petermetz requested changes Sep 19, 2024

View reviewed changes

jenniferlianne force-pushed the copm_draft branch from a8ae134 to c50865d Compare September 19, 2024 19:05

jenniferlianne force-pushed the copm_draft branch from c50865d to 48b4e82 Compare September 19, 2024 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(copm): add fabric COPM implementation #3546

feat(copm): add fabric COPM implementation #3546

jenniferlianne commented Sep 18, 2024

sandeepnRES Sep 19, 2024

jenniferlianne Sep 19, 2024 •

edited

Loading

sandeepnRES Sep 19, 2024 •

edited

Loading

jenniferlianne Sep 19, 2024

sandeepnRES Sep 19, 2024

jenniferlianne Sep 19, 2024

sandeepnRES Sep 19, 2024

jenniferlianne Sep 19, 2024 •

edited

Loading

petermetz left a comment

petermetz Sep 19, 2024

jenniferlianne Sep 19, 2024

petermetz Sep 19, 2024

jenniferlianne Sep 19, 2024

petermetz Sep 19, 2024

jenniferlianne Sep 19, 2024

petermetz Sep 19, 2024

petermetz Sep 19, 2024

jenniferlianne Sep 19, 2024

feat(copm): add fabric COPM implementation #3546

Are you sure you want to change the base?

feat(copm): add fabric COPM implementation #3546

Conversation

jenniferlianne commented Sep 18, 2024

Choose a reason for hiding this comment

jenniferlianne Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

sandeepnRES Sep 19, 2024 • edited Loading

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

jenniferlianne Sep 19, 2024 • edited Loading

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

jenniferlianne Sep 19, 2024 •

edited

Loading

sandeepnRES Sep 19, 2024 •

edited

Loading

jenniferlianne Sep 19, 2024 •

edited

Loading