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.
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(store-sync): apply logs from receipt #3215
base: main
Are you sure you want to change the base?
feat(store-sync): apply logs from receipt #3215
Changes from 16 commits
15cddae
7a7611e
b5ae562
d18bbd2
42169ee
63fcc48
a1d88b8
f9352b9
cbc15b8
7f78d10
580b842
89e7305
2cff36a
ae9b7a3
ad4eb36
7d685e5
2c74e64
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
does the
storageAdapterLock$
logic need to be rxjs based or could this be a boolean variable? i always need extra brain cycles to think through rxjs logicThere 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.
it's an observable because we need to subscribe to it in two places (one here to wait for the lock to free up and one in
waitForTransaction
)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.
Maybe it's because it's late but i've stared at this part of the code for a while and am still not fully sure what's going on 🙈 What is the
concat
of three streams doing, out of which the first and last have aignoreElements
? why is the last streamof(true)
?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.
couldn't we do something like
or
?
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.
Okay this took a bunch of iteration to get here but here's the idea:
The current approach turns an async generator into a stream of blocks, one emission per yield of the generator. I originally had your suggested approach above, but found we were applying optimistic logs for each block rather than each range of blocks, doing a lot more work than is necessary, especially when hydrating from RPC.
Instead what this does is takes the block range stream (
storedBlock$
but could rename for clarity) and adds an operation before and after the stream: before to wait for + take out a lock and after to apply optimistic logs and release the lock. Concatenating arbitrary things to the nice stream ofstoredBlocks$
means we'd have to find/filter them out later, but we can encapsulate that withignoreElements()
which basically empties the stream we added to the start/end and only completes/errors the stream.So it works like this:
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.