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

Request a fail early process for missing fees in univ2 forks #601

Open
NIXBNT opened this issue May 3, 2024 · 2 comments
Open

Request a fail early process for missing fees in univ2 forks #601

NIXBNT opened this issue May 3, 2024 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@NIXBNT
Copy link
Collaborator

NIXBNT commented May 3, 2024

Is your feature request related to a problem? Please describe.
Univ2 forks without fee specified in multichain_addresses.csv cause trade calculation failures. Since this is a known issue, any univ2 forks without fees should be flagged early in the process.

Describe the solution you'd like
When the multichain_addresses.csv is read in, any univ2 forks without a specified fee should raise an error immediately

Additional context
In the current bot process, the absence of the fee isnt noticed until an arbitrage is found, and the calculate_trade_outputs function is called. At which point a sensible input (trade) amount is converted to a NaN throwing a large error.

  File "\fastlane-bot\fastlane_bot\helpers\tradeinstruction.py", line 366, in _convert_to_wei
    return int(amount * 10**decimals)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: cannot convert NaN to integer

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "\fastlane-bot\main.py", line 447, in run
    handle_subsequent_iterations(
  File "\fastlane-bot\fastlane_bot\events\utils.py", line 1051, in handle_subsequent_iterations
    bot.run(
  File "\fastlane-bot\fastlane_bot\bot.py", line 952, in run
    self._run(
  File "\fastlane-bot\fastlane_bot\bot.py", line 380, in _run
    tx_hash, tx_receipt = self._handle_trade_instructions(CCm, arb_mode, r, replay_from_block)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "\fastlane-bot\fastlane_bot\bot.py", line 784, in _handle_trade_instructions
    calculated_trade_instructions = tx_route_handler.calculate_trade_outputs(
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "\fastlane-bot\fastlane_bot\helpers\routehandler.py", line 1463, in calculate_trade_outputs
    ) = self._solve_trade_output(
        ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "\fastlane-bot\fastlane_bot\helpers\routehandler.py", line 1292, in _solve_trade_output
    amount_out_wei = TradeInstruction._convert_to_wei(amount_out, tkn_out_decimals)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "\fastlane-bot\fastlane_bot\helpers\tradeinstruction.py", line 370, in _convert_to_wei
    int(float(str(amount) * 10**decimals))
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: could not convert string to float: 'NaNNaNNaNNaNNaNNaNNaNNaNNaNNaNN

^ that NaN part fills the entire terminal output :)

@NIXBNT NIXBNT added the enhancement New feature or request label May 3, 2024
@barakman
Copy link
Collaborator

barakman commented May 3, 2024

Why are there rows with no fee in them?
Why is this issue relevant only for univ2 exchanges?
There are other exchanges in this file with no fee defined for them.
If this issue is relevant only for univ2, then why are there other exchanges for which there are some rows with fee and some rows without fee?
Logically, it sounds like for each exchange, either the fee should be included in every row or omitted from every row.

This issue needs to be resolved from the cause root.
Either by removing lines with invalid data, or by updating them to consist of valid data.

PR #607 attempts to do so by removing lines with fork=univ2 and no fee.
But there are a few doubts remaining:

  • Should these lines perhaps be instead updated to consist of valid data?
  • Should other lines in this file be updated to consist of valid data?
  • Should other lines in this file be removed?
  • Should other lines in any other file be updated to consist of valid data?
  • Should other lines in any other file be removed?

@barakman barakman added bug Something isn't working and removed enhancement New feature or request labels May 3, 2024
@NIXBNT
Copy link
Collaborator Author

NIXBNT commented May 9, 2024

Why are there rows with no fee in them?

The fee is not required for every exchange since its only hardcoded for uniswapv2. I.e. passing the fee for uniswap v3 makes no sense since fees are decided at the pool level not the contract level

Why is this issue relevant only for univ2 exchanges?

I believe (might need to check me on this) but uniswap_v2 forks are the only ones that require the fee to be read in here because its hardcoded at the contract level and thus unreadable in a dynamic sense. So in essence, the fee here for univ2 is the single source of truth for univ2 fork fee in the bot.

There are other exchanges in this file with no fee defined for them.

As above, we could consider passing NA but I dont know the implications of passing 0 instead of None.

If this issue is relevant only for univ2, then why are there other exchanges for which there are some rows with fee and some rows without fee? Logically, it sounds like for each exchange, either the fee should be included in every row or omitted from every row.

I think this is adequately described above.

This issue needs to be resolved from the cause root. Either by removing lines with invalid data, or by updating them to consist of valid data.

Absolutely. Since the multichain_addresses.csv can only be populated manually, it is possible to make human error entering the data. The root cause is human error in this case and so I suggest that we have an early failure or at least very visible warning that alerts users to the fact that the data isnt populated correctly.

PR #607 attempts to do so by removing lines with fork=univ2 and no fee. But there are a few doubts remaining:

  • Should these lines perhaps be instead updated to consist of valid data?
  • Should other lines in this file be updated to consist of valid data?
  • Should other lines in this file be removed?
  • Should other lines in any other file be updated to consist of valid data?
  • Should other lines in any other file be removed?

I think this PR has since been closed but nevertheless, yes the lines should be flagged as incorrect so that the can be updated manually (as there is no way to automate this). Yes we can/should review the file contents entirely. No other files are not relevant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants