-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[batcher] Cleanup batcher channel inclusion block logic #12363
[batcher] Cleanup batcher channel inclusion block logic #12363
Conversation
dba59bc
to
84ea6a3
Compare
84ea6a3
to
6c4fcb5
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.
I'm on board with the direction of this, but I'm wondering why we need to store the inclusion blocks in the channel
struct at all.
What if isTimedOut
just computes these values on the fly? If they are needed at any other point (I don't think they are), we could have another getter method for them.
966c987
to
a2e3d75
Compare
a2e3d75
to
a1940c3
Compare
2621ef8
to
746b161
Compare
# Conflicts: # op-batcher/batcher/channel.go
Happy to add dynamic getters for them, but note they are used in the logs in |
1ac85ca
…imism#12363) * Cleanup batcher channel inclusion block logic * Add comment to isTimedOut * Remove updateInclusionBlocks altogether * Revert receiver variable rename * Fix tests * Fix isTimedOut for ChannelTimeouts of 1 (ensure that some txs have been confirmed) * Added comment about confirmed txs to isFullySubmitted
Description
This PR removes the
confirmedTxUpdated
bool, instead just callingupdateInclusionBlocks
eagerly rather than lazily from within theisTimedOut()
andisFullySubmitted()
functions. This has no performance implications as those methods are called at this point anyway. TheisTimedOut()
andisFullySubmitted()
functions are getters so it's nice to clean up any struct mutation from these functions.Tests
No logic changes, existing tests should provide coverage. Updated the
TestChannelTimeout
to better test the timeout by calling theTxConfirmed
method.Additional context
This is a small cleanup from #12337 that I wanted to cherry-pick in, even though that PR was closed.