Skip to content

Improve NatSpec documentation across interface files #236

@maceip

Description

@maceip

random inputs from me, a guy who cares about the web: i had some down time in munich and went reviewing the new interfaces in #231, where I noticed several documentation gaps across the broader interface surface that could trip up integrators.

None of these are bugs — the code works correctly — but the interfaces are the primary API surface for external devs, and a few targeted NatSpec additions might prevent a lot of confusion.

Grouped by impact below, with suggested diffs.


High Impact

1. The anyId union type is undocumented

~15 functions across IStandardRegistry and IPermissionedRegistry accept anyId — a uint256 that silently accepts a labelhash, tokenId, or EAC resource. The normalization logic (version-bit stripping) is completely invisible at the interface level. Integrators who cache a tokenId from a previous registration may not realize it silently resolves to a new entry after re-registration.

📄 contracts/src/registry/interfaces/IStandardRegistry.sol — line 6
 /// @title IStandardRegistry
 /// @notice A tokenized registry.
 /// @dev Interface selector: `0xb844ab6c`
+///
+/// Many functions accept an `anyId` parameter. This is a union type that accepts any of:
+///   - **labelhash**: `keccak256(label)` — the raw hash of the subdomain label.
+///   - **tokenId**: labelhash with version bits encoded (changes on re-registration).
+///   - **resource**: the EAC resource identifier associated with the token.
+///
+/// All three forms are normalized internally to look up the same storage entry.
+/// Callers should be aware that a stale `tokenId` from a previous registration
+/// will still resolve to the current entry's storage — always check `getStatus()`
+/// or `ownerOf()` before mutating.
 interface IStandardRegistry is IRegistry {

2. register() silently doubles as reserve() when owner == address(0)

The register() function emits NameReserved and skips minting when owner is zero, but the interface NatSpec says "Registers a new name" with no mention of reservation. This is a significant semantic overload.

📄 contracts/src/registry/interfaces/IStandardRegistry.sol — line 38
-    /// @notice Registers a new name.
-    /// @param label The label to register.
-    /// @param owner The address of the owner of the name.
+    /// @notice Registers a new name, or reserves it if `owner` is `address(0)`.
+    /// @dev When `owner == address(0)`, no token is minted and the name enters `RESERVED` status.
+    ///      A reserved name can later be registered by calling `register()` again with a non-zero owner.
+    ///      Emits `NameRegistered` for registrations or `NameReserved` for reservations.
+    /// @param label The label to register.
+    /// @param owner The address of the owner of the name, or `address(0)` to reserve.
     /// @param registry The registry to set as the name.

3. State struct comments and missing state machine documentation

The State struct uses an unusual // getStatus() comment style instead of NatSpec, and the Status enum doesn't document its state transitions.

📄 contracts/src/registry/interfaces/IPermissionedRegistry.sol — line 14
     /// @notice The registration status of a subdomain.
+    /// @dev State transitions:
+    ///   AVAILABLE -> RESERVED  (via `register()` with `owner == address(0)`)
+    ///   AVAILABLE -> REGISTERED (via `register()` with non-zero `owner`)
+    ///   RESERVED  -> REGISTERED (via `register()` with non-zero `owner`)
+    ///   REGISTERED -> AVAILABLE (via `unregister()` or expiry)
+    ///   RESERVED  -> AVAILABLE  (via `unregister()` or expiry)
     enum Status {
         AVAILABLE,
         RESERVED,
         REGISTERED
     }

     /// @notice The registration state of a subdomain.
     struct State {
-        Status status; // getStatus()
-        uint64 expiry; // getExpiry()
-        address latestOwner; // latestOwnerOf()
-        uint256 tokenId; // getTokenId()
-        uint256 resource; // getResource()
+        /// @dev The current registration status (AVAILABLE, RESERVED, or REGISTERED).
+        Status status;
+        /// @dev The expiration timestamp (seconds since epoch). Zero if never registered.
+        uint64 expiry;
+        /// @dev The last owner address. Persists after burn/expiry, unlike `ownerOf()` which returns `address(0)`.
+        address latestOwner;
+        /// @dev The current token ID (includes version bits; changes on re-registration).
+        uint256 tokenId;
+        /// @dev The EAC resource identifier used for role-based access control.
+        uint256 resource;
     }

Medium Impact

4. latestOwnerOf vs ownerOf distinction is under-documented

These two functions return different values for expired/burned tokens but the interface doesn't make this clear. Integrators will almost certainly call the wrong one.

📄 contracts/src/registry/interfaces/IPermissionedRegistry.sol — line 48
-    /// @notice Get the latest owner of a token.
-    ///         If the token was burned, returns null.
+    /// @notice Get the latest owner of a token, even if the token has been burned or expired.
+    /// @dev Unlike `ownerOf()` (inherited from IERC1155Singleton), which returns `address(0)` for
+    ///      expired or burned tokens, this function returns the last address that held the token.
+    ///      Returns `address(0)` only if the name was never registered or was only reserved.
     /// @param tokenId The token ID to query.
     /// @return The latest owner address.

5. Role bitmap encoding is opaque

The EAC uses nybble-packed bitmaps (not simple bitflags), but the interface gives no hint of this. getAssigneeCount() returns packed values with no explanation.

📄 contracts/src/access-control/interfaces/IEnhancedAccessControl.sol — line 6
 /// @notice Interface for Enhanced Access Control system that allows for:
 /// * Resource-based roles
 /// * Obtaining assignee count for each role in each resource
 /// * Root resource override
 /// * Up to 32 roles and 32 corresponding admin roles
 /// * Up to 15 assignees per role
+///
+/// Role bitmaps use a nybble-packed encoding — each role occupies 4 bits, not 1 bit.
+/// Roles 0..31 are packed in the lower 128 bits, admin roles 0..31 in the upper 128 bits.
+/// Use the corresponding roles library to construct valid bitmaps.
+///
+/// Accounts with roles on `ROOT_RESOURCE` implicitly hold those roles on *every* resource.
+/// This is a powerful override — grant root roles with extreme caution.
 /// @dev Interface selector: `0x8f452d62`
📄 contracts/src/access-control/interfaces/IEnhancedAccessControl.sol — line 99
-    /// @dev Get the no. of assignees for the roles in the given role bitmap.
+    /// @dev Get the no. of assignees for the roles in the given role bitmap.
+    /// @param resource The resource to query.
+    /// @param roleBitmap The roles to query (nybble-packed).
+    /// @return counts Nybble-packed assignee counts — each 4-bit nybble holds the count (0..15) for the
+    ///         corresponding role. Extract role N's count via `(counts >> (N * 4)) & 0xF`.
+    /// @return mask A bitmask indicating which nybbles in `counts` are populated.
     function getAssigneeCount(

6. IRegistry.getSubregistry() / getResolver() — zero return is ambiguous

Both return address(0) for "never existed" and "expired", with no way to distinguish the two.

📄 contracts/src/registry/interfaces/IRegistry.sol — line 58
-    /// @dev Fetches the registry for a subdomain.
+    /// @dev Fetches the registry for a subdomain. Returns `address(0)` both when the label was
+    ///      never registered and when it has expired. Use `IPermissionedRegistry.getStatus()` to
+    ///      distinguish between the two cases.
     /// @param label The label to resolve.
-    /// @return The address of the registry for this subdomain, or `address(0)` if none exists.
+    /// @return The address of the registry for this subdomain, or `address(0)` if none exists or expired.
     function getSubregistry(string calldata label) external view returns (IRegistry);

-    /// @dev Fetches the resolver responsible for the specified label.
+    /// @dev Fetches the resolver responsible for the specified label. Returns `address(0)` both when
+    ///      the label was never registered and when it has expired. Use `IPermissionedRegistry.getStatus()`
+    ///      to distinguish between the two cases.
     /// @param label The label to fetch a resolver for.
-    /// @return resolver The address of a resolver responsible for this name, or `address(0)` if none exists.
+    /// @return resolver The address of a resolver responsible for this name, or `address(0)` if none exists or expired.
     function getResolver(string calldata label) external view returns (address);

Low Impact

7. IETHRegistrarreferrer is informational-only but undocumented

📄 contracts/src/registrar/interfaces/IETHRegistrar.sol — lines 109, 133
     /// @param paymentToken The ERC-20 to use for payment.
-    /// @param referrer The referrer hash.
+    /// @param referrer An informational tag emitted in events for attribution (does not affect pricing or permissions).
     /// @return tokenId The registered token ID.
+    /// @dev Payment is collected via ERC-20 `transferFrom` from `msg.sender`.
+    ///
     /// @param label The name to renew.
     /// @param duration The registration extension, in seconds.
     /// @param paymentToken The ERC-20 to use for payment.
-    /// @param referrer The referrer hash.
+    /// @param referrer An informational tag emitted in events for attribution (does not affect pricing or permissions).

8. IRentPriceOracle.rentPrice() — unclear why owner affects pricing

📄 contracts/src/registrar/interfaces/IRentPriceOracle.sol — line 54
     /// @param label The name.
-    /// @param owner The new owner address.
+    /// @param owner The prospective owner address. Included to support owner-dependent pricing
+    ///        (e.g. loyalty discounts, allowlist-based pricing, or subsidised registrations).
     /// @param duration The duration to price, in seconds.

9. IWrapperRegistry.initialize() — missing function-level NatSpec

📄 contracts/src/registry/interfaces/IWrapperRegistry.sol — line 10
-    /// @param node Namehash of this registry.
+    /// @notice One-shot proxy initializer. Reverts if called more than once.
+    /// @dev Sets up the wrapper registry to manage a locked NameWrapper name.
+    /// @param node Namehash of this registry.

10. IERC1155Singleton — selector intentionally matches ERC721

📄 contracts/src/erc1155/interfaces/IERC1155Singleton.sol — line 8
-/// @dev Interface selector: `0x6352211e`
+/// @dev Interface selector `0x6352211e` intentionally matches ERC721's `ownerOf(uint256)` selector
+///      for cross-standard compatibility.
 interface IERC1155Singleton is IERC1155 {
+    /// @notice Returns the owner of `id`, or `address(0)` if unowned.
     function ownerOf(uint256 id) external view returns (address owner);

lmk if you want a PR for any/all of these. feedback welcome on which ones are worth doing vs. intentional design

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions