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

Implementation of CIP-38 #345

Merged
merged 7 commits into from
Mar 25, 2024
Merged

Implementation of CIP-38 #345

merged 7 commits into from
Mar 25, 2024

Conversation

fhenneke
Copy link
Collaborator

@fhenneke fhenneke commented Mar 5, 2024

This PR implements changes to solver rewards scripts for CIP-38 on ranking by surplus.

The main change is that rewards are computed based on surplus (and protocol fees) and not based on surplus and network fees (and protocol fees). Additionally, there is no cost reimbursement but the slippage computation includes a contribution from fees.

The implementation in this PR is quite minimal.
The batch rewards query ignores network fees for payment but explicitly returns aggregated network fees to add them to slippage. The other changes are only slight renames (e.g.
from payment_eth to primary_reward_eth). Test have been adapted accordingly.
The slippage query does not change. Instead, when generating the final merged dataframe, network fees are explicitly added to slippage. Test have been adapted and a few comments were added.

A few things can be implemented differently:

  • Instead of computing payments from surplus and protocol fees, one could have used winning_score. This is a bit simpler but would require checking somewhere that the score is consistent with the on-chain settlement.
  • The slippage computation could have been adapted to not exclude network fees and use external prices for converting those fees to ETH. This would require uploading explicit information on fees and their native prices to dune. This should be done at some point, but maybe not for the release.

The changes to computing rewards in batch_rewards.sql need to be ported to dune-sync.

@fhenneke fhenneke requested a review from harisang March 5, 2024 12:39
src/fetch/payouts.py Outdated Show resolved Hide resolved
@harisang
Copy link
Contributor

harisang commented Mar 6, 2024

Should we try to remove unnecessary parts of the query? One thing that comes to mind is carrying around execution_cost although never needing it.

Copy link
Contributor

@harisang harisang left a comment

Choose a reason for hiding this comment

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

Added a few comments

@fhenneke
Copy link
Collaborator Author

fhenneke commented Mar 7, 2024

Should we try to remove unnecessary parts of the query? One thing that comes to mind is carrying around execution_cost although never needing it.

I thought about removing lots of stuff from the batch rewards query. I did not do it because I want to use a copy (of the first part of) the query without changes.We might want to sync more data on dune.

@harisang
Copy link
Contributor

harisang commented Mar 7, 2024

I ran the script locally and it crashed in this line.

performance_reward = complete_payout_df["reward_cow"].sum()

I suspect we need to rename reward_cow to primary_reward_cow

update batch rewards query
- remove network fee from payment computation (alternative: use winning_score)
- remove execution cost in final result
- add network fees in final result
- combine data aggregation in the end
- rename *_wei to *
- rename payment_eth to primary_reward_eth
- adapt tests

update payouts
- remove cost from reward type and data frames
- remove execution cost from reward and reimbursement computation
- add network fees to slippage (alternative: modify slippage computation)
- rename payment_eth to primary_reward_eth
- rename reward_* to primary_reward_*
- adapt tests
- added `network_fee_eth` to checked dataframe columns, adapt test
- added comments to batch rewards query to clarify what happens when data is missing
- fix typo in data validation
we should at some point add a test for that
Copy link
Contributor

@harisang harisang left a comment

Choose a reason for hiding this comment

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

Looks good.

@harisang
Copy link
Contributor

@fhenneke Have you done a dry run to double-check that results make sense?

@harisang harisang merged commit 262e58e into main Mar 25, 2024
6 checks passed
@harisang harisang deleted the cip_38 branch March 25, 2024 00:31
@github-actions github-actions bot locked and limited conversation to collaborators Mar 25, 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