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

Fix the "fee,fee_float" scheme #314

Open
barakman opened this issue Jan 18, 2024 · 0 comments
Open

Fix the "fee,fee_float" scheme #314

barakman opened this issue Jan 18, 2024 · 0 comments
Labels
bug Something isn't working

Comments

@barakman
Copy link
Collaborator

barakman commented Jan 18, 2024

The current implementation of the "fee,fee_float" scheme is extremely inconsistent across different files and modules.
It also contains a lot of redundant information, as these two entities essentially represent the same value, and that too - reflects yet another inconsistency - in most cases,fee stands for the raw (scaled-up) fee in the contract, while fee_float stands for the normalized value of it (a value between 0 and 1), but in some rare cases, they actually hold the exact same value.

Very generally, there should be a static data file containing the fee-resolution of each pool (or even each DEX), for example:

  • Carbon, 1e6
  • Uniswap, 1e12
  • Balancer, 1e18

But this issue is probably better off split into two phases, where that static data file will be added only during the 2nd phase:

  • 1st phase: replace all occurrences of fee_float with fee_resolution
  • 2nd phase: move the fee_resolution to be stored only in the static data file, instead of being replicated everywhere

The first phase should include setting the type of both fee and fee_resolution to int.
At present, the type of fee varies between int, float and str across different files.

Calculating of fee_float should take place only at the very low level, where the real (normalized) value is required.
Preferably via Decimal(fee) / fee_resolution, but for starters, float(fee) / fee_resolution would also do fine.

This change is horizontal across core python modules, jupyter tests, and all files named static_pool_data.csv.

UPDATE

Despite the redundant information stored in the "fee, fee_float" tuple, it turns out that they are THEORETICALLY used for two different things:

  • The 'fee' is used as a pool-identifier in the Uniswap Rounter paradigm
  • The 'fee_float' is used in the actual calculation of the swap which takes place inside the pool

But as stated above, because they have so far been synonymous to each other, one could easily be tempted to take 'fee' and conclude 'fee_float' at any point in the code.

And with the future introduction of the NileV3 DEX (PR #518), this turns into a potential risk.

The NileV3 DEX exposes two different contract functions:

  1. fee - which returns the initial fee set for the given pool, and later used as the pool-identifier
  2. currentFee - which returns the current fee used in the actual calculation of the swap

Hence, function 1 should be used for setting 'fee', and function 2 should be used for setting 'fee_float'.
And in this specific case - these two values are NO LONGER guaranteed to be storing the same information.

Hence, for the specific case of NileV3, any point in the code where 'fee' is used in order to deduce 'fee_float' embeds a potential bug.

@barakman barakman added the bug Something isn't working label Jan 18, 2024
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

1 participant