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-tcs-huawei):Modify the initial version. Add some feature. #2559

Closed

Conversation

123wyl123
Copy link

Modify the initial version. Add some feature.

Copy link
Contributor

@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.

@123wyl123 Thank you very much for the contribution! We've just merged a major PR that migrated us from Yarn v1 to v3 and that has lead to a bunch of conflicts on this branch. Please rebase onto the latest upstream main before we do a review in detail (so that we avoid mangling the diff/comments too much)

P.S: I've tried to resolve the conflicts for you just now, but I don't have permission to push to your fork. If you'd like to take advantage of my help then please grant permission (not necessary - just in case)

@123wyl123
Copy link
Author

@petermetz
Thanks for the reply, I've set you up as a collaborator on the repository. As you can see, I'm trying both adding socketio and implementing the IPluginLedgerConnector interface. ipluginLedgerConnector currently lacks a Dockerfile file, based on other implementations such as quorum, by RUN npm i @hyperledger/ cactus-plugin-ledger-connector-quorum@${NPM_PKG_VERSION} --production to compile the image, do I need to build the npm package first to complete this step?

Also I would like to make a complete demo to show our implementation of these, for example if I want to have a two way transaction from cactus with tcs, I would first make both parties start the startMonitor method, then trigger the transaction via tcs, trigger the blp event, then cactus completes the transaction, and finally trigger the blp event without triggering a new transaction. Or trigger a transaction on a chain via cactus, trigger the blp event, then tcs completes the transaction, and finally trigger the blp event without triggering a new transaction. That is to say, I need to do some transaction filtering after triggering the blp event, so that the transaction doesn't trigger cyclically, I don't know if this is achievable?For example, in the existing example, the transaction initiated from tcs will transfer money from 06fc56347d91c6ad2dae0c3ba38eb12ab0d72e97 to the address 9d624f7995e8bd70251f8265f2f9f2b49f169c55, and I'm filtering out a certain address in the blp to achieve this effect.

WORK IN PROGRESS

Signed-off-by: wangyinglun <wangyinglun3@huawei.com>
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz
Copy link
Contributor

do I need to build the npm package first to complete this step?

@123wyl123 If you want to use that specific method of installing the package in the container image, then the package must have already been published on the npm registry, which (for now) it isn't [1] unfortunately.

So for now your only option is to add the package files to the image from your local machine instead of fetching the package files from npm. And so to answer the question: Yes, you need to build your package first so that the dist folder is populated with the files needed to run.

[1] https://www.npmjs.com/search?q=%40hyperledger%2Fcactus-plugin-ledger-connector-tcs-huawei

},
cert: fs.readFileSync(config.read("sslParam.clientCert")),
key: fs.readFileSync(config.read("sslParam.clientKey")),
rejectUnauthorized: false,

Check failure

Code scanning / CodeQL

Disabling certificate validation High

Disabling certificate validation is strongly discouraged.
@petermetz
Copy link
Contributor

I don't know if this is achievable?

@123wyl123 To my knowledge it should be achievable yes, but the Fujitsu maintainers can probably confirm it for sure (@izuru0 @takeutak )

Copy link
Contributor

@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.

@123wyl123

  1. Please re-write the commit message to explain what the new feature(s) are. The thing to keep in mind is that your commit message will get directly copied into the release note so it should be as informative as possible.
  2. Is there any chance that you could add test cases verifying the functionality that you are implementing?

@petermetz
Copy link
Contributor

@123wyl123 FYI: I force pushed to your branch just now, the merge conflicts have been resolved!

@123wyl123
Copy link
Author

I apologize for only replying now, I've been busy with other things lately, I'll add the unit tests and commit message changes as soon as I'm done with other things.

@petermetz
Copy link
Contributor

I apologize for only replying now, I've been busy with other things lately, I'll add the unit tests and commit message changes as soon as I'm done with other things.

@123wyl123 No worries, I'm grateful for the work and the updates! Let me know if you run into any problems and I can try to help!

@petermetz
Copy link
Contributor

@123wyl123 Let's re-open this once you are ready. In the meantime it can stay closed as long as it needs to just so that we have a tidy review queue. If you need any help with getting it over the finish line please feel free to reach out here or in the pair programming calls or on the discord channels.

cc: @hyperledger/cacti-maintainers

@petermetz petermetz closed this May 18, 2024
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.

2 participants