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(cactus-plugin-ledger-connector-ethereum): update web3js to 4.X #2581

Merged

Conversation

outSH
Copy link
Contributor

@outSH outSH commented Jul 25, 2023

  • 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 https://github.com/hyperledger/cacti/issues/2580
Closes https://github.com/hyperledger/cacti/issues/2656
Closes https://github.com/hyperledger/cacti/issues/2594

Depends on https://github.com/hyperledger/cacti/pull/2535
Depends on https://github.com/hyperledger/cacti/pull/2578

Signed-off-by: Michal Bajer michal.bajer@fujitsu.com

@outSH
Copy link
Contributor Author

outSH commented Jul 25, 2023

Please review only the latest commit (others are dependencies).

Note to myself: Update references to geth image once it's available, remove outsh from cspell ignore file.

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
outSH added a commit to outSH/cactus that referenced this pull request Aug 21, 2023
…s prices

- Add legacy and EIP1559 gas configuration options to transaction requests.
- Legacy gas configuration is updated to EIP1559 using the same logic as web3 libraries.
- Update the tests to work with new API.
- Added test suite to test new features - `geth-transact-and-gas-fees.test.ts`

Depends on: hyperledger-cacti#2581

Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
@github-actions
Copy link

This PR/issue depends on:

  • hyperledger/cacti#2535
  • hyperledger/cacti#2578
    By Dependent Issues (🤖). Happy coding!

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.

@outSH The cactus-plugin-ledger-connector-ethereum job is failing, could you please check on it?

Attaching the logs in case they expire in the meantime on the backend:
2023-09-18T12-42-58-cacti-ci-test-connector-ethereum-crash.log

@outSH
Copy link
Contributor Author

outSH commented Sep 18, 2023

@outSH The cactus-plugin-ledger-connector-ethereum job is failing, could you please check on it?

Attaching the logs in case they expire in the meantime on the backend: 2023-09-18T12-42-58-cacti-ci-test-connector-ethereum-crash.log

Yes, I will request a review once this PR is ready for merge, didn't address previous comments yet as well

@outSH outSH force-pushed the ethereum_connector_web3_bump_pr branch from ea65c2d to a7f6a99 Compare September 19, 2023 12:29
@outSH
Copy link
Contributor Author

outSH commented Sep 19, 2023

@outSH The cactus-plugin-ledger-connector-ethereum job is failing, could you please check on it?

Attaching the logs in case they expire in the meantime on the backend: 2023-09-18T12-42-58-cacti-ci-test-connector-ethereum-crash.log

@petermetz Both test suites for this connector should be passing from now on. Sorry for breaking the CI with previous PR, I wasn't checking the test results because I kinda assumed you're doing it anyway :) I should have make sure they pass before even requesting a review.

I've also fixed https://github.com/hyperledger/cacti/issues/2594 while working on cactus-test-plugin-ledger-connector-ethereum test. There was a race condition - web3js socket was being disconnected before watchBlock eth_unsubscribe request was sent. Ledger didn't know it was OK and was closing the connection again, causing unhandled error.

Now, the shutdown method will first unsubscribe all remaining watchBlocks (in case the client was not disconnected yet), wait for all pending requests to pass and then disconnect the provider. To see request queue I had to access protected field of WS Provider - I will create an issue on web3js github to improve this (either handle it on web3js side or export some public field like queueLength). I hope this is acceptable for now and we can improve on that on future PRs.

One more change I did was adding wsApi.disconnectSockets(true) to ApiServer shutdown. Reasoning: Socketio connections should be closed before shuting down the plugins (we don't want to handle new async requests while working on clearing up resources). This could possibly be extended to HTTP too, that is: stop connections, stop the servers, don't accept new connection and only then do plugin shutdowns, but I didn't want to introduce such change in this PR (closing of connections is more urgent).

BTW I wanted to include closing of socketio connections in the connector, but I can't do that since in theory single connection can be used to access multiple plugins (they are api-server responsibility, not connectors). We could use socketio rooms to partition the sockets and move overnship of the active connection to the connector, but I'm not sure if it's beneficial given our time contraint.

@petermetz
Copy link
Contributor

I kinda assumed you're doing it anyway :)

@outSH Hehe, I usually do, rigorously, but with the upcoming v2.0.0 release I've been able to spend a little less time on these things as well TBH. I'm glad you noticed though, thank you again! :-)

I've also fixed https://github.com/hyperledger/cacti/issues/2594 while working on cactus-test-plugin-ledger-connector-ethereum test. There was a race condition - web3js socket was being disconnected before watchBlock eth_unsubscribe request was sent. Ledger didn't know it was OK and was closing the connection again, causing unhandled error.

Now, the shutdown method will first unsubscribe all remaining watchBlocks (in case the client was not disconnected yet), wait for all pending requests to pass and then disconnect the provider. To see request queue I had to access protected field of WS Provider - I will create an issue on web3js github to improve this (either handle it on web3js side or export some public field like queueLength). I hope this is acceptable for now and we can improve on that on future PRs.

Sounds good to me, thank you so much for going the extra mile and investigating the race condition! It makes things easier down the line! It's a reasonable trade-off IMO as well and hopefully web3js will provide some method for us to use that makes this trivial to implement.

One more change I did was adding wsApi.disconnectSockets(true) to ApiServer shutdown. Reasoning: Socketio connections should be closed before shuting down the plugins (we don't want to handle new async requests while working on clearing up resources). This could possibly be extended to HTTP too, that is: stop connections, stop the servers, don't accept new connection and only then do plugin shutdowns, but I didn't want to introduce such change in this PR (closing of connections is more urgent).

LGTM on this too, we can refactor later if it's no longer needed! Thank you!

BTW I wanted to include closing of socketio connections in the connector, but I can't do that since in theory single connection can be used to access multiple plugins (they are api-server responsibility, not connectors). We could use socketio rooms to partition the sockets and move overnship of the active connection to the connector, but I'm not sure if it's beneficial given our time contraint.

Let's talk more about this in an issue that I'll try to open (and not forget)!

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.

LGTM

@outSH outSH force-pushed the ethereum_connector_web3_bump_pr branch from a7f6a99 to 0dd877b Compare September 22, 2023 08:30
Copy link
Contributor

@izuru0 izuru0 left a comment

Choose a reason for hiding this comment

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

LGTM

@petermetz petermetz enabled auto-merge (rebase) September 27, 2023 19:58
- 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 <peter.somogyvari@accenture.com>

Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
@petermetz petermetz force-pushed the ethereum_connector_web3_bump_pr branch from 0dd877b to 39408c9 Compare September 29, 2023 20:38
@petermetz petermetz merged commit 55f82c9 into hyperledger-cacti:main Sep 29, 2023
42 of 67 checks passed
outSH added a commit to outSH/cactus that referenced this pull request Oct 2, 2023
…s prices

- Add legacy and EIP1559 gas configuration options to transaction requests.
- Legacy gas configuration is updated to EIP1559 using the same logic as web3 libraries.
- Update the tests to work with new API.
- Added test suite to test new features - `geth-transact-and-gas-fees.test.ts`

Depends on: hyperledger-cacti#2581

Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
outSH added a commit to outSH/cactus that referenced this pull request Oct 2, 2023
…s prices

- Add legacy and EIP1559 gas configuration options to transaction requests.
- Legacy gas configuration is updated to EIP1559 using the same logic as web3 libraries.
- Update the tests to work with new API.
- Added test suite to test new features - `geth-transact-and-gas-fees.test.ts`

Depends on: hyperledger-cacti#2581

Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
petermetz pushed a commit to outSH/cactus that referenced this pull request Oct 2, 2023
…s prices

- Add legacy and EIP1559 gas configuration options to transaction requests.
- Legacy gas configuration is updated to EIP1559 using the same logic as web3 libraries.
- Update the tests to work with new API.
- Added test suite to test new features - `geth-transact-and-gas-fees.test.ts`

Depends on: hyperledger-cacti#2581

Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
sandeepnRES pushed a commit to sandeepnRES/cacti that referenced this pull request Dec 21, 2023
…s prices

- Add legacy and EIP1559 gas configuration options to transaction requests.
- Legacy gas configuration is updated to EIP1559 using the same logic as web3 libraries.
- Update the tests to work with new API.
- Added test suite to test new features - `geth-transact-and-gas-fees.test.ts`

Depends on: hyperledger-cacti#2581

Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants