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(extsload): expose generic storage getter #147

Merged
merged 3 commits into from
Jul 25, 2023
Merged

Conversation

Rubilmax
Copy link
Collaborator

@Rubilmax Rubilmax commented Jul 19, 2023

This solution can supplant #138 by just exposing a generic storage getter named extsload.

2 design choices were made:

  1. The getter expects an array of slots instead of a single slot, because it is expected that integrators need multiple, non-contiguous storage values at once
  2. There is no contiguous storage getter because for now, we don't have storage values larger than 32 bytes

If this change is accepted, I believe we should build a simple library helping integrators access the storage value they want, without thinking about the storage slots:

library BlueStorageSlots {
  function totalSupply(Id id) internal view returns (bytes32) {
    return keccak256(abi.encode(id, 5));
  }
}

Note that if we'd like to provide easy integrations, we could also provide a library for calculating accrued interests:

library BlueLib {
  function accruedInterests(IBlue blue, Id id) internal view returns (uint256 interests, uint256 feeShares) {
    // Prepare storage slot array
    // Call extsload
    // Mimick interests calculation
  }
}

But then the only advantage of doing so to have a simpler Blue contract (we don't have to separate calculation and storage update, like it's done in #138) but we force integrators to have duplicated code (interests calculation). I'm not sure this trade-off is cool for integrators...

Jean-Grimal
Jean-Grimal previously approved these changes Jul 19, 2023
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.

Love it, and I agree with most points in the description in the PR. In particular, I agree that there should be a library for the slots, can you create an issue @Rubilmax ?

But then the only advantage of doing so to have a simpler Blue contract (we don't have to separate calculation and storage update, like it's done in #138) but we force integrators to have duplicated code (interests calculation). I'm not sure this trade-off is cool for integrators...

Thinking about it, it may be that integrators have potentially different use cases that cannot all be covered in the core contract. For example, they might want to know what would be the rate given that they add some borrow, and what would be the interest computation given a particular rate, etc. I agree that it comes at the cost of increase in bytecode size of the integrator contract, but it may not be significant for simple use cases, and for more complex cases they could use a similar solution to deploying a lens contract

src/Blue.sol Outdated Show resolved Hide resolved
test/forge/Blue.t.sol Outdated Show resolved Hide resolved
test/forge/Blue.t.sol Outdated Show resolved Hide resolved
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.

really agree with the idea, but I think we should put this PR on hold since each we'll change the storage layout the test will basically break OR we implement very general test where we randomly override the state and try to read it. Wdyt?

test/forge/Blue.t.sol Show resolved Hide resolved
src/Blue.sol Show resolved Hide resolved
@MathisGD MathisGD merged commit 98d3132 into main Jul 25, 2023
@MathisGD MathisGD deleted the refactor/extsload branch July 25, 2023 21:49
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.

7 participants