Fix solver competition reading #3171
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sunce86
commented
Dec 18, 2024
| FROM solver_competitions sc | ||
| -- outer joins because the data might not have been indexed yet | ||
| LEFT OUTER JOIN settlements s ON sc.id = s.auction_id | ||
| -- exclude settlements from another environment for which observation is guaranteed to not exist | ||
| LEFT OUTER JOIN settlement_observations so |
There was a problem hiding this comment.
LEFT OUTER JOIN makes sure there is no change in logic whether solver competition entry is returned or not
sunce86
commented
Dec 18, 2024
| @@ -38,10 +38,14 @@ pub async fn load_by_id( | |||
| id: AuctionId, | |||
| ) -> Result<Option<LoadCompetition>, sqlx::Error> { | |||
| const QUERY: &str = r#" | |||
| SELECT sc.json, sc.id, COALESCE(ARRAY_AGG(s.tx_hash) FILTER (WHERE s.tx_hash IS NOT NULL), '{}') AS tx_hashes | |||
| SELECT sc.json, sc.id, COALESCE(ARRAY_AGG(s.tx_hash) FILTER (WHERE so.block_number IS NOT NULL), '{}') AS tx_hashes | |||
There was a problem hiding this comment.
Return transaction hash only if settlement_observation entry exists.
sunce86
commented
Dec 18, 2024
| @@ -72,11 +80,17 @@ WITH competition AS ( | |||
| SELECT sc.id | |||
| FROM solver_competitions sc | |||
| JOIN settlements s ON sc.id = s.auction_id | |||
| JOIN settlement_observations so | |||
There was a problem hiding this comment.
JOIN because we don't want to return solver competition entry for a transaction that was settled in different environment.
MartinquaXD
approved these changes
Dec 18, 2024
|
The only drawback of this solution is that But I think this is a trade off we can live with. |
|
I agree that this drawback shouldn't block this fix. |
squadgazzz
approved these changes
Dec 18, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes a bug where transaction hashes from different environment (prod/staging) are shown on solver competition endpoint.
Example: https://api.cow.fi/mainnet/api/v1/solver_competition/9861815
Here, transaction
0xb608f1e2a644befda65a6a87ce6b93fc8a3441d9729b8ce4b2a2db02c1247e60is from production enviroment and valid one, while0xc85cd0ad8e3aac389836a71da9795bdd1bedce7b21da1950b48f93165c9f6be8is from staging environment and mistakenly added to result.The reason why this happens is:
auction_idcan be saved in both environments, regardless if the competition ended up with mined settlement (which is fine).settlement::Observersaves the relationship <auction, settlement> even for settlements from another environment (this is a decision made long time ago, before refactoring, reference code here )But, what this PR introduces is leveraging the fact that, for settlement from another environment, there is no matching entry in
settlement_observationstable, reference code here. Note how the saving is executed only if thesettlementexists, which if false for settlements from another environment.Changes
How to test
Updated unit test.
Existing e2e tests.
Manually tested all updated functions on production database, works as expected.