AuctionHouseV2: price feed, gas optimizations #799
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
many tests and refinements TODO
trying to assess auction house gas v1 vs v2 best to use what's used on mainnet now
taking a more tolerant appraoch since auction house can work fine without the oracle
settleAuction is protected from re-entry by requiring `settled` to be false and setting it to true first thing after the require statements
should make it clearer that this function can't be abused via reentry msg.value is required to be higher than the previous amount + min increment by setting `auction.amount` right after that requirement, it's clear the next reentry will have to provide a higher amount and so on
per this issue found in the latest C4 audit: code-423n4/2022-08-nounsdao-findings#382
created AuctionV2 that fits into 2 storage slots instead of 5 previously added a migration contract between V1 and V2 that rewrites auction data for V2
a bit more gas savings
in case the DAO wants to start the spend or burn sooner
davidbrai
reviewed
Oct 4, 2023
davidbrai
reviewed
Oct 4, 2023
davidbrai
reviewed
Oct 4, 2023
davidbrai
reviewed
Oct 4, 2023
davidbrai
reviewed
Oct 4, 2023
davidbrai
reviewed
Oct 4, 2023
|
|
||
| /// @notice Whether this contract is paused or not | ||
| /// @dev Replaces the state variable from PausableUpgradeable, to bit pack this bool with `auction` and save gas | ||
| bool public __paused; |
There was a problem hiding this comment.
@eladmallel I don't this is actually packs it in storage:
Items following struct or array data always start a new storage slot.
from: https://docs.soliditylang.org/en/latest/internals/layout_in_storage.html
There was a problem hiding this comment.
you can run the build with storage layout output to see it also:
{
"astId":50,
"contract":"contracts/NounsAuctionHouseV2.sol:NounsAuctionHouseV2",
"label":"auction",
"offset":0,
"slot":"202",
"type":"t_struct(AuctionV2)1382_storage"
},
{
"astId":53,
"contract":"contracts/NounsAuctionHouseV2.sol:NounsAuctionHouseV2",
"label":"__paused",
"offset":0,
"slot":"204",
"type":"t_bool"
},
There was a problem hiding this comment.
if you want to pack it, I think it has to go into the auction struct
There was a problem hiding this comment.
(copy pasting comment from discord)
placing it after the AuctionV2 struct puts it in a new storage slot.
to gain gas savings, it needs to be in the same slot as the AuctionV2, because only that is accessed during settleCurrentAndCreateNewAuction.
I'm apprehensive to add it to the struct because:
- it's not logically part of that struct
- we'll need to change the
auction()view function to return a struct without paused for backwards compatibility.
in case the settlements were warmed up, the timestamp in change to 1 and the previous check would fail
to reduce diff and it's easier to debug in many cases
eladmallel
commented
Oct 9, 2023
it's not bit-packed when placed after the auction struct and for now we prefer to not force it into the auction struct esp. since bigger gas savings are coming from descriptor storage read optimizations
getPrices(n)
- returns exactly `n` latest auction prices
- skips auctions with no bids
- skips nounder noun ids
- reverts if auction should have data but doesn't
- reverts if not enough items in history (not need to trim)
-> either returns array of n-length or reverts
getSettlements(n, skipEmptyValues)
- returns up to `n` latest settlement (in case we reach id 0)
- includes auction with no bids (blockTimestamp will be > 1)
- skips nounder noun ids if skipEmptyValues = true, otherwise return raw
- skip unset data if skipEmptyValues = true, otherwise return raw
getSettlements(x, y, skipEmptyValues)
- returns settlements for ids [x,y)
- includes auction with no bids (blockTimestamp will be > 1)
- skips nounder noun ids if skipEmptyValues = true, otherwise return raw
- skip unset data if skipEmptyValues = true, otherwise return raw
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.