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

Fix merging of data frames for payments #453

Merged
merged 5 commits into from
Dec 12, 2024

Conversation

fhenneke
Copy link
Collaborator

@fhenneke fhenneke commented Dec 9, 2024

This PR changes which columns are expected from dune results and restricts columns before merging.

This is necessary since multiple queries now contain a solver_name field. In merging, the filed was renamed to solver_name_x, solver_name_y, ..., resulting in uncaught exceptions.

This can be tested by running python -m src.fetch.transfer_file --start 2024-11-26 --post-tx --dry-run --min-transfer-amount-wei "1000000000000000" --min-transfer-amount-cow-atoms "1000000000000000000" --ignore-slippage with suitable environments set up for Gnosis and Arbitrum One.

@fhenneke fhenneke requested a review from harisang December 9, 2024 14:03
@harisang
Copy link
Contributor

harisang commented Dec 9, 2024

In principle, the name should have nothing to do with slippage though, right? Solver address and the slippage amount should be all you need. So could you elaborate on what is the exact issue here? (I remember also running into these but don't remember what was the core reason tbh)

@fhenneke
Copy link
Collaborator Author

Until now, the solver name is taken from the Dune slippage query (unless slippage is skipped in which case the name is taken from reward targets). Since, after recent rewrites, three Dune queries return solver names (slippage, service fee, reward targets), some merges of corresponding data frames result in renaming of those duplicate columns, e.g. to solver_name_x. This can result in crashing of subsequent processing which assumes the existence of a solver_name column.

With this PR, the solver name is taken from the reward target query. This seems most reasonable to me. An alternative would be to ignore names altogether, but that would require additional changes. All columns returned by Dune queries which are not strictly required, are removed before merging. For example, the solver name from the slippage query is ignored.

A similar approach is used in the rewrite #435, where some of the column checking is made stricter as well.

@harisang
Copy link
Contributor

Ok sorry for my complaint. I had misread the code and thought the name was being added in the slippage dataframe. Soorry.

Yeah, i agree that the name, if needed, should be taken from the reward target query.

@fhenneke fhenneke merged commit 9305c01 into main Dec 12, 2024
4 checks passed
@fhenneke fhenneke deleted the restrict_dataframes_before_merging branch December 12, 2024 09:22
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2024
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.

2 participants