Autopilot refactor 4/4: Use domain::Settlement in OnSettlementEventUpdater #2861
Conversation
| return Err(anyhow!("{hash:?}, infra error {err}")) | ||
| } | ||
| Err(err) => { | ||
| tracing::warn!(?hash, ?err, "invalid settlement"); |
There was a problem hiding this comment.
nit: maybe add a todo here that this is where we would trigger the circuit breaker.
Ideally, #2865 is merged and running it for a week in prod would give high confidence we can merge this PR. |
# Description Related to comment #2861 (comment) Attaches `auction_id` to all errors that happen only once the `auction_id` is successfully recovered from calldata. This enables callers of the `Settlement::new` to fetch `auction_id` if it exists. # Changes <!-- List of detailed changes (how the change is accomplished) --> - [ ] Added auction id to `SettlementError` - [ ] Dependent error types refactored to fit nicely into `Settlement` needs
Dependent on #2866 # Description Instead of logging settlement object, it automatically compares old and new implementation and prints if it detects a difference of behaviour. Note that code quality is not of paramount importance here as the code will be removed once testing and done and #2861 lands.
# Description Extracts part of https://github.com/cowprotocol/services/pull/2861/files that can be merged now, and don't have to be part of that PR. This enables me to have a nice pub interface of `Settlement` and continue adding functions for #2862 and not wait for ☝️ PR to be merged. Also #2861 becomes smaller and easier to revert if we encounter some issue on weekly release.
sunce86
changed the title
There was a problem hiding this comment.
Generally the same issues as in the other PRs (similarly named structs without clear logical differentiation nor documentation, opportunity for more though through error handling instead of simply creating new variants), but otherwise looks good.
I'm really looking forward to making use of the new domain for impactful tasks such as listing CoW AMM orders and protocol fee amounts.
| auction_id: domain::auction::Id, | ||
| settlement: Option<&domain::settlement::Settlement>, |
There was a problem hiding this comment.
Bit unclear from just reading the function signature under which circumstances it makes sense to have settlement be None.
I assume this happens in the case where we index a settlement event coming from the other environment so we are not able to find the necessary data in the DB?
In that case does it not make sense to split this function into 2?
One for handling just the event and one for storing the settlement information (in case there is some).
There was a problem hiding this comment.
In that case does it not make sense to split this function into 2
Probably, but then we would have two commit statements, for which we would need to significantly change how the OnSettlementEventUpdater works. It would have to be split in two updates, one update for <auction, settlement> relation and other update that would have to check maybe settlement_observation table if it misses an entry for any of the settlement events etc. Anyway, I think this should be done but outside of scope of this PR.
There was a problem hiding this comment.
I don't have the full context back in my head but is passing the transaction to the second function not an option to get around the 2-commit issue?
There was a problem hiding this comment.
Maybe, but the point of infra is to abstract away specifics of underlying persistence layer and exposing transaction object to the caller is breaking this. I think we already discussed this somewhere whether we should allow it or not (few months ago).
| @@ -96,7 +96,7 @@ impl Settlement { | |||
| #[derive(Debug, thiserror::Error)] | |||
| pub enum Error { | |||
| #[error("failed communication with the database: {0}")] | |||
| Infra(sqlx::Error), | |||
| Infra(anyhow::Error), | |||
There was a problem hiding this comment.
why did this change? Where are we turning a non sqlx error into a database error (since the to_string annotation still mentions databased communication failure)?
I think having an event index that doesn't fit into the right type could be considered InconsistentData (the existing error variant just below)
There was a problem hiding this comment.
DatabaseError(sqlx::Error) was changed to DatabaseError(anyhow::Error) since I wanted to get rid of the error::Settlement and error::SettlementEvent and convert InvalidEvent into DatabaseError, and this was not possible for DatabaseError(sqlx::Error).
Converting DatabaseError(sqlx::Error) to DatabaseError(anyhow::Error) is probably fine since all db errors are treated the same at the callsite.
|
Merging 🤞 |
Dependent on #2875
Description
Actually uses
domain::Settlementobject inOnSettlementEventUpdater.Changes
How to test