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

No service fee on penalties #424

Merged
merged 6 commits into from
Jan 17, 2025
Merged

No service fee on penalties #424

merged 6 commits into from
Jan 17, 2025

Conversation

fhenneke
Copy link
Collaborator

@fhenneke fhenneke commented Nov 8, 2024

With this PR, if a solver receives a negative reward for an accounting week, that amount is not multiplied by reward_scaling.

Before the change, negative rewards were multiplied by reward_scaling independent of the sign of the reward. For positive rewards, this does what it should, as a fraction of the reward is withheld from the solver and becomes part of a bounty budget. For negative rewards, the calculation implied that part of the penalty is withheld and it reduces the bounty budget.

With the change, negative batch rewards are not multiplied by reward_scaling. Quote rewards are left as is.

The proposed implementation changes some code in 3 places. Ideally there would only be one change required. Note, that quote rewards are always non-negative and no change in the formula is required.

Alternatively, the total cow reward could be used to decide on the scaling, instead of handling batch and quote rewards separately.

Tests have not been adapted yet.

@fhenneke fhenneke mentioned this pull request Nov 8, 2024
Base automatically changed from service_fee_log to main November 11, 2024 07:34
@fhenneke fhenneke force-pushed the no_service_fee_on_penalty branch from 134f9e3 to ff05504 Compare January 15, 2025 13:24
@fhenneke
Copy link
Collaborator Author

I rebased the PR on main.

One thing which still needs a decision is about which parts of the different rewards would receive special treatment when being negative. @harisang

Currently we have 2 independent payments of COW:

  1. Batch rewards, potentially modified for negative payments from buffer accounting.
  2. Quote rewards.

The wording of the CIP is "withholding 15% of the weekly COW rewards solvers are receiving". That would imply batch rewards and quote rewards are affected. Only batch rewards can be negative. If we do not want to modify payments on a per auction basis, this leaves us with two interesting cases: negative batch rewards but positive batch reward + quote reward, and negative batch reward + quote reward.

batch_reward < 0, batch_reward + quote_reward > 0 batch_reward + quote_reward < 0
currently 0.85 * batch_reward + 0.85 * quote_reward, 0.85 * batch_reward + 0.85 * quote_reward
this PR batch_reward + 0.85 * quote_reward batch_reward + 0.85 * quote_reward
alternative 0.85 * batch_reward + 0.85 * quote_reward batch_reward + quote_reward
  • The current approach has the issue of reducing penalties (in all cases), but reduces quote rewards always.
  • This PR only removes the reduction of penalties and keeps the reduction of quote rewards. This has the advantage that the COW transfer for quotes does not depend on batch rewards. The different types of rewards stay uncoupled.
  • The alternative is simplest in terms of total rewards. But it would couple the quote reward payment and batch reward payment. (The naming in the code is a bit misleading as the term total rewards refers to batch rewards and does not include quote rewards.)

I am in favor of using the current approach. If we want to go for the alternative, additional code changes would be required.

@fhenneke fhenneke marked this pull request as ready for review January 15, 2025 13:58
fhenneke and others added 3 commits January 15, 2025 15:21
to also check
- consistency of cow and eth rewards
- total service fee
since those quantities are modified as well
@harisang
Copy link
Contributor

harisang commented Jan 17, 2025

Could you confirm as a sanity check that the current PR still does

0.85 * batch_rewards + 0.85 * quote_rewards

in case batch_rewards > 0 and quote_rewards > 0?

@harisang
Copy link
Contributor

Btw, current approach is definitely fine for more. Basically, didn't like the fact that we were scaling negative batch rewards as it looked very wrong

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.

Yeah looks good, and i am fine moving forward with this.

@fhenneke
Copy link
Collaborator Author

Could you confirm as a sanity check that the current PR still does

0.85 * batch_rewards + 0.85 * quote_rewards

in case batch_rewards > 0 and quote_rewards > 0?

I added a new test for this now. We have tests for the cases (batch_rewards > 0, quote_rewards = 0), (batch_rewards = 0, quote_reward < 0), (batch_rewards > 0, quote_rewards > 0), (batch_rewards < 0, quote_rewards > 0, batch_reward + buffer_accounting > 0). In principle there is also the case (batch_rewards < 0, quote_rewards = 0) or cases where there is an actual overdraft. If you would like those cases tested, you can open an issue.

@fhenneke fhenneke merged commit dc27aeb into main Jan 17, 2025
6 checks passed
@fhenneke fhenneke deleted the no_service_fee_on_penalty branch January 17, 2025 16:15
@github-actions github-actions bot locked and limited conversation to collaborators Jan 17, 2025
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