bancorprotocol / fastlane-bot

Fast Lane, an open-source arbitrage protocol, allows any user to perform arbitrage between Bancor ecosystem protocols and external exchanges and redirect arbitrage profits back to the protocol.
https://github.com/bancorprotocol/fastlane-bot
MIT License
134 stars 58 forks source link

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

Open NIXBNT opened 4 months ago

NIXBNT commented 4 months ago

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 :)

barakman commented 4 months ago

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:

NIXBNT commented 4 months ago

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