The Burn: spend or burn excess ETH, every N Noun mints #803
Conversation
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
packages/nouns-contracts/test/foundry/BurnUpgradeForkMainnetTest.t.sol
Outdated
Show resolved
Hide resolved
packages/nouns-contracts/script/executorV3AndExcessETHBurner/ProposeExecutorV3UpgradeBase.s.sol
Show resolved
Hide resolved
packages/nouns-contracts/test/foundry/governance/ExcessETHBurner.t.sol
Outdated
Show resolved
Hide resolved
this is unlikely, but may happen in case of a bad upgrade
… into verbs-spend-or-burn-excess-eth-every-n-auctions
davidbrai
changed the base branch from
verbs-auctionv2-infinite-oracle
to
master
packages/nouns-contracts/contracts/governance/ExcessETHBurner.sol
Outdated
Show resolved
Hide resolved
✅ Deploy Preview for nouns-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for nouns-home ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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
…rn-excess-eth-every-n-auctions
following feedback from 9999, we're doing a small modification to the burn spec in regards to when it can be invoked 🔥 The problem 9999 helped us identify: Let's say the current nextBurnNounId is 900 If at noun id 900 there is no excess ETH (which is also the point of the mechanism), then at any moment from then when there is excess ETH, a burn can be invoked immediately, because the current noun id is > 900 We think this defeats the purpose of predictable burn times The suggested fix: There will be a burn window of a few nouns, e.g. 3, during which a burn can be invoked If there was no burn in that window, then the next window will be at a multiple of nounIdsBetweenBurns Example: If we set the initial burn noun id to 1000, and the nounIdsBetweenBurns to 100, and the burn window to 3 The first burn can happen while the auctioned noun has id in the range 1000-1003. Only one burn can happen in a window Next burns can happen in the windows 1100-1103, 1200-1203, etc. regardless of whether a burn happened or not
| function warmUpSettlementState(uint256[] calldata nounIds) external { | ||
| for (uint256 i = 0; i < nounIds.length; ++i) { | ||
| if (settlementHistory[nounIds[i]].blockTimestamp == 0) { | ||
| settlementHistory[nounIds[i]] = SettlementState({ blockTimestamp: 1, amount: 0, winner: address(0) }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It's likely worth optimizing this function since it may be called to warm up thousands of slots.
e.g.
function warmUpSettlementState(uint256[] calldata nounIds) external {
uint256 nounIdCount = nounIds.length;
SettlementState storage settlementState;
for (uint256 i; i < nounIdCount;) {
settlementState = settlementHistory[nounIds[i]];
if (settlementState.blockTimestamp == 0) {
settlementState.blockTimestamp = 1;
}
unchecked {
++i;
}
}
}
| // Skip Nouner reward Nouns, they have no price | ||
| // Also skips IDs with no price data | ||
| if (settlementState.winner == address(0)) { | ||
| --latestNounId; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I'm not so sure that we should skip auctioned nouns with no price data. Any gaps in historical population may return unexpected price results that are confusing at best and could affect the burn at worst.
There was a problem hiding this comment.
@solimander At first I thought you meant nouns that are auctioned but got no bids, and therefore burned.
But now I think I understand you meant historical data gaps for which we didn't populate data.
Trying to build a more concrete example:
- We deploy this and start saving data starting from noun 900
- We populate past data for nouns 800-850
- Latest noun is at 925 and another contract calls
getPrices(50), then they get the prices for nouns 825-850 & 900-925 (ignoring nounder rewards for now), which is not what they're expecting.
- Is this what you meant? if not, could you provide an example that demonstrates the issue?
- Thinking what would be a better behavior in this case. Some options:
a. Don't skip the ids, but also don't return data for them. e.g. forgetPrices(50), returns only prices for nouns 900-925. forgetPrices(100)it would return prices for nouns 825-850 and 900-925.
b. Return 0 as the price for ids with no data. e.g.getPrices(50)would get prices for nouns 900-925 and 25 zeroes. Then the caller of the function would need to decide what to do with zeroes.
c. Any other ideas? - In both suggestions above, my understanding is that you think we should skip nounder rewards nouns, is that correct? To do that I think we would need to add the
id <= 1820 && id % 10 == 0logic here, right?
There was a problem hiding this comment.
- Is this what you meant? if not, could you provide an example that demonstrates the issue?
Yes, that's the case I'm referring to.
- Thinking what would be a better behavior in this case. Some options:
a. Don't skip the ids, but also don't return data for them. e.g. for getPrices(50), returns only prices for nouns 900-925. for getPrices(100) it would return prices for nouns 825-850 and 900-925.
b. Return 0 as the price for ids with no data. e.g. getPrices(50) would get prices for nouns 900-925 and 25 zeroes. Then the caller of the function would need to decide what to do with zeroes.
c. Any other ideas?
I think that's an improvement on the existing logic. Alternatively, we could revert if there aren't enough consecutive prices populated to service the query (excluding Nounder nouns). Perhaps we expose both and let the consumer decide which fits their use-case.
- In both suggestions above, my understanding is that you think we should skip nounder rewards nouns, is that correct? To do that I think we would need to add the id <= 1820 && id % 10 == 0 logic here, right?
Yes, that's right.
There was a problem hiding this comment.
Another case to consider:
A noun auctioned that got no bids, I think it this case it does make sense to skip, because it's not missing data.
There was a problem hiding this comment.
I think it this case it does make sense to skip
Wouldn't it make more sense to include that price in the response as it was the actual settlement price?
There was a problem hiding this comment.
Thoughts on the following? -
function getPrices(uint256 auctionCount) external view returns (uint256[] memory prices) {
uint256 latestNounId = auctionStorage.nounId;
if (!auctionStorage.settled && latestNounId > 0) {
latestNounId -= 1;
}
prices = new uint256[](auctionCount);
SettlementState memory settlementState;
uint256 actualCount;
for (uint256 id = latestNounId; id > 0 && actualCount < auctionCount; --id) {
if (id % 10 == 0 && id <= 1820) continue; // Skip Nounder nouns.
settlementState = settlementHistory[id];
if (settlementState.blockTimestamp <= 1) revert NOT_ENOUGH_CONSECUTIVE_PRICES();
prices[actualCount] = uint64PriceToUint256(settlementState.amount);
unchecked {
++actualCount;
}
}
if (auctionCount > actualCount) {
// this assembly trims the observations array, getting rid of unused cells
assembly {
mstore(prices, actualCount)
}
}
}There was a problem hiding this comment.
@solimander although unlikely, in case an auction doesn't get any bids and eventually the noun is burned, I think it's would be incorrect to assume the price there is zero. Since there was no actual transaction, I think the correct thing would be to skip it.
example: maybe there are no bids cause for some reason ethereum was down or gas prices were extreme.
wdyt?
There was a problem hiding this comment.
I see your point. Perhaps a more likely case is when a reserve price has been set and is not met in an auction.
Okay, +1 for skip when there is no winner.
|
|
||
| prices = new uint256[](auctionCount); | ||
| uint256 actualCount = 0; | ||
| while (actualCount < auctionCount && latestNounId > 0) { |
There was a problem hiding this comment.
May be worth optimizing this loop as it could run 90+ times in the proposed design.
e.g.
SettlementState memory settlementState;
uint256 actualCount;
for (uint256 id = latestNounId; id > 0 && actualCount < auctionCount; --id) {
if (id % 10 == 0 && id <= 1820) continue; // Skip Nounder nouns.
settlementState = settlementHistory[id];
if (settlementState.winner == address(0)) continue; // Skip IDs with no price data.
prices[actualCount] = uint64PriceToUint256(settlementState.amount);
unchecked {
++actualCount;
}
}
| */ | ||
| function treasuryValueInETH() public view returns (uint256) { | ||
| address owner_ = owner(); | ||
| return owner_.balance + stETH.balanceOf(owner_) + wETH.balanceOf(owner_) + rETHBalanceInETH(owner_); |
There was a problem hiding this comment.
Thoughts on including token buyer's ETH in this calculation as well? 10% of the treasury currently sits in the token buyer.
There was a problem hiding this comment.
let's discuss this.
to consider: token buyer's ETH is not part of the forked assets, so it doesn't participate in the BV
There was a problem hiding this comment.
@solimander we haven't considered including token buyer's ETH in the calculation so far.
agree that currently a big chunk of the treasury is there, but I think that's an anomaly rather than the norm. We're got to much ETH sent there because many were looking at the state of token buyer as a candidate and those transactions haven't since updated. I feel like this is a more of a bug with token buyer rather than a normal state. Perhaps token buyer should send ETH back to the DAO if it has too much? haven't thought about it enough.
Regardless, I think there's some logic in excluding it from the calculation here, because it doesn't participate in the assets sent to a fork. I don't feel strongly about it, so curious to hear your thoughts here.
also @eladmallel
There was a problem hiding this comment.
Okay, makes sense. I agree it likely doesn't make sense to add since it's not included in BV, but we should probably address this issue at some point.
There was a problem hiding this comment.
I thought about it for a brief moment some weeks ago and decided to leave it out for the reason you mentioned - since that ETH is anyway currently not part of the forking mechanism.
but since it has the potential to hold significant amounts of ETH, and has the potential to be used to DAO spending, it can be a weird way of gaming the burn against our intentions, and because it seems fairly simple to take it into account - I am now leaning towards including its ETH in this calculation.
@davidbrai let's just add it?
| * @notice Get the mean auction price of the last `numberOfPastAuctionsForMeanPrice` auctions. | ||
| * @dev Reverts if there is not enough auction history to calculate the mean auction price. | ||
| */ | ||
| function meanAuctionPrice() public view returns (uint256) { |
There was a problem hiding this comment.
May be worth adding a mean view function to the auction house itself to remove the need to re-loop over all prices. It also feels like it may be desirable for others who want to build on Nouns.
e.g. getMeanPrice(auctionCount)
| nounIdsBetweenBurns = newNounIdsBetweenBurns; | ||
| } | ||
|
|
||
| function setBurnWindowSize(uint16 newBurnWindowSize) external onlyOwner { |
…-excess-eth-every-n-auctions
No description provided.