Refine domain logic #343
Refine domain logic #343
Conversation
Stop accepting null `domain` param and remove redundant logic.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 54ab2d3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| return false; | ||
| if (releaseTimestamp && releaseTimestamp.time > atTimestamp.time) { | ||
| // if the name is not yet released, we can't register it | ||
| // TODO: check for possible off-by-1 errors in the logic above |
There was a problem hiding this comment.
@lightwalker-eth can you extend on off-by-1 spec?
There was a problem hiding this comment.
We need to see how ENS smart contracts actually work. Ex: Actual expiration time? 1 second earlier? 1 second later?
There was a problem hiding this comment.
The above implements seems correct due to the following ENS SC reference:
Conclusion: both functions are saying a domain registration's timestamp needs to be greater than a domain's releaseTimestamp
|
|
||
| // first label must be of sufficient length | ||
| return charCount(name.labels[0]) >= MIN_ETH_REGISTRABLE_LABEL_LENGTH; | ||
| if (releaseTimestamp && releaseTimestamp.time > atTimestamp.time) { |
There was a problem hiding this comment.
| if (releaseTimestamp && releaseTimestamp.time > atTimestamp.time) { | |
| if (releaseTimestamp && releaseTimestamp.time >= atTimestamp.time) { |
| return charCount(name.labels[0]) >= MIN_ETH_REGISTRABLE_LABEL_LENGTH; | ||
| if (releaseTimestamp && releaseTimestamp.time > atTimestamp.time) { | ||
| // if the name is released, we can't renew it anymore | ||
| // TODO: check for possible off-by-1 errors in the logic above |
There was a problem hiding this comment.
This function was extracted from BaseRegistrar and reveals that above logic could look different: added a commit suggestion above!
| atTimestamp.time < existingRegistration.registrationTimestamp.time | ||
| ) { | ||
| // if the renewal is requested before the registration, we can't renew it | ||
| // TODO: check for possible off-by-1 errors in the logic above |
There was a problem hiding this comment.
OK...
I'd say there is no problem with current logic.
If atTimestamp is the same than registrationTimestamp, it is already possible to renew it for longer periods, meaning canRenew should go on in the function scope, but, if it is smaller, it really is not possible to renew this domain: as per my review this has achieved its goal.
| throw new Error(`Invariant violation`); // TODO: refine message... just making the type system happy. | ||
| } | ||
|
|
||
| // TODO: review for possible off-by-1 errors |
There was a problem hiding this comment.
Our logic seems to implement the same as below:
When summing expiries[id] + duration to determine the new timestamp.
No description provided.