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

Risk calculation in solvers crate #1919

Merged
merged 12 commits into from
Oct 12, 2023
Merged

Risk calculation in solvers crate #1919

merged 12 commits into from
Oct 12, 2023

Conversation

sunce86
Copy link
Contributor

@sunce86 sunce86 commented Oct 4, 2023

Description

Implements #1899

Changes

Risk parameters are now part of solvers crate configuration.
Based on risk parameters, success_probability is calculated and forwarded to driver.

This PR is aware of bad gas estimates used. Better gas estimates might come from #1531
Currently, Dex and Baseline and Naive solvers are using these bad gas estimates

Release

Risk parameters need to be defined in the configuration. Risk parameters should be extracted from https://github.com/cowprotocol/services/blob/main/crates/solver/src/solver/risk_computation.rs#L53

IF we want to stop using bad gas estimates and just fallback to success_probability=1, then risk parameters need to be set as [0,0,0,f64::MAX].

@sunce86 sunce86 requested a review from a team as a code owner October 4, 2023 14:44
@sunce86 sunce86 linked an issue Oct 4, 2023 that may be closed by this pull request
Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

while Naive solver returns success_probability=1 since it does not have any gas estimate.

The naive solver always operates on a single liquidity instance, which has a gas estimate attached to it (requires adding the constant settlement overhead). Can we add this logic to the PR (should be quite small)?

crates/solvers/src/infra/config/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

LGTM. Approving assuming:

  • The tests::Config::None variant is removed if it is indeed no longer used
  • @fleupold's comment of getting the gas estimate for the naive solver is implemented (should be doable in the same/similar way to the Baseline solver)

Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

LGTM, just an issue with the gas math imo.

crates/solvers/src/boundary/naive.rs Outdated Show resolved Hide resolved
@sunce86 sunce86 enabled auto-merge (squash) October 12, 2023 07:57
@sunce86 sunce86 merged commit 863e280 into main Oct 12, 2023
@sunce86 sunce86 deleted the risk-calculator-in-solvers branch October 12, 2023 08:18
@github-actions github-actions bot locked and limited conversation to collaborators Oct 12, 2023
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.

Implement/move RiskCalculator to solvers crate
4 participants