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

feat: add fee #63

Closed
wants to merge 16 commits into from
Closed

feat: add fee #63

wants to merge 16 commits into from

Conversation

pakim249CAL
Copy link
Contributor

@pakim249CAL pakim249CAL commented Jul 5, 2023

Fixes #62

Just contains minimal fee logic for a global fee with a global fee recipient. We can expand on this in future PRs.

Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

approving the idea!

src/Blue.sol Outdated Show resolved Hide resolved
src/Blue.sol Outdated Show resolved Hide resolved
src/Blue.sol Outdated Show resolved Hide resolved
@MerlinEgalite
Copy link
Contributor

@pakim249CAL can you continue this PR?

src/Blue.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@QGarchery QGarchery left a comment

Choose a reason for hiding this comment

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

Cool implementation, it's nice that we only have to change that one place and that it's done at each interaction ❤️

src/Blue.sol Outdated Show resolved Hide resolved
src/Blue.sol Outdated Show resolved Hide resolved
@MerlinEgalite
Copy link
Contributor

Can we make it a per market fee @pakim249CAL ?

makcandrov
makcandrov previously approved these changes Jul 7, 2023
src/Blue.sol Outdated Show resolved Hide resolved
src/Blue.sol Outdated Show resolved Hide resolved
src/Blue.sol Show resolved Hide resolved
@@ -78,6 +79,11 @@ contract Blue {
owner = newOwner;
}

function setFee(MarketParams calldata marketParams, uint fee) external onlyOwner {
require(fee <= WAD, "fee must be <= 1");
_marketStorage[marketParams.toId()].market.fee = fee;
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not cost much to check that the market where we change the fee exists (since this is an admin function)

Copy link
Collaborator

@Rubilmax Rubilmax Jul 10, 2023

Choose a reason for hiding this comment

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

May have been enforced via a private or internal getter :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, this is what I proposed here. It has been closed because input validation is pretty minimal right now, only 2 things to check that are not related: the id corresponds to an existing market, the amount is non zero. I'm ok with either solution

Copy link
Contributor

Choose a reason for hiding this comment

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

The fee should be capped, just created an issue for that #110

QGarchery
QGarchery previously approved these changes Jul 10, 2023
Rubilmax
Rubilmax previously approved these changes Jul 11, 2023
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

Could you move the base to main, I think we agree on the fee mechanism but it's not obvious that we all agree on the storage refactoring

@@ -78,6 +79,11 @@ contract Blue {
owner = newOwner;
}

function setFee(MarketParams calldata marketParams, uint fee) external onlyOwner {
require(fee <= WAD, "fee must be <= 1");
_marketStorage[marketParams.toId()].market.fee = fee;
Copy link
Contributor

Choose a reason for hiding this comment

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

The fee should be capped, just created an issue for that #110

@@ -296,6 +302,12 @@ contract Blue {
uint256 accruedInterests = marketTotalBorrow.wMul(borrowRate).wMul(block.timestamp - m.lastUpdate);
m.totalSupply = marketTotalSupply + accruedInterests;
m.totalBorrow = marketTotalBorrow + accruedInterests;
if (m.fee != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (m.fee != 0) {
if (m.fee != 0) {

Let's make the code breath a bit

@@ -296,6 +302,12 @@ contract Blue {
uint256 accruedInterests = marketTotalBorrow.wMul(borrowRate).wMul(block.timestamp - m.lastUpdate);
m.totalSupply = marketTotalSupply + accruedInterests;
m.totalBorrow = marketTotalBorrow + accruedInterests;
if (m.fee != 0) {
uint256 fee = accruedInterests.wMul(m.fee);
uint256 feeShares = fee.wMul(m.totalSupplyShares).wDiv(m.totalSupply - fee);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we explain the reasoning of the "-"?

@pakim249CAL pakim249CAL changed the base branch from refactor/consolidate-storage to main July 11, 2023 14:11
@pakim249CAL pakim249CAL dismissed stale reviews from Rubilmax, QGarchery, and makcandrov July 11, 2023 14:11

The base branch was changed.

@pakim249CAL
Copy link
Contributor Author

Redone here on main #116

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.

Implement fee logic
5 participants