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

Implement FISCO BCOS adapter #515

Merged
merged 1 commit into from
Oct 24, 2019
Merged

Implement FISCO BCOS adapter #515

merged 1 commit into from
Oct 24, 2019

Conversation

vita-dounai
Copy link
Contributor

@vita-dounai vita-dounai commented Jul 18, 2019

FISCO BCOS is a secure and reliable financial-grade open-source blockchain platform led by Chinese enterprises. Its performance has reached over 10,000 TPS with single-chain setup. The platform provides rich features including group architecture, cross-chain communication protocols, pluggable consensus mechanisms, privacy protection algorithms, OSCCAapproved (Office of state Commercial Cryptography Administration) cryptography algorithms, and distributed storage. Its performance, usability and security has been testified by many institutional users and successful business applications in live production environment.

For more information about FISCO BCOS, please refer: https://fisco-bcos-documentation.readthedocs.io/en/latest/

This PR contains the implementation of Caliper adaptor for FISCO BCOS.

@feihujiang
Copy link
Contributor

A great addition, 👍

@nklincoln
Copy link
Contributor

@vita-dounai

Hi, thanks for this PR and I'm sorry it is taking so long to get it reviewed

We are keen to get this into the code base though there are a few (small) hurdles first:

  • We would love for you to present this on a maintainers call (every Wednesday 3pm UTC, but we are happy to modify this time if it is unsuitable)
  • As the contributor of the adaptor, we would ask that you maintain the adaptor, and associated documentation, upon merge into the main Caliper code base

We welcome any conversation on the above both here and in the rocket chat channel: https://chat.hyperledger.org/channel/caliper-contributors

Thanks!

@vita-dounai
Copy link
Contributor Author

vita-dounai commented Aug 9, 2019

@vita-dounai

Hi, thanks for this PR and I'm sorry it is taking so long to get it reviewed

We are keen to get this into the code base though there are a few (small) hurdles first:

  • We would love for you to present this on a maintainers call (every Wednesday 3pm UTC, but we are happy to modify this time if it is unsuitable)
  • As the contributor of the adaptor, we would ask that you maintain the adaptor, and associated documentation, upon merge into the main Caliper code base

We welcome any conversation on the above both here and in the rocket chat channel: https://chat.hyperledger.org/channel/caliper-contributors

Thanks!

Yes, I'm glad to accept that 2 requirements.
😊

@nklincoln nklincoln self-assigned this Sep 11, 2019
@nklincoln nklincoln added the PR/under review The PR is currently under review label Sep 11, 2019
Copy link
Contributor

@nklincoln nklincoln left a comment

Choose a reason for hiding this comment

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

@vita-dounai Thanks for this PR 👍 Just a few minor review comments. The sample really helps - we should probably get this into the CI build too so that we can ensure continued operation. Happy to assist you with that either as part of this PR, or in a new PR

Looks like fiscoBcosApi.js has an unused import os too

packages/caliper-core/lib/report/report.js Outdated Show resolved Hide resolved
* @return {Promise<object>} The promise for the result of the execution.
*/
async invokeSmartContract(context, contractID, contractVer, args, timeout) {
const fiscoBcosSettings = CaliperUtils.parseYaml(this.configPath)['fisco-bcos'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it required to parse this here, or could it be cached? The invokeSmartContract is a hot path, so if repeated parsing can be avoided, it will be more efficient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved :)

* @return {any} The result of the query.
*/
async queryState(context, contractID, contractVer, key, fcn) {
const fiscoBcosSettings = CaliperUtils.parseYaml(this.configPath)['fisco-bcos'];
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as that in invokeSmartContract

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved :)

"branches": 8,
"functions": 7,
"lines": 5
},
Copy link
Contributor

Choose a reason for hiding this comment

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

We are also including a license checker now, to ensure that all files have the correct license during the CI build process. This is performed at the package level, with the check being started as a pre test phase. Would you be willing to include/use the same settings as the other packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test is valid now

@nklincoln
Copy link
Contributor

In order to enable a merge, you will need to sign the commits - the DCO bot will prevent merge without doing so. Details here

@lgtm-com
Copy link

lgtm-com bot commented Sep 20, 2019

This pull request introduces 1 alert when merging 385ec32 into 6ddd600 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Sep 20, 2019

This pull request introduces 1 alert when merging dbbbff6 into 6ddd600 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Sep 20, 2019

This pull request introduces 1 alert when merging cb2a5a9 into 6ddd600 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Sep 23, 2019

This pull request introduces 1 alert when merging bee69f6 into 6ddd600 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@vita-dounai
Copy link
Contributor Author

In order to enable a merge, you will need to sign the commits - the DCO bot will prevent merge without doing so. Details here

Thanks for your reviewing! I will improve my code after Oct. 7th because I am a little busy recently for doing another stuff :)

@lgtm-com
Copy link

lgtm-com bot commented Sep 30, 2019

This pull request introduces 11 alerts when merging 6c76e34 into 3f9293a - view on LGTM.com

new alerts:

  • 11 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Sep 30, 2019

This pull request introduces 11 alerts when merging e740461 into 3f9293a - view on LGTM.com

new alerts:

  • 11 for Unused variable, import, function or class

@vita-dounai
Copy link
Contributor Author

In order to enable a merge, you will need to sign the commits - the DCO bot will prevent merge without doing so. Details here

signature now is off

@vita-dounai
Copy link
Contributor Author

Hello, I've revised my code by following your modification opinions, please take a look:) @nklincoln

nklincoln
nklincoln previously approved these changes Oct 3, 2019
@nklincoln nklincoln dismissed their stale review October 3, 2019 09:04

sorry - too soon!

@aklenik aklenik added this to the v0.2.0 milestone Oct 7, 2019
@vita-dounai vita-dounai requested a review from nklincoln October 17, 2019 02:24
author Chenxi Li <[email protected]> 1565147492 +0800
committer Chenxi Li <[email protected]> 1569833600 +0800

implements FISCO BCOS adapter

Signed-off-by: Chenxi Li <[email protected]>
@vita-dounai
Copy link
Contributor Author

@nklincoln Hi,could you please review this PR?^_^

@nklincoln
Copy link
Contributor

nklincoln commented Oct 17, 2019

@vita-dounai - we're trying to get this running locally but the adaptor does not appear to be registering transaction, which means that there are no transaction statistics being made available ... can you confirm that this is working for you on the current code base please?

You can run against the latest code with:

node ../caliper-cli/caliper.js benchmark run  --caliper-benchconfig ./benchmark/fisco-bcos/helloworld/config.yaml --caliper-networkconfig ./network/fisco-bcos/4nodes1group/fisco-bcos.json --caliper-workspace ./

When in the caliper-samples directory, though please note you will need to add the following to your benchmark config file:

observer:
  interval: 1
  type: local

@vita-dounai
Copy link
Contributor Author

@vita-dounai - we're trying to get this running locally but the adaptor does not appear to be registering transaction, which means that there are no transaction statistics being made available ... can you confirm that this is working for you on the current code base please?

I'm not sure about the meaning of registering transaction, but on my local computer, caliper with FISCO BCOS adapter can run smoothly.

For example, in caliper/packages/caliper-tests-integration directory, I execute the command npx caliper benchmark run --caliper-workspace ../caliper-samples --caliper-benchconfig benchmark/fisco-bcos/helloworld/config.yaml --caliper-networkconfig network/fisco-bcos/4nodes1group/fisco-bcos.json , and the test result is as following:
image

If you can not run FISCO BCOS adaptor, please contact me and send the logs to me to analyze the cause, thanks!

@nklincoln
Copy link
Contributor

Screenshot 2019-10-17 at 10 44 57

Unfortunately, I'm not seeing the same :/

@aklenik
Copy link
Contributor

aklenik commented Oct 18, 2019

@vita-dounai I have the same problem as Nick. Pulled your changes the following way (basically emulating a pr merge) :

git clone <caliper repo>
cd caliper 
git remote add upstream <your repo>
git fetch upstream 
git merge upstream/fisco-bcos

Then ran the command as specified by Nick.

@aklenik
Copy link
Contributor

aklenik commented Oct 21, 2019

@vita-dounai So the problem is, that you used the caliper-core npm package, which is bogus. The official package is @hyperledger/caliper-core. The affected files:

./packages/caliper-fisco-bcos/lib/fiscoBcosApi.js:17:const CaliperUtils = require('caliper-core').CaliperUtils;
./packages/caliper-fisco-bcos/lib/fiscoBcosClientWorker.js:20:} = require('caliper-core');
./packages/caliper-fisco-bcos/lib/sendRawTransactions.js:17:const { CaliperUtils, TxStatus } = require('caliper-core');
./packages/caliper-fisco-bcos/lib/fiscoBcos.js:20:} = require('caliper-core');
./packages/caliper-fisco-bcos/lib/invokeSmartContract.js:20:} = require('caliper-core');
./packages/caliper-fisco-bcos/lib/installSmartContract.js:17:const CaliperUtils = require('caliper-core').CaliperUtils;
./packages/caliper-fisco-bcos/lib/generateRawTransactions.js:18:const { CaliperUtils, TxStatus } = require('caliper-core');
./packages/caliper-fisco-bcos/lib/common.js:21:const CaliperUtils = require('caliper-core').CaliperUtils;
./packages/caliper-fisco-bcos/package.json:26:        "caliper-core": "^0.1.0",

Copy link
Contributor

@nklincoln nklincoln left a comment

Choose a reason for hiding this comment

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

Some fixes to be applied post merge

@nklincoln nklincoln merged commit 8f629b5 into hyperledger-caliper:master Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/under review The PR is currently under review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants