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.
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
Add Launch Auction Price Oracle with reduced half life #70
Changes from 9 commits
7dd9033
6b37b28
facd077
0e056b4
edea68f
0a6684a
4774c14
6fb4de2
a2f7087
17a5a48
0f0589f
d4534f7
9f22d48
ea08a84
61c873e
1563df6
f5826c9
71394e9
74310cb
1e46dc1
01ac27f
e9b954d
eba4cfa
ac267c2
11936e7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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:
https://github.com/base-org/basenames/blob/b147ced84476b0d93b71e3500f1a90823b60a00f/src/L2/RegistrarController.sol#L273
Then, we do a pair of transactions in quick succession to "turn on" registrations.
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.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.
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? RenametotalDays
toauctionDuration
, store it as animmutable
and skip usingendValue
entirely (unless it's needed somewhere else?).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.
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"
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 subtractendValue
on the UI to show users. By my estimates,endValue
should be100 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:
https://github.com/base-org/basenames/blob/b147ced84476b0d93b71e3500f1a90823b60a00f/src/L2/RegistrarController.sol#L389-L398
I think that the natspec here is accurate. This method only returns the pure price premium. The implementation handles the use-case specific logic.