-
Notifications
You must be signed in to change notification settings - Fork 31
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
[Marketplace] Builder API adjustments #3493
Conversation
let start = Instant::now(); | ||
|
||
if let Ok(Ok(urls)) = async_timeout( | ||
self.builder_timeout, | ||
self.auction_results_provider | ||
.fetch_auction_result(block_view), | ||
) | ||
.await | ||
{ | ||
let mut futures = Vec::new(); | ||
|
||
for url in urls.urls() { | ||
futures.push(async_timeout( | ||
self.builder_timeout.saturating_sub(start.elapsed()), | ||
async { | ||
let client = crate::builder::v0_3::BuilderClient::new(url); | ||
client.bundle(*block_view).await | ||
}, | ||
)); | ||
} | ||
|
||
let mut bundles = Vec::new(); | ||
|
||
for bundle in join_all(futures).await { | ||
match bundle { | ||
Ok(Ok(b)) => bundles.push(b), | ||
_ => continue, | ||
} | ||
} | ||
|
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.
This section looks good to me
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.
Looks fine to me too, though I think it would be nice to have this logic be a separate function. We can make an issue for that/do it later though.
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.
Yeah, we can do it when implementing this part of the spec:
Query all builders in parallel, have a timeout that is constrained by a max system timeout of 1 second, and a minimum set by 2/3 of the builders, then add 10% additional time for the remaining 1/3 to resolve.
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.
Although I'm not opposed to factoring it out now, won't take long.
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 not particularly familiar with this portion of the code, but as far as I can tell it looks good to me!
Closes #0000
This PR:
AuctionResultsProvider
to transaction task, as it needs to use the builder client, which isn't available inhotshot-types
This PR does not:
Key places to review:
Everything