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.

Sign up for GitHub

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

Jump to bottom

Allow whitelisting addresses for protocol fees discounts #2573

Merged
merged 3 commits into from
Apr 5, 2024
Merged

Allow whitelisting addresses for protocol fees discounts #2573

merged 3 commits into from
Apr 5, 2024

Conversation

m-lord-renkse
Copy link
Contributor

Description

Allow configuring contracts which are exempt from protocol fees.
Our CoW AMM experiment is placing a lot of limit orders, some of which are being classified as out of market orders (mainly because they are partially fillable and usually a bit larger than what is currently fillable at the specified price).
It therefore pays significant protocol fees, impacting it's own PnL

Changes

  • Configure the driver to accept whitelisted address for protocol fee discount
  • Skip protocol fee for whitelisted addresses

How to test

  1. e2e tests
  2. unit tests

Fixes #2461

@m-lord-renkse m-lord-renkse requested a review from a team as a code owner March 28, 2024 15:20
m-lord-renkse
m-lord-renkse commented Mar 28, 2024
View reviewed changes
crates/driver/src/infra/config/file/load.rs Outdated
protocol_fee_discounted_addresses.kind,
)
})
.collect::<HashMap<_, _>>(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Convert into a HashMap in order to have O(1) access when checking the addresses.

@m-lord-renkse m-lord-renkse force-pushed the 2461/allow-whitelisting-addresses-for-protocol-fee-discounts branch from 04072f9 to ee1f11d Compare March 28, 2024 15:23
m-lord-renkse
m-lord-renkse commented Mar 28, 2024
View reviewed changes
crates/driver/src/infra/solver/protocol_fees.rs Outdated Show resolved Hide resolved
crates/driver/src/tests/setup/driver.rs Outdated Show resolved Hide resolved
crates/driver/src/infra/solver/protocol_fees.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse force-pushed the 2461/allow-whitelisting-addresses-for-protocol-fee-discounts branch from ee1f11d to 6036bc1 Compare March 28, 2024 15:34
@m-lord-renkse m-lord-renkse self-assigned this Mar 28, 2024
@m-lord-renkse m-lord-renkse force-pushed the 2461/allow-whitelisting-addresses-for-protocol-fee-discounts branch from 6036bc1 to bf3f8f5 Compare March 28, 2024 15:46
@MartinquaXD
Copy link
Contributor

I cut the review a bit short since addressing the autopilot comment will most likely cause a bunch of changes anyway.

@m-lord-renkse m-lord-renkse force-pushed the 2461/allow-whitelisting-addresses-for-protocol-fee-discounts branch from bf3f8f5 to 2862193 Compare April 2, 2024 18:03
m-lord-renkse
m-lord-renkse commented Apr 2, 2024
View reviewed changes
crates/autopilot/src/domain/fee/mod.rs Outdated
@@ -110,9 +129,18 @@ impl ProtocolFees {
buy: quote.buy_amount,
fee: quote.fee,
};
let protocol_fees = protocol_fees
let apply_protocol_fee = if let Ok(Some(address)) = order
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be done better, it is mostly a placeholder to apply different kind of discounts in the future.

@m-lord-renkse m-lord-renkse force-pushed the 2461/allow-whitelisting-addresses-for-protocol-fee-discounts branch 3 times, most recently from e835e06 to 55d45ef Compare April 2, 2024 18:21
MartinquaXD
MartinquaXD reviewed Apr 2, 2024
View reviewed changes
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

Looks way better!
I would just recommend to cross the bridge of different fee discounts when we actually need to and not support it from the get go. Overall I like to think in terms of "lines spent" since every line of code that is actually not needed just adds mental overhead and opportunities for bugs to sneak in.

crates/autopilot/src/arguments.rs Outdated Show resolved Hide resolved
crates/autopilot/src/domain/fee/mod.rs Outdated Show resolved Hide resolved
crates/autopilot/src/domain/fee/mod.rs Outdated Show resolved Hide resolved
crates/autopilot/src/domain/fee/mod.rs Outdated Show resolved Hide resolved
crates/autopilot/src/domain/fee/mod.rs Outdated Show resolved Hide resolved
crates/autopilot/src/domain/fee/mod.rs Outdated Show resolved Hide resolved
MartinquaXD
MartinquaXD reviewed Apr 3, 2024
View reviewed changes
crates/autopilot/src/arguments.rs Outdated Show resolved Hide resolved
crates/e2e/tests/e2e/protocol_fee.rs Outdated Show resolved Hide resolved
MartinquaXD
MartinquaXD approved these changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

LGTM.

@m-lord-renkse m-lord-renkse requested a review from squadgazzz April 3, 2024 10:35
@squadgazzz
Copy link
Contributor

This merge gonna add some conflicts to the protocol fee e2e tests clean-up PR(

@fleupold
Copy link
Contributor

fleupold commented Apr 3, 2024

This merge gonna add some conflicts to the protocol fee e2e tests clean-up PR

Maybe merge this one first then. It's been pending for a long time and shouldn't get more delayed.

fleupold
fleupold approved these changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@fleupold fleupold 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, I don't think you need to pass down the domain separator (you can get the owner straight from the order)

crates/autopilot/src/arguments.rs Outdated Show resolved Hide resolved
crates/autopilot/src/domain/fee/mod.rs Outdated Show resolved Hide resolved
sunce86
sunce86 approved these changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@sunce86 sunce86 left a comment

Choose a reason for hiding this comment

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

Lg.

@fleupold should we skip integrator fees for these addresses as well? Could this edge case happen in practice?

crates/autopilot/src/arguments.rs Outdated Show resolved Hide resolved
crates/autopilot/src/arguments.rs Outdated Show resolved Hide resolved
crates/autopilot/src/domain/fee/mod.rs Outdated Show resolved Hide resolved
@m-lord-renkse m-lord-renkse force-pushed the 2461/allow-whitelisting-addresses-for-protocol-fee-discounts branch 2 times, most recently from 4abdf2a to 5f88090 Compare April 4, 2024 10:57
@m-lord-renkse m-lord-renkse requested a review from sunce86 April 4, 2024 11:00
@squadgazzz
Copy link
Contributor

Maybe merge this one first then. It's been pending for a long time and shouldn't get more delayed.

Dang, I missed that comment...

@m-lord-renkse m-lord-renkse force-pushed the 2461/allow-whitelisting-addresses-for-protocol-fee-discounts branch from 5f88090 to 3073517 Compare April 4, 2024 11:03
@squadgazzz
Copy link
Contributor

The following messages need to be updated according to the current elements count:
https://github.com/cowprotocol/services/pull/2573/files#diff-a94610fcc6f4359d3fc7c3a62ed17d113ce17c8254e65677d052f667ae4e4f84R161
https://github.com/cowprotocol/services/pull/2573/files#diff-a94610fcc6f4359d3fc7c3a62ed17d113ce17c8254e65677d052f667ae4e4f84L241

@m-lord-renkse
Allow whitelisting addresses for protocol fees discounts
5d95f07 
Remove the extra logic to determine the kind of discounts

Support only whitelisting of addresses and not struct

Remove commented code

Inline the fee policy whitelisted addresses filter

Remove domain separator
@m-lord-renkse m-lord-renkse force-pushed the 2461/allow-whitelisting-addresses-for-protocol-fee-discounts branch from 3073517 to 89133ed Compare April 4, 2024 17:17
@m-lord-renkse m-lord-renkse force-pushed the 2461/allow-whitelisting-addresses-for-protocol-fee-discounts branch from 89133ed to e024627 Compare April 4, 2024 17:18
@m-lord-renkse m-lord-renkse requested a review from squadgazzz April 4, 2024 17:18
@m-lord-renkse m-lord-renkse force-pushed the 2461/allow-whitelisting-addresses-for-protocol-fee-discounts branch from 9c280e6 to e024627 Compare April 4, 2024 17:48
@m-lord-renkse m-lord-renkse merged commit 6c17db6 into main Apr 5, 2024
9 checks passed
@m-lord-renkse m-lord-renkse deleted the 2461/allow-whitelisting-addresses-for-protocol-fee-discounts branch April 5, 2024 07:28
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@squadgazzz squadgazzz squadgazzz approved these changes

@MartinquaXD MartinquaXD MartinquaXD approved these changes

@fleupold fleupold fleupold approved these changes

@sunce86 sunce86 Awaiting requested review from sunce86

Assignees

@m-lord-renkse m-lord-renkse

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

chore: Allow configuring contracts which are exempt from protocol fees
5 participants
@m-lord-renkse @MartinquaXD @squadgazzz @fleupold @sunce86