Use blocks to determine other environment settlements #3053
Conversation
sunce86
added
the
E:6.2 Time to Happy Moo
There was a problem hiding this comment.
IIUIC, the approach works as far as both prod and staging update the solver_competition table with the same speed. But looks like this is not the case for most of the cases. So, at some point, both staging and prod will work with the same auction_id, and the block number diff will be less than the hardcoded value. Did I miss anything?
I don't anticipate this happening before we fix the auction ids with #2848 |
|
|
||
| /// A supported Ethereum Chain ID. | ||
| #[derive(Clone, Copy, Debug, Eq, PartialEq)] | ||
| pub enum ChainId { |
There was a problem hiding this comment.
Maybe not to do in this PR, but we have logic depending on the chain id scattered all over the place. Maybe it could be unified in one place, and implement all functions related to the chain (e.i. default_amount_to_estimate_native_prices_with())
There was a problem hiding this comment.
I am not against it. Although I think we agreed somewhere that we are fine with some level of code duplication to avoid sharing too much code between autopilot, driver, solvers etc, and to support DDD way of organizing code.
There was a problem hiding this comment.
Yes, but for this particular case I think it comes handy. So, in the future, if we were to support more networks, it is all in one place.
But no need for action in this PR, I just wanted to open the debate. This is something I want to do in the upcoming weeks.
There was a problem hiding this comment.
In that case, I'd suggest opening an issue for this (if we don't have it already). Some people might want to discuss it further. 👍
| @@ -20,7 +20,7 @@ pub struct Addresses { | |||
| } | |||
|
|
|||
| impl Contracts { | |||
| pub async fn new(web3: &DynWeb3, chain: &ChainId, addresses: Addresses) -> Self { | |||
| pub async fn new(web3: &DynWeb3, chain: &domain::eth::chain::Id, addresses: Addresses) -> Self { | |||
There was a problem hiding this comment.
nit: domain::eth::chain seems weird.
A structure like this make more sense to me:
chain
----> id
----> mod
There was a problem hiding this comment.
Hm, not following, how would the path go in your case?
There was a problem hiding this comment.
domain::chain::Id instead of domain::eth::chain::Id.
It doesn't make much sense to me to have the less specific chain as a child of the more specific eth module.
There was a problem hiding this comment.
Hm, to me chain is not that important concept to live directly under domain. Previously it lived under infra::blockchain, maybe we should move it there, so we would have infra::blockchain::Id?
There was a problem hiding this comment.
Moved to infra, probably better now
Description
This PR updates how the
settlement::Observerdistinguishes between settlements from staging and production environments.Staging is usually deployed first, resulting in higher auction IDs, while production lags behind. Since both environments share a settlement contract, they must:
Detecting staging settlements in production is straightforward and handled by the "AuctionNotFound" error, as staging auctions have higher IDs that don't exist in production yet.
However, detecting production settlements in staging has been problematic. Previously, we used the "auction_has_settlement" logic, but this wasn’t reliable. For example, if a staging auction is saved to the database but not settled by solvers, production might settle the same auction after 270 days, leading to incorrect detection.
This PR introduces a new approach: if a settlement refers to a VERY old auction (in terms of block time), it’s classified as from the other environment. While this may also catch settlements that violated auction block deadline, from the same environment, this issue hasn't occurred in practice.
This solution isn’t perfect but is a reasonable improvement for now.
Changes
How to test
Will observe the effects on staging.