Check for duplicate queue entries in WebsocketProvider

[wbt]

Description

This is intended to address the issue found in #3568 by detecting the condition causing that issue, and emitting an error instead of sending data to incorrect formatters and callbacks. This error will not be emitted in cases where intended operation can continue because only ids that have already been fully used and deleted from queues are being reused, as the existing behavior is not specifically problematic in that case, though this may lead some bugs to go undiscovered in light testing.

I have marked this as a draft because in limited testing with the example code of #3573, this does not produce helpful output such as the text of the error message; execution just silently stops and it's not clear why. If someone else can do more testing or edits to make that clearer, that would be helpful.

A corresponding change in 2.x might be a good idea.

Fixes #3568

Type of change

I am not sure if this should be considered a bug fix or breaking change. It would only break code in cases where the code is already broken but where an existing issue might not be evident, making the issue more evident.

Checklist:

This is as complete as of initial PR submission as the original submitter will get it:

• I have selected the correct base branch.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• Any dependent changes have been merged and published in downstream modules.
• I ran npm run dtslint with success and extended the tests and types if necessary.
• I ran npm run test:unit with success.
• I ran npm run test:cov and my test cases cover all the lines and branches of the added code.
• I ran npm run build-all and tested the resulting files from dist folder in a browser.
• I have updated the CHANGELOG.md file in the root folder.
• I have tested my code on the live network.
• I have checked the Deploy Preview and it looks correct.

[cgewecke]

@wbt

Tangentially, had a thought that your use case might also be helped by using yarn's selective version resolutions feature. This lets you coerce all instances of a package in the dependency tree to a single version, flat-packed once at the outer level. We use it here in E2E testing with projects that consume Web3 via truffle-contract.

You add the following key to your package.json and install with yarn:

"resolutions": {
  "*/**/web3": "1.2.8",
  "*/**/web3-bzz": "1.2.8",
  "*/**/web3-core-helpers": "1.2.8",
  "*/**/web3-core-method": "1.2.8",
  "*/**/web3-core-promievent": "1.2.8",
  "*/**/web3-core-requestmanager": "1.2.8",
  "*/**/web3-core-subscriptions": "1.2.8",
  "*/**/web3-core": "1.2.8",
  "*/**/web3-eth-abi": "1.2.8",
  "*/**/web3-eth-accounts": "1.2.8",
  "*/**/web3-eth-contract": "1.2.8",
  "*/**/web3-eth-ens": "1.2.8",
  "*/**/web3-eth-iban": "1.2.8",
  "*/**/web3-eth-personal": "1.2.8",
  "*/**/web3-eth": "1.2.8",
  "*/**/web3-net": "1.2.8",
  "*/**/web3-providers-http": "1.2.8",
  "*/**/web3-providers-ipc": "1.2.8",
  "*/**/web3-providers-ws": "1.2.8",
  "*/**/web3-shh": "1.2.8",
  "*/**/web3-utils": "1.2.8"
},

Nested Web3 dependencies will be installed at 1.2.8. You can check this by running yarn list web3 and verifying that only a single version is listed for your project.

Another detail is that @truffle/contract automatically attaches a confirmation handler to each request which fires 24X by default. Setting web3.eth.transactionConfirmationBlocks to a lower number should address this.

Side-note: if you're interested in running the ganache tests locally you can with

npm run test:e2e:ganache

To run against geth w/ websocket tests included (takes ~3 min)

npm run test:e2e:geth:auto

[wbt]

@cgewecke Thanks for the pointers!
I am not quite comfortable coercing truffle packages to use a version of web3 they aren't yet designed to work with, largely due to past experience about the interaction between those projects and due to having spent too many weeks in "dependency hell."

[cgewecke]

largely due to past experience about the interaction between those projects and due to having spent too many weeks in "dependency hell."

Understandable, although the interaction bug you discovered here with multiple versions installed is pretty bad.

FWIW we use resolutions in testing here to inject the latest state of the lib into another project and run their units tests with it. So there's at least some ongoing validation that Web3 works with @truffle/contract. Would not pretend it's exhaustive though.

[wbt]

So there's at least some ongoing validation that Web3 works with @truffle/contract. Would not pretend it's exhaustive though.

That comes as a bit of a surprise, looking at the failure of this PR, where a using project breaks absent changes in the using project following an attempt to upgrade a patch-level web3 change. (The interaction issues are not all old ones!) However, it's nice to hear there's some attempts to continue compatibility checking.

[cgewecke]

@wbt

That PR is not getting past a typescript compilation step on install. Think it's something that could be resolved if they reworked their TS a bit.

We've continued to make sure the core logic doesn't break @truffle/contract since that module is widely used on it's own. We're also trying to make sure that Web3's types changes are non-breaking by testing the latest state against a production TS project (gnosis/dex-react) here.

Lastly, there's a tracking issue at truffle 2688 that documents the main things Truffle would need to address to upgrade Web3.

[github-actions]

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[wbt]

I don't think this should be autoclosed; I think it could benefit from some collaborative effort to get it over the finish line.

[github-actions]

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[wbt]

I still don't think this should be marked as stale.

[github-actions]

This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

[wbt]

I still don't think this should be autoclosed; I think it could benefit from some collaborative effort to get it over the finish line.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[wbt]

I don't remember the CLA requirement being part of the deal when taking over from the Ethereum Foundation.

[GregTheGreek]

Please see #3846 . CLA is not required.

[jdevcs]

This PR is still WIP and there is no activity, so closing . @wbt feel free to reopen this when you have time for contributions. Thanks