Skip to content

fix: properly calculate deadline, use chrono for calculating timestamps #123

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Evalir
Copy link
Member

@Evalir Evalir commented Jul 2, 2025

We were calculating the deadline by subtracting from an Instant::now(), which we shouldn't do. We now simply subtract the buffer from the remaining time in the slot and add the result to the current instant instead.

This also switches to using chrono for anything regarding timestamps instead of using std, for parity with the slot calculator. Everything else uses Instant or Duration already.

Closes ENG-1167
Closes ENG-1019

Copy link
Member Author

Evalir commented Jul 2, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Evalir Evalir changed the title chore: use chrono, debug fix: properly calculate deadline, use chrono for calculating timestamps Jul 3, 2025
@Evalir Evalir marked this pull request as ready for review July 3, 2025 13:48
@Evalir Evalir requested a review from dylanlott as a code owner July 3, 2025 13:48
@Evalir Evalir requested review from prestwich and rswanson July 3, 2025 13:48
@prestwich prestwich force-pushed the evalir/builder-debug-stuff branch from fa955fe to 9da0142 Compare July 3, 2025 23:50
Copy link
Member Author

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

lgtm, just one nit

.send_transaction(req)
.instrument(span.clone())
.await
.inspect_err(|e| error!(error = %e, "sending transaction"))
Copy link
Member Author

Choose a reason for hiding this comment

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

this was probably written this way because it's implied with the error i assume, but i still think it can be confusing at first glance

Suggested change
.inspect_err(|e| error!(error = %e, "sending transaction"))
.inspect_err(|e| error!(error = %e, "error sending transaction"))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants