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

Warn and merge if solvers appear in prod and barn #411

Merged
merged 6 commits into from
Oct 28, 2024

Conversation

fhenneke
Copy link
Collaborator

This PR adds default handling in case solvers appear in prod and barn. In such cases, a warning is emitted and data is merged.

At the moment, if settlement in prod and barn are settled by the same address (or rather if the same address is reported during bidding), solvers can appear in multiple rows in batch_rewards_df here. Since we later merge on solvers, we might assign slippage or quote rewards to a solver twice. A similar thing can happen with quote_rewards_df.

With this PR, duplicate entries are detected after fetching data and suitably merged. All numerical columns are summed in merging. The columns containing list entries on partner fees are concatenated.

The assumption on unique solvers is later checked at the point where it is required.

This is more of a hot fix and refactoring is required to make the code understandable.

One alternative would be to implement all processing of the fetched data in payouts.py where the rest of the processing happens. With this PR there is a mix of fetching, checking, merging of rewards, followed by checking and merging of rewards, followed by checking them again in construct_payout_dataframe. Before this PR, there was fetching, merging of rewards, then merging of rewards, then checking and merging. So generally quote confusing.

Testing plan

I tested the code for the accounting week 2024-10-15 -- 2024-10-22 and it produced the roughly same amounts which were used for payments. There was a difference of 0.003 ETH and around 100 COW. This difference if probably because in the actual payment, data was not merged but removed.

I have not added a test yet.

Copy link
Contributor

@bram-vdberg bram-vdberg left a comment

Choose a reason for hiding this comment

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

LGTM!

src/pg_client.py Outdated
merged = []
for lst in series:
if lst is not None:
merged.extend(lst)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do a check here to see if the merge adds duplicates? It might make debugging easier to figure out where the duplicates are introduced.

Copy link
Collaborator Author

@fhenneke fhenneke Oct 25, 2024

Choose a reason for hiding this comment

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

This sounds useful, but I tried to come up with a clean solution and failed. So if you have a good idea, feel free to propose it.

In principle, we could check the final dataframe after merging. That is probably easiest.

If we want to check if while merging, we probably only want it for the addresses and not for the values. If we already have two very similar but different functions, we can probably just modify the two columns in function. Then we might as well just remove duplicates right away. But the code looks significantly more indirect then, with aggregating multiple columns into multiple values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added more printing but for now there is no additional warning in this function.

@fhenneke fhenneke merged commit b0e7bd5 into main Oct 28, 2024
5 checks passed
@fhenneke fhenneke deleted the solvers_in_prod_and_barn branch October 28, 2024 15:53
@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2024
@harisang
Copy link
Contributor

Did some local testing and results make sense

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