-
Notifications
You must be signed in to change notification settings - Fork 286
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(geth-all-in-one): add ethereum test image and helper class #2578
feat(geth-all-in-one): add ethereum test image and helper class #2578
Conversation
|
- Update web3js packages from 1.10 to 4.0.3 in `cactus-plugin-ledger-connector-ethereum` and `cactus-test-plugin-ledger-connector-ethereum`. This allows interacting with most recent geth nodes. - Refactor all ethereum tests. Most of the test cases were duplicated multiple times (between different quorum ledger versions test and deployment methods). I've removed all this duplication while maintaining similar level of test coverage. New tests use Geth test ledger instead of Quorum one. - Add web3js type conversions methods to minimize impact of poor dynamic typing in this early release of 4.X. - Update API. In 4.X all numeric responses has been converted to BigNum. To keep up with this some fields has been changed to string instead of number when necessary. Add some missing fields as well. - Add `estimateMaxFeePerGas` method for estimating current transaction cost. - Fix invalid `runTransact` response type. - Add test script for checking integration with Alchemy that must be executed manually (it's excluded from CI at the moment) - `geth-alchemy-integration-manual-check.test`. Instructions on how to run it has been added to package README. Future improvements: - Support London fork gas fees (i.e. EIP-1559) - Refactor API to allow future extensions. - Fix several TODO items in this connector. Closes: hyperledger-cacti#2580 Depends on hyperledger-cacti#2535 Depends on hyperledger-cacti#2578 Signed-off-by: Michal Bajer <[email protected]>
- Update web3js packages from 1.10 to 4.0.3 in `cactus-plugin-ledger-connector-ethereum` and `cactus-test-plugin-ledger-connector-ethereum`. This allows interacting with most recent geth nodes. - Refactor all ethereum tests. Most of the test cases were duplicated multiple times (between different quorum ledger versions test and deployment methods). I've removed all this duplication while maintaining similar level of test coverage. New tests use Geth test ledger instead of Quorum one. - Add web3js type conversions methods to minimize impact of poor dynamic typing in this early release of 4.X. - Update API. In 4.X all numeric responses has been converted to BigNum. To keep up with this some fields has been changed to string instead of number when necessary. Add some missing fields as well. - Add `estimateMaxFeePerGas` method for estimating current transaction cost. - Fix invalid `runTransact` response type. - Add test script for checking integration with Alchemy that must be executed manually (it's excluded from CI at the moment) - `geth-alchemy-integration-manual-check.test`. Instructions on how to run it has been added to package README. Future improvements: - Support London fork gas fees (i.e. EIP-1559) - Refactor API to allow future extensions. - Fix several TODO items in this connector. Closes: hyperledger-cacti#2580 Depends on hyperledger-cacti#2535 Depends on hyperledger-cacti#2578 Signed-off-by: Michal Bajer <[email protected]>
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.
Please publish tools/docker/geth-all-in-one/Dockerfile to hyperledger ghcr so I can replace the references to my repo before merge.
@outSH Here's the tag I just pushed: ghcr.io/hyperledger/cacti-geth-all-in-one:2023-07-27-2a8c48ed6
@outSH I can't. rwat must comment on it first otherwise GitHub won't let me assign it. |
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.
- Side note: As explained in the PR message, I had to add separate package for this class.
@outSH Yeah, the explanation makes perfect sense, I'll just ask that the new package can be a home to other similar changes in the future as well (same thing could happen to some other ledger's underlying dependencies) which could be achieved by calling it cacti-test-tooling
or cacti-test-tooling-2
@outSH I did, it did come up in the past! The thing is, the way I work on the code, the desired structure is having a single folder
I know we already have With all that said I know that @VRamakrishna and @sandeepnRES also prefer the hierarchical directories so I'm definitely in the minority here with my preference and therefore we should just make the move to the hierarchical directory structure so I'll ask you the same thing I told them a while back: [1] $ ./node_modules/.bin/lerna ls --all
lerna-lite notice cli v1.17.0
check-connection-ethereum-validator (PRIVATE)
@hyperledger/cactus-example-carbon-accounting-backend
@hyperledger/cactus-example-carbon-accounting-business-logic-plugin
@hyperledger/cactus-example-carbon-accounting-frontend
@hyperledger/cactus-example-discounted-asset-trade (PRIVATE)
@hyperledger/cactus-example-electricity-trade (PRIVATE)
@hyperledger/cactus-example-supply-chain-backend
@hyperledger/cactus-example-supply-chain-business-logic-plugin
@hyperledger/cactus-example-supply-chain-frontend
@hyperledger/cactus-example-tcs-huawei
@hyperledger/cactus-workshop-examples-2022-11-14
@hyperledger/cactus-plugin-htlc-coordinator-besu
@hyperledger/cactus-plugin-object-store-ipfs
@hyperledger/cacti-cmd-gui-app
@hyperledger/cactus-api-client
@hyperledger/cactus-cmd-api-server
@hyperledger/cactus-cmd-socketio-server
@hyperledger/cactus-common
@hyperledger/cactus-core-api
@hyperledger/cactus-core
@hyperledger/cactus-plugin-consortium-manual
@hyperledger/cactus-plugin-htlc-eth-besu-erc20
@hyperledger/cactus-plugin-htlc-eth-besu
@hyperledger/cactus-plugin-keychain-aws-sm
@hyperledger/cactus-plugin-keychain-azure-kv
@hyperledger/cactus-plugin-keychain-google-sm
@hyperledger/cactus-plugin-keychain-memory-wasm
@hyperledger/cactus-plugin-keychain-memory
@hyperledger/cactus-plugin-keychain-vault
@hyperledger/cactus-plugin-ledger-connector-besu
@hyperledger/cactus-plugin-ledger-connector-cdl-socketio
@hyperledger/cactus-plugin-ledger-connector-corda
@hyperledger/cactus-plugin-ledger-connector-fabric-socketio
@hyperledger/cactus-plugin-ledger-connector-fabric
@hyperledger/cactus-plugin-ledger-connector-go-ethereum-socketio
@hyperledger/cactus-plugin-ledger-connector-iroha
@hyperledger/cactus-plugin-ledger-connector-iroha2
@hyperledger/cactus-plugin-ledger-connector-quorum
@hyperledger/cactus-plugin-ledger-connector-sawtooth-socketio
@hyperledger/cactus-plugin-ledger-connector-tcs-huawei-socketio
@hyperledger/cactus-plugin-ledger-connector-ubiquity
@hyperledger/cactus-plugin-ledger-connector-xdai
@hyperledger/cactus-plugin-odap-hermes
@hyperledger/cactus-plugin-persistence-ethereum
@hyperledger/cactus-plugin-persistence-fabric
@hyperledger/cactus-test-api-client
@hyperledger/cactus-test-cmd-api-server
@hyperledger/cactus-test-geth-ledger
@hyperledger/cactus-test-plugin-consortium-manual
@hyperledger/cactus-test-plugin-htlc-eth-besu-erc20
@hyperledger/cactus-test-plugin-htlc-eth-besu
@hyperledger/cactus-test-plugin-ledger-connector-besu
@hyperledger/cactus-test-plugin-ledger-connector-quorum
@hyperledger/cactus-test-tooling
@hyperledger/cactus-test-verifier-client
@hyperledger/cactus-verifier-client
@hyperledger/cacti-weaver-protos-js
@hyperledger/cacti-weaver-driver-fabric
@hyperledger/cacti-weaver-iin-agent
@hyperledger/cacti-weaver-besu-cli
@hyperledger/cacti-weaver-besu-simpleasset
@hyperledger/cacti-weaver-besu-simplestate
@hyperledger/cacti-weaver-fabric-cli
@hyperledger/cacti-weaver-sdk-besu
@hyperledger/cacti-weaver-sdk-fabric
lerna-lite success found 65 packages |
2a8c48e
to
ebf364c
Compare
Thanks, done!
Sorry, I'm not sure if I understand that correctly :/ Do you want to have two similar packages of |
Thanks for such detailed explanation! I'll work on a proposal then :) |
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.
Do you want to have two similar packages of
cacti-test-tooling
?
Yeah the similarity here is meant to be a feature not a bug, but I recognize that this is highly subjective too, so Ill just dump some more of the details of my reasoning/thoughts and then leave the decision with you and accept whichever way it goes because I also don't want to get into analysis paralysis, we have to keep it the work going as fast as possible instead of me mumbling from some ivory tower of architecture. I already feel bad for typing this much and then you having to read it when we both could've been just writing code instead, so sorry in advance!
- If I'm looking for a class that is used to pull up a test ledger, I'll know that it must be in one of those two packages. (Ideally of course I would know that it is in the package - the one and only - but that is no longer an option because the web3 upgrade is forcing our hand unfortunately so the next best option is 2 packages IMO)
- In general I'm usually pretty supportive of adding new packages (because of a bunch of benefits like reducing the blast radius of API changes, reducing the probability of circular dependencies, etc.) but if the package is so specifically named that it only makes sense to have the one class in it then we restrict ourselves from adding other classes that also need a different package because of the same problem. For example if (when) newer versions of Besu/Quorum start us requiring to use web3 v4 as well, then we might find ourselves in the same pickle as now, and then we'll have to add extra packages for all those classes as well (but my intention is that when that happens we could add those classes to this one instead)
How would anyone keep track of where each feature is located if there's no clear distinction between them?
Totally fair point. I don't have a good answer. Maybe we don't name the new one the same, but something just slightly more generic like cacti-test-tooling-web3v4
and that way new versions of the besu, quorum, etc. test ledgers can go in there next to the one you are adding now.
I wanted to keep only this geth ledger class in a separate package (hence the question regarding directory structure).
Additional motivation for that is that such class may be useful on it's own in other projects for testing purposes.
I guess that's my main worry that we are creating a design/architecture precedent where there is a package that has literally the same name as a single class that it contains and in a way committing to making more of these in the future for the other web3v4 test ledgers.
E.g., If a package's purpose is so narrow that it only contains a single class, then we should expand the purpose of the package a little bit (definitely not too much, but a little bit).
Extreme example with hyperbole/exaggeration: if we continue down this path then we could end up with hundreds of packages for our hundreds of classes that we have - it produces the best re-usability but at a cost of much higher complexity, build times, etc.
- Update web3js packages from 1.10 to 4.0.3 in `cactus-plugin-ledger-connector-ethereum` and `cactus-test-plugin-ledger-connector-ethereum`. This allows interacting with most recent geth nodes. - Refactor all ethereum tests. Most of the test cases were duplicated multiple times (between different quorum ledger versions test and deployment methods). I've removed all this duplication while maintaining similar level of test coverage. New tests use Geth test ledger instead of Quorum one. - Add web3js type conversions methods to minimize impact of poor dynamic typing in this early release of 4.X. - Update API. In 4.X all numeric responses has been converted to BigNum. To keep up with this some fields has been changed to string instead of number when necessary. Add some missing fields as well. - Add `estimateMaxFeePerGas` method for estimating current transaction cost. - Fix invalid `runTransact` response type. - Add test script for checking integration with Alchemy that must be executed manually (it's excluded from CI at the moment) - `geth-alchemy-integration-manual-check.test`. Instructions on how to run it has been added to package README. Future improvements: - Support London fork gas fees (i.e. EIP-1559) - Refactor API to allow future extensions. - Fix several TODO items in this connector. Closes: hyperledger-cacti#2580 Depends on hyperledger-cacti#2535 Depends on hyperledger-cacti#2578 Signed-off-by: Michal Bajer <[email protected]>
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.
Removing my approval because Jagpreet has also approved and now we are at risk of merging this accidentally without quorum.
@outSH Please re-request review once someone else approved it from a different org as well (e.g. quorum)
- Update web3js packages from 1.10 to 4.0.3 in `cactus-plugin-ledger-connector-ethereum` and `cactus-test-plugin-ledger-connector-ethereum`. This allows interacting with most recent geth nodes. - Refactor all ethereum tests. Most of the test cases were duplicated multiple times (between different quorum ledger versions test and deployment methods). I've removed all this duplication while maintaining similar level of test coverage. New tests use Geth test ledger instead of Quorum one. - Add web3js type conversions methods to minimize impact of poor dynamic typing in this early release of 4.X. - Update API. In 4.X all numeric responses has been converted to BigNum. To keep up with this some fields has been changed to string instead of number when necessary. Add some missing fields as well. - Add `estimateMaxFeePerGas` method for estimating current transaction cost. - Fix invalid `runTransact` response type. - Add test script for checking integration with Alchemy that must be executed manually (it's excluded from CI at the moment) - `geth-alchemy-integration-manual-check.test`. Instructions on how to run it has been added to package README. Future improvements: - Support London fork gas fees (i.e. EIP-1559) - Refactor API to allow future extensions. - Fix several TODO items in this connector. Closes: hyperledger-cacti#2580 Depends on hyperledger-cacti#2535 Depends on hyperledger-cacti#2578 Signed-off-by: Michal Bajer <[email protected]>
- Update web3js packages from 1.10 to 4.0.3 in `cactus-plugin-ledger-connector-ethereum` and `cactus-test-plugin-ledger-connector-ethereum`. This allows interacting with most recent geth nodes. - Refactor all ethereum tests. Most of the test cases were duplicated multiple times (between different quorum ledger versions test and deployment methods). I've removed all this duplication while maintaining similar level of test coverage. New tests use Geth test ledger instead of Quorum one. - Add web3js type conversions methods to minimize impact of poor dynamic typing in this early release of 4.X. - Update API. In 4.X all numeric responses has been converted to BigNum. To keep up with this some fields has been changed to string instead of number when necessary. Add some missing fields as well. - Add `estimateMaxFeePerGas` method for estimating current transaction cost. - Fix invalid `runTransact` response type. - Add test script for checking integration with Alchemy that must be executed manually (it's excluded from CI at the moment) - `geth-alchemy-integration-manual-check.test`. Instructions on how to run it has been added to package README. Future improvements: - Support London fork gas fees (i.e. EIP-1559) - Refactor API to allow future extensions. - Fix several TODO items in this connector. Closes: hyperledger-cacti#2580 Depends on hyperledger-cacti#2535 Depends on hyperledger-cacti#2578 Signed-off-by: Michal Bajer <[email protected]>
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.
@outSH I see that Izuru has approved it too so now we are ready to go just please do the usual rebase+resolve merge conflicts and pass it back for review!
Yeah, this is a tricky problem :/ I've found another possible solution, yarn allows installing multiple versions of the same package through aliases - https://classic.yarnpkg.com/lang/en/docs/cli/add/#toc-yarn-add-alias - the docs are for yarn V1 but I've tested it in V2 and it works the same (but it's not documented in a new manual for some reason, so maybe it's not officially supported - we must confirm before use). As for your previous message, I see the points you mentioned, agree we can easily flood in dozen of tiny packages that will add a lot of headaches and additional complexity that may not be worth it at all in the end. On the other hand, I'm leaning towards feature-based partitioning more that dependency-based one because:
I think it's hard to make this decision now, since it can go bad both ways. I can try merging |
ebf364c
to
947426c
Compare
Done |
- Add new `geth-all-in-one` test image for running ethereum tests in mainnet-like environment. Image is based on `client-go:v1.12.0` and uses Clique (PoS). There is one coinbase account with publicly available keys like in other, similar packages in cacti. - New image was introduced because currently used open-ethereum one is deprecated. - Add `geth-all-in-one-publish` CI for publishing new images. - Add `@hyperledger/cactus-test-geth-ledger` for using new geth ledger container in the tests. The class has been moved out of `cactus-test-tooling` because of conflicting `web3js` versions. Other than that, it's similar to open-ethereum test class. - Add basic tests for `@hyperledger/cactus-test-geth-ledger`. More tests are being developed right now, and should be available in subsequent PRs. Closes: hyperledger-cacti#2577 Signed-off-by: Michal Bajer <[email protected]>
947426c
to
fb4231f
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
- Update web3js packages from 1.10 to 4.0.3 in `cactus-plugin-ledger-connector-ethereum` and `cactus-test-plugin-ledger-connector-ethereum`. This allows interacting with most recent geth nodes. - Refactor all ethereum tests. Most of the test cases were duplicated multiple times (between different quorum ledger versions test and deployment methods). I've removed all this duplication while maintaining similar level of test coverage. New tests use Geth test ledger instead of Quorum one. - Add web3js type conversions methods to minimize impact of poor dynamic typing in this early release of 4.X. - Update API. In 4.X all numeric responses has been converted to BigNum. To keep up with this some fields has been changed to string instead of number when necessary. Add some missing fields as well. - Add `estimateMaxFeePerGas` method for estimating current transaction cost. - Fix invalid `runTransact` response type. - Add test script for checking integration with Alchemy that must be executed manually (it's excluded from CI at the moment) - `geth-alchemy-integration-manual-check.test`. Instructions on how to run it has been added to package README. Closes: hyperledger-cacti#2580 Depends on hyperledger-cacti#2535 Depends on hyperledger-cacti#2578 Signed-off-by: Michal Bajer <[email protected]>
- Update web3js packages from 1.10 to 4.0.3 in `cactus-plugin-ledger-connector-ethereum` and `cactus-test-plugin-ledger-connector-ethereum`. This allows interacting with most recent geth nodes. - Refactor all ethereum tests. Most of the test cases were duplicated multiple times (between different quorum ledger versions test and deployment methods). I've removed all this duplication while maintaining similar level of test coverage. New tests use Geth test ledger instead of Quorum one. - Add web3js type conversions methods to minimize impact of poor dynamic typing in this early release of 4.X. - Update API. In 4.X all numeric responses has been converted to BigNum. To keep up with this some fields has been changed to string instead of number when necessary. Add some missing fields as well. - Add `estimateMaxFeePerGas` method for estimating current transaction cost. - Fix invalid `runTransact` response type. - Add test script for checking integration with Alchemy that must be executed manually (it's excluded from CI at the moment) - `geth-alchemy-integration-manual-check.test`. Instructions on how to run it has been added to package README. Closes: hyperledger-cacti#2580 Depends on hyperledger-cacti#2535 Depends on hyperledger-cacti#2578 Signed-off-by: Michal Bajer <[email protected]>
- Update web3js packages from 1.10 to 4.0.3 in `cactus-plugin-ledger-connector-ethereum` and `cactus-test-plugin-ledger-connector-ethereum`. This allows interacting with most recent geth nodes. - Refactor all ethereum tests. Most of the test cases were duplicated multiple times (between different quorum ledger versions test and deployment methods). I've removed all this duplication while maintaining similar level of test coverage. New tests use Geth test ledger instead of Quorum one. - Add web3js type conversions methods to minimize impact of poor dynamic typing in this early release of 4.X. - Update API. In 4.X all numeric responses has been converted to BigNum. To keep up with this some fields has been changed to string instead of number when necessary. Add some missing fields as well. - Add `estimateMaxFeePerGas` method for estimating current transaction cost. - Fix invalid `runTransact` response type. - Add test script for checking integration with Alchemy that must be executed manually (it's excluded from CI at the moment) - `geth-alchemy-integration-manual-check.test`. Instructions on how to run it has been added to package README. Closes: hyperledger-cacti#2580 Depends on hyperledger-cacti#2535 Depends on hyperledger-cacti#2578 Signed-off-by: Michal Bajer <[email protected]>
- Update web3js packages from 1.10 to 4.0.3 in `cactus-plugin-ledger-connector-ethereum` and `cactus-test-plugin-ledger-connector-ethereum`. This allows interacting with most recent geth nodes. - Refactor all ethereum tests. Most of the test cases were duplicated multiple times (between different quorum ledger versions test and deployment methods). I've removed all this duplication while maintaining similar level of test coverage. New tests use Geth test ledger instead of Quorum one. - Add web3js type conversions methods to minimize impact of poor dynamic typing in this early release of 4.X. - Update API. In 4.X all numeric responses has been converted to BigNum. To keep up with this some fields has been changed to string instead of number when necessary. Add some missing fields as well. - Add `estimateMaxFeePerGas` method for estimating current transaction cost. - Fix invalid `runTransact` response type. - Add test script for checking integration with Alchemy that must be executed manually (it's excluded from CI at the moment) - `geth-alchemy-integration-manual-check.test`. Instructions on how to run it has been added to package README. Closes: hyperledger-cacti#2580 Depends on hyperledger-cacti#2535 Depends on hyperledger-cacti#2578 Co-authored-by: Peter Somogyvari <[email protected]> Signed-off-by: Michal Bajer <[email protected]> Signed-off-by: Peter Somogyvari <[email protected]>
- Update web3js packages from 1.10 to 4.0.3 in `cactus-plugin-ledger-connector-ethereum` and `cactus-test-plugin-ledger-connector-ethereum`. This allows interacting with most recent geth nodes. - Refactor all ethereum tests. Most of the test cases were duplicated multiple times (between different quorum ledger versions test and deployment methods). I've removed all this duplication while maintaining similar level of test coverage. New tests use Geth test ledger instead of Quorum one. - Add web3js type conversions methods to minimize impact of poor dynamic typing in this early release of 4.X. - Update API. In 4.X all numeric responses has been converted to BigNum. To keep up with this some fields has been changed to string instead of number when necessary. Add some missing fields as well. - Add `estimateMaxFeePerGas` method for estimating current transaction cost. - Fix invalid `runTransact` response type. - Add test script for checking integration with Alchemy that must be executed manually (it's excluded from CI at the moment) - `geth-alchemy-integration-manual-check.test`. Instructions on how to run it has been added to package README. Closes: #2580 Depends on #2535 Depends on #2578 Co-authored-by: Peter Somogyvari <[email protected]> Signed-off-by: Michal Bajer <[email protected]> Signed-off-by: Peter Somogyvari <[email protected]>
- Update web3js packages from 1.10 to 4.0.3 in `cactus-plugin-ledger-connector-ethereum` and `cactus-test-plugin-ledger-connector-ethereum`. This allows interacting with most recent geth nodes. - Refactor all ethereum tests. Most of the test cases were duplicated multiple times (between different quorum ledger versions test and deployment methods). I've removed all this duplication while maintaining similar level of test coverage. New tests use Geth test ledger instead of Quorum one. - Add web3js type conversions methods to minimize impact of poor dynamic typing in this early release of 4.X. - Update API. In 4.X all numeric responses has been converted to BigNum. To keep up with this some fields has been changed to string instead of number when necessary. Add some missing fields as well. - Add `estimateMaxFeePerGas` method for estimating current transaction cost. - Fix invalid `runTransact` response type. - Add test script for checking integration with Alchemy that must be executed manually (it's excluded from CI at the moment) - `geth-alchemy-integration-manual-check.test`. Instructions on how to run it has been added to package README. Closes: hyperledger-cacti#2580 Depends on hyperledger-cacti#2535 Depends on hyperledger-cacti#2578 Co-authored-by: Peter Somogyvari <[email protected]> Signed-off-by: Michal Bajer <[email protected]> Signed-off-by: Peter Somogyvari <[email protected]>
geth-all-in-one
test image for running ethereum tests in mainnet-like environment. Image is based onclient-go:v1.12.0
and uses Clique (PoS). There is one coinbase account with publicly available keys like in other, similar packages in cacti.geth-all-in-one-publish
CI for publishing new images.@hyperledger/cactus-test-geth-ledger
for using new geth ledger container in the tests. The class has been moved out ofcactus-test-tooling
because of conflictingweb3js
versions. Other than that, it's similar to open-ethereum test class.@hyperledger/cactus-test-geth-ledger
. More tests are being developed right now, and should be available in subsequent PRs.Closes: #2577