Marketplace builder in tests #1903
Conversation
QuentinI
requested review from
nomaxg,
philippecamacho,
ImJeremyHe,
sveitser,
jbearer,
tbro and
imabdulbasit
as code owners
marketplace-builder/src/builder.rs
Outdated
| tracing::info!("Running builder against hotshot events API at {events_api_url}",); | ||
|
|
||
| let stream = | ||
| marketplace_builder_core::utils::EventServiceStream::<SeqTypes, V0_1>::connect( |
There was a problem hiding this comment.
Instead of V0_1, this should take in SequencerApiVersion
| async-compatibility-layer = { workspace = true } | ||
| async-lock = "3.4.0" | ||
| async-lock = "2.2" |
There was a problem hiding this comment.
Why don't we update async-lock to 3.4 in all deps?
There was a problem hiding this comment.
We can, but we should start with HotShot, otherwise we run into incompatibilities
sequencer/src/api.rs
Outdated
| if <V as Versions>::Upgrade::VERSION >= MarketplaceVersion::VERSION { | ||
| println!("Running marketplace builder!"); | ||
| let (task, url) = run_marketplace_builder::<{ NUM_NODES }>( | ||
| cfg.network_config.builder_port(), |
There was a problem hiding this comment.
we can run both builders simultaneously so different ports?
There was a problem hiding this comment.
Yes, for example in case we upgrade from 0.2 -> 0.3
There was a problem hiding this comment.
yeah, but both of these builders are taking the same port i.e cfg.network_config.builder_port(),
There was a problem hiding this comment.
Oh, I misinterpreted your comment. Fixed!
sequencer/src/api.rs
Outdated
| let mut legacy_builder_url = "http://example.com".parse().unwrap(); | ||
| let mut marketplace_builder_url = "http://example.com".parse().unwrap(); | ||
|
|
||
| if <V as Versions>::Base::VERSION <= MarketplaceVersion::VERSION { |
There was a problem hiding this comment.
should this be just <, since we want to get marketplace builder when version == marketplace_version?
There was a problem hiding this comment.
Right, I'll fix that, thanks
QuentinI
changed the title
QuentinI
force-pushed
the
ag/real-builder-in-tests
branch
from
15e5cb0
to
0d7bd83
Compare
| let events_url = hotshot_events_api_url.clone(); | ||
| tracing::info!("Running permissionless builder against hotshot events API at {events_url}",); | ||
| // Start builder event loop | ||
| builder_state.event_loop(); |
There was a problem hiding this comment.
Why do we no longer need the async_spawn(async move { ... }) wrap?
Closes #<ISSUE_NUMBER>
This PR:
TestNetworkMarketplaceVersionThis PR does not:
Key places to review: