Skip to content

fix: bug related to pop name reservation #236

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.

Sign up for GitHub

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

Merged
sphamjoli merged 2 commits into from
Aug 17, 2026
Merged

fix: bug related to pop name reservation #236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94db427
fix: bug related to pop name reservation
sphamjoli Aug 17, 2026
fdc191f
fix: address PR comments
sphamjoli Aug 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 28 additions & 14 deletions contracts/registrars/DotnsPopController.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,7 @@ contract DotnsPopController is
bytes32 reservedHash;
bool hasReservation = bytes(params.reservedBaseLabel).length != 0;
if (hasReservation) {
(IPopRules.PopStatus required,) = rules.classifyName(params.reservedBaseLabel);
require(
required != IPopRules.PopStatus.Reserved
&& rules.isBaseName(params.reservedBaseLabel),
InvalidBaseLabel()
);
(reservedHash,) = _validateBaseLabel(params.reservedBaseLabel);
(reservedHash,) = _validateReservableBaseLabel(rules, params.reservedBaseLabel);
}

_reserveLite(rules, params.lite);
Expand All @@ -245,13 +239,7 @@ contract DotnsPopController is
onlyGateway
{
IPopRules rules = _popRules();
(IPopRules.PopStatus required,) = rules.classifyName(params.reservedBaseLabel);
require(
required != IPopRules.PopStatus.Reserved && rules.isBaseName(params.reservedBaseLabel),
InvalidBaseLabel()
);

(bytes32 reservedHash,) = _validateBaseLabel(params.reservedBaseLabel);
(bytes32 reservedHash,) = _validateReservableBaseLabel(rules, params.reservedBaseLabel);
_advanceExpiredHead(reservedHash);
_removeUserFromQueue(params.user);
_enqueueReservation(rules, reservedHash, params.reservedBaseLabel, params.user);
Expand Down Expand Up @@ -814,6 +802,32 @@ contract DotnsPopController is
(labelhash, node) = LabelUtils.deriveNode(protocolRegistry.tldNode(), baseLabel);
}

/// @notice Validates a base label as reservable and returns its hashes.
/// @dev Shared by both reservation entrypoints so the guard cannot drift between them. Runs
/// three checks and reverts on the first failure, before any reservation state is mutated: the
/// label must classify outside the governance-reserved tier and be a base name, be a canonical
/// single label, and have no owner on the registrar. The last check is the fix for a
/// reservation queued over an already-registered name: the queue keys by stem, so such a
/// reservation could never be redeemed yet would lock every two-digit variant of the stem for
/// the full reservation window. `exists` (owner set) mirrors exactly what makes the eventual
/// claim's mint revert, so a label that passes here is one a claim can still register.
function _validateReservableBaseLabel(
IPopRules rules,
string calldata baseLabel
)
internal
view
returns (bytes32 labelhash, bytes32 node)
{
(IPopRules.PopStatus required,) = rules.classifyName(baseLabel);
require(
required != IPopRules.PopStatus.Reserved && rules.isBaseName(baseLabel),
InvalidBaseLabel()
);
(labelhash, node) = _validateBaseLabel(baseLabel);
require(!_registrar().exists(uint256(node)), BaseNameAlreadyRegistered());
}

/// @notice Reverts when a non-empty chat key is not exactly `CHAT_KEY_LENGTH` bytes.
/// @dev Mirrors the resolver's own length gate so the gateway sees a controller-local
/// `InvalidChatKey` revert before any mint state is written.
Expand Down
20 changes: 16 additions & 4 deletions contracts/registrars/IDotnsPopController.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,10 @@ interface IDotnsPopController is IDotnsController {
/// @notice Thrown when a supplied base label is not a canonical DNS label.
error InvalidBaseLabel();

/// @notice Thrown when a reserved base label already has an owner on the registrar, so the
/// queued reservation could never be redeemed at mint time.
error BaseNameAlreadyRegistered();

/// @notice Thrown when a supplied chat key is non-empty and not exactly 65 bytes long.
/// @dev Mirrors the resolver's `InvalidChatKeyLength` so the controller surfaces a
/// controller-local error before the mint runs.
Expand Down Expand Up @@ -259,8 +263,13 @@ interface IDotnsPopController is IDotnsController {
/// @custom:emits PendingClaimStashed, with @custom:emits NameRegistered deferred to
/// @custom:function claimLabelStore when the user settles. The base-name leg only runs
/// when `reservedBaseLabel` is non-empty: it validates the DNS-label shape and requires a
/// true base label with no trailing digits (otherwise @custom:reverts InvalidBaseLabel)
/// before any queue mutation so a bad reservation never touches the queue, advances the
/// true base label with no trailing digits (otherwise @custom:reverts InvalidBaseLabel) and
/// with no owner on the registrar (otherwise @custom:reverts BaseNameAlreadyRegistered),
/// since a name that already has an owner could never be claimed. This validation runs
/// before both the lite mint and any queue mutation, so an already-registered
/// `reservedBaseLabel` aborts the whole call and the candidate receives no lite username
/// either; callers should validate the reserved label before attesting rather than relying
/// on this revert. It then advances the
/// head past expired entries (emitting @custom:emits ReservationExpired for each one),
/// removes the user from any prior queue position so a single user holds at most one live
/// reservation across all labels, and enqueues a fresh entry (emitting
Expand All @@ -280,7 +289,8 @@ interface IDotnsPopController is IDotnsController {
/// typed overload's full revert surface bubbles up byte-for-byte: gateway-only access
/// (otherwise @custom:reverts NotGateway), lite-label shape (otherwise
/// @custom:reverts InvalidLiteLabel), base-label shape (otherwise
/// @custom:reverts InvalidBaseLabel), duplicate-reservation guard (otherwise
/// @custom:reverts InvalidBaseLabel), already-registered base label (otherwise
/// @custom:reverts BaseNameAlreadyRegistered), duplicate-reservation guard (otherwise
/// @custom:reverts AlreadyReserved), and queue capacity (otherwise
/// @custom:reverts QueueFull). The success path likewise emits the same events as the
/// typed call: @custom:emits LiteNameReserved and @custom:emits NameRegistered on the lite
Expand All @@ -296,7 +306,9 @@ interface IDotnsPopController is IDotnsController {
/// gateway flow: @custom:function reserveLiteName mints the lite username first, then this
/// function reserves the full/base label in a separate transaction so proof-size stays below
/// per-call limits. Reverts with @custom:reverts InvalidBaseLabel when the label is empty,
/// non-canonical, digit-suffixed, or governance-reserved. The caller remains agnostic about
/// non-canonical, digit-suffixed, or governance-reserved, and with
/// @custom:reverts BaseNameAlreadyRegistered when the label already has an owner on the
/// registrar and so could never be claimed. The caller remains agnostic about
/// backend batching; it simply exposes a small retryable primitive.
/// @param params Reservation request; see @custom:struct BaseNameReservation.
function reserveBaseNameOnly(BaseNameReservation calldata params) external;
Expand Down
61 changes: 48 additions & 13 deletions test/unit/registrar/DotnsPopController.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,25 +521,24 @@ contract DotnsPopControllerTests is BaseDotns {
dotnsRegistry.setSubnodeOwner(subnodeRecord);
}

function test_pop_reservation_of_already_public_minted_name_fails_on_claim() public {
function test_pop_reservation_of_already_public_minted_name_reverts_at_reserve_time() public {
// A name minted through the public commit-reveal flow already has an owner, so a PoP
// reservation over it could never be redeemed. The guard rejects it at reserve time
// rather than admitting it and only failing at claim, which would have locked every
// two-digit variant of the stem for the full reservation window.
_commitAndRegister("longnamebob", ed, true);

_grantPopFull(tiago);
vm.expectRevert(IDotnsPopController.BaseNameAlreadyRegistered.selector);
_reservePop(tiago, LITE_LABEL_A, _validChatKey(0xaa), "longnamebob");

(bool reserved, address holder) = dotnsPopController.isReservedForClaim("longnamebob");
assertTrue(reserved);
assertEq(holder, tiago);
// The guard runs before the lite mint, so the whole call aborts and no lite name is
// minted for the candidate.
assertFalse(dotnsRegistrar.exists(uint256(_nodeOf(LITE_LABEL_A))));

IDotnsPopController.Link memory link = _linkWithLite(LITE_LABEL_A);
vm.expectRevert(
abi.encodeWithSelector(
IDotnsRegistrar.NameNotAvailable.selector, uint256(_nodeOf("longnamebob"))
)
);
_gatewayRegisterBaseName(
IDotnsPopController.FullRegistration({label: "longnamebob", user: tiago, link: link})
);
// No reservation was recorded, so the stem family stays open to other candidates.
(bool reserved,) = dotnsPopController.isReservedForClaim("longnamebob");
assertFalse(reserved);

@re-gius re-gius Aug 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cheap addition :

assertFalse(dotnsRegistrar.exists(uint256(_nodeOf(LITE_LABEL_A))));

}

function test_enqueue_becomesHead_writes_popRules_reservation() public {
Expand Down Expand Up @@ -592,6 +591,24 @@ contract DotnsPopControllerTests is BaseDotns {
_reservePop(ed, LITE_LABEL_A, _validChatKey(0xaa), "longnamebob01");
}

function test_reserveBaseName_reverts_when_reserved_label_already_registered() public {
// George worked example: ed reserves and then claims the base name, which frees the
// stem slot on PopRules. A later lite candidate must not be able to queue a reservation
// over the now-registered name; the queue keys by stem, so an unclaimable reservation
// would lock every two-digit variant of the stem for the full reservation window.
_grantPopFull(ed);
_reservePop(ed, LITE_LABEL_A, _validChatKey(0xaa), "longnamebob");
_gatewayRegisterBaseName(
IDotnsPopController.FullRegistration({
label: "longnamebob", user: ed, link: _linkWithLite(LITE_LABEL_A)
})
);

_grantPopFull(tiago);
vm.expectRevert(IDotnsPopController.BaseNameAlreadyRegistered.selector);
_reservePop(tiago, LITE_LABEL_B, _validChatKey(0xbb), "longnamebob");
}

function test_public_stranger_can_mint_after_claim_clears_reservation() public {
_grantPopFull(ed);
_reservePop(ed, LITE_LABEL_A, _validChatKey(0xaa), "longnamebob");
Expand Down Expand Up @@ -920,6 +937,24 @@ contract DotnsPopControllerTests is BaseDotns {
);
}

function test_reserveBaseNameOnly_reverts_when_label_already_registered() public {
// The standalone reservation entrypoint shares the same guard: a base name that already
// has an owner on the registrar can never be redeemed, so the reservation is rejected up
// front rather than discovered to be unusable at claim time.
_grantPopFull(ed);
_reservePop(ed, LITE_LABEL_A, _validChatKey(0xaa), "longnamebob");
_gatewayRegisterBaseName(
IDotnsPopController.FullRegistration({
label: "longnamebob", user: ed, link: _linkWithLite(LITE_LABEL_A)
})
);

vm.expectRevert(IDotnsPopController.BaseNameAlreadyRegistered.selector);
_gatewayReserveBaseNameOnly(
IDotnsPopController.BaseNameReservation({user: tiago, reservedBaseLabel: "longnamebob"})
);
}

function test_reserveBaseNameOnly_does_not_mint_lite_or_base_name() public {
_gatewayReserveBaseNameOnly(
IDotnsPopController.BaseNameReservation({user: ed, reservedBaseLabel: BASE_LABEL_A})
Expand Down
Loading