[ON HOLD] Build `domain::Settlement` based on `/solve` response #2730

sunce86 · 2024-05-13T22:13:34Z

Edit: blocked by #2839 and #3001

Description

Driver responds to /solve with competition::Solution. competition::Solution contains traded amounts for user trades only (JIT orders are skipped). Therefore, it is expected that converting into settlement::Solutionisn't always 1:1, as the same solution when settled onchain might contain JIT orders..

This is fine, as converting will, (for now) be used solely to calculate the score, for which only user trades are considered anyway.

Changes

Added constructor that builds settlement::Solution based on competition::Solution
Added logging at callsite in autopilot runloop

How to test

I tested locally limit order, partial limit order, multiple protocol fees order. Every time, the autopilot calculated score was identical to driver provided score.
However, since this is crucial part of the competition, I've added a log to Runloop that will detect differences. I want to leave this running for a week in production to make sure no regression bugs are introduced, before continuing with #2720

crates/autopilot/src/domain/settlement/mod.rs

sunce86 · 2024-05-14T07:49:48Z

crates/autopilot/src/run_loop.rs

@@ -147,6 +147,42 @@ impl RunLoop {
                }
            };

+            {
+                // Also calculate the score from the solution itself


This is a temporary code that will be replaced when we switch to using autopilot calculated score. So did not spend too much time polishing it. It's used to detect differences in driver/autopilot score calculations.

sunce86 · 2024-05-14T14:19:25Z

crates/autopilot/src/domain/settlement/mod.rs

+    ///
+    /// Since `competition::Solution` does not contain JIT orders, the resulting
+    /// settlement might be a subset of it's onchain observable twin.
+    pub fn from_solution(


Q: would you prefer this functionality to be part of the competition::Solution constructor and used solely for Score calculation? This way it would be prevented from reuse, as I am afraid it's not clear enough that this constructor might build Settlement object that doesn't have JIT orders = meaning it's potentially different from it's onchain settled brother.

What's the difference between Settlement and Solution conceptually? Are we able to compute the score on a Solution or do we need a Settlement? I think we should clarify this nomenclature more.

Solution should represent a solver's response to /solve request, in whatever form the autopilot requests this answer to be (so some cow protocol API format of data).

Settlement should represent the decoded tx calldata solely (and, as an exception, it can contain data that is deductible from calldata, like it has OrderUid right now). All other data that would be needed to calculate something on this struct is passed through arguments (example). The reason is reusability and higher decoupling of onchain/offchain data.

With that in mind, creating Settlement from Solution is cheating since Solution doesn't contain all data Settlement would contain, BUT it contains enough data to calculate the score. So, my point is that we should consider not impementing this conversion as a public method on Settlement, but rather enclose it in a Solution constructor to calculate the score and that's it.

fleupold

I would really appreciate some more clarity/consensus around the different data types and their semants:

Settlement
Solutions
Transaction
...etc

Otherwise I feel like we are entering a world of pain and confusion.

fleupold · 2024-05-14T18:38:10Z

crates/autopilot/src/domain/settlement/mod.rs

+    ///
+    /// Since `competition::Solution` does not contain JIT orders, the resulting
+    /// settlement might be a subset of it's onchain observable twin.
+    pub fn from_solution(


What's the difference between Settlement and Solution conceptually? Are we able to compute the score on a Solution or do we need a Settlement? I think we should clarify this nomenclature more.

crates/autopilot/src/domain/settlement/mod.rs

fleupold · 2024-05-14T18:43:47Z

crates/autopilot/src/domain/settlement/mod.rs

+    #[derive(Debug, thiserror::Error)]
+    pub enum Encoding {
+        #[error("unable to decode settlement calldata: {0}")]
+        Decoding(#[from] web3::ethabi::Error),
+        #[error("unable to tokenize calldata into expected format: {0}")]
+        Tokenizing(#[from] ethcontract::tokens::Error),
+        #[error("unable to recover order uid: {0}")]
+        OrderUidRecover(#[from] tokenized::Error),
+    }


Do we use this type for anything else than the blanket from implementations?

I don't think those #[from]s are generally very good to use (they are convenient but bad), since now whenever you run into a tokenized::Error it will auto converted into OrderUidRecover even if let's say you are trying to parse the AuctionId at the end of the calldata. At the risk of writing 3 more lines of code, I think each callsite should decide which error type to throw (instead of making ? work everywhere)

I believe this was refactored as part of #2746, so [from] is removed for OrderUidRecover variant.

fleupold · 2024-05-14T18:46:36Z

crates/autopilot/src/domain/settlement/mod.rs

+        auction: &auction::Auction,
+        auction_id: auction::Id,


We have a struct called AuctionWithId which may be appropriate here.

True, but it would require unnecessary copy of the whole Auction object. I can do it if you think it's worth it.

True, but it would require unnecessary copy of the whole Auction object.

Really? Why does the auction exist without its ID in the runloop? Can we not move it into type AuctionWithId and then pass a reference to that around? This shouldn't require a clone.

As soon as an auction has an ID why do we still pass it around without?

I believe we can just adjust the code in the run_loop that eagerly destructures the AuctionWithId to get around the clone.

crates/autopilot/src/run_loop.rs

crates/autopilot/src/domain/settlement/mod.rs

crates/autopilot/src/run_loop.rs

MartinquaXD

Comments have been addressed.

github-actions · 2024-05-29T00:07:40Z

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.

sunce86 · 2024-07-11T12:13:44Z

Reopening as I think it will be needed for autopilot refactor.

github-actions · 2024-07-19T00:07:06Z

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.

fleupold

From the description:

This is fine, as converting will, (for now) be used solely to calculate the score, for which only user trades are considered anyway.

With the imminent launch of CoW AMMs this is no longer true, is it? Is this PR still supposed to be merged as is (we should update the description the reflect the most recent considerations though)?

Generally the code looks good imo.

fleupold · 2024-07-22T09:45:47Z

crates/autopilot/src/domain/settlement/solution/mod.rs

+    /// Since `competition::Solution` does not contain JIT orders, the resulting
+    /// `settlement::Solution` might be a subset of it's onchain observable
+    /// twin.
+    pub fn from_competition_solution(


nit:

Suggested change

pub fn from_competition_solution(

pub fn from_competition(

fleupold · 2024-07-22T09:50:28Z

crates/autopilot/src/domain/settlement/solution/mod.rs

+                        // settlement contract uses this formula to convert executed amounts:
+                        // SELL order: executedBuyAmount = executed * sellPrice / buyPrice;
+                        // BUY order: executedSellAmount = executed * buyPrice / sellPrice;
+                        //
+                        // With an example of converting 1 GNO for 100 DAI:
+                        // SELL order: executedBuyAmount = 1 * 100 / 1 = 100 =>
+                        // (sellPrice = 100, buyPrice = 1)
+                        // BUY order: executedSellAmount = 100 * 1 / 100 = 1 =>
+                        // (sellPrice = 100, buyPrice = 1)


nanonit: if it was for me you could remove this comment (I think it's quite clear that the exchange rate can be represented with the effective buy/sell amounts)

sunce86 · 2024-07-25T21:21:25Z

From the description:

This is fine, as converting will, (for now) be used solely to calculate the score, for which only user trades are considered anyway.

With the imminent launch of CoW AMMs this is no longer true, is it? Is this PR still supposed to be merged as is (we should update the description the reflect the most recent considerations though)?

Generally the code looks good imo.

We discussed this here. Copy pasting the text to be visible here as well:

Now that cow amm orders are surplus capturing, they are also part of the /solve response, but autopilot doesn't know all necessary data for these orders to build the settlement::Solution and consequently Score AT COMPETITION TIME. The solution would be to expand /solve response with sell_token, sell_amount, but_token, buy_amount, side. With that, score could be calculated.

So, the conclusion is that this PR can't be merged until /solve response is expanded with additional jit order data. Will create issue.

github-actions · 2024-08-02T00:07:08Z

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 · 2024-08-10T00:07:12Z

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 · 2024-08-20T00:07:29Z

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 · 2024-08-30T00:07:41Z

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.

fleupold · 2024-09-12T14:39:15Z

Can this be closed?

sunce86 · 2024-09-18T10:32:31Z

Can this be closed?

Not yet. Waiting for #3001 to land and then we can add this logic that is crucial for calculating score on autopilot side.

# Description A follow up for #2984 where we added a new format of /solve response, after which there were two ways to submit /solve response. This PR deprecates the old way and uses only the new format and adjusts the autopilot domain code to use it (which is needed so we can finally merge #2730 Will not merge it until all solvers switch to new format.

github-actions · 2024-09-26T00:07:51Z

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.

sunce86 added 9 commits May 10, 2024 15:04

initial commit

33a1e57

error handling

c43192d

add logging

c7bf825

add comment

b31c69b

Merge branch 'main' into from-solution-settlement

535d713

calculate score instead of surplus

f7700f2

update

72d7d25

Merge branch 'main' into from-solution-settlement

3400293

reuse competition::Solution

4354517

sunce86 added the E:7.1 Ext. solvers operating driver See https://github.com/cowprotocol/pm/issues/57 for details label May 13, 2024

sunce86 self-assigned this May 13, 2024

sunce86 requested a review from a team as a code owner May 13, 2024 22:13

sunce86 mentioned this pull request May 13, 2024

Add score calculation in domain::Settlement #2719

Closed

fix comments

219a1e3

m-lord-renkse approved these changes May 14, 2024

View reviewed changes

crates/autopilot/src/domain/settlement/mod.rs Outdated Show resolved Hide resolved

crates/autopilot/src/domain/settlement/mod.rs Outdated Show resolved Hide resolved

sunce86 commented May 14, 2024

View reviewed changes

crates/autopilot/src/domain/settlement/mod.rs Outdated Show resolved Hide resolved

sunce86 commented May 14, 2024

View reviewed changes

squadgazzz approved these changes May 14, 2024

View reviewed changes

fleupold reviewed May 14, 2024

View reviewed changes

sunce86 added 3 commits May 15, 2024 11:13

fix bug for buy orders

4afdda3

small cr fix

aba9925

Merge branch 'main' into from-solution-settlement

e1bb5da

MartinquaXD reviewed May 16, 2024

View reviewed changes

crates/autopilot/src/domain/settlement/mod.rs Outdated Show resolved Hide resolved

crates/autopilot/src/domain/settlement/mod.rs Outdated Show resolved Hide resolved

crates/autopilot/src/run_loop.rs Outdated Show resolved Hide resolved

Merge branch 'main' into from-solution-settlement

02ed7ba

MartinquaXD approved these changes May 21, 2024

View reviewed changes

github-actions bot added the stale label May 29, 2024

github-actions bot closed this Jun 6, 2024

sunce86 reopened this Jul 11, 2024

github-actions bot added the stale label Jul 19, 2024

sunce86 removed the stale label Jul 19, 2024

sunce86 added 2 commits July 19, 2024 09:49

Merge branch 'main' into from-solution-settlement

74aee00

small fixes due to merged code

8f6b35f

fleupold reviewed Jul 22, 2024

View reviewed changes

sunce86 mentioned this pull request Jul 25, 2024

feat: Add surplus capturing JIT order data to /solve response #2839

Closed

sunce86 changed the title ~~Build domain::Settlement based on /solve response~~ [ON HOLD] ]Build domain::Settlement based on /solve response Jul 25, 2024

sunce86 changed the title ~~[ON HOLD] ]Build domain::Settlement based on /solve response~~ [ON HOLD] Build domain::Settlement based on /solve response Jul 25, 2024

github-actions bot added the stale label Aug 2, 2024

sunce86 removed the stale label Aug 2, 2024

github-actions bot added the stale label Aug 10, 2024

sunce86 removed the stale label Aug 12, 2024

github-actions bot added the stale label Aug 20, 2024

sunce86 removed the stale label Aug 22, 2024

github-actions bot added the stale label Aug 30, 2024

sunce86 removed the stale label Sep 6, 2024

sunce86 mentioned this pull request Sep 17, 2024

Use only new format of solve response #3001

Merged

github-actions bot added the stale label Sep 26, 2024

github-actions bot closed this Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ON HOLD] Build `domain::Settlement` based on `/solve` response #2730

[ON HOLD] Build `domain::Settlement` based on `/solve` response #2730

sunce86 commented May 13, 2024 •

edited

Loading

sunce86 May 14, 2024 •

edited

Loading

sunce86 May 14, 2024

fleupold May 14, 2024

sunce86 May 15, 2024 •

edited

Loading

sunce86 May 15, 2024

fleupold left a comment

fleupold May 14, 2024

fleupold May 14, 2024

sunce86 May 20, 2024 •

edited

Loading

fleupold May 14, 2024

sunce86 May 15, 2024

fleupold May 15, 2024

MartinquaXD May 16, 2024

MartinquaXD left a comment

github-actions bot commented May 29, 2024

sunce86 commented Jul 11, 2024

github-actions bot commented Jul 19, 2024

fleupold left a comment

fleupold Jul 22, 2024

fleupold Jul 22, 2024

sunce86 commented Jul 25, 2024 •

edited

Loading

github-actions bot commented Aug 2, 2024

github-actions bot commented Aug 10, 2024

github-actions bot commented Aug 20, 2024

github-actions bot commented Aug 30, 2024

fleupold commented Sep 12, 2024

sunce86 commented Sep 18, 2024

github-actions bot commented Sep 26, 2024

[ON HOLD] Build domain::Settlement based on /solve response #2730

[ON HOLD] Build domain::Settlement based on /solve response #2730

Conversation

sunce86 commented May 13, 2024 • edited Loading

Description

Changes

How to test

sunce86 May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunce86 May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunce86 May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinquaXD left a comment

Choose a reason for hiding this comment

github-actions bot commented May 29, 2024

sunce86 commented Jul 11, 2024

github-actions bot commented Jul 19, 2024

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunce86 commented Jul 25, 2024 • edited Loading

github-actions bot commented Aug 2, 2024

github-actions bot commented Aug 10, 2024

github-actions bot commented Aug 20, 2024

github-actions bot commented Aug 30, 2024

fleupold commented Sep 12, 2024

sunce86 commented Sep 18, 2024

github-actions bot commented Sep 26, 2024

[ON HOLD] Build `domain::Settlement` based on `/solve` response #2730

[ON HOLD] Build `domain::Settlement` based on `/solve` response #2730

sunce86 commented May 13, 2024 •

edited

Loading

sunce86 May 14, 2024 •

edited

Loading

sunce86 May 15, 2024 •

edited

Loading

sunce86 May 20, 2024 •

edited

Loading

sunce86 commented Jul 25, 2024 •

edited

Loading