feat(pop): keep the separator in lite names and record issuance provenance - #275
Merged
Conversation
Contributor
CI Summary
4naly3er AnalysisMedium (5)
Low (8)
Gas (14)
Informational (11)
Slither AnalysisHigh (4)
Medium (40)
Low (80)
Informational (57)
Contract Tests (Unit + Fuzz)BasicDotnsIntegrationReverts (test/intergration/BasicDotns.reverts.t.sol)
BasicDotnsIntegration (test/intergration/BasicDotns.t.sol)
DeployCreate3FactoryTest (test/unit/deploy/DeployCreate3Factory.t.sol)
DeterministicDeploymentTest (test/unit/deploy/DeterministicDeployment.t.sol)
DotnsContentResolverTests (test/unit/resolver/DotnsContentResolver.t.sol)
DotnsCostModelRegistryTests (test/unit/pop/DotnsCostModelRegistry.t.sol)
DotnsFlatPricingTests (test/unit/pop/DotnsFlatPricing.t.sol)
DotnsNameEscrowTest (test/unit/escrow/DotnsNameEscrow.t.sol)
DotnsNameEscrowFuzzTest (test/fuzz/escrow/DotnsNameEscrowFuzz.t.sol)
DotnsNameEscrowRedeemTest (test/unit/escrow/DotnsNameEscrowRedeem.t.sol)
DotnsNameEscrowRefundsTest (test/unit/escrow/DotnsNameEscrowRefunds.t.sol)
DotnsNameWhitelistTests (test/unit/whitelist/DotnsNameWhitelist.t.sol)
DotnsNameWhitelistFuzz (test/fuzz/whitelist/DotnsNameWhitelistFuzz.t.sol)
DotnsPopControllerTests (test/unit/registrar/DotnsPopController.t.sol)
DotnsPopControllerFuzz (test/fuzz/registrar/DotnsPopControllerFuzz.t.sol)
DotnsPopResolverTests (test/unit/resolver/DotnsPopResolver.t.sol)
DotnsProtocolRegistryTldTests (test/unit/registry/DotnsProtocolRegistry.t.sol)
DotnsProtocolRegistryFuzzTest (test/fuzz/registry/DotnsProtocolRegistryFuzz.t.sol)
DotnsRegistrarTests (test/unit/registrar/DotnsRegistrar.t.sol)
DotnsRegistrarControllerTest (test/unit/registrar/DotnsRegistrarController.t.sol)
DotnsRegistrarControllerFuzzTest (test/fuzz/registrar/DotnsRegistrarControllerFuzz.t.sol)
DotnsRegistrarControllerLifecycleTest (test/unit/registrar/DotnsRegistrarControllerLifecycle.t.sol)
DotnsRegistryTests (test/unit/registry/DotnsRegistry.t.sol)
DotnsRegistryFuzzTest (test/fuzz/registry/DotnsRegistryFuzz.t.sol)
DotnsResolverTests (test/unit/resolver/DotnsResolver.t.sol)
DotnsReverseResolverTests (test/unit/resolver/DotnsReverseResolver.t.sol)
DotnsScarcityPricingTests (test/unit/pop/DotnsScarcityPricing.t.sol)
LabelStoreTests (test/unit/store/LabelStore.t.sol)
LabelStoreFuzzTest (test/fuzz/store/LabelStoreFuzz.t.sol)
NameGrantFlow (test/intergration/NameGrantFlow.t.sol)
NoStatusDepositLifecycle (test/intergration/NoStatusDepositLifecycle.t.sol)
PopRulesFuzzTest (test/fuzz/pop/PopFuzz.t.sol)
PopLifecycleFlow (test/intergration/PopLifecycleFlow.t.sol)
PopRulesTests (test/unit/pop/PopRules.t.sol)
PopRulesClassificationTests (test/unit/pop/PopRulesClassification.t.sol)
StoreFactoryTests (test/unit/store/StoreFactory.t.sol)
StoreIntegrationTest (test/intergration/StoreIntegration.t.sol)
StoreStressTest (test/stress/store/StoreStress.t.sol)
StringUtilsTests (test/unit/utils/StringUtils.t.sol)
UserStoreTests (test/unit/store/UserStore.t.sol)
UserStoreFuzzTest (test/fuzz/store/UserStoreFuzz.t.sol)
Contract Tests (Invariant)CostModelVersionInvariantTest (test/invariant/pop/CostModelVersionInvariant.t.sol)
DotnsNameEscrowInvariantTest (test/invariant/escrow/DotnsNameEscrowInvariant.t.sol)
DotnsNameWhitelistInvariant (test/invariant/whitelist/DotnsNameWhitelistInvariant.t.sol)
DotnsPopControllerInvariant (test/invariant/registrar/DotnsPopControllerInvariant.t.sol)
DotnsRegistrarControllerInvariantTest (test/invariant/registrar/DotnsRegistrarControllerInvariant.t.sol)
DotnsRegistrarReservedGrantInvariantTest (test/invariant/registrar/DotnsRegistrarReservedGrantInvariant.t.sol)
DotnsRegistrarSoulboundInvariantTest (test/invariant/registrar/DotnsRegistrarSoulboundInvariant.t.sol)
DotnsRegistryInvariantTest (test/invariant/registry/DotnsRegistryInvariant.t.sol)
StoreInvariantTest (test/invariant/store/StoreInvariant.t.sol)
Gas ReportDotnsProtocolRegistry
DotnsRegistrar
DotnsRegistrarController
DotnsRegistry
DotnsReverseResolver
PopRules
Deploy ContractsDeployed addresses vs the committed manifestExpected is the committed manifest; actual is this CI deployment of the same pipeline.
Labelssmartcontracts, scope: registration, scope: resolver, type: test, type: docs, scope: pop |
github-actions
Bot
added
smartcontracts
re-gius
marked this pull request as ready for review
September 4, 2026 13:12
5 tasks
sphamjoli
reviewed
Sep 4, 2026
sphamjoli
left a comment
sphamjoli
left a comment
Member
There was a problem hiding this comment.
A few items that do not map to a line in this diff:
- Queue invariant blind spot:
invariant_popRules_head_matches_queue_head_or_zero(inDotnsPopControllerInvariant.t.sol, not touched here) asserts nothing when the queue head is empty, so an orphaned PopRules slot outliving the queue passes silently. Adding the zero-direction assertion (head empty implies the slot is cleared or not live) closes it. - Dead PopLite public-reserve branch: with the separated form, the
priced.status == PopLitewrite inDotnsRegistrarController(not in this diff) is now unreachable and only kept alive by guard tests. Deleting the branch, or softening the "single cross-flow authority in both directions" comment, would match the new behaviour. - Description wording: "registrable" reads as open-sale, but
web3(Reserved) andandrew-x(PopFull) are grammar-valid rather than openly registrable; onlylongnamebob01is. And "Closes #274" is accurate for the contract portion; the host and dotli criteria live in another repo.
|
|
||
| /// @dev Reserved storage space to allow for layout changes in future upgrades. | ||
| uint256[50] private __gap; | ||
| uint256[49] private __gap; |
Member
There was a problem hiding this comment.
We deploy this fresh rather than upgrading in place, so the gap does not need to shrink for the appended _popIssued. Put it back to uint256[50]. The upgrade-only additions can come out with it: the reconstructed legacy interface id in supportsInterface and the "survives an upgrade" wording have no upgrade path to serve on a fresh deploy.
Contributor
Author
There was a problem hiding this comment.
Changed in 5785561 for other contracts too
| /// 100 reuses are swallowed by the caller's try/catch. | ||
| /// @dev Shape: `<tag><4 letters from actor>.<2 digits>`, the separated form the gateway | ||
| /// accepts. The stem is 7 characters, which classifies as PopLite under PopRules, and | ||
| /// the separator and digits are the suffix. Tag disambiguates the reserve vs claim call |
Member
There was a problem hiding this comment.
Two properties this PR introduces are covered only by unit and single-fuzz cases, but both hold across arbitrary action sequences, so they belong in the invariant: first-to-mint arbitration between the gateway and the public register, and isPopIssued being written once and never cleared. The handler exposes no public-register action and no ERC-721 transfer action, so the campaign cannot reach either today. Adding those two actions, plus a ghost that fails if isPopIssued flips back to false after a transfer, would cover both at the right level.
| /// which is the form People Chain holds, so no normalisation happens here. The shape check | ||
| /// runs before classification so a malformed label reverts | ||
| /// @custom:reverts InvalidLiteLabel, which the gateway pallet decodes by selector; letting | ||
| /// `_validateLiteLabel` catch it instead would surface an undecodable PopRules string. |
Member
There was a problem hiding this comment.
The @dev points at the wrong function: _validateLiteLabel reverts InvalidLiteLabel, a decodable selector, not an undecodable PopRules string. That string comes from classifyName via _requireLabel. The logic holds; only the named function is wrong.
| /// People Chain emits and what DotNS accepts. The pallet treats the value as a | ||
| /// minimum; DotNS requires exactly this many, so a three-digit suffix is rejected | ||
| /// here even though the pallet would accept it. | ||
| uint256 internal constant MIN_LITE_SUFFIX_DIGITS = 2; |
Member
There was a problem hiding this comment.
The code uses this as an exact count: the separator is fixed at length - MIN_LITE_SUFFIX_DIGITS - 1, so a three-digit suffix is rejected, and the NatSpec then has to explain that the pallet treats it as a minimum. If exactly two is the intended policy, LITE_SUFFIX_DIGITS names the behaviour without the MIN_ the code contradicts.
| /// @dev The union is the full set of issuable labels, so a near miss such as `alice.4` or | ||
| /// `a.b.42` still reverts. @custom:function _requireStem is the stricter guard for | ||
| /// reservation keys, which never carry a separator. | ||
| function _requireLabel(string calldata name) internal pure { |
Member
There was a problem hiding this comment.
These views (classifyName, price, priceWithCheck and the rest) now accept a lite label through _requireLabel, so a dotted input returns a non-reverting answer where it used to revert. Minting is still gated at the controllers, so this does not change who can register, but any off-chain caller that read a non-revert here as proof of a public flat label needs to stop.
Contributor
Author
There was a problem hiding this comment.
Added to the PR description
| /// resolves to depends on which reading you take, and provenance is what tells them | ||
| /// apart. No production controller entry point can create the parent: the public and | ||
| /// reserved paths require three characters, and both gateway paths are letters only. | ||
| /// An owner-authorised controller can still call the registrar directly, which is what |
Member
There was a problem hiding this comment.
These subname cases are thorough at the unit level, but the property they check is the one the dotted-name design rests on, and it holds across arbitrary sequences: no subname can create a dotted node, and no subname node can equal a PoP node. The registry invariant handler hardcodes the sublabel as "sub", so the campaign never fuzzes a separator into a subname, and no invariant interleaves subname creation with gateway mints. Fuzzing the sublabel and asserting no created node's label carries a dot, plus a combined handler that checks no subnode hash equals a tracked PoP node and that isPopIssued disambiguates, would lift this to the level it needs.
Contributor
Author
Added the zero direction, restricted to head >= tail in 8983b38
Deleted
Reworded |
sphamjoli
self-requested a review
September 4, 2026 19:04
sphamjoli
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
The gateway sends
michael.42.DotnsPopController._reserveLitestripped the separator and storedalice42, making the contracts the only layer that disagreed with People Chain, the gateway pallet and the host apps. This keeps it: a lite name is stored, minted and shown asmichael.42, hashed as one whole label.A dotted string is ambiguous on its own. To the personhood system
michael.42is one person; to the hierarchical system it ismichaelbeneath42, the same shape asshop.google. The characters cannot say which. So provenance becomes explicit rather than inferred:isPopIssued(label)is set at mint, never cleared, and keyed by the bare label rather than the node — a reader holding onlymichael.42cannot derive the node without first deciding how to hash the separator, which is the question it is asking.The rule
A name a person chose is lowercase ASCII letters.
joseph.42josephIt mirrors People Chain, where
validate_usernamerequires the lite stem and the full-person username to beis_ascii_lowercase, andBaseLabel::is_valid_personis documented as "lowercase ASCII letters only, no digits or hyphens". A label outside the shape cannot have been issued.Ordinary public names are untouched. They stay
isSingleLabel: letters, digits and hyphens, any digit count. Soweb3,andrew-xandlongnamebob01remain well-formed public labels, and only the gateway paths are letters-only. Being well-formed is not being for sale: classification still decides that, and of the three onlylongnamebob01is open to anyone.andrew-xrequires full personhood andweb3is governance-reserved.Length is policy, not format. Neither predicate has a floor. How short a name may be is the governance-reserved band in
PopRules, soalice.42is a well-formed lite label that classification rejects. Mirroring People Chain'sMinUsernameLengthwould duplicate that and drift when the runtime changes it.Base length is the label as written, except for a lite label, whose separator and two allocated digits come off first. The gateway allocates those digits to distinguish people who chose the same stem, so removing them recovers what the candidate picked; no such allocation stands behind the digits in
web3.joseph.42PopLiteelizabeth.42NoStatusalice.42Reservedjoseph42PopFullweb3Reservedlongnamebob01NoStatusThe label-taking views now answer for a dotted label.
classifyName,price,priceWithCheckand the rest admit a lite label through_requireLabel, somichael.42returns a classification where it previously reverted. Minting is unchanged:registerstill requiresisSingleLabel, so the public path cannot submit one. Any off-chain caller that treated a non-revert from these views as proof of a public flat label needs to stop reading it that way.Type
Scope
Related Issues
Follows #270. Supersedes #273, which reached the same goal by narrowing the public grammar. That route changed what public names mean to satisfy a representation need originating in the personhood subsystem.
Fixes
Closes #274. The host and gateway acceptance criteria belong to #273 instead.
Checklist
Code
forge buildpassesforge testpassesTesting
Security
selfdestructordelegatecallDocumentation
Breaking Changes
Breaking changes:
isPopIssuedchangestype(IDotnsPopController).interfaceId.supportsInterfaceanswers both the new and the pre-isPopIssuedid, derived asinterfaceId ^ isPopIssued.selectorrather than hardcoded so it cannot drift from a copied constant. Regression test included._popIssuedappended before__gap, which shrank from 50 to 49.alice-bobandmicha3lare valid DNS labels and were issuable as identities; they no longer are. They remain registrable as ordinary public names. The previous check caught only a trailing digit, so it admitted both.PopLiteis reachable only through the gateway. No flat label classifiesPopLite, because digits in an ordinary label say nothing about personhood.alice42moves fromReservedtoPopFull. Converselyweb3andmp3, previously rejected outright for carrying a one-digit suffix, are now ordinary labels.liteNamesOfandfullNamesOfboth require provenance, so together they cover what the gateway issued rather than everything an account holds. A public registration appears in neither, where previously any single label landed infullNamesOf. This is the change most likely to surface as a missing row rather than an error, so dotli and UI owners should see it.How to test
Notes
One subtlety worth reviewing closely.
_classifyValidatedNamederived the digit count asbytes(name).length - baseLength. For a separated label that yields 3, never 2, so a subtraction-based check silently classifies every lite name asPopFull— no revert, no event, just the wrong tier. It now asks the shape predicate. Mutation-tested: reverting to the subtraction fails two tests with2 != 1.Why the legacy set cannot be recovered.
isPopIssuedis written at mint and postdates those names; the registrar's soulbound flag is absent fromcontracts/atv0.5.8-rc1and is also mint-time only, so no upgrade backfills it.Both lens signals are load-bearing.
isPopIssuedis written in_completeGatewayRegistration, so it answers true for lite and full names alike and cannot say which kind a name is. Provenance decides whether a name is an identity; the separator decides which kind. Collapsing either into the other breaks a listing.A separator remains impossible on a subname, which is what reserves the dotted space to the gateway.