search

cowprotocol / solver-rewards

Data Aggregation for Solver Reimbursement & Rewards Distributor
Other
8 stars 5 forks source link

Add flag to skip slippage computations #407

Closed harisang closed 1 week ago

harisang commented 4 weeks ago

This PR adds a flag to completely skip slippage calculations, if they are not needed.

fhenneke commented 4 weeks ago

I only glanced over the changes and they look reasonable.

Is the changed code part of any test? Or is there a simple way to manually test this change?

harisang commented 4 weeks ago

I only glanced over the changes and they look reasonable.

Is the changed code part of any test? Or is there a simple way to manually test this change?

Not yet. I pushed a change in order to make existing tests pass. I will add some test so that we can confirm the flag works as expected.

harisang commented 4 weeks ago

I only glanced over the changes and they look reasonable.

Is the changed code part of any test? Or is there a simple way to manually test this change?

Following up on this, I am not sure how easy it would be to add clean tests for this, so don't know how soon i will address this. This is also meant for local testing so I would say this is not urgent so let's leave this PR open until i address the testing part

fhenneke commented 2 weeks ago

Not merging this might complicate other things like deploying on arbitrum. If you are confident that without the flag, results are not changed, we should proceed with merging.

harisang commented 2 weeks ago

Not merging this might complicate other things like deploying on arbitrum. If you are confident that without the flag, results are not changed, we should proceed with merging.

There are some issues when i did some local testing today and will try to figure out what is going on. I saw this alert AssertionError: Slippage validation Failed with columns: {'slippage_usd', 'tx_hash', 'block_time', 'solver', 'slippage_wei'}

fhenneke commented 2 weeks ago

This error is probably due to the fact that the dummy slippage data frame does not have the required columns. Thus the check in validate_df_columns fails.

The dataframe slippage_df needs to have at least the columns SLIPPAGE_COLUMNS, i.e. {"solver", "solver_name", "eth_slippage_wei"}. The current code seems to miss solver_name and eth_slippage_wei in the case you checked.