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

Add Launch Auction Price Oracle with reduced half life #70

Merged
merged 25 commits into from
Aug 14, 2024

Conversation

stevieraykatz
Copy link
Collaborator

@stevieraykatz stevieraykatz commented Jul 21, 2024

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

endValue = startPremium >> totalDays;

To

       endValue = startPremium >> (totalDays * 24); // 1 hour half life

And

From

uint256 secondsInPeriod = 1 days;
/// @dev 50% decay per period in wad format
uint256 perPeriodDecayPercentWad = FixedPointMathLib.WAD / 2;
uint256 premium = EDAPrice.currentPrice(startPremium, elapsed, secondsInPeriod, perPeriodDecayPercentWad);

To

    /// @notice The half-life of the premium price decay
    uint256 constant PRICE_PREMIUM_HALF_LIFE = 1 hours;
...
        return EDAPrice.currentPrice(startPremium, elapsed, PRICE_PREMIUM_HALF_LIFE, perPeriodDecayPercentWad);

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Aug 5, 2024

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

Comment on lines 61 to 71
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

src/L2/LaunchAuctionPriceOracle.sol Fixed Show fixed Hide fixed
src/L2/LaunchAuctionPriceOracle.sol Fixed Show fixed Hide fixed
@stevieraykatz stevieraykatz changed the title Add parameterized auction half life Add Launch Auction Price Oracle with reduced half life Aug 5, 2024
@stevieraykatz stevieraykatz marked this pull request as ready for review August 5, 2024 21:35
Copy link
Collaborator

@ilikesymmetry ilikesymmetry left a 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 Show resolved Hide resolved
/// @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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

Comment on lines +62 to +70
if (expires > block.timestamp) {
return 0;
}
uint256 elapsed = block.timestamp - expires;
uint256 premium_ = decayedPremium(elapsed);
if (premium_ > endValue) {
return premium_ - endValue;
}
return 0;
Copy link
Collaborator

@ilikesymmetry ilikesymmetry Aug 5, 2024

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?).

Suggested change
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);

Copy link
Collaborator Author

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.

@ilikesymmetry ilikesymmetry self-requested a review August 7, 2024 18:07
@cb-heimdall
Copy link
Collaborator

Review Error for ilikesymmetry @ 2024-08-07 18:08:31 UTC
User failed mfa authentication, see go/mfa-help

/// https://github.com/ensdomains/ens-contracts/blob/staging/contracts/ethregistrar/ExponentialPremiumPriceOracle.sol
///
/// @author Coinbase (https://github.com/base-org/usernames)
contract LaunchAuctionPriceOracle is StablePriceOracle {
Copy link
Contributor

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?

Copy link
Collaborator Author

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:

  1. Deploy this new LaunchAuctionPriceOracle along with the GA Contracts
  2. Use this oracle for the launch auction duration
  3. After it concludes, switch over to using the audited ExponentialPremiumPriceOracle

Copy link
Contributor

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?

Copy link
Collaborator Author

@stevieraykatz stevieraykatz Aug 8, 2024

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:

IPriceOracle prices_,

Then, we do a pair of transactions in quick succession to "turn on" registrations.

  1. Set the Launch Time using:
    function setLaunchTime(uint256 launchTime_) external onlyOwner {
  2. Set the RegistrarController as the controller for the BaseRegistrar via:
    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);
Copy link
Contributor

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

Copy link
Collaborator Author

@stevieraykatz stevieraykatz Aug 8, 2024

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 {
Copy link
Contributor

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?

Copy link
Contributor

@wilsoncusack wilsoncusack Aug 8, 2024

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

Copy link
Collaborator Author

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!

wilsoncusack
wilsoncusack previously approved these changes Aug 8, 2024
@cb-heimdall cb-heimdall dismissed wilsoncusack’s stale review August 8, 2024 20:20

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
@stevieraykatz stevieraykatz mentioned this pull request Aug 9, 2024
@stevieraykatz
Copy link
Collaborator Author

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.

Copy link
Contributor

@wilsoncusack wilsoncusack left a 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

@cb-heimdall
Copy link
Collaborator

Review Error for wilsoncusack @ 2024-08-12 17:27:20 UTC
User failed mfa authentication, see go/mfa-help

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 {
Copy link
Contributor

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)

  1. How the price of an unclaimed name changes before and after switching the oracles
  2. 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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. 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);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. 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).

Copy link
Collaborator

@ilikesymmetry ilikesymmetry left a 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!

Comment on lines 27 to 28
function test_decayedPremium_threePeriods() public view {
uint256 elapsed = 3 hours;
Copy link
Collaborator

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.

Copy link
Collaborator Author

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) {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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:

/// @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)
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed comment.

@cb-heimdall
Copy link
Collaborator

Review Error for ilikesymmetry @ 2024-08-13 16:46:24 UTC
User failed mfa authentication, see go/mfa-help

@stevieraykatz stevieraykatz merged commit df3b07f into main Aug 14, 2024
5 of 6 checks passed
@stevieraykatz stevieraykatz deleted the launch-auction branch August 14, 2024 21:37
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;
Copy link
Collaborator

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?

  1. the base prices will be the same for both oracles.
  2. 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, before MIN_REGISTRATION_DURATION + GRACE_PERIOD

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

@stevieraykatz stevieraykatz Aug 19, 2024

Choose a reason for hiding this comment

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

  1. Base prices will be the same for both price oracles
  2. Yes! See integration test for expected switch over flow

Copy link
Collaborator

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.
Copy link
Collaborator

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);
Copy link
Collaborator

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)

Copy link
Collaborator Author

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;
Copy link
Collaborator

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

Comment on lines +21 to +28
/*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/
/* 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;
Copy link
Collaborator

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.

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.

8 participants