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

Adds support for custom flashloan fees #360

Closed
wants to merge 11 commits into from

Conversation

mikewcasale
Copy link
Contributor

Handles issue 346

@mikewcasale mikewcasale linked an issue Feb 7, 2024 that may be closed by this pull request
@mikewcasale mikewcasale changed the base branch from main to develop February 7, 2024 19:15
@mikewcasale mikewcasale changed the base branch from develop to release-candidate February 7, 2024 19:16
@mikewcasale mikewcasale changed the base branch from release-candidate to develop February 8, 2024 13:46
# should be in the form of {network: fee * 1e6}
"ethereum": 0,
"coinbase_base": 0,
"fantom": 300, # Flashloans come with a 0.03% Fee from Beethoven X (Balancer fork)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why represent percentage like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this not a standard approach in the industry, to specify in PPM? In any case, just following what we did in the simulator, and as well how Uniswap does it, etc. Happy to change if there is a more preferred method.

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. It is not standard, but an implementation-decision (typically per DEX).
  2. Unless this is related to something the you need to process before passing as input to a contract function or after receiving as output from a contract function, there is no reason for you to use this resolution to begin with; the only reason for it being used onchain, is the lack of non-integer-arithmetic support, which is obviously not an issue in offchain scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, what is your preferred implementation approach / alternative?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just use 0.0003 (i.e., multiply by that value wherever needed)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd go with the most obvious - either 0.0003 # 0.03% or 0.03 # %

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 has been updated.

Copy link
Collaborator

@barakman barakman Feb 13, 2024

Choose a reason for hiding this comment

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

An important note to make here is that Mike's original decision to use a PPM factor, was actually a good one in the sense of avoiding floating-point calculations as much as possible (i.e., the calculation would subsequently turn into amount * fee // 1e6, which is purely integer-arithmetic).
But not critical at this point, especially after he's already changed that.

fastlane_bot/bot.py Show resolved Hide resolved
fastlane_bot/bot.py Show resolved Hide resolved
fastlane_bot/helpers/routehandler.py Outdated Show resolved Hide resolved
@@ -529,7 +530,7 @@ def _get_flashloan_platform_id(self, tkn: str) -> int:
else:
return 7

def _get_flashloan_struct(self, trade_instructions_objects: List[TradeInstruction]) -> List:
def _get_flashloan_struct(self, trade_instructions_objects: List[TradeInstruction]) -> Tuple[List, int]:
Copy link
Contributor

@zavelevsky zavelevsky Feb 11, 2024

Choose a reason for hiding this comment

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

where do you actually return the fee? I can't find where you add the fee to the Tuple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be corrected now.

@barakman barakman self-requested a review February 11, 2024 13:50
@@ -964,14 +969,17 @@ def _handle_trade_instructions(
# Get the flashloan token
fl_token = calculated_trade_instructions[0].tknin_address
fl_token_symbol = calculated_trade_instructions[0].tknin_symbol
flashloan_amount = int(calculated_trade_instructions[0].amtin_wei)
flashloan_fee = FLASHLOAN_FEE_MAP.get(self.ConfigObj.NETWORK, 0)
flashloan_fee_amt = int(flashloan_fee * flashloan_amount)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the initialization of FLASHLOAN_FEE_MAP, you are using a percentage value of 0.03% (at least by the comment next to it), i.e., a value between 0 and 100.
But the calculation in the line above implies that you are using a normalized value (between 0 and 1).
So either change the map to consist of normalized values, or add in this line a division by 100.

# should be in the form of {network: fee * 1e6}
"ethereum": 0,
"coinbase_base": 0,
"fantom": 0.03 # %
Copy link
Collaborator

Choose a reason for hiding this comment

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

My previous comment assumes that 0.03 here stands for 0.03%.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Handling of Flashloan Fee
3 participants