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.

Sign up for GitHub

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

Update last total assets in accept cap #1

Closed
wants to merge 4 commits into from
Closed

Update last total assets in accept cap #1

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
36f930d
fix: update last total assets in accept cap
Jean-Grimal Dec 7, 2023
6c9a74e
refactor: use updatelasttotalassets function
Jean-Grimal Dec 7, 2023
bda9cd6
fix: move lasttotalassets udpate from accept cap to set cap
Jean-Grimal Dec 7, 2023
e365952
feat: add test
Jean-Grimal Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/MetaMorpho.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph

/// @notice Accepts the pending cap of the market defined by `id`.
function acceptCap(Id id) external afterTimelock(pendingCap[id].validAt) {
_updateLastTotalAssets(_accrueFee());

// Safe "unchecked" cast because pendingCap <= type(uint184).max.
_setCap(id, uint184(pendingCap[id].value));
}
Expand Down Expand Up @@ -779,6 +781,8 @@ contract MetaMorpho is ERC4626, ERC20Permit, Ownable2Step, Multicall, IMetaMorph
}

marketConfig.enabled = true;

_updateLastTotalAssets(totalAssets());
Rubilmax marked this conversation as resolved.
Show resolved Hide resolved
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
_updateLastTotalAssets(totalAssets());
_updateLastTotalAssets(newTotalAssets + MORPHO.expectedSupplyAssets(marketParams, address(this)));

And we could expect _setCap to have MarketParams as input (and in turn expect acceptCap) to have MarketParams as input

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that it changes the ABI, so we must notify @julien-devatom if we do it

Copy link
Contributor Author

@Jean-Grimal Jean-Grimal Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like _updateLastTotalAssets(totalAssets()); does the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we could just not accrue fees and only have _updateLastTotalAssets(lastTotalAssets + MORPHO.expectedSupplyAssets(marketParams, address(this))), this might be the better option no ?

And we could use idToMarketParams so no need for _setCap to have MarketParams

Copy link
Contributor

@Rubilmax Rubilmax Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This would symmetrical to update withdraw queue indeed 👍

However I push for having MarketParams as input because we can afford to have it in this context, whereas we can't in ERC4626 functions (and is this the reason we use idToMarketParams)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So Do I add MarketParams as input for acceptCap and submitMarketRemoval ? Or do I just change the Id input by MarketParams ?

Copy link
Contributor

Choose a reason for hiding this comment

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

To me the id can always be calculated given the market params so i think only the market params is necessary
I'd wait for others' thoughts beforehand thoug

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that it changes the ABI, so we must notify @julien-devatom if we do it

Not only me, all the integrators !! It's also critical for integrators (can break everything)

Copy link
Contributor

@MerlinEgalite MerlinEgalite Dec 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What do you mean by "can break everything", the fee accounting you mean?

}

marketConfig.removableAt = 0;
Expand Down
Loading