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

Jump to bottom

Storage Refactor #269

Closed
wants to merge 13 commits into from
Closed

Storage Refactor #269

wants to merge 13 commits into from

Conversation

pakim249CAL
Copy link
Contributor

@pakim249CAL pakim249CAL commented Aug 11, 2023 •
edited
Loading

Fixes #46

  • Some tests break because storage slots cheats don't work with packed slots

To be merged after #283

Rubilmax
Rubilmax reviewed Aug 11, 2023
View reviewed changes
Copy link
Collaborator

@Rubilmax Rubilmax left a comment

Choose a reason for hiding this comment

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

Insightful because you can see how many lines of code and changes required to go from basic solidity to OOP

@pakim249CAL pakim249CAL marked this pull request as ready for review August 11, 2023 12:29
@pakim249CAL pakim249CAL changed the title refactor: initial storage refactor draft Storage Refactor Aug 11, 2023
@pakim249CAL
Copy link
Contributor Author

Hardhat gas comparison

Before:
image

After:
image

@pakim249CAL
Copy link
Contributor Author

When ordering is changed to this:
image

Gas cost is:

image

@Rubilmax
Copy link
Collaborator

Hardhat gas comparison

Before:
image

After:
image

The gas diff of this is significant. It's like 10% improvement on average!

Rubilmax
Rubilmax reviewed Aug 12, 2023
View reviewed changes
src/libraries/MarketLib.sol Outdated

struct MarketState {
uint128 totalSupply; // Market total supply.
uint128 totalSupplyShares; // Market total supply shares.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the constraint of having shares as uint128 realistic? For a 18 decimals token, they are expected to have 36 decimals while 2^128 ~= 1e38

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me and @Jean-Grimal have discussed this a bit. We believe that it may be necessary to decrease the number of virtual shares to allow for this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#282

@pakim249CAL pakim249CAL changed the base branch from main to refactor/virtual-shares August 13, 2023 15:07
makcandrov
makcandrov reviewed Aug 14, 2023
View reviewed changes
Copy link
Contributor

@makcandrov makcandrov left a comment

Choose a reason for hiding this comment

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

I definitely think that this kind of refactoring is needed. However, I'm not completely satisfied with how it has been implemented here. Imo, MarketLib could be utilized to abstract away even more complexity from the main contract and further reduce the number of sloads. Also, I think that MarketLib currently contains way too many little functions. Still considering how it could be improved...

src/libraries/MarketLib.sol Outdated Show resolved Hide resolved
src/libraries/MarketLib.sol Outdated Show resolved Hide resolved
src/Morpho.sol Show resolved Hide resolved
MerlinEgalite
MerlinEgalite reviewed Aug 14, 2023
View reviewed changes
src/Morpho.sol Show resolved Hide resolved
src/libraries/MarketLib.sol Outdated Show resolved Hide resolved
src/Morpho.sol Show resolved Hide resolved
src/Morpho.sol Outdated Show resolved Hide resolved
@pakim249CAL pakim249CAL mentioned this pull request Aug 14, 2023
Closed
@pakim249CAL
Copy link
Contributor Author

I definitely think that this kind of refactoring is needed. However, I'm not completely satisfied with how it has been implemented here. Imo, MarketLib could be utilized to abstract away even more complexity from the main contract and further reduce the number of sloads. Also, I think that MarketLib currently contains way too many little functions. Still considering how it could be improved...

I agree with you, and don't think this should be the end of all refactoring. I am open to suggestions on how to improve things, but I would prefer that any additional changes come with new PRs as long as the current PR is an improvement and does not block further refactoring.

Jean-Grimal
Jean-Grimal previously approved these changes Aug 15, 2023
View reviewed changes
MathisGD
MathisGD reviewed Aug 15, 2023
View reviewed changes
src/libraries/MarketLib.sol
struct UserBalances {
uint128 borrowShares; // User' borrow balances.
uint128 collateral; // User' collateral balance.
uint128 supplyShares; // User' supply balances.
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
uint128 supplyShares; // User' supply balances.
uint256 supplyShares; // User' supply balances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree with this since I find it problematic to use different storage types for the same type of variable. Is it possible that this particular point be isolated to another PR with its own scope of discussion?

Made an issue for it #309

@Jean-Grimal
Copy link
Contributor

Jean-Grimal commented Aug 15, 2023 •
edited
Loading

When testing I realized that using uint128 for storage doesn't decrease the max amount that can be supplied or withdrawn.
Indeed, if totalBorrow is greater than 2 ** 128, and that someone wants to borrow 1/10th of the current total borrow, even with only virtualShares = 10, this line would revert :
uint256 borrowed = borrowShares[id][user].toAssetsUp(totalBorrow[id], totalBorrowShares[id]);
So using uint128 for storage brings no additional limitations to the max amounts.

@pakim249CAL pakim249CAL mentioned this pull request Aug 15, 2023
Closed
This was referenced Aug 17, 2023
Closed
Closed
Closed
@QGarchery QGarchery mentioned this pull request Aug 18, 2023
Merged
Base automatically changed from refactor/virtual-shares to main August 18, 2023 16:22
@MathisGD MathisGD dismissed Jean-Grimal’s stale review August 18, 2023 16:22

The base branch was changed.

@MathisGD
Copy link
Contributor

We choose #355 (see #350)

@MathisGD MathisGD closed this Aug 18, 2023
@makcandrov makcandrov deleted the refactor/storage branch August 18, 2023 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MathisGD MathisGD MathisGD left review comments

@Jean-Grimal Jean-Grimal Jean-Grimal left review comments

@QGarchery QGarchery Awaiting requested review from QGarchery

@Rubilmax Rubilmax Awaiting requested review from Rubilmax

@MerlinEgalite MerlinEgalite Awaiting requested review from MerlinEgalite

@makcandrov makcandrov Awaiting requested review from makcandrov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consolidate storage
6 participants
@pakim249CAL @Rubilmax @Jean-Grimal @MathisGD @MerlinEgalite @makcandrov