Add Nile V3 specific fee support

[Lesigh-3100]

Nile V3 is a Uni V3 fork that has an override for the fee actually charged by the pool, however, the tick spacing correlated to the original fee remains the same.

This means that the bot must:

• Add handling to fetch the fee override & use it directly (divided by resolution 1e6) for the fee_float field in the TerraFormer & Exchange class.
• Use the fee field for everything related to Uniswap V3 ticks.
• Use the fee field for the customInt input to the Arb contract.
• Use the fee_float field for exact calculations performed in RouteHandler.py.

[barakman]

The required part of this ABI seems identical to UNISWAP_V3_POOL_ABI, so you can technically get rid of it (as well as any associated area in the code where you redirect to this ABI instead of the other one).

I'm writing this under the assumption that we need this pool's fee function, and not currentFee function, which is currently used in the code.

If the latter is indeed the one needed, then you should keep this ABI, but please:

1. Move it to the section where all V3 Pool ABIs are located, for ease of comparison in the future
2. Minimize it to include only the part necessary for the bot to interact with the contract:

NILE_V3_POOL_ABI = [
    {
        "type": "event",
        "name": "Swap",
        "anonymous": False,
        "inputs": [{"indexed": True, "internalType": "address", "name": "sender", "type": "address"}, {"indexed": True, "internalType": "address", "name": "recipient", "type": "address"}, {"indexed": False, "internalType": "int256", "name": "amount0", "type": "int256"}, {"indexed": False, "internalType": "int256", "name": "amount1", "type": "int256"}, {"indexed": False, "internalType": "uint160", "name": "sqrtPriceX96", "type": "uint160"}, {"indexed": False, "internalType": "uint128", "name": "liquidity", "type": "uint128"}, {"indexed": False, "internalType": "int24", "name": "tick", "type": "int24"}]
    },
    {
        "type": "function",
        "name": "currentFee",
        "stateMutability": "view",
        "inputs": [],
        "outputs": [{"internalType": "uint24", "name": "", "type": "uint24"}]
    },
    {
        "type": "function",
        "name": "liquidity",
        "stateMutability": "view",
        "inputs": [],
        "outputs": [{"internalType": "uint128", "name": "", "type": "uint128"}]
    },
    {
        "type": "function",
        "name": "slot0",
        "stateMutability": "view",
        "inputs": [],
        "outputs": [{"internalType": "uint160", "name": "sqrtPriceX96", "type": "uint160"}, {"internalType": "int24", "name": "tick", "type": "int24"}, {"internalType": "uint16", "name": "observationIndex", "type": "uint16"}, {"internalType": "uint16", "name": "observationCardinality", "type": "uint16"}, {"internalType": "uint16", "name": "observationCardinalityNext", "type": "uint16"}, {"internalType": "uint8", "name": "feeProtocol", "type": "uint8"}, {"internalType": "bool", "name": "unlocked", "type": "bool"}]
    },
    {
        "type": "function",
        "name": "tickSpacing",
        "stateMutability": "view",
        "inputs": [],
        "outputs": [{"internalType": "int24", "name": "", "type": "int24"}]
    },
    {
        "type": "function",
        "name": "token0",
        "stateMutability": "view",
        "inputs": [],
        "outputs": [{"internalType": "address", "name": "", "type": "address"}]
    },
    {
        "type": "function",
        "name": "token1",
        "stateMutability": "view",
        "inputs": [],
        "outputs": [{"internalType": "address", "name": "", "type": "address"}]
    }
]

[barakman]

Why function currentFee and not function fee?
For UniV3 pools, we are using function fee.
Might be worth verifying exactly which function is required here.
You can do that by looking into the source code of this pool contract, checking which fee is used in the swap function, and then following it to see which of these two functions returns that specific fee.

[barakman]

And why you using 1000000 in some cases and 1e6 in other cases?

[barakman]

Also, creating a web3.eth.contract instance clearly doesn't belong in here (as evident by how it is done elsewhere in the code).
You can't just "throw stuff in" without taking into consideration the broader SW design and structure.

[barakman]

Also, it doesn't make sense that we need to make an RPC at this point, as the fee has already been obtained previously.
So during that previous point - either it was already obtained correctly using this contract function, or it there was an attempt to use the other contract function, which would have reverted for this contract.

[Lesigh-3100]

1. The 1000000 vs 1e6 is just poor consistency - I will update this to use 1e6.
2. Nile V3 uses a fee override function - currentFee. This results in an awkward situation in the Terraformer, as in all other cases the bot obtains the fee from the pool creation event. Due to the override, the best way to obtain the pool's current fee is by directly calling the contract.
3. Regarding the overall SW design, do you have a suggestion for a more appropriate place for this, given that it's only relevant for Nile V3? I'm happy to move it if it makes more sense elsewhere.

[barakman]

Please see the PR spawned off of this one.
In order to handle this in the uniswap_v3 module, for example, I used a paradigm similar to the one implemented in the solidly_v2 module.
That might give you an idea on how to deal with it here.

The part:

pool = web3.eth.contract(address=web3.to_checksum_address(pool_address), abi=NILE_V3_POOL_ABI)
fee_float = pool.caller.currentFee() / 1000000

Doesn't look like it belongs here.
Even without being acquainted with this module, a simple observation reveals that no other DEX requires an RPC for fetching the fee at this point, so why should the Nile V3 DEX require one?
Subsequently, placing it here breaks the structure of the code and the overall design of this module.
Wherever the fee for all the other DEXs is fetched, that's where the fee for Nile V3 should be fetched.

[Lesigh-3100]

You are correct that no other Uni V3 fork requires an additional call to fetch the fee, however, Nile V3 does.

The fee for other Uni V3 forks is obtained from the pool creation event. However, this is not sufficient for Nile V3 due to implementing a fee override.

Where else do you suggest placing this call given that no other Uni V3 forks require it?

[barakman]

How is this change related to this PR?

[Lesigh-3100]

Nile V3 includes a fee override.

This PR stores the fee override for the pool in the fee_float field, and the original fee (which is necessary for Uniswap V3 details such as tick-spacing etc) in the fee field.

The Fast Lane arb contract must know the fee of the Uni V3 pool being traded on, so using the fee_float to calculate the fee is no longer valid in instances of override.

Consequently the fee field is now used as it does not change.

[barakman]

I did not understand this description.
But Nile V3 should not be handled differently than any other DEX, other than in the dedicated sections (for example, in the uniswap_v3 module, where all of these DEXs are handled).

[Lesigh-3100]

Correct, and to handle it the same, we must now use the fee field - which remains constant, instead of the fee_float which has an override for Nile V3.

[barakman]

How is this change related to this PR?

[Lesigh-3100]

I maintained the original format of this by switching fee_float -> fee due to fee_float no longer being reliable. See above.

[barakman]

How is this change related to this PR?

[Lesigh-3100]

See above regarding fee_float & fee.

[barakman]

There is something really wrong here, as in one case you are dividing by the resolution, and in one case you don't.
So fee_float is sometimes returned after being divided by 16e, and sometimes being returned as read from the contract.
How could that possibly work correctly further down the road, when the caller of this function receives this returned value and needs to processes it?
Also note that you seem to have made a similar change in file run_blockchain_terraformer.py, where you actually do divide by 1000000 in both cases.
So even without taking the above into consideration, it is pretty clear that at least one of these two changes is incorrect.

[barakman]

In addition to that, it makes much more sense to implement a separate file called nile_v3.py.
This type of logic becomes increasingly confusing:

if self.exchange_name in [PANCAKESWAP_V3_NAME, AGNI_V3_NAME, FUSIONX_V3_NAME, ECHODEX_V3_NAME, SECTA_V3_NAME]:
    return PANCAKESWAP_V3_POOL_ABI
elif self.exchange_name == NILE_V3_NAME:
    return NILE_V3_POOL_ABI
return UNISWAP_V3_POOL_ABI

The guiding thumb-rule should be that once a simple branching (if/else) becomes too obfuscated, it is time to consider an alternative approach.
Note that an additional logic was also applied in function get_fee further down in this file, and on top of making it more cumbersome, this one actually introduces a bug:

fee_float = float(fee) / 1e6
if self.exchange_name == NILE_V3_NAME:
    fee_float = await contract.functions.currentFee().call()

As the value of fee_float is scaled down by 1e6 in some scenarios, and not scaled down at all in other scenarios.
So unless you apply the same logic (if exchange_name == NILE_V3_NAME) in every function which calls this function, an incorrect calculation most likely takes place.

[barakman]

After a short research, the best approach here would be a paradigm similar to the one in solidly_v2.py.

[barakman]

There is something really wrong here, as in one case you are dividing by the resolution, and in one case you don't.
So fee_float is sometimes returned after being divided by 16e, and sometimes being returned as read from the contract.
How could that possibly work correctly further down the road, when the caller of this function receives this returned value and needs to processes it?
Also note that you seem to have made a similar change in file run_blockchain_terraformer.py, where you actually do divide by 1000000 in both cases.
So even without taking the above into consideration, it is pretty clear that at least one of these two changes is incorrect.

[barakman]

Potential risk of a runtime problem related to this change is described in issue #314.