-
Notifications
You must be signed in to change notification settings - Fork 282
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(copm): add fabric COPM implementation #3546
base: main
Are you sure you want to change the base?
Conversation
2342278
to
a8ae134
Compare
|
||
message AssetAccountV1PB { | ||
|
||
string network = 232872497; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jenniferlianne Can you follow the suggestion in OpenAPITools/openapi-generator#9646 to set field numbers starting from 1
? That will make the proto message files more readable.
(I don't want this to block the merge of this PR though. If it's inconvenient, you can revise this in the next PR.)
packages/cacti-copm-core/src/main/proto/generated/openapi/models/view_address_v1_pb.proto
Show resolved
Hide resolved
packages/cacti-copm-core/src/main/proto/generated/openapi/models/view_v1_pb.proto
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!
...ages/cacti-plugin-copm-fabric/src/test/typescript/integration/test-copm-pledge-claim.test.ts
Show resolved
Hide resolved
c50865d
to
48b4e82
Compare
|
||
rpc PledgeAssetV1 (PledgeAssetV1Request) returns (PledgeAssetV1200ResponsePB); | ||
|
||
rpc ProvestateV1 (ProvestateV1Request) returns (google.protobuf.Empty); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API Name isn't updated here
packages/cacti-copm-core/src/main/proto/generated/openapi/services/default_service.proto
Outdated
Show resolved
Hide resolved
packages/cacti-plugin-copm-fabric/src/main/typescript/endpoints/prove-state-impl-v1.ts
Outdated
Show resolved
Hide resolved
packages/cacti-plugin-copm-fabric/src/main/typescript/service-implementation.ts
Outdated
Show resolved
Hide resolved
48b4e82
to
ab02c7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jenniferlianne LGTM, thank you very much!
@jenniferlianne Can you also check the documentation (with names updated as per your discussion with @sandeepnRES) from https://jenniferlianne.github.io/cacti/references/openapi/cacti-copm-core_openapi/ into this PR? That way, when we add code to the main repo, there is (at least some) documentation explaining what it does. (BTW....sorry for my late comments. I've still not recovered from my illness, and am taking it slow.) |
The openapi documentation should be generated under this link https://hyperledger-cacti.github.io/cacti/references/openapi/ when the code is merged. An updated preview (with the changes discussed) is here: https://jenniferlianne.github.io/cacti/references/openapi/cacti-copm-core_openapi/ |
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 <jenniferlianne@gmail.com>
ab02c7f
to
c3daece
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jenniferlianne Please resolve the merge conflict and make sure to update the package versions to match the latest release which was just issued
- make clean-network | ||
- tear down the current network | ||
|
||
The asset exchanges and asset transfer network modes are currently mutually exclusive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the pledge-network
and lock-network
targets in the Makefile
doing the same thing currently. I guess you will differentiate them in a later PR?
Also, I'd like to understand why a network can be instantiated only with on capability (pledge/lock) and not the other. In the legacy Weaver test networks, we launch different example applications (e.g., the simpleasset
and simpleassettransfer
chaincodes) exclusively to demonstrate the different protocols (asset transfer, asset exchange). But there's no reason why we can't have both running on a network at the same time (a Fabric network can have any number of chaincodes deployed.) The common Fabric chaincode (from which you are drawing the COPM functions) contains the complete set of capabilities, which can be invoked on demand (just like we envision for the COPM).
Anyway, this is not a blocker for the current PR. We can discuss offline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks very good to me. Very well-written and well-organized code, conforming to the Cactus patterns and the Weaver functionality (as was intended for this integration effort).
Just a couple of comments, one of which (proto field numbering) you can choose to address in this PR or in the next one.
Primary Change:
Secondary Changes:
dependencies to be present at build time
allow it to be built by ci.yaml
were not generated by the make process
Signed-off-by: Jennifer Bell jenniferlianne@gmail.com
Pull Request Requirements
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.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.