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.

Sign up for GitHub

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

Jump to bottom

implement IntervalStream in Clock::stream_slots #281

Closed
wants to merge 4 commits into from
Closed

implement IntervalStream in Clock::stream_slots #281

wants to merge 4 commits into from

Conversation

yash-atreya
Copy link

Hey @ralexstokes, I picked up #215.

Refactored using IntervalStream

ralexstokes
ralexstokes requested changes Oct 9, 2023
View reviewed changes
Copy link
Owner

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

looks good! left some feedback

ethereum-consensus/src/clock.rs Show resolved Hide resolved
ethereum-consensus/src/clock.rs Outdated Show resolved Hide resolved
@ralexstokes ralexstokes mentioned this pull request Oct 9, 2023
Closed
@yash-atreya
Copy link
Author

lets just make an interval using interval_at and add some docs about not having a tick for the current slot (which I think is a fine compromise for now...)

I'll first do this then create a separate PR for the custom stream implementation later?

@yash-atreya
stream_slots using interval_at
a97a1da 
removed async-stream
@ralexstokes
Copy link
Owner

yeah sounds great on moving the custom stream impl to a future PR

ralexstokes
ralexstokes requested changes Oct 10, 2023
View reviewed changes
Copy link
Owner

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

nice updates! a few things to fix and then this should be ready to merge

ethereum-consensus/src/clock.rs Show resolved Hide resolved
ethereum-consensus/src/clock.rs Outdated Show resolved Hide resolved
@ralexstokes
Copy link
Owner

closing in lieu of #385

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ralexstokes ralexstokes ralexstokes requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yash-atreya @ralexstokes