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 objective Flag And use SuplusFeesCosts for Price Estimates #229

Merged
merged 5 commits into from
May 27, 2022

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented May 25, 2022

This PR adds a new field to SolverConfig to allow configuring Quasimodo for different environments. Specifically, requests to Quasimodo from the solver should use a different objective then when requesting price estimates (when solving, Quasimodo defaults to optimizing with capped surplus, which we don't want for price estimates).

Test Plan

Run the orderbook locally and see the new parameter:

% cargo run -p orderbook -- --price-estimators Quasimodo --log-filter shared::http_solver=trace
...
2022-05-25T13:26:42.718Z TRACE shared::http_solver: request url=https://.../solve?instance_name=2022-05-25_13%3A26%3A42.718259_UTC_Ethereum___Mainnet_1_0&time_limit=2&max_nr_exec_orders=100&use_internal_buffers=false&objective=surplusfeescosts body=...
...

@nlordell nlordell requested a review from a team as a code owner May 25, 2022 13:28
@twalth3r
Copy link

The parameter is called --objective in Quasimodo, not optimize!

@nlordell
Copy link
Contributor Author

Oops 🙈 - fixed.

Copy link

@twalth3r twalth3r left a comment

Choose a reason for hiding this comment

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

Shouldn't it better be objective in all places where you wrote optimize or OptimizeFor?

crates/orderbook/src/main.rs Outdated Show resolved Hide resolved
@@ -38,7 +38,7 @@ use shared::{
},
baseline_solver::BaseTokens,
current_block::current_block_stream,
http_solver::{DefaultHttpSolverApi, SolverConfig},
http_solver::{DefaultHttpSolverApi, Objective, SolverConfig},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change needed here, or is the new parameter part of SolverConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its a new parameter part of SOlverConfig.

Driver creates one with default objective (so nothing specified in the query URL) for solving batches and the orderbook creates on with objective set to surplusfeescosts for price estimates.

@nlordell
Copy link
Contributor Author

Sorry @twalth3r, so many brain-farts. Hopefully should be correct now.

@nlordell nlordell changed the title Add optimize Flag And use SuplusFeesCosts for Price Estimates Add objective Flag And use SuplusFeesCosts for Price Estimates May 25, 2022
Copy link

@twalth3r twalth3r left a comment

Choose a reason for hiding this comment

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

Looks good to me now - thanks @nlordell !

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.

Nice. Lgtm

@nlordell nlordell merged commit 5e2f927 into main May 27, 2022
@nlordell nlordell deleted the solver-optimize-for branch May 27, 2022 07:36
@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants