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

Pass Marketplaceconfig to HotShot #1805

Merged
merged 13 commits into from
Aug 8, 2024
Merged

Pass Marketplaceconfig to HotShot #1805

merged 13 commits into from
Aug 8, 2024

Conversation

tbro
Copy link
Contributor

@tbro tbro commented Jul 31, 2024 •
edited
Loading

Pass Marketplaceconfig to HotShot

tbro
tbro commented Jul 31, 2024
View reviewed changes
types/src/v0/impls/auction.rs Outdated Show resolved Hide resolved
@tbro tbro force-pushed the tb/marketplace/add-builder-url-to-auction-results branch from 8a27973 to 7e68405 Compare August 1, 2024 01:38
@tbro tbro changed the title add builder to urls returned from SolverAuctionResults::urls() Instanciate SolverAuctionResultsProvider w/ `solver_url Aug 1, 2024
@tbro tbro marked this pull request as ready for review August 1, 2024 01:39
tbro
tbro commented Aug 1, 2024
View reviewed changes
builder/src/permissioned.rs Outdated Show resolved Hide resolved
@tbro tbro force-pushed the tb/marketplace/add-builder-url-to-auction-results branch from 0cfa920 to 7a777f8 Compare August 1, 2024 12:22
tbro
tbro commented Aug 1, 2024
View reviewed changes
types/src/v0/impls/auction.rs Outdated Show resolved Hide resolved
@tbro tbro force-pushed the tb/marketplace/add-builder-url-to-auction-results branch from 24b353e to 2de8730 Compare August 2, 2024 16:51
@tbro tbro changed the title Instanciate SolverAuctionResultsProvider w/ `solver_url Pass Marketplaceconfig to HotShot Aug 2, 2024
@tbro tbro force-pushed the tb/marketplace/add-builder-url-to-auction-results branch from 2de8730 to 367bcab Compare August 2, 2024 16:53
tbro added 5 commits August 7, 2024 18:01
@tbro
Pass MarketplaceConfig to hotshot
8e1cbed 
  * Instanciate SolverAuctionResultsProvider with `solver_url`
  * Use Base version (Instead of hard coding)
  * update hotshot and query-service
@tbro
lock
9f1c0c4
@tbro
fix
4ca4f2d
@tbro
lint
f278ad4
@tbro
add some generics
3f1687a 
Broken
@tbro tbro force-pushed the tb/marketplace/add-builder-url-to-auction-results branch from e3e6d27 to 3f1687a Compare August 8, 2024 07:45
@tbro tbro requested a review from ss-es August 8, 2024 09:26
QuentinI
QuentinI reviewed Aug 8, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated
hotshot-stake-table = { git = "https://github.com/EspressoSystems/hotshot", tag = "0.5.68" }
hotshot-builder-api = { git = "https://github.com/EspressoSystems/HotShot.git", tag = "rc-0.5.70" }
hotshot-builder-core = { git = "https://github.com/EspressoSystems/hotshot-builder-core", tag = "rc-0.1.40" }
marketplace-builder-core = { git = "https://github.com/EspressoSystems/marketplace-builder-core", rev = "c1e60e76893557f21e8aea7e86994ebe107b4914" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that what 0.0.2 points to anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where that got pulled in from, possibly an artifact of conflict resolution. But I'll check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed c5bb984

builder/src/permissioned.rs Outdated
@@ -384,7 +386,13 @@ pub async fn init_hotshot<
.unwrap(),
ConsensusMetricsValue::new(metrics),
da_storage,
TestAuctionResultsProvider::default(),
// TODO if this builder is to be used for marketplace, we need real urls.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not, we can just pass bogus config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed c5bb984

sequencer/src/options.rs
/// URL of generic builder
#[clap(
long,
env = "ESPRESSO_GENERIC_BUILDER_URL",
Copy link
Contributor

Choose a reason for hiding this comment

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

We have renamed generic builder to fallback builder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is used for MarketplaceConfig I would prefer to wait until the name gets changed there. Which I don't believe has happened yet.

sequencer/src/options.rs Outdated
#[clap(
long,
env = "ESPRESSO_AUCTION_RESULTS_SOLVER_URL",
default_value = "http://localhost:8083"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please make it 25000, the same as default solver port in .env

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed c5bb984

sequencer/src/options.rs Outdated
#[clap(
long,
env = "ESPRESSO_GENERIC_BUILDER_URL",
default_value = "http://localhost:8083"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as solver, can you please make it 31004 to conform to .env?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed c5bb984

tbro and others added 2 commits August 8, 2024 20:11
@tbro @QuentinI
Update Cargo.toml
66eb062 
Co-authored-by: Artemii Gerasimovich <[email protected]>
@tbro
fixes for review
c5bb984 
Respond to comments

  * set default ports to `.env` values
  * remove TODO
  * pin marketplace-builder to tag
  * remove as yet unused cli params
@tbro tbro requested a review from QuentinI August 8, 2024 17:21
imabdulbasit
imabdulbasit reviewed Aug 8, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
imabdulbasit
imabdulbasit reviewed Aug 8, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
imabdulbasit
imabdulbasit reviewed Aug 8, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
imabdulbasit
imabdulbasit reviewed Aug 8, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
imabdulbasit
imabdulbasit reviewed Aug 8, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
imabdulbasit
imabdulbasit reviewed Aug 8, 2024
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@tbro
marketplace-builder fix
74899c6 
Fix conflict, resolution errors. Add marketplace-builder back in, and
update marketplace-builder-core version
@tbro tbro merged commit 6c25482 into main Aug 8, 2024
15 checks passed
@tbro tbro deleted the tb/marketplace/add-builder-url-to-auction-results branch August 8, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elliedavidson elliedavidson elliedavidson left review comments

@imabdulbasit imabdulbasit imabdulbasit approved these changes

@nomaxg nomaxg Awaiting requested review from nomaxg

@philippecamacho philippecamacho Awaiting requested review from philippecamacho philippecamacho is a code owner

@ImJeremyHe ImJeremyHe Awaiting requested review from ImJeremyHe ImJeremyHe is a code owner

@sveitser sveitser Awaiting requested review from sveitser sveitser is a code owner

@jbearer jbearer Awaiting requested review from jbearer jbearer is a code owner

@ss-es ss-es Awaiting requested review from ss-es

@QuentinI QuentinI Awaiting requested review from QuentinI

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

Successfully merging this pull request may close these issues.
4 participants
@tbro @QuentinI @imabdulbasit @elliedavidson