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

Autopilot refactor 2/3: Build Observation object #2835

Merged
merged 4 commits into from
Jul 25, 2024
Merged

Conversation

sunce86
Copy link
Contributor

@sunce86 sunce86 commented Jul 25, 2024

Description

Ports building observation data which is supposed to be saved to the database.

Equivalent to: https://github.com/cowprotocol/services/blob/main/crates/autopilot/src/on_settlement_event_updater.rs#L243-L260

How to test

Added Observation object printing to OnSettlementEventUpdater

@sunce86 sunce86 self-assigned this Jul 25, 2024
@sunce86 sunce86 requested a review from a team as a code owner July 25, 2024 09:25
@sunce86 sunce86 enabled auto-merge (squash) July 25, 2024 20:44
@sunce86 sunce86 merged commit 4238471 into main Jul 25, 2024
9 of 10 checks passed
@sunce86 sunce86 deleted the observation-pr2 branch July 25, 2024 20:50
@github-actions github-actions bot locked and limited conversation to collaborators Jul 25, 2024
@@ -20,25 +26,41 @@ pub use {auction::Auction, solution::Solution, transaction::Transaction};
#[derive(Debug)]
pub struct Settlement {
solution: Solution,
transaction: Transaction,
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed this. If we are adding the transaction here, can't we get rid of the solution field? It can now be computed on the fly (as long as we also have the domain separator somehow) and we avoid storing redundant information.

}

/// CIP38 score calculation
pub fn score(&self) -> Result<competition::Score, solution::error::Score> {
self.solution.score(&self.auction)
}

/// Returns the observation of the settlement.
pub fn observation(&self) -> Observation {
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 the object that is meant to replace SettlementUpdate? I think we should simply pass the Settlement object and make all those fields getter variables on Settlement

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.

3 participants