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

Add dynamic fee support for protocol and swap fees #3

Merged
merged 3 commits into from Jun 11, 2020

Conversation

AugustoL
Copy link

@AugustoL AugustoL commented May 10, 2020

This PR implements the smart contract changes discussed in issue #1.

Changes in TokenPair factory:

  • Adds protocolFee public variable, used as divider for the swap fee. protocolFeePercentage = swapFeePercentage * 1/protocolFee+1 changes
    uint8 public protocolFee = 5; // uses 0.05% (1/6 of 0.30%) per trade as default
  • Adds setProtocolFee and setSwapFee functions to token pair factory, they can be executed only by the feeToSetter address.changes
    function setProtocolFee(uint8 _protocolFee) external {
        require(msg.sender == feeToSetter, 'UniswapV2: FORBIDDEN');
        require(_protocolFee > 0, 'UniswapV2: FORBIDDEN_FEE');
        protocolFee = _protocolFee;
    }

    function setSwapFee(address pair, uint8 swapFee) external {
        require(msg.sender == feeToSetter, 'UniswapV2: FORBIDDEN');
        IUniswapV2Pair(pair).setSwapFee(swapFee);
    }
  • Adds swapFee public variable in token pair contract using 0.3 % as default.(changes)[]
    uint8 public swapFee = 30; // uses 0.3% fee as default
  • Adds setSwapFee function in the token pair, which can be called only by the pair factory contract. changes
    function setSwapFee(uint8 _swapFee) external {
        require(msg.sender == factory, 'UniswapV2: FORBIDDEN'); // sufficient check
        require(_swapFee <= 30, 'UniswapV2: FORBIDDEN_FEE'); // fee percentage check
        swapFee = _swapFee;
    }
  • Swap fee denominator is 10000 instead of 1000, allowing use two decimals swap fee. changes
  • Get protocolFee to be used in token pair form pair factory. changes

Added two test cases, one that tests the new protocolFee after changed and another that tests the swapFee after changed in #6

@AugustoL AugustoL self-assigned this May 10, 2020
@AugustoL AugustoL marked this pull request as draft May 10, 2020 21:06
@AugustoL AugustoL marked this pull request as ready for review May 12, 2020 13:00
@AugustoL
Copy link
Author

Tests added for different being used, on each test case multiple trades are executed and we check the right protocolFee is transferred in the next liquidity change.

@AugustoL
Copy link
Author

Next question is the protocolFee variable, should we change the name to something that implies that is the value for whats the swapFee is divided? Like swapFeeDivider and explain with a comment in the contract how to calculate the protocolFee based on that.
@jpkcambridge @pimato

@AugustoL AugustoL added this to In Progress in DXswap May 12, 2020
@pimato
Copy link
Collaborator

pimato commented May 12, 2020

@AugustoL what about protocolFeeDivisor ?

@AugustoL
Copy link
Author

@AugustoL what about protocolFeeDivisor ?

protocolFeeDivider ? ^^

@AugustoL AugustoL requested a review from pimato May 13, 2020 23:49
@AugustoL
Copy link
Author

@pimato We will leave the change of the protocolFee for the next PR.

- Adds protocolFee variable with a setter in the token factory.
- Adds a swapFee with a setter in the token pair and token pair factory contracts.
- Get the fees from the contract public variables when is used in the math.
@AugustoL
Copy link
Author

@pimato We will leave the change of the protocolFee for the next PR.

I just changed it here for protocolFeeDenominator

@nicoelzer nicoelzer self-requested a review May 15, 2020 13:14
@AugustoL AugustoL changed the base branch from master to develop June 11, 2020 13:52
@AugustoL AugustoL merged commit 2c19d79 into develop Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
DXswap
In Progress
Development

Successfully merging this pull request may close these issues.

None yet

6 participants