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

Zap contracts review #3

Open
AugustoL opened this issue Dec 19, 2022 · 0 comments
Open

Zap contracts review #3

AugustoL opened this issue Dec 19, 2022 · 0 comments

Comments

@AugustoL
Copy link

On commit 821e7ea

  1. Receive native currency only from native currency wrappers: The contract is payable, but it will only receive the native network currency from native currency wrapper contracts, maybe this can be a requirement in the contract.
  2. line 280 tokens[i] -> nativeCurrencyAddress.
  3. Short unnecessary names in line 294 tokenBal and line 551, there is no problem with using long variable names.
  4. Remove the function that removes a supported DEX, we have a setSupportedDex function, and the set function is commonly used to add/remove.
  5. _getFeeAndTransferTokens is only used once, remove the function and place the code of it where it is used.
  6. Comment is wrong in line 453, the fee is removed if the protocol fee is higher than zero and the sender address is not whitelisted.
  7. Proposed refactor to affiliate fees, with the fee being applied over the total amount, like the protocol fee. Also, each affiliate can have its specific fee (it cant be higher than the protocol fee), a deadline for the fee too. So the affiliate status can be removed, the affiliate fee can be applied independently from the protocol fee and it has a deadline. The idea here would be to sell special fee services to other dexes. Dexes can pay X amount of money to have a special fee enabled till a certain date.
  8. The transferResidual can be based on an amount, like transferResidualTokenA and transferResidualTokenB, and if the token residual is higher than the specified amount send the residual to the swap executor. This is better than using a boolean and having to estimate the transfer residual that will be left, with this you can specify and only do it when you know you won't spend more gas on the residual transfer than needed. Also, the transferResidual is not tested.
  9. Tests against other dexes: Flattened code from other dexes factories and router can be added and generic tests can be run over them.
  10. Tests with USDT and KNC tokens. There is a comment on the contact where USDT and KNC tokens are mentioned in line 637, write tests that prove the need for that double approve.
  11. No complete tests coverage, more tets should be added to have ~100% tests coverage.
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

No branches or pull requests

1 participant