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.

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

Add driver submission address to the autopilot #3065

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

Conversation

m-lord-renkse
Copy link
Contributor

@m-lord-renkse m-lord-renkse commented Oct 17, 2024

Description

This PR should solve: #3045 and #2780

Changes

  • Add driver submission address to the autopilot
  • Do not request /solve to drivers which have been deny listed
  • Filter the results by the driver submission address

How to test

  1. Regression test (e2e modified to use the new config)
  2. e2e test to check the filtering works

Related Issues

Fixes #3045
Fixes #2780

@m-lord-renkse m-lord-renkse force-pushed the 3045/add-submission-address-autopilot branch 6 times, most recently from b52a744 to 42bf53a Compare October 17, 2024 12:00
@m-lord-renkse m-lord-renkse marked this pull request as ready for review October 17, 2024 12:06
@m-lord-renkse m-lord-renkse requested a review from a team as a code owner October 17, 2024 12:06
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

The code to make things optional is pretty convoluted. Let's make it required from the start.
Would also be good to prepare the infra PR in the meantime.

crates/autopilot/src/run_loop.rs Outdated Show resolved Hide resolved
crates/autopilot/src/run_loop.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse force-pushed the 3045/add-submission-address-autopilot branch 11 times, most recently from 4910d52 to df34e7e Compare October 17, 2024 16:18
Copy link
Contributor

@sunce86 sunce86 left a comment

Choose a reason for hiding this comment

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

Can we also add a manual step (to the tutorial for solver submission keys rotation in notion) to

  1. Update the infra configuration for autopilot
  2. To make sure the solver switched their submission to use new account

crates/autopilot/src/run_loop.rs Outdated Show resolved Hide resolved
crates/autopilot/src/arguments.rs Outdated Show resolved Hide resolved
crates/autopilot/src/infra/solvers/mod.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse force-pushed the 3045/add-submission-address-autopilot branch from ff9095c to ef08450 Compare December 11, 2024 13:13
Copy link

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.

Copy link
Contributor

@mstrug mstrug left a comment

Choose a reason for hiding this comment

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

Only one change proposal.

crates/autopilot/src/infra/solvers/mod.rs Outdated Show resolved Hide resolved
Comment on lines +551 to +570
let drivers_futures = args
.drivers
.into_iter()
.map(|driver| async move {
infra::Driver::try_new(
driver.url,
driver.name.clone(),
driver.fairness_threshold.map(Into::into),
driver.submission_account,
)
.await
.map(Arc::new)
.expect("failed to load solver configuration")
})
.collect::<Vec<_>>();

let drivers = futures::future::join_all(drivers_futures)
.await
.into_iter()
.collect();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let drivers_futures = args
.drivers
.into_iter()
.map(|driver| async move {
infra::Driver::try_new(
driver.url,
driver.name.clone(),
driver.fairness_threshold.map(Into::into),
driver.submission_account,
)
.await
.map(Arc::new)
.expect("failed to load solver configuration")
})
.collect::<Vec<_>>();
let drivers = futures::future::join_all(drivers_futures)
.await
.into_iter()
.collect();
let drivers_futures = args
.drivers
.into_iter()
.map(|driver| async move {
infra::Driver::try_new(
driver.url,
driver.name.clone(),
driver.fairness_threshold.map(Into::into),
driver.submission_account,
)
.await
.map(Arc::new)
})
.collect::<Vec<_>>();
let drivers = futures::future::try_join_all(drivers_futures)
.await
.expect("failed to load solver configuration");

Copy link

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.

@github-actions github-actions bot added the stale label Jan 14, 2025
@sunce86 sunce86 removed the stale label Jan 14, 2025
@sunce86
Copy link
Contributor

sunce86 commented Jan 17, 2025

PR is ready to be merged. Sepolia swap passed successfully.

Just a few more nits in infra PR and we can merge them both at the same time.

Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

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

LGTM

PR is ready to be merged. Sepolia swap passed successfully.
Just a few more nits in infra PR and we can merge them both at the same time.

Could you link the infra PR explicitly in the description somewhere and also change join_all to try_join_all, as was suggested above?

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