-
Notifications
You must be signed in to change notification settings - Fork 405
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 Ethereum adapter #432
Conversation
@russanto Great addition to the platform support 👍
|
dc75fb3
to
3347ae3
Compare
Ethereum adapter is a great addition. +1 |
9c05837
to
f670884
Compare
083b4fe
to
1b3ae92
Compare
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@russanto I'll start to review the pr soon. I'll leave pr remarks, so we can keep the discussion here |
@russanto Can you join the Caliper call next week (Oct. 2, 3PM UTC+0)? Or even today, but I know it's really short notice :) |
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.
General remarks:
- Please squash your commits into a single commit after applying the requested changes.
- See other remarks among the code remarks.
], | ||
"dependencies": { | ||
"caliper-core" : "0.1.0", | ||
"web3": "^1.2.0" |
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.
Web3 should be put under devDeps. It will be installed during binding, similarly to the other adapters. So you need to modify the following file also:
https://github.com/hyperledger/caliper/blob/6ddd6003da8772e9065cf5f179b177e8b47705d9/packages/caliper-cli/lib/bind/config.yaml#L29
sut:
ethereum:
1.0.0:
packages: [ '[email protected]' ]
There could be more 1.X.0 entries of course.
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 added the latest version, I don't know if it can be useful to add all the 1.X ones
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.
@russanto this is awesome! Please do a squash commit so there is only one commit being merged.
90dd29c
to
f9bfc87
Compare
@@ -0,0 +1,26 @@ | |||
# |
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.
consider adding the SPDX header for Apache 2
@@ -0,0 +1,16 @@ | |||
# | |||
# Licensed under the Apache License, Version 2.0 (the "License"); |
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.
consider adding the SPDX header for Apache 2
@@ -0,0 +1,18 @@ | |||
/* |
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.
consider adding the SPDX header for Apache 2
@@ -0,0 +1,213 @@ | |||
/** |
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.
consider adding the SPDX header for Apache 2
@@ -0,0 +1,56 @@ | |||
/** |
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.
consider adding the SPDX header for Apache 2
@@ -0,0 +1,25 @@ | |||
# |
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.
consider adding the SPDX header for Apache 2
@@ -0,0 +1,17 @@ | |||
# Licensed under the Apache License, Version 2.0 (the "License"); |
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.
consider adding the SPDX header for Apache 2
@@ -0,0 +1,17 @@ | |||
# Licensed under the Apache License, Version 2.0 (the "License"); |
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.
consider adding the SPDX header for Apache 2
@@ -0,0 +1,53 @@ | |||
# |
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.
consider adding the SPDX header for Apache 2
@@ -0,0 +1,56 @@ | |||
/** |
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.
consider adding the SPDX header for Apache 2
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.
On new files with license headers, please consider adding the SPDX header as well.
@ryjones I think we'll need to add this to the automatic license check format on the repo-level. Currently, the CI only enforces the above header formats. |
Understood! Don't block this PR based on my comments. |
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.
Just some minor modifications needed (aside from querySmartContract, but that's also easy).
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.
There are two linting errors (missing semicolons), should be an easy fix
lerna ERR! execute /home/travis/build/hyperledger/caliper/packages/caliper-ethereum/lib/ethereum.js
lerna ERR! execute 70:79 error Missing semicolon semi
lerna ERR! execute 202:10 error Missing semicolon semi
@russanto After buliding the
This is originating from your
UPDATE: Adding this to the command of the docker-compose file solves the problem: |
261a396
to
19d8724
Compare
Signed-off-by: Antonio Russo <[email protected]>
19d8724
to
c73bf0f
Compare
No description provided.