-
Notifications
You must be signed in to change notification settings - Fork 32
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
Add Launch Auction Price Oracle with reduced half life #70
Conversation
🟡 Heimdall Review Status
|
src/L2/LaunchAuctionPriceOracle.sol
Outdated
function _premium(string memory, uint256 expires, uint256) internal view override returns (uint256) { | ||
if (expires > block.timestamp) { | ||
return 0; | ||
} | ||
uint256 elapsed = block.timestamp - expires; | ||
uint256 premium = decayedPremium(elapsed); | ||
if (premium > endValue) { | ||
return premium - endValue; | ||
} | ||
return 0; | ||
} |
Check notice
Code scanning / Slither
Block timestamp Low
Dangerous comparisons:
- expires > block.timestamp
- premium_ > endValue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last comment is most important. Curious on why we need endValue
versus just storing a auctionDuration
and comparing against that for the premium calculation.
Another general comment is to make the half-life a dynamic constructor argument as well. This paired with the auctionDuration
change feels like the most ergonomic deployment process let product fine-tune these two values up until launch without code changes
src/L2/LaunchAuctionPriceOracle.sol
Outdated
/// @param totalDays The total duration (in days) for the dutch auction. | ||
constructor(uint256[] memory rentPrices, uint256 startPremium_, uint256 totalDays) StablePriceOracle(rentPrices) { | ||
startPremium = startPremium_; | ||
endValue = startPremium >> (totalDays * 24); // 1 hour halflife |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a relationship between the half-life constant and the math here. Could we rewrite so that this relationship is codified using the constant vs relying on comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another case of trying to minimize code changes while achieving the desired functionality. Originally, I unified them (and parameterized half life) but I think that the risk introduced from a larger tear-up isn't worth the squeeze.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ended up unifying these concepts. It became necessary once the units of the halflife and the units of the auction were both denominated in hours. Check it out and lmk what you think.
if (expires > block.timestamp) { | ||
return 0; | ||
} | ||
uint256 elapsed = block.timestamp - expires; | ||
uint256 premium_ = decayedPremium(elapsed); | ||
if (premium_ > endValue) { | ||
return premium_ - endValue; | ||
} | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am really struggling to get a clear grasp of this code and how it behaves at the boundaries even after a few rereads. What do you think of rewriting without the endValue
abstraction and just focusing on using timestamps like your comment describes? Rename totalDays
to auctionDuration
, store it as an immutable
and skip using endValue
entirely (unless it's needed somewhere else?).
if (expires > block.timestamp) { | |
return 0; | |
} | |
uint256 elapsed = block.timestamp - expires; | |
uint256 premium_ = decayedPremium(elapsed); | |
if (premium_ > endValue) { | |
return premium_ - endValue; | |
} | |
return 0; | |
if (expires > block.timestamp || block.timestamp - expires > auctionDuration) { | |
return 0; | |
} | |
return decayedPremium(block.timestamp - expires); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I generally agree that this code could use some cleanup, it's a forked and audited contract from ENS that we're making a slight tweak to. Not sure that the cleanup is worth the risk of introducing a regression.
Review Error for ilikesymmetry @ 2024-08-07 18:08:31 UTC |
/// https://github.com/ensdomains/ens-contracts/blob/staging/contracts/ethregistrar/ExponentialPremiumPriceOracle.sol | ||
/// | ||
/// @author Coinbase (https://github.com/base-org/usernames) | ||
contract LaunchAuctionPriceOracle is StablePriceOracle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will there be a separate PR that utilizes this new oracle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow. In case this context helps, the flow for launch will be:
- Deploy this new LaunchAuctionPriceOracle along with the GA Contracts
- Use this oracle for the launch auction duration
- After it concludes, switch over to using the audited ExponentialPremiumPriceOracle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How and when does 2. happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's set as the price oracle upon deployment of the GA Registrar Controller:
basenames/src/L2/RegistrarController.sol
Line 273 in b147ced
IPriceOracle prices_, |
Then, we do a pair of transactions in quick succession to "turn on" registrations.
- Set the Launch Time using:
basenames/src/L2/RegistrarController.sol
Line 330 in b147ced
function setLaunchTime(uint256 launchTime_) external onlyOwner { - Set the RegistrarController as the controller for the BaseRegistrar via:
basenames/src/L2/BaseRegistrar.sol
Line 208 in b147ced
function addController(address controller) external onlyOwner {
|
||
contract ExponentialPremiumFuzzTest is LaunchAuctionPriceOracleBase { | ||
function test_decayedPremium_decreasingPrices(uint256 elapsed) public view { | ||
elapsed = bound(elapsed, 0, totalDays * 1 days); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe have a test where you don't bound and assert always 0 after elapsed? or could have a conditional in the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added fuzz test test_fuzzDecayedPremium_matchesExpectedValue
which fuzzes over 100 years of elapsed time, leveraging new _calculateDecayedPremium
helper.
import {LaunchAuctionPriceOracleBase} from "./LaunchAuctionPriceOracleBase.t.sol"; | ||
|
||
contract DecayedPremium is LaunchAuctionPriceOracleBase { | ||
function test_decayedPremium_zeroElapsed() public view { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't totally follow how this file differs from the the fuzz test one? Why are they named differently? Is the only difference fuzz test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and then I wonder if fuzz needs their own file? if so, I wonder what we think about some like .fuzz.sol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured that sometimes you might want to run the fuzz test file with special params and so having it in its own file might make sense. I like the idea of coalescing around a pattern for fuzzing though. fuzz.sol
is a good start!
Approved review 2228757758 from wilsoncusack is now dismissed due to new commit. Re-request for approval.
…in hours, validate that the duration is divisible by the halflife
New product requirements are for the Launch Auction to be 1.5hr halflife over a 36hr auction duration. Needed to make a couple slight tweaks to support this. @wilsoncusack @ilikesymmetry @cb-elileers would appreciate your eyes on this in its current form. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to have one test with some nice round numbers, like check that after half a day the price is halved or whatever, to sanity check
Review Error for wilsoncusack @ 2024-08-12 17:27:20 UTC |
Co-authored-by: Alexis Williams <148368153+awilliams1-cb@users.noreply.github.com>
import {IntegrationTestBase} from "./IntegrationTestBase.t.sol"; | ||
import {RegistrarController} from "src/L2/RegistrarController.sol"; | ||
|
||
contract LaunchAuctionRegistrations is IntegrationTestBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevieraykatz Thanks so much for these. Was thinking it could help to see (lmk if I missed)
- How the price of an unclaimed name changes before and after switching the oracles
- How a auction price changes before and after oracles (I know we only have 28 day registration so this is impossible, but if it's easy to test it would be interesting to see)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I added a couple lines to test
test_simulatePostAuctionConfig_register
which asserts that the price for a name is the same before/after the oracle gets switched. See:
// Get price before oracle changes
uint256 priceBeforeSwitch = registrarController.registerPrice(name, duration);
...
assertEq(priceBeforeSwitch, price);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Added new test to demonstrate the case you're interested in
test_showAuctionPriceDifference
. Output at the beginning of the auction:
Logs:
Price with launch auction: 100000993812011095261
Price with prod auction: 1000000522935317369675
output if we jump 1 day into the auction:
Logs:
Price with launch auction: 2519690917345161
Price with prod auction: 500000522935317368675
Basically we get what we expect. The price is 100 ether for the launch auction and 1000 ether with the prod auction contract at t = expiry + GRACE_PERIOD
. At t = expiry + GRACE_PERIOD + 1 day
we get a much lower launch auction price (because it decays so rapidly) and a halved prod auction (1-day half life).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some clarifications. Thanks for putting together the contingency tests. I like how clearly we can see these plans laid out and align on them ahead of needing to do it!
function test_decayedPremium_threePeriods() public view { | ||
uint256 elapsed = 3 hours; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: but this is no longer 3 periods because the half life is 1.5 hours, so should be 4.5 hours. Maybe rewrite the test to import the same constant and do uint256 elapsed = 3 * PRICE_PREMIUM_HALF_LIFE
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to using PRICE_PREMIUM_HALF_LIFE
constant
/// @param elapsed Seconds elapsed since the auction started. | ||
/// | ||
/// @return Dacayed price premium denominated in wei. | ||
function decayedPremium(uint256 elapsed) public view returns (uint256) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand the computation mechanism premium_ - endValue
in _premium()
correctly, our frontend should take whatever value is returned by this and subtract endValue
on the UI to show users. By my estimates, endValue
should be 100 ether * 0.5 ** (36 / 1.5) => 0.0000059605 ether
. I recall we have some refund mechanism built into the contracts because the decay will reduce the price even further by the time the transaction validates so this isn't a big deal then.
Noting this more to confirm my own understanding, but maybe worth a dev natspec on this function that this value is an overestimate of what the user actually needs to pay given those two factors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
F/E doesn't need to do anything special. The RegistrarController does the heavy lifting parsing the response. See:
basenames/src/L2/RegistrarController.sol
Lines 389 to 398 in b147ced
/// @notice Checks the register price for a provided `name` and `duration`. | |
/// | |
/// @param name The name to check the register price of. | |
/// @param duration The time that the name would be registered. | |
/// | |
/// @return The all-in price for the name registration, denominated in wei. | |
function registerPrice(string memory name, uint256 duration) public view returns (uint256) { | |
IPriceOracle.Price memory price = rentPrice(name, duration); | |
return price.base + price.premium; | |
} |
I think that the natspec here is accurate. This method only returns the pure price premium. The implementation handles the use-case specific logic.
// Jump forward 1 day | ||
vm.warp(LAUNCH_TIME + 1 days); | ||
|
||
// Set the launch time to 0 (no-op for launch pricing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does no-op for launch pricing
mean? Does setting launch time to 0
effectively kill the ability for people to register?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah bad comment. RegistrarController "launchTime" is used as a replacement for expiry on new names. This is what enables the launch auction (when normally names would only get an auction after expiring). I'll fix the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed comment.
Review Error for ilikesymmetry @ 2024-08-13 16:46:24 UTC |
uint256 constant LAUNCH_TIME = 1723420800; // Monday, August 12, 2024 12:00:00 AM GMT | ||
|
||
uint256 constant EXPIRY_AUCTION_START_PRICE = 1000 ether; | ||
uint256 constant EXPIRY_AUCTION_DURATION_DAYS = 21; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevieraykatz do the deploy scripts need to be updated? DeployPriceOracle
still uses 28 days there, 21 days here.
I'm currently having these assumptions, can you confirm if they're true for the actual deployment?
- the base prices will be the same for both oracles.
- the prod oracle will replace the launch oracle without having a sudden change in the
rentPrice
. Practically, this means when both premiums match, I assume you do this when both premiums are 0 after the max of both auction duration times (21 days). Also need to do this before you can re-register an account, beforeMIN_REGISTRATION_DURATION + GRACE_PERIOD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that script is outdated. We use internal tooling to deploy. Exp. premium price oracle will use 21-day duration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Base prices will be the same for both price oracles
- Yes! See integration test for expected switch over flow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, makes sense to me then
/// | ||
/// @param elapsed Seconds elapsed since the auction started. | ||
/// | ||
/// @return Dacayed price premium denominated in wei. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Dacayed" -> "Decayed"
baseRegistrar.addController(address(registrarController)); | ||
|
||
vm.prank(owner); | ||
registrarController.setLaunchTime(LAUNCH_TIME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevieraykatz what's the process for assuring LAUNCH_TIME
is not set in the future relative to block.timestamp
?
Also at launch maybe us a multicall approach or set the launch time on the registrarController
before adding it to the baseRegistrar
as a controller for any spam transactions trying to lodge them in between both (not sure if this is possible with the sequencer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're unable to use multicall due to the ownership restrictions on both methods. We're going to set the launchTime to say 12:00PST and then execute the TX at 12:00 so we'll miss the first couple seconds of the auction but it won't be in the future.
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ | ||
|
||
/// @notice The half-life of the premium price decay in seconds. | ||
uint256 constant PRICE_PREMIUM_HALF_LIFE = 1.5 hours; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PRICE_PREMIUM_HALF_LIFE
constant can be made public
/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ | ||
/* STORAGE */ | ||
/*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ | ||
/// @notice Starting premium for the dutch auction. | ||
uint256 public immutable startPremium; | ||
|
||
/// @notice Ending value of the auction, calculated on construction. | ||
uint256 public immutable endValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Immutable variables are not actually part of contract's storage if that's the intention of STORAGE
comment.
Add a new contract for handling the Launch auction. We accomplish this by changing the half life encoded in ExponentialPremiumPriceOracle from 1 day to 1.5 hours.
We achieve this in two places:
From
basenames/src/L2/ExponentialPremiumPriceOracle.sol
Line 15 in b147ced
To
And
From
basenames/src/L2/ExponentialPremiumPriceOracle.sol
Lines 39 to 42 in b147ced
To