Implement FISCO BCOS adapter #515

vita-dounai · 2019-07-18T07:18:22Z

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 · 2019-08-05T02:37:53Z

A great addition, 👍

nklincoln · 2019-08-08T08:07:28Z

@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 · 2019-08-09T07:15:37Z

@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

@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

nklincoln · 2019-09-18T06:18:06Z

packages/caliper-fisco-bcos/lib/fiscoBcos.js

+     * @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'];


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

nklincoln · 2019-09-18T06:18:36Z

packages/caliper-fisco-bcos/lib/fiscoBcos.js

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


same comment as that in invokeSmartContract

nklincoln · 2019-09-18T06:22:23Z

packages/caliper-fisco-bcos/package.json

+        "branches": 8,
+        "functions": 7,
+        "lines": 5
+    },


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?

test is valid now

nklincoln · 2019-09-18T06:54:48Z

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 · 2019-09-20T06:40:25Z

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 · 2019-09-20T07:04:55Z

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 · 2019-09-20T08:21:34Z

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 · 2019-09-23T08:37:14Z

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 · 2019-09-26T11:23:59Z

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 · 2019-09-30T03:14:27Z

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 · 2019-09-30T04:49:55Z

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 · 2019-09-30T07:31:35Z

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 · 2019-09-30T09:31:17Z

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

sorry - too soon!

author Chenxi Li <lichenxi.webank@gmail.com> 1565147492 +0800 committer Chenxi Li <lichenxi.webank@gmail.com> 1569833600 +0800 implements FISCO BCOS adapter Signed-off-by: Chenxi Li <lichenxi.webank@gmail.com>

vita-dounai · 2019-10-17T06:17:59Z

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

nklincoln · 2019-10-17T08:17:34Z

@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 · 2019-10-17T08:38:22Z

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

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

nklincoln · 2019-10-17T09:52:30Z

Unfortunately, I'm not seeing the same :/

aklenik · 2019-10-18T09:15:53Z

@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 · 2019-10-21T13:42:28Z

@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",

nklincoln

Some fixes to be applied post merge

shippomx approved these changes Jul 19, 2019

View reviewed changes

vita-dounai mentioned this pull request Aug 4, 2019

Hello, could you please send someone to review my pull request? #539

Closed

vita-dounai mentioned this pull request Aug 6, 2019

Travis CI got stuck in "npm ERR! code E404" #542

Closed

nklincoln self-assigned this Sep 11, 2019

nklincoln added the PR/under review The PR is currently under review label Sep 11, 2019

vita-dounai requested review from aklenik, feihujiang, jiangyaoguo, liam-grace, nklincoln and panyu4 as code owners September 12, 2019 05:00

nklincoln suggested changes Sep 18, 2019

View reviewed changes

vita-dounai requested a review from nklincoln September 30, 2019 07:31

nklincoln previously approved these changes Oct 3, 2019

View reviewed changes

aklenik added this to the v0.2.0 milestone Oct 7, 2019

vita-dounai requested a review from nklincoln October 17, 2019 02:24

parent 3f9293a

714d4ad

author Chenxi Li <lichenxi.webank@gmail.com> 1565147492 +0800 committer Chenxi Li <lichenxi.webank@gmail.com> 1569833600 +0800 implements FISCO BCOS adapter Signed-off-by: Chenxi Li <lichenxi.webank@gmail.com>

nklincoln approved these changes Oct 24, 2019

View reviewed changes

nklincoln merged commit 8f629b5 into hyperledger:master Oct 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement FISCO BCOS adapter #515

Implement FISCO BCOS adapter #515

vita-dounai commented Jul 18, 2019 •

edited

Loading

feihujiang commented Aug 5, 2019

nklincoln commented Aug 8, 2019

vita-dounai commented Aug 9, 2019 •

edited

Loading

nklincoln left a comment •

edited

Loading

nklincoln Sep 18, 2019

vita-dounai Sep 30, 2019

nklincoln Sep 18, 2019

vita-dounai Sep 30, 2019

nklincoln Sep 18, 2019

vita-dounai Sep 30, 2019

nklincoln commented Sep 18, 2019

lgtm-com bot commented Sep 20, 2019

lgtm-com bot commented Sep 20, 2019

lgtm-com bot commented Sep 20, 2019

lgtm-com bot commented Sep 23, 2019

vita-dounai commented Sep 26, 2019

lgtm-com bot commented Sep 30, 2019

lgtm-com bot commented Sep 30, 2019

vita-dounai commented Sep 30, 2019

vita-dounai commented Sep 30, 2019

vita-dounai commented Oct 17, 2019

nklincoln commented Oct 17, 2019 •

edited

Loading

vita-dounai commented Oct 17, 2019

nklincoln commented Oct 17, 2019

aklenik commented Oct 18, 2019

aklenik commented Oct 21, 2019

nklincoln left a comment

Implement FISCO BCOS adapter #515

Implement FISCO BCOS adapter #515

Conversation

vita-dounai commented Jul 18, 2019 • edited Loading

feihujiang commented Aug 5, 2019

nklincoln commented Aug 8, 2019

vita-dounai commented Aug 9, 2019 • edited Loading

nklincoln left a comment • edited Loading

Choose a reason for hiding this comment

nklincoln Sep 18, 2019

Choose a reason for hiding this comment

vita-dounai Sep 30, 2019

Choose a reason for hiding this comment

nklincoln Sep 18, 2019

Choose a reason for hiding this comment

vita-dounai Sep 30, 2019

Choose a reason for hiding this comment

nklincoln Sep 18, 2019

Choose a reason for hiding this comment

vita-dounai Sep 30, 2019

Choose a reason for hiding this comment

nklincoln commented Sep 18, 2019

lgtm-com bot commented Sep 20, 2019

lgtm-com bot commented Sep 20, 2019

lgtm-com bot commented Sep 20, 2019

lgtm-com bot commented Sep 23, 2019

vita-dounai commented Sep 26, 2019

lgtm-com bot commented Sep 30, 2019

lgtm-com bot commented Sep 30, 2019

vita-dounai commented Sep 30, 2019

vita-dounai commented Sep 30, 2019

vita-dounai commented Oct 17, 2019

nklincoln commented Oct 17, 2019 • edited Loading

vita-dounai commented Oct 17, 2019

nklincoln commented Oct 17, 2019

aklenik commented Oct 18, 2019

aklenik commented Oct 21, 2019

nklincoln left a comment

Choose a reason for hiding this comment

vita-dounai commented Jul 18, 2019 •

edited

Loading

vita-dounai commented Aug 9, 2019 •

edited

Loading

nklincoln left a comment •

edited

Loading

nklincoln commented Oct 17, 2019 •

edited

Loading