feat(copm): add Corda COPM implementation #3625

jenniferlianne · 2024-11-12T18:07:38Z

Primary change:

Implements COPM primitives for Corda as a cacti plugin. The implementation follows the model of
the Corda ledger connector plugin, where a typescript pass-through implementation is registered on the plugin server, and commands are implemented on a separate grpc server in the Kotlin Spring framework.

Secondary changes:

The lock claim protobuf structure was updated to reduce the number of parameters.
A no-op endpoint was added, per project specification
The format of the inter-network command for requesting the status of a pledge varies by remote network type and asset. Adds a function to the typescript interop configuration interface to supply the appropriate command given the remote network and asset.
Adds test package to test combinations of network types
Updates CI script to use new testing framework
Updates linter ignore to include weaver build directories, allowing yarn lint to pass on a system where weaver libraries have been built. Also excludes docs directory to avoid linting minified js from documentation packages.

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.

.github/actions/copm_test/action.yaml

packages/cacti-copm-core/src/main/json/openapi.tpl.json

petermetz

@jenniferlianne Looking pretty high quality in general, but as usual I have some requests. On top of the comments I left inline, please make sure to include a test case which verifies the plugin being functional while used through the API server (you'll most likely need a separate package because this one cannot depend on the API server due to circular dependency issues)
If you need examples just let me know.

...acti/plugin/cacti/plugin/copm/core/services/defaultservice/DefaultServiceOuterClassGrpcKt.kt

petermetz · 2024-11-12T18:39:10Z

packages/cacti-copm-test/config/corda_rpc.json

@@ -0,0 +1 @@
+{"O=PartyA, L=London, C=GB@Corda_Network":{"host":"127.0.0.1","username":"clientUser1","password":"test","port":10006},"O=PartyB, L=London, C=GB@Corda_Network":{"host":"127.0.0.1","username":"clientUser1","password":"test","port":10009},"O=PartyA, L=London, C=GB@Corda_Network2":{"host":"127.0.0.1","username":"clientUser1","password":"test","port":30006},"O=PartyB, L=London, C=GB@Corda_Network2":{"host":"127.0.0.1","username":"clientUser1","password":"test","port":30009}}


@jenniferlianne Is this something that has to be shipped to production or just a testing aritfact? If it's only for testing then I'd recommend placing it somewhere under src/test/json/resources or similar like src/test/json/fixtures/

The file has been moved to /src/test/json

packages/cacti-plugin-copm-corda/start-app.sh

packages/cacti-plugin-copm-corda/src/main/typescript/plugin-copm-corda.ts

...plugin-copm-corda/src/main/kotlin/org/hyperledger/cacti/plugin/copm/corda/CordaAssetClaim.kt

petermetz

@jenniferlianne LGTM, thank you!

jenniferlianne · 2024-11-14T16:05:43Z

@jenniferlianne Looking pretty high quality in general, but as usual I have some requests. On top of the comments I left inline, please make sure to include a test case which verifies the plugin being functional while used through the API server (you'll most likely need a separate package because this one cannot depend on the API server due to circular dependency issues) If you need examples just let me know.

As mentioned on the call, the plugin is brought up via the ApiServer in both fabric and corda cases. Please see:
packages/cacti-copm-test/src/main/typescript/corda/copm-tester-corda.ts line 118
packages/cacti-copm-test/src/main/typescript/fabric/copm-tester-fabric.ts line 122

sandeepnRES · 2024-11-18T13:26:45Z

packages/cacti-copm-core/Makefile

@@ -0,0 +1,194 @@
+WORKSPACE ?= $(shell realpath `pwd`/../../)


Assuming this makefile is for starting the test networks and testing the copm end-to-end, as per structure of cacti, shouldn't this go to examples instead of packages?
@petermetz @VRamakrishna let me know if I make sense...

The file can be moved to cacti-copm-test.

cacti-copm-test uses the weaver samples to do end-to-end testing.

I'd prefer it be moved to cacti-copm-test then. Let's completely separate the testing setup and logic from the generic components.

Ideally, I'd separate the test setups from the testing logic too, so that the testing logic could get a "spec" for the test setup and run generic tests on the network it's been given, but I think the Cacti package structure doesn't make this easy.

Moving this logic to the cacti-copm-test package seems to be the low-hanging-fruit option now, so let's do that. Perhaps later, we can refactor this to move the testnet launch logic to examples (as @sandeepnRES suggests), thereby making cacti-copm-test also oblivious to the network structure.

I have moved all the Makefiles to cacti-copm-test. The Makefile_corda and Makefile_fabric files have 3 key targes: setup, lock-network, and pledge-network. These can be referenced either directly:

make -f Makefile_corda setup

or via wildcard targets in the main makefile:

make corda-setup

Future network types can follow this pattern.

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

packages/cacti-copm-test/package.json

sandeepnRES · 2024-11-19T11:09:31Z

packages/cacti-copm-test/src/main/typescript/lib/weaver-interop-configuration.ts

+
+export class WeaverInteropConfiguration implements CopmIF.InteropConfiguration {
+  private log: Logger;
+  private weaverRelativePath = "../../../../../../weaver/";


There are hardcoded paths in this file, since this is for testing purpose, may be make it more configurable, instead of hardcoded values?
Also this is also why I believe this package should be in examples and not in packages directory...

I see this in several places above (and probably below) too. Given that this is a test package, I suppose hardcoding these paths is OK for now. But I wonder if you can allow such values to be passed through constructors instead, with the hardcoded paths then being moved to the commands that start and run the tests. That will make the code base more maintainable.

okay yeah agree with Rama.

This file is an example implementation of an abstract interface, and not meant to be re-used. In addition to the hard-coded location of the weaver fabric-cli json settings (stored in a file), the class also contains hard-coded reference to the weaver contracts.

In general, I would advise against forcing use of a local json file for data storage. While it's nice for getting an example system going, it scales poorly and precludes other options that would be easier for end-users to maintain.

sandeepnRES · 2024-11-19T18:18:33Z

packages/cacti-plugin-copm-corda/sample/src/main/kotlin/com/copmCorda/SampleInteropConfig.kt

@@ -0,0 +1,119 @@
+package com.copmCorda


Why is this file is a sample? Asking because I don't see any hard coded values or anything specific to testnet.

This is in the sample subfolder within the Corda plugin folder. That said, it does look like the logic is generic, though the remoteCopmContracts variable seems specific to the asset exchange contracts.

sandeepnRES · 2024-11-19T18:20:06Z

...-corda/sample/src/main/kotlin/com/copmCorda/interop/corda/RemoteCopmContractCordaFungible.kt

+    }
+
+    override fun getPledgeInfoCmd(claim: ValidatedClaimPledgedAssetV1Request): DLTransactionParams {
+        return DLTransactionParams("com.cordaSimpleApplication.flow",


hardcoded value here, we should not refer sample application anywhere unless its a test-code. I'll suggest take it from parameters, or configurations.

Sorry I just saw that this is inside sample directory, so for every application someone needs to write code similar to these files? I'd prefer some config jsons/yaml instead of a kotlin code. But that's my preference, others can comment too if they are okay with it @VRamakrishna @petermetz

Correct. This is left up to the app for now (as opposed to json files) as in larger applications configuration info could be stored in, for instance, a database to make it easier to administer.

As I have mentioned elsewhere, I also think some of this application configuration could be pushed down to the contracts. As requested by @VRamakrishna , have a working draft where asset type registration is done in the interop contract: https://github.com/jenniferlianne/cacti/tree/copm_register_assets_in_chaincode

okay makes sense then.
Only thing is, if someone's application is to do remote call only once, they will have overhead to store everything in contract which will be useless after one call.

Its useful only if the remote transaction is used regularly.

sandeepnRES · 2024-11-19T18:21:03Z

...-copm-corda/sample/src/main/kotlin/com/copmCorda/interop/corda/RemoteCopmContractCordaNFT.kt

+    }
+
+    override fun getPledgeInfoCmd(claim: ValidatedClaimPledgedAssetV1Request): DLTransactionParams {
+        return DLTransactionParams("com.cordaSimpleApplication.flow",


same here... and below line 16 too

sandeepnRES · 2024-11-19T18:22:34Z

...orda/sample/src/main/kotlin/com/copmCorda/interop/fabric/RemoteCopmContractFabricFungible.kt

+                                  claim.sourceCert,
+                                  claim.destinationAccount.organization,
+                                  claim.destCert)
+        return DLTransactionParams("simpleassettransfer","GetTokenAssetPledgeStatus", params )


Here too hardcoded...

sandeepnRES · 2024-11-19T18:22:47Z

...opm-corda/sample/src/main/kotlin/com/copmCorda/interop/fabric/RemoteCopmContractFabricNFT.kt

+    }
+
+    override fun getPledgeInfoCmd(claim: ValidatedClaimPledgedAssetV1Request): DLTransactionParams {
+        return DLTransactionParams("simpleassettransfer",


sandeepnRES · 2024-11-19T18:27:37Z

packages/cacti-plugin-copm-corda/src/main/typescript/plugin-copm-corda.ts

+  }
+
+  public getPackageName(): string {
+    return `@hyperledger-cacti/cacti-plugin-copm-corda`;


is there a way to get this programmatically?
Because the prefix might change

This follows the example of other cactus packages.

I'd prefer to get this from a configuration file or an environmental variable. Personally, I'd suggest that other Cactus packages do something similar too, @petermetz, rather than hardcode such values.

Check if we can get it from package.json, I think that would be better.

As discussed on the project channel, this is outside of the scope of the copm project.

sandeepnRES · 2024-11-19T18:29:57Z

packages/cacti-plugin-copm-corda/Makefile

+setup: build-local-deps server-docker-image
+
+
+pledge-network:  


Would suggest moving these makefile targets, like starting network and other components to tests, and those should ideally be not in packages I believe.

Makefiles have been moved to cacti-copm-test and the cacti-plugin-copm-corda/src/test/kotlin directories.

sandeepnRES · 2024-11-19T18:40:45Z

@jenniferlianne Thank you for the PR, overall looks good to me. Just some small suggestions above I've shared.

VRamakrishna · 2024-11-21T09:42:58Z

jest.config.js

@@ -6,6 +6,7 @@ module.exports = {
  moduleNameMapper: {
    "^(\\.\\.?\\/.+)\\.jsx?$": "$1",
  },
+  modulePathIgnorePatterns: ["./weaver/core/identity-management"],


This module does get exercised in Weaver transaction pipelines, though currently only the iin-agent subfolder (other subfolders are placeholders for mechanisms that are specified in the Weaver RFCs but haven't been implemented yet).

Is this module causing any issues that necessitates its exclusion?

This file is specific to the jest testing system, and there are no jest tests in weaver. The weaver convention of copying packages when building causes collisions in the haste map, so jest tests are unable to run. In my branch where I am building the dependencies as part of running the test, I have had to make additional exceptions.

VRamakrishna · 2024-11-21T09:52:52Z

packages/cacti-copm-core/Makefile

+	make convert-compose-method2 && \
+	make start-server COMPOSE_ARG='--env-file docker/testnet-envs/.env.n2' 
+
+# IIN --------------------------------------------------------------------------------------


@sandeepnRES I think we don't need the iin agents for scenarios involving Corda, right? They won't do anything useful.

@jenniferlianne Is this used for Fabric-Fabric testing too, or just ones that involve Corda networks?

This file is used for testing all combinations. I have reorganized the Makefiles into cacti-copm-test.

VRamakrishna · 2024-11-21T10:45:39Z

packages/cacti-copm-core/src/main/json/openapi.json

+          }
+        },
+        "operationId": "noopV1",
+        "summary": "A no-op operation.  May be used as a heath check",


"heath" --> "health"

VRamakrishna · 2024-11-21T10:50:19Z

packages/cacti-copm-core/src/main/json/openapi.json

-          "source_certificate",
-          "hash_info"
-        ],
+        "required": ["lock_id", "destination", "asset", "hash_info"],


Why remove source (or source_certificate)?

I changed the lock claim for the fabric NFT case to use the lock id instead of the asset agreement to claim the lock, to be consistent with the Corda implementation. After making this change you don't need information about source any more.

VRamakrishna · 2024-11-21T10:54:32Z

packages/cacti-copm-core/src/main/json/openapi.json

+      "post": {
+        "x-hyperledger-cacti": {
+          "http": {
+            "path": "/api/v1/plugins/@hyperledger/cacti-plugin-copm/no-op",


Change @hyperledger --> @hyperledger-cacti here and in other locations.

VRamakrishna · 2024-11-21T11:05:26Z

packages/cacti-copm-core/src/main/typescript/lib/types.ts

@@ -15,7 +16,8 @@ export type CopmContractNames = {
 };

 export type RemoteNetworkConfig = {
-  channelName: string;
+  channelName: string; // fabric-specific
+  chaincode: string; // fabric-specific


Please rename this to chaincodeName.

Probably networkName instead of network in the below line 21?

After further discussion on the project channel I have removed the chaincode and flowPackage variables from the remote network configuration struct as remote networks may use multiple chaincode packages in both fabric and corda. The network variable has been renamed networkName.

VRamakrishna · 2024-11-21T11:50:18Z

packages/cacti-copm-test/package.json

+  "author": {
+    "name": "Hyperledger Cacti Contributors",
+    "email": "[email protected]",
+    "url": "https://www.hyperledger.org/use/cacti"


This URL now automatically redirects to https://www.lfdecentralizedtrust.org/projects/cacti, so perhaps you can change it.

This has been updated.

VRamakrishna · 2024-11-21T11:54:33Z

packages/cacti-copm-test/src/main/typescript/corda/copm-tester-corda.ts

+  }
+
+  public networkNames(): string[] {
+    return ["Corda_Network", "Corda_Network2"];


I wonder if this should be hardcoded or lifted from a configuration file. @sandeepnRES ?

I see the user identities and org names are also hardcoded below. I looked at the older code, where such values were hardcoded in the test folders of the corda-interop-app module, so if this package is the equivalent of that test suite, then I'm fine with the hardcoding.

VRamakrishna · 2024-11-22T09:29:25Z

packages/cacti-plugin-copm-corda/Makefile

+	cp $(WORKSPACE)/weaver/core/network/corda-interop-app/interop-workflows/build/libs/interop-workflows-*.jar localdeps
+	cp $(WORKSPACE)/weaver/core/network/corda-interop-app/interop-contracts/build/libs/interop-contracts-*.jar localdeps
+	cp $(WORKSPACE)/weaver/samples/corda/corda-simple-application/contracts-kotlin/build/libs/contracts-kotlin-*.jar localdeps
+	cp $(WORKSPACE)/weaver/samples/corda/corda-simple-application/workflows-kotlin/build/libs/workflows-kotlin-*.jar localdeps


I can understand the rest, but why do you need weaver/samples/... as dependencies?

The current corda interop chaincode require passing down the asset-specific CommandData instances for issue and burn, as well as getting the pledgeId. This is something that could be avoided with asset type registration.

Here are the asset-specific commands being passed to the chaincode in the corda CLI:

cacti/weaver/samples/corda/corda-simple-application/clients/src/main/kotlin/com/cordaSimpleApplication/client/AssetTransferManager.kt

Line 116 in 27a24dd

AssetContract.Commands.Delete(),

VRamakrishna · 2024-11-22T09:45:05Z

packages/cacti-plugin-copm-corda/package.json

+    "@hyperledger-cacti/cacti-copm-core": "2.0.0",
+    "@hyperledger-cacti/cacti-copm-test": "2.0.0",
+    "@hyperledger/cacti-weaver-protos-js": "2.0.0",
+    "@hyperledger/cacti-weaver-sdk-fabric": "2.0.0",


2.0.0 version doesn't exist for the two packages above. Because of the organization change, the last version tagged under @hyperledger was [2.0.0-rc.7](https://github.com/orgs/hyperledger/packages/container/cacti-weaver-driver-fabric/283162801?tag=2.0.0-rc.7). Weaver publish actions failed during the 2.0.0 release because paths were not updated. These actions should succeed when the next release (2.0.1) is done, hopefully in the next few days. So you have two options: (1) wait for the next release and then update these version numbers here and elsewhere in your code, or (2) use the last release version 2.0.0-rc.7.

These refer to the local monorepo versions, which is why they are able to compile. The packages are built in the cactus way, with npm run configure at the top level. This uses the tsconfig file at at the root level which builds the packages in a specific order and keeps a reference of local packages. One of the challenges of this project was getting everything to build successfully given the differences in build philosophy between cactus and weaver.

Primary change: Implements COPM primitives for Corda as a cacti plugin. The implementation follows the model of the Corda ledger connector plugin, where a typescript pass-through implementation is registered on the plugin server, and commands are implemented on a separate grpc server in the Kotlin Spring framework. Secondary changes: - The lock claim protobuf structure was updated to reduce the number of parameters. - A no-op endpoint was added, per project specification - The format of the inter-network command for requesting the status of a pledge varies by remote network type and asset. Adds a function to the typescript interop configuration interface to supply the appropriate command given the remote network and asset. - Adds test package to test combinations of network types - Updates CI script to use new testing framework - Updates linter ignore to include weaver build directories, allowing yarn lint to pass on a system where weaver libraries have been built. Also excludes docs directory to avoid linting minified js from documentation packages. Signed-off-by: Jennifer Bell <[email protected]>

jenniferlianne requested review from petermetz, izuru0, jagpreetsinghsasan, VRamakrishna, sandeepnRES and outSH as code owners November 12, 2024 18:07

petermetz reviewed Nov 12, 2024

View reviewed changes

.github/actions/copm_test/action.yaml Show resolved Hide resolved

petermetz reviewed Nov 12, 2024

View reviewed changes

packages/cacti-copm-core/src/main/json/openapi.tpl.json Outdated Show resolved Hide resolved

petermetz requested changes Nov 12, 2024

View reviewed changes

jenniferlianne force-pushed the copm_corda branch 6 times, most recently from e107eb5 to e04747c Compare November 14, 2024 13:21

petermetz approved these changes Nov 14, 2024

View reviewed changes

jenniferlianne force-pushed the copm_corda branch from e04747c to 69d8450 Compare November 14, 2024 15:11

sandeepnRES reviewed Nov 18, 2024

View reviewed changes

packages/cacti-copm-core/src/main/proto/generated/openapi/models/asset_lock_claim_v1_pb.proto Show resolved Hide resolved

sandeepnRES reviewed Nov 18, 2024

View reviewed changes

packages/cacti-copm-test/package.json Show resolved Hide resolved

sandeepnRES reviewed Nov 19, 2024

View reviewed changes

VRamakrishna reviewed Nov 21, 2024

View reviewed changes

jenniferlianne force-pushed the copm_corda branch from 69d8450 to 28bc627 Compare November 21, 2024 16:53

VRamakrishna reviewed Nov 22, 2024

View reviewed changes

jenniferlianne force-pushed the copm_corda branch 11 times, most recently from f9859fc to ac09efd Compare November 25, 2024 21:27

jenniferlianne force-pushed the copm_corda branch from ac09efd to f88723e Compare November 25, 2024 21:47

jenniferlianne requested review from VRamakrishna and sandeepnRES November 25, 2024 21:51

		@@ -0,0 +1 @@
		{"O=PartyA, L=London, C=GB@Corda_Network":{"host":"127.0.0.1","username":"clientUser1","password":"test","port":10006},"O=PartyB, L=London, C=GB@Corda_Network":{"host":"127.0.0.1","username":"clientUser1","password":"test","port":10009},"O=PartyA, L=London, C=GB@Corda_Network2":{"host":"127.0.0.1","username":"clientUser1","password":"test","port":30006},"O=PartyB, L=London, C=GB@Corda_Network2":{"host":"127.0.0.1","username":"clientUser1","password":"test","port":30009}}

		@@ -0,0 +1,194 @@
		WORKSPACE ?= $(shell realpath `pwd`/../../)

		setup: build-local-deps server-docker-image


		pledge-network:

feat(copm): add Corda COPM implementation #3625

Are you sure you want to change the base?

feat(copm): add Corda COPM implementation #3625

Conversation

jenniferlianne commented Nov 12, 2024

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

petermetz left a comment

Choose a reason for hiding this comment

jenniferlianne commented Nov 14, 2024

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

sandeepnRES Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

VRamakrishna Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

sandeepnRES Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

sandeepnRES Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandeepnRES Nov 22, 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

sandeepnRES Nov 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

sandeepnRES Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandeepnRES commented Nov 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

jenniferlianne Nov 21, 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

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 Nov 25, 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

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

sandeepnRES Nov 19, 2024 •

edited

Loading

VRamakrishna Nov 22, 2024 •

edited

Loading

sandeepnRES Nov 19, 2024 •

edited

Loading

sandeepnRES Nov 19, 2024 •

edited

Loading

sandeepnRES Nov 22, 2024 •

edited

Loading

sandeepnRES Nov 19, 2024 •

edited

Loading

sandeepnRES Nov 19, 2024 •

edited

Loading

sandeepnRES commented Nov 19, 2024 •

edited

Loading

jenniferlianne Nov 21, 2024 •

edited

Loading

jenniferlianne Nov 25, 2024 •

edited

Loading