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

Maker fee #64

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Maker fee #64

wants to merge 5 commits into from

Conversation

ivanzhelyazkov
Copy link
Collaborator

  • Add maker fee (strategy owner fee) logic
  • Add maker fee tests

Copy link
Contributor

@yudilevi yudilevi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just a few cosmetic comments

contracts/carbon/Strategies.sol Outdated Show resolved Hide resolved
contracts/carbon/Strategies.sol Outdated Show resolved Hide resolved
contracts/carbon/Strategies.sol Outdated Show resolved Hide resolved
@ivanzhelyazkov ivanzhelyazkov requested a review from yudilevi April 12, 2023 11:52
Copy link
Collaborator

@barakman barakman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a closer look (carefully review) of the impact of the maker fee in function _updateStrategy.

Specifically, where value is addressed a second time:

            // refund native token when there's no deposit in the order
            // note that deposit handles refunds internally
            if (token.isNative() && value > 0 && newOrders[i].y <= orders[i].y) {
                payable(address(owner)).sendValue(value);
            }

@@ -156,6 +158,8 @@ abstract contract Strategies is Initializable {

uint256 private constant ONE = 1 << 48;

uint256 private constant DEFAULT_MAKER_FEE = 5e14; // 0.0005 ETH
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that you can simply use 0.0005 ether

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to 0.0005 ether, removed comment - seems a bit clearer

contracts/carbon/Strategies.sol Show resolved Hide resolved
* @dev checks if maker fee has been sent with the transaction and returns updated tx value
*/
function _deductMakerFee(bool revertOnExcess, uint256 txValue) private returns (uint256) {
uint fee = _makerFee;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: uint256

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

contracts/carbon/Strategies.sol Show resolved Hide resolved
@ivanzhelyazkov ivanzhelyazkov requested a review from barakman April 12, 2023 13:21
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

Successfully merging this pull request may close these issues.

4 participants