This repository was archived by the owner on Mar 5, 2025. It is now read-only.
Fix historical event subscriptions when made as first provider request - #3455
Merged
Conversation
13 tasks
Collaborator
|
@cgewecke had a short look, but I'll leave conflict resolving to you here (after merging the Websocket Improvement PR), not sure on all changes |
holgerd77
previously approved these changes
Apr 6, 2020
| }); | ||
| }); | ||
| }); | ||
|
|
Collaborator
There was a problem hiding this comment.
Nice and clean test setup and procedural structure. Really cool! 👍
| this.options.requestManager.send({ | ||
| method: 'eth_getLogs', | ||
| params: [payload.params[1]] | ||
| params: [blockParams] |
Collaborator
There was a problem hiding this comment.
Had some broader look at surrounding code blocks, should be side-effects free.
Collaborator
|
(@cgewecke can you eventually update other outstanding PRs as well? Think this will make the process easier.) |
holgerd77
approved these changes
Apr 6, 2020
holgerd77
left a comment
holgerd77
left a comment
Collaborator
There was a problem hiding this comment.
Reapproval after merge-conflict resolution.
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Revival of #3401, targeted at 1.x
Resolves a race condition that arises when the first request made by a Websocket provider is a subscription to an event which includes a request for historical logs.
Web3 was deleting the
fromBlockparameter made for the historical logs request while the Websocket connection established itself, resulting in the "missing logs" bug reported in #3389.Commit (#3401) 64c1302 is a test that can be seen failing in Travis here
Commit (#3401) 7f3f42b fixes it by copying the params.
The copied params object looks like....
Fixes #3389
Type of change
Checklist:
npm run dtslintwith success and extended the tests and types if necessary.npm run test:unitwith success.npm run test:covand my test cases do cover all lines and branches of the added code.npm run build-alland tested the resulting file/'s fromdistfolder in a browser.CHANGELOG.mdfile in the root folder.