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

Make the partner fee max cap configurable in autopilot #2559

Merged
merged 1 commit into from
Mar 26, 2024
Merged

Make the partner fee max cap configurable in autopilot #2559

merged 1 commit into from
Mar 26, 2024

Conversation

m-lord-renkse
Copy link
Contributor

Description

We should make configurable in the autopilot the maximum partner fee amount.

Changes

  • Introduce a new configuration parameter in the autopilot for the partner fee max cap with a default value 0.01
  • Enforce the configured partner fee max cap

How to test

  1. e2e test
  2. unit test

Related Issues

Fixes #2554

@m-lord-renkse m-lord-renkse requested a review from a team as a code owner March 22, 2024 09:58
@squadgazzz
Copy link
Contributor

I'd suggest rebasing onto this branch to avoid a lot of conflicts.

squadgazzz
squadgazzz reviewed Mar 22, 2024
View reviewed changes
crates/e2e/tests/e2e/protocol_fee.rs Outdated
@@ -320,7 +371,7 @@ fn is_approximately_equal(executed_value: U256, expected_value: U256) -> bool {

async fn execute_test(
web3: Web3,
fee_policy: FeePolicyKind,
autopilot_config: Vec<String>,
Copy link
Contributor

@squadgazzz squadgazzz Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nit: don't really feel confident to pass strings here. Would rather see a protocol fees config struct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I am changing to accept anything which implements ToString. The reason of it is that we can keep the current FeePolicyKind enum without needing to convert it to string, and at the same time it adds customization to what you want to pass.

The idea of doing it like that is to avoid creating a struct containing FeePolicyKind and a new field which is going to be used just is one test, and at the same time it opens to possibility of adding more tests with different configurations without altering the execute_test() signature.

@m-lord-renkse
Copy link
Contributor Author

m-lord-renkse commented Mar 22, 2024 •
edited
Loading

I'd suggest rebasing onto this branch to avoid a lot of conflicts.

Thanks for the heads up! I saw the conflicts and aren't many. I will wait until your PR is merged so I don't pass the conflicts to you.

MartinquaXD
MartinquaXD approved these changes Mar 22, 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.

Code looks good to me.

crates/autopilot/src/domain/fee/mod.rs
@@ -47,9 +51,11 @@ impl ProtocolFee {
{
if let Some(partner_fee) = validated_app_data.protocol.partner_fee {
let fee_policy = vec![Policy::Volume {
factor: FeeFactor::partner_fee_capped_from(
factor: FeeFactor::try_from_capped(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to double check. The fee policy extracted from the app data here will get stored in the DB when the order gets executed during an auction, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a really good question! @sunce86 correct me if I am wrong, but as far as I understood it, by placing the code there (domain/fee/mod.rs), the action is cached and then taken as the next auction to process and find a solution in the single_run() (run_loop.rs) it is stored the order event.

fleupold
fleupold approved these changes Mar 23, 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.

LG one comment

@m-lord-renkse m-lord-renkse force-pushed the 2554/make-partner-fee-cap-configurable branch from 556dd23 to fe2c19e Compare March 26, 2024 08:14
squadgazzz
squadgazzz approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@squadgazzz squadgazzz 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 merged commit 358d483 into main Mar 26, 2024
9 checks passed
@m-lord-renkse m-lord-renkse deleted the 2554/make-partner-fee-cap-configurable branch March 26, 2024 09:30
@github-actions github-actions bot locked and limited conversation to collaborators Mar 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sunce86 sunce86 sunce86 approved these changes

@squadgazzz squadgazzz squadgazzz approved these changes

@fleupold fleupold Awaiting requested review from fleupold

@MartinquaXD MartinquaXD Awaiting requested review from MartinquaXD

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: Add configuration for partner fee cap in autopilot configuration
5 participants
@m-lord-renkse @squadgazzz @fleupold @MartinquaXD @sunce86