From 989cf5986ed298f5412d33ce8ec81abeb33851fc Mon Sep 17 00:00:00 2001 From: giuseppere Date: Tue, 13 Oct 2026 10:07:58 +0200 Subject: [PATCH 1/2] feat(escrow): lock cross-payer registrations as deposits and make open-name transfers free --- README.md | 10 +- contracts/escrow/DotnsNameEscrow.sol | 38 +--- contracts/escrow/IDotnsNameEscrow.sol | 27 +-- contracts/pop/IPopRules.sol | 4 +- contracts/pop/PopRules.sol | 2 + .../registrars/DotnsRegistrarController.sol | 27 +-- contracts/registrars/IDotnsRegistrar.sol | 9 +- .../registrars/IDotnsRegistrarController.sol | 11 +- .../escrow/DotnsNameEscrowInvariant.t.sol | 2 +- test/invariant/escrow/EscrowHandler.t.sol | 54 ++---- test/unit/escrow/DotnsNameEscrow.t.sol | 171 ++++++------------ test/unit/escrow/DotnsNameEscrowRedeem.t.sol | 66 ++----- test/unit/pop/PopRules.t.sol | 13 +- test/unit/registrar/DotnsRegistrar.t.sol | 6 +- .../DotnsRegistrarControllerLifecycle.t.sol | 38 ++-- 15 files changed, 159 insertions(+), 319 deletions(-) diff --git a/README.md b/README.md index 9a0d59333..a306a07a3 100644 --- a/README.md +++ b/README.md @@ -86,7 +86,7 @@ The path decides whether the amount is a refundable deposit, a non-refundable fe | Name | Base length | Path | Amount | Held as | |---|---|---|---|---| | `gavinwood` | 9 | public, own key | 10 DOT | refundable deposit | -| `gavinwood` | 9 | public, someone else pays | 10 DOT | protocol fee | +| `gavinwood` | 9 | public, someone else pays | 10 DOT | refundable deposit, owned by the name's owner | | `andrewsays` | 10 | public, own key | 10 DOT | refundable deposit | | `andrew` | 6 | public, own key (switch on, proof of personhood) | 10 DOT | refundable deposit | | `alicebob42` | 10 | public, own key | 10 DOT | refundable deposit | @@ -97,11 +97,11 @@ The path decides whether the amount is a refundable deposit, a non-refundable fe Registering a name under your own key locks a refundable deposit equal to the name's price. The deposit is bound to the name rather than to you, so it travels with the token on every transfer and unlocks only when the current holder releases the name back to escrow. -A name someone else pays for, and a transfer, pay a non-refundable fee instead. When a third party pays for another wallet's registration, the charge is the same owner-side price, with no separate friction added; it routes to a single protocol fee pot, and the owner's escrow slot is seeded with a zero amount so the release lifecycle stays reachable. A gateway grant carries neither a deposit nor a fee. The protocol fee pot only ever grows: it backs no refunds, and nothing burns, sweeps, or withdraws from it. Refunds draw solely on the separate per-asset reserve that deposits fund. A holder's own deposit is their money held in trust and is never moved into fees. +A name someone else pays for locks the same deposit, owned by the name's owner: a third party who pays for another wallet's registration gives that owner the deposit, which the owner collects on release. A transfer that owes a fee pays a non-refundable fee instead, into a single protocol fee pot. A gateway grant carries neither a deposit nor a fee. The protocol fee pot only ever grows: it backs no refunds, and nothing burns, sweeps, or withdraws from it. Refunds draw solely on the separate per-asset reserve that deposits fund. A holder's own deposit is their money held in trust and is never moved into fees. ### Transfers re-price at the name's own length -Publicly registered names transfer freely and charge the name's own price, but only in two cases: the recipient cannot clear the name's band, or the move is a personhood downgrade, where the recipient's tier is lower than the sender's. Passing a six-character name to a wallet that could never have registered it costs the name's own price, so there is no cheap way to hand a band-gated name to a party who could not have earned it. A move between two wallets that both clear the band, and a move to the same address, cost nothing. The fee, when one is owed, settles into the protocol fee pot. The deposit, when present, rides with the name: the escrow position rebinds to the new holder rather than refunding, and only releasing the name back to escrow unlocks the locked deposit. +Names of nine characters or more are open to all, so they transfer for free whatever the tiers. Any other transferable name charges its own price in two cases: the recipient cannot clear the name's band, or the move is a personhood downgrade, where the recipient's tier is lower than the sender's. Passing a six-character name to a wallet that could never have registered it costs the name's own price, so there is no cheap way to hand a band-gated name to a party who could not have earned it. A move between two wallets that both clear the band, and a move to the same address, cost nothing. The fee, when one is owed, settles into the protocol fee pot. The deposit, when present, rides with the name: the escrow position rebinds to the new holder rather than refunding, and only releasing the name back to escrow unlocks the locked deposit. Names minted through the gateway pallet are soulbound: they stay bound to the person who earned them and cannot be transferred at all. Any transfer of a gateway-issued name reverts, and quoting a transfer fee for one reverts rather than returning a price. Everything else about the name works normally, so its owner still sets records, issues subnames, and manages the name. @@ -125,7 +125,7 @@ Both are stamped at release time, so a governance change never moves the clocks - `redeem` returns the token and moves no value. The position keeps its recipient, asset and amount, so the deposit stays locked and the name lands back in its exact pre-release state, releasable again later on a fresh pair of clocks. This is the undo for an accidental release. - `withdraw` credits the deposit and forfeits the right to redeem. A holder paid for the name cannot also take it back, otherwise they would hold a name no deposit backs. -Once `redeemableUntil` passes, `reclaim` is permissionless through the ordinary commit-reveal path, whether or not the previous holder ever withdrew. If the position still holds value, reclaim settles it: the amount is credited to the previous holder's pull-payment balance and stays claimable with no deadline. The value follows the departing holder; the name does not wait for them. Cross-paid registrations seed zero-amount positions, so those names have nothing to withdraw and nothing to settle. +Once `redeemableUntil` passes, `reclaim` is permissionless through the ordinary commit-reveal path, whether or not the previous holder ever withdrew. If the position still holds value, reclaim settles it: the amount is credited to the previous holder's pull-payment balance and stays claimable with no deadline. The value follows the departing holder; the name does not wait for them. Gateway-issued (soulbound) names never enter this lifecycle. Releasing a name transfers it into escrow custody, and a soulbound name reverts on any transfer, so it holds no position and has no release, redeem, or reclaim path. It stays with the person who earned it. @@ -231,7 +231,7 @@ A separated label reaches the chain only through the gateway path: the public pa Tier assignment is read on every pricing call, not stored: PopRules queries the alias-accounts personhood precompile at DotnsConstants.PERSONHOOD with the dotns context (bytes32("dotns")), and translates the returned status byte into a PopStatus (0=NoStatus, 1=Devicehood, 2=Personhood; the precompile itself names them None, Lite and Full). Unknown tier bytes collapse to NoStatus, so a future precompile addition fails closed rather than silently being treated as a higher tier. There is no on-chain self-attestation; users obtain personhood off-chain through the People-chain ring proof and the alias-accounts pallet propagates the result via XCM. -Classification is not the same thing as effective holder context. A long label such as andrewsays is always a NoStatus-tier label by shape, but it may be held by a Personhood, Devicehood, or NoStatus account. Consumers that need to know what rules apply to that live name should combine three reads: classify the label, query the registrar owner, then query the owner's dotns-context PoP status through the precompile (`PopRules.popStatusOf`). The escrow position is the economic qualifier: if the token has an active release position with a non-zero amount, it came through the refundable NoStatus deposit path; if no such deposit exists, a verified holder can own the same long label without it being deposit-backed. In other words, andrewsays does not become a Personhood-tier label when a Personhood account owns it, but the owner can still hold personhood for transfer pricing, reverse resolution, and UI display. +Classification is not the same thing as effective holder context. A long label such as andrewsays is always a NoStatus-tier label by shape, but it may be held by a Personhood, Devicehood, or NoStatus account. Consumers that need to know what rules apply to that live name should combine three reads: classify the label, query the registrar owner, then query the owner's dotns-context PoP status through the precompile (`PopRules.popStatusOf`). The escrow position is the economic qualifier: if the token has an active release position with a non-zero amount, it came through the paid public path, whoever paid; if no such deposit exists, the name was issued for free, so a verified holder can own the same long label without it being deposit-backed. In other words, andrewsays does not become a Personhood-tier label when a Personhood account owns it, but the owner can still hold personhood for reverse resolution and UI display. A long name transfers for free whatever its holder's tier. Name grants are the exception path for users or organisations that need to register without satisfying the live PoP tier check. dotNS still does not accept self-attestation: the contracts only consume PoP status from the personhood precompile, and a user cannot set or prove their own status inside dotNS. Instead, `DotnsNameWhitelist` binds a specific label to a specific beneficiary, and registerReserved mints it at zero base cost, bypassing the PoP pricing gate while still using the normal commit-reveal and availability checks. Only governance issues a grant: every admin action on the whitelist requires a substrate Root dispatch, so no key can grant a name, the contract owner included. A Root dispatch can also mint a reserved name directly, without a grant. That covers the whitelist path; the registrar's controller set and the protocol registry's keys are still owner-managed, so an owner retains other routes to the same outcome. diff --git a/contracts/escrow/DotnsNameEscrow.sol b/contracts/escrow/DotnsNameEscrow.sol index 99507e3e0..e11518f38 100644 --- a/contracts/escrow/DotnsNameEscrow.sol +++ b/contracts/escrow/DotnsNameEscrow.sol @@ -82,7 +82,7 @@ contract DotnsNameEscrow is mapping(uint256 tokenId => uint256 indexPlusOne) private _releasedIndexPlusOne; /// @notice Cumulative balance of non-refundable protocol fees; only accumulates. - /// @dev Credited by cross-paid registration fees and transfer fees. Never debited: protocol + /// @dev Credited by transfer fees. Never debited: protocol /// fees do not back refunds, which draw solely on the per-asset reserve. uint256 public protocolFees; @@ -273,9 +273,9 @@ contract DotnsNameEscrow is ReleasePosition storage position = _positions[params.tokenId]; - // Use `recipient` as the "is this slot funded?" sentinel so zero-amount - // positions (seeded by cross-paid registrations, which pay a fee rather than a deposit) - // still count as present and cannot be re-seeded with a different recipient. + // Use `recipient` as the "is this slot funded?" sentinel so zero-amount positions + // (seeded by free registrations) still count as present and cannot be re-seeded with a + // different recipient. require(position.recipient == address(0), PositionAlreadyFunded(params.tokenId)); require(!position.released, AlreadyReleased(params.tokenId)); @@ -296,27 +296,6 @@ contract DotnsNameEscrow is emit OverpaymentRefunded(recipient, msg.value); } - /// @inheritdoc IDotnsNameEscrow - function depositProtocolFee(ProtocolFeeDepositParams calldata params) - external - payable - override - onlyController - { - require(msg.value > 0, InvalidAmount()); - - protocolFees += msg.value; - - emit CrossTierFeePaid( - params.tokenId, - params.payer, - params.recipient, - msg.value, - /* isRegistration */ - true - ); - } - /// @inheritdoc IDotnsNameEscrow function chargeTransferFee(ChargeTransferFeeParams calldata params) external @@ -373,8 +352,8 @@ contract DotnsNameEscrow is ReleasePosition storage position = _positions[tokenId]; // Recipient is the canonical "is this position present?" sentinel; zero-amount positions - // seeded for cross-paid registrations are still releasable so every minted name has a - // reachable lifecycle. + // seeded for free registrations are still releasable so every minted name has a reachable + // lifecycle. require(position.recipient != address(0), DepositNotConfigured(tokenId)); require(!position.released, AlreadyReleased(tokenId)); @@ -460,9 +439,8 @@ contract DotnsNameEscrow is // Nothing to settle: return before touching `claimed`. That flag is what `redeem` reads to // decide whether the holder has already been paid for the name, so setting it here would // make a zero-amount `withdraw`, which pays nothing and emits nothing, silently forfeit - // the holder's right to recover their own name for no consideration at all. Cross-paid - // registrations seed exactly these positions, as does any name whose curve price is zero, - // and `withdraw` is a path holders will take. + // the holder's right to recover their own name for no consideration at all. Free + // registrations seed exactly these positions, and `withdraw` is a path holders will take. if (owed == 0) return; // Effects: from here the deposit really is being handed over, so the flag is set. diff --git a/contracts/escrow/IDotnsNameEscrow.sol b/contracts/escrow/IDotnsNameEscrow.sol index cd49a758d..bffd2d97b 100644 --- a/contracts/escrow/IDotnsNameEscrow.sol +++ b/contracts/escrow/IDotnsNameEscrow.sol @@ -20,17 +20,6 @@ interface IDotnsNameEscrow { address recipient; } - /// @notice Parameters for recording a cross-paid registration fee into the protocol fee pot. - /// @dev The pot is non-refundable and only accumulates; it never backs a refund. `payer` is - /// preserved purely for event accounting since the fee itself is non-refundable. - /// @param payer Original `msg.sender` of the controller's `register` call. - /// @param recipient The NFT registrant the fee was paid on behalf of. - struct ProtocolFeeDepositParams { - uint256 tokenId; - address payer; - address recipient; - } - /// @notice Inputs for charging transfer friction and rebinding the escrow position. /// @dev The fee charged is the name's own price returned by @custom:function /// PopRules.transferFloor, settled to the protocol fee pot. The deposit, when present, @@ -147,10 +136,10 @@ interface IDotnsNameEscrow { /// @param recipient Address the NFT was returned to, which is also the position recipient. event NameRedeemed(uint256 indexed tokenId, address indexed recipient); - /// @notice Emitted when a cross-paid fee is paid into the protocol fee pot. + /// @notice Emitted when a transfer fee is paid into the protocol fee pot. /// @param payer Original `msg.sender` whose value funded the fee. - /// @param isRegistration True when emitted from `depositProtocolFee`; false from - /// `chargeTransferFee`. + /// @param isRegistration Always false: a registration locks a refundable deposit and pays no + /// fee, so only @custom:function chargeTransferFee emits this. event CrossTierFeePaid( uint256 indexed tokenId, address indexed payer, @@ -309,8 +298,8 @@ interface IDotnsNameEscrow { /// @notice Records an asset deposit position for a token. /// @dev Only the configured controller may call this, otherwise @custom:reverts NotController. /// `params.amount` must equal `msg.value`, otherwise @custom:reverts InvalidAmount; a - /// zero amount is accepted so cross-payer and free-tier registrations can still seed a - /// position that the release lifecycle can advance. Only native deposits are accepted + /// zero amount is accepted so free registrations can still seed a position that the + /// release lifecycle can advance. Only native deposits are accepted /// today, so a non-zero `params.asset` triggers @custom:reverts AssetNotSupported, and /// a zero `params.recipient` triggers @custom:reverts InvalidRecipient. The slot for /// `params.tokenId` must be empty (sentinel: `position.recipient == address(0)`): @@ -319,12 +308,6 @@ interface IDotnsNameEscrow { /// Emits @custom:emits NativeDepositRecorded once the deposit is booked. function deposit(DepositParams calldata params) external payable; - /// @notice Records a cross-paid registration fee into the protocol fee pot. - /// @dev Only the configured controller may call this, otherwise @custom:reverts NotController. - /// `msg.value` must be non-zero, otherwise @custom:reverts InvalidAmount. Emits - /// @custom:emits CrossTierFeePaid with `isRegistration = true` once the fee is booked. - function depositProtocolFee(ProtocolFeeDepositParams calldata params) external payable; - /// @notice Credits `msg.value` to `recipient`'s pull-payment ledger so the caller can later /// pull the balance with @custom:func claimWithdrawal. /// @dev Only the configured controller may call this, otherwise @custom:reverts NotController. diff --git a/contracts/pop/IPopRules.sol b/contracts/pop/IPopRules.sol index b5a5dc160..dd22a9b71 100644 --- a/contracts/pop/IPopRules.sol +++ b/contracts/pop/IPopRules.sol @@ -313,7 +313,9 @@ interface IPopRules { /// @notice Transfer-time floor: the greater of the recipient-reach component and the /// sender-tier-downgrade component, each priced at the name's own length. - /// @dev Re-prices the name at its own length on every move: returns the name's curve price when + /// @dev A name open to all (base length nine or more) always returns zero: any recipient could + /// have registered it, so moving it costs nothing whatever the tiers. Any other name is + /// re-priced at its own length on every move: returns the name's curve price when /// either (i) the recipient does not meet the label's required tier, or (ii) the /// recipient's personhood tier is strictly below the sender's, and zero when neither /// holds. Passing a name to a wallet that could never have registered it therefore costs diff --git a/contracts/pop/PopRules.sol b/contracts/pop/PopRules.sol index e31e8c05c..90bfb0977 100644 --- a/contracts/pop/PopRules.sol +++ b/contracts/pop/PopRules.sol @@ -307,6 +307,8 @@ contract PopRules is _requireLabel(name); if (from == to) return 0; (PopStatus required,, uint256 baseLength) = _classifyValidatedName(name); + // A name open to all moves freely: any recipient could have registered it. + if (required == PopStatus.NoStatus) return 0; uint256 ownPrice = _priceValidatedName(baseLength); PopStatus toTier = _personhoodTier(to); diff --git a/contracts/registrars/DotnsRegistrarController.sol b/contracts/registrars/DotnsRegistrarController.sol index c91d4001f..eace0dd02 100644 --- a/contracts/registrars/DotnsRegistrarController.sol +++ b/contracts/registrars/DotnsRegistrarController.sol @@ -243,7 +243,7 @@ contract DotnsRegistrarController is .setOwner(node, registration.owner); } - _settleEscrow(escrow, tokenId, registration.owner, isDirect, totalCharged); + _settleEscrow(escrow, tokenId, registration.owner, totalCharged); if (msg.value > totalCharged) { uint256 refund = msg.value - totalCharged; @@ -256,36 +256,23 @@ contract DotnsRegistrarController is } } - /// @notice Settles every escrow side-effect of a successful registration. - /// @dev Extracted to keep `register` under the stack-depth ceiling. On a direct - /// registration the full `chargeAmount` lands in the refundable deposit position - /// keyed to `nameOwner`. On a cross-payer registration the deposit position is - /// seeded with a zero amount so the release lifecycle stays reachable, and the same - /// `chargeAmount` routes to the protocol fee pot via `depositProtocolFee` keyed to - /// `msg.sender` as the payer. + /// @notice Locks the registration charge as the owner's refundable deposit. + /// @dev Extracted to keep `register` under the stack-depth ceiling. The deposit position is + /// keyed to `nameOwner` whoever paid, so a cross-payer registration gives the owner the + /// deposit. function _settleEscrow( address escrow, uint256 tokenId, address nameOwner, - bool isDirect, uint256 chargeAmount ) internal { - uint256 depositAmount = isDirect ? chargeAmount : 0; - IDotnsNameEscrow(payable(escrow)).deposit{value: depositAmount}( + IDotnsNameEscrow(payable(escrow)).deposit{value: chargeAmount}( IDotnsNameEscrow.DepositParams({ - tokenId: tokenId, asset: address(0), amount: depositAmount, recipient: nameOwner + tokenId: tokenId, asset: address(0), amount: chargeAmount, recipient: nameOwner }) ); - - if (!isDirect && chargeAmount > 0) { - IDotnsNameEscrow(payable(escrow)).depositProtocolFee{value: chargeAmount}( - IDotnsNameEscrow.ProtocolFeeDepositParams({ - tokenId: tokenId, payer: msg.sender, recipient: nameOwner - }) - ); - } } /// @inheritdoc IDotnsRegistrarController diff --git a/contracts/registrars/IDotnsRegistrar.sol b/contracts/registrars/IDotnsRegistrar.sol index 97b337aa0..6c6779737 100644 --- a/contracts/registrars/IDotnsRegistrar.sol +++ b/contracts/registrars/IDotnsRegistrar.sol @@ -145,10 +145,11 @@ interface IDotnsRegistrar is IERC721 { function labelOf(uint256 tokenId) external view returns (string memory label); /// @notice Quotes the additional native fee required to transfer a token to `to`. - /// @dev Returns the fee from @custom:function PopRules.transferFloor: the name's own price - /// as the maximum of (i) the reach component charged when the recipient does not meet - /// the label's required tier and (ii) the downgrade component charged when the - /// recipient tier is strictly below the sender tier. Self-transfers and + /// @dev Returns the fee from @custom:function PopRules.transferFloor: zero for a name open to + /// all (base length nine or more), otherwise the name's own price as the maximum of (i) the + /// reach component charged when the recipient does not meet the label's required tier and + /// (ii) the downgrade component charged when the recipient tier is strictly below the sender + /// tier. Self-transfers and /// escrow-touching transfers (release into escrow, reclaim out of escrow) return /// zero. A token whose sender has no stored label also returns zero because there /// is no label-derived price to charge against. Soulbound names are non-transferable diff --git a/contracts/registrars/IDotnsRegistrarController.sol b/contracts/registrars/IDotnsRegistrarController.sol index 254d4913f..2d0a4ffde 100644 --- a/contracts/registrars/IDotnsRegistrarController.sol +++ b/contracts/registrars/IDotnsRegistrarController.sol @@ -152,15 +152,14 @@ interface IDotnsRegistrarController is IDotnsController { /// configured escrow address from the protocol registry (otherwise /// @custom:reverts EscrowNotConfigured). Splits on direct vs cross-payer at /// `msg.sender == registration.owner`. The direct path runs `priceWithCheck` (PoP-status - /// + reservation gate) and routes the charge to a refundable escrow deposit owned by - /// `registration.owner`. The cross-payer path skips the PoP-status revert in + /// + reservation gate). The cross-payer path skips the PoP-status revert in /// `priceWithCheck` but applies it directly via @custom:reverts OwnerStatusInsufficient /// when the owner's recorded tier does not meet the label's required tier, and still /// rejects governance-reserved labels with @custom:reverts GovernanceReserved and live - /// cross-user base-name reservations with @custom:reverts NameReserved. The cross-payer charge - /// is the owner-side registration price; the path applies no separate transfer friction. The - /// charge routes to the escrow protocol fee pot while seeding a zero-amount deposit slot so - /// the release lifecycle stays reachable. The reveal prices the name at the committed + /// cross-user base-name reservations with @custom:reverts NameReserved. Both paths charge the + /// registration price and lock it as a refundable escrow deposit owned by + /// `registration.owner`, so a payer who registers for someone else gives that owner the + /// deposit. The reveal prices the name at the committed /// `pricingVersion`, so a model change between commit and reveal leaves the amount unchanged, /// and rejects a total charge above the committed ceiling with @custom:reverts PriceExceedsMax /// before checking payment. The caller must supply at least the charge diff --git a/test/invariant/escrow/DotnsNameEscrowInvariant.t.sol b/test/invariant/escrow/DotnsNameEscrowInvariant.t.sol index 99752fcef..cbeb04096 100644 --- a/test/invariant/escrow/DotnsNameEscrowInvariant.t.sol +++ b/test/invariant/escrow/DotnsNameEscrowInvariant.t.sol @@ -76,7 +76,7 @@ contract DotnsNameEscrowInvariantTest is BaseDotns { /// @notice The handler's mirror of protocol-fee inflows must equal the escrow's on-chain /// protocol-fee balance. Protocol fees only ever accrue, so every inflow the handler - /// tracks (cross-tier register and payable transfer) must sum to exactly the balance. + /// tracks (payable transfers) must sum to exactly the balance. function invariant_protocol_fees_match_tracked_inflows() public view { assertEq( handler.ghost_protocolFeesPaidIn(), diff --git a/test/invariant/escrow/EscrowHandler.t.sol b/test/invariant/escrow/EscrowHandler.t.sol index 3fd67404b..f8a5577ab 100644 --- a/test/invariant/escrow/EscrowHandler.t.sol +++ b/test/invariant/escrow/EscrowHandler.t.sol @@ -52,8 +52,8 @@ contract EscrowHandler is Test { mapping(uint256 tokenId => string label) public labelByTokenId; /// @notice Cumulative native amount credited into protocol fees by handler-driven flows. - /// @dev Increments via cross-tier register (`depositProtocolFee`) and payable `transferFrom` - /// (`chargeTransferFee`). Protocol fees only ever accrue, so a conservation invariant + /// @dev Increments via payable `transferFrom` (`chargeTransferFee`). Protocol fees only + /// ever accrue, so a conservation invariant /// asserts this equals `escrow.protocolFees()`. uint256 public ghost_protocolFeesPaidIn; @@ -160,10 +160,9 @@ contract EscrowHandler is Test { /// @dev Bounds inputs with `bound()` to keep handler runs within meaningful state. /// Picks a payer and an owner from the actor set (different where possible), /// randomly aligns or splits their PoP statuses, and dispatches the controller - /// `register()` call from the payer. Routes to the deposit, depositProtocolFee, - /// or skip branch depending on the resulting tier prices. Revert-safe: if the - /// computed price is zero on both sides (Devicehood/Personhood no-cost path) the call - /// still completes but ghost state is only updated where state actually changed. + /// `register()` call from the payer. The charge is the owner-side price, locked as + /// the owner's refundable deposit. Revert-safe: a zero price skips the call, and a + /// reverting registration leaves ghost state untouched. /// @param ownerSeed Seed selecting the registrant. /// @param payerSeed Seed selecting the payer. /// @param statusSeed Seed selecting the random tier-elevation policy. @@ -212,49 +211,26 @@ contract EscrowHandler is Test { uint256 minAge = controller.minCommitmentAge(); vm.warp(block.timestamp + minAge + 1); - // Cross-tier path uses priceWithoutCheck for the owner. - uint256 ownerPrice = popRules.priceWithoutCheck(label, ownerAddr).price; - // `payerPrice` is read for documentation continuity even though the A1 - // max-not-sum charge no longer adds it on top of `ownerPrice`. Bind into - // a local so the read is observable in traces and a future branch - // expansion can reuse it without re-reading from the oracle. - uint256 payerPrice = popRules.priceWithoutCheck(label, payer).price; - payerPrice; - - // Under the A1 max-not-sum rule the controller charges - // `max(priced.price, friction)` on the cross-payer path and routes the - // whole charge into protocol fees via `depositProtocolFee`. The - // refundable deposit position is seeded at zero amount, so the only - // mutation invariant tracking has to mirror here is the protocol-fee leg. + // Cross-tier path uses priceWithoutCheck for the owner, and the whole charge becomes the + // owner's refundable deposit. + uint256 charge = popRules.priceWithoutCheck(label, ownerAddr).price; + bytes32 labelhash = keccak256(bytes(label)); bytes32 node = keccak256(abi.encodePacked(DOT_NODE, labelhash)); uint256 tokenId = uint256(node); - // The handler picks the registrar's required value live so it stays in - // sync with whatever the A1 formula returns for the current tier mix. - // `priceWithoutCheck` gives the owner-side price and `transferFloor` - // returns the payer-to-owner friction; the max collapses both into the - // single charge the controller demands. - uint256 frictionForCharge = popRules.transferFloor(label, payer, ownerAddr); - uint256 charge = ownerPrice > frictionForCharge ? ownerPrice : frictionForCharge; - - // Skip when no value moves: a zero charge produces a free zero-amount - // position with no protocol-fee or reserves delta, so adding it to the - // ghost-state token set adds noise without exercising any new branch. + // Skip when no value moves: a zero charge produces a free zero-amount position with no + // reserves delta, so adding it to the ghost-state token set adds noise without + // exercising any new branch. if (charge == 0) return; vm.prank(payer); try controller.register{value: charge}(registration) { _depositedTokenIds.push(tokenId); labelByTokenId[tokenId] = label; - // Cross-payer registrations seed a zero-amount refundable position; - // ghost-state mirrors that by leaving `depositAmounts` at zero. - depositAmounts[tokenId] = 0; - - // The whole cross-payer charge becomes protocol fee. Accumulate the - // independently-computed `charge` so the conservation invariant verifies the - // escrow credited exactly what the caller was charged, not an echo of its own state. - ghost_protocolFeesPaidIn += charge; + // Mirror the independently-computed charge so the reserves invariant verifies the + // escrow locked exactly what the payer was charged, not an echo of its own state. + depositAmounts[tokenId] = charge; } catch { return; } diff --git a/test/unit/escrow/DotnsNameEscrow.t.sol b/test/unit/escrow/DotnsNameEscrow.t.sol index 2fe4feb4a..9f76b23da 100644 --- a/test/unit/escrow/DotnsNameEscrow.t.sol +++ b/test/unit/escrow/DotnsNameEscrow.t.sol @@ -480,30 +480,23 @@ contract DotnsNameEscrowTest is BaseDotns { ); } - function test_transfer_charges_friction_and_rebinds_position() public { - // Downward cross-tier transfer of a funded NoStatus name: the friction fee settles to - // protocol fees and the deposit travels with the NFT. There is no transfer-time refund: - // `position.recipient` rebinds to the new holder, the locked deposit follows, and only - // the new holder can later release into escrow. Promoting `ed` to Personhood before the - // transfer forces `PopRules.transferFloor` to return the base fee D while the position - // still carries the original `BASE_DEPOSIT` deposit, so both legs of `chargeTransferFee` - // run in one call. + function test_downgrade_transfer_is_free_and_rebinds_position() public { + // A personhood holder moving an open name to a NoStatus account pays nothing: the + // deposit travels with the NFT, reserves stay put, and nobody is refunded at transfer + // time. Promoting `ed` to Personhood after registration makes the move a downgrade. uint256 tokenId = _registerNoStatus(LABEL, ed); _grantPersonhood(ed); - uint256 startingPrice = popRules.price(LABEL); - uint256 quotedFee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); - assertEq(quotedFee, startingPrice, "Personhood holder downgrading to NoStatus pays D"); + assertEq(dotnsRegistrar.quoteTransferFee(tokenId, leonardo), 0, "an open name moves free"); uint256 reservesBefore = dotnsNameEscrow.reserves(address(0)); - uint256 protocolFeesBefore = dotnsNameEscrow.protocolFees(); + uint256 escrowBalanceBefore = address(dotnsNameEscrow).balance; uint256 edRefundsBefore = dotnsNameEscrow.pendingRefundCount(ed); uint256 leonardoRefundsBefore = dotnsNameEscrow.pendingRefundCount(leonardo); - vm.deal(ed, quotedFee); vm.prank(ed); - dotnsRegistrar.transferFrom{value: quotedFee}(ed, leonardo, tokenId); + dotnsRegistrar.transferFrom(ed, leonardo, tokenId); IDotnsNameEscrow.ReleasePosition memory pos = dotnsNameEscrow.getReleasePosition(tokenId); assertEq(pos.recipient, leonardo, "position must rebind to the new holder"); @@ -515,9 +508,9 @@ contract DotnsNameEscrowTest is BaseDotns { "reserves must not move when the deposit follows the NFT" ); assertEq( - dotnsNameEscrow.protocolFees() - protocolFeesBefore, - startingPrice, - "friction fee settles to protocol fees independently of the deposit" + address(dotnsNameEscrow).balance, + escrowBalanceBefore, + "no value reaches the escrow on a transfer" ); assertEq( dotnsNameEscrow.pendingRefundCount(ed), @@ -758,126 +751,72 @@ contract DotnsNameEscrowTest is BaseDotns { tokenId = _tokenIdForLabel(label); } - /// @notice Verified payer sponsoring a NoStatus owner on a NoStatus-tier label pays - /// exactly D under the max-not-sum rule. - /// @dev `priced.price` equals D because the owner is NoStatus, and `transferFloor` returns - /// D because the payer's Personhood tier downgrades into the owner's NoStatus tier. The - /// controller charges `max(priced.price, friction) = D` and routes the entire charge - /// into protocol fees; the owner-side refundable position is seeded with zero - /// amount, so reserves must not move. - function test_cross_payer_verified_sponsors_nostatus_pays_only_D() public { - string memory label = "crosspayr01"; - - _grantPersonhood(leonardo); - // ed left at default NoStatus tier. - - uint256 priorProtocolFees = dotnsNameEscrow.protocolFees(); + /// @notice A cross-payer registration locks the price as the owner's refundable deposit. + /// @dev The payer is debited the price and the owner's position holds it, so reserves grow + /// by the same amount: the sponsor gives the owner the deposit. + function _assertCrossPayerDeposit( + string memory label, + address payer, + address nameOwner + ) + internal + { + uint256 ownPrice = popRules.price(label); uint256 priorReserves = dotnsNameEscrow.reserves(address(0)); - uint256 priorBalance = leonardo.balance; + uint256 priorBalance = payer.balance; - uint256 tokenId = _crossPayerRegister(label, leonardo, ed, BASE_DEPOSIT); + uint256 tokenId = _crossPayerRegister(label, payer, nameOwner, ownPrice); - // Protocol fees must grow by exactly D and reserves must stay flat: the entire charge - // routes to the friction reserve on the cross-payer path under the max rule. - assertEq( - dotnsNameEscrow.protocolFees() - priorProtocolFees, - BASE_DEPOSIT, - "protocol fees must grow by exactly D on cross-payer NoStatus sponsorship" - ); + assertEq(priorBalance - payer.balance, ownPrice, "payer is debited exactly the price"); assertEq( - dotnsNameEscrow.reserves(address(0)), - priorReserves, - "reserves must not move when the cross-payer charge routes entirely to protocol fees" - ); - assertEq( - priorBalance - leonardo.balance, - BASE_DEPOSIT, - "payer must be debited exactly D, never above" + dotnsNameEscrow.reserves(address(0)) - priorReserves, + ownPrice, + "the charge is held as a refundable deposit" ); - // The owner-side position is seeded with zero amount so the release lifecycle stays - // reachable; the recipient sentinel binds to the registrant. IDotnsNameEscrow.ReleasePosition memory pos = dotnsNameEscrow.getReleasePosition(tokenId); - assertEq(pos.recipient, ed, "position recipient must be the registrant"); - assertEq(pos.amount, 0, "no refundable deposit seeded for cross-payer registration"); + assertEq(pos.recipient, nameOwner, "position recipient must be the registrant"); + assertEq(pos.amount, ownPrice, "the owner holds the deposit"); } - /// @notice A cross-payer sponsoring a verified owner pays the name's own curve price into - /// protocol fees; personhood buys the owner access, not a free ride for the sponsor. - /// @dev `priced.price` is the curve price for the owner and `transferFloor` is at most that - /// same price, so the controller charges the owner-side price and `_settleEscrow` routes - /// it through `depositProtocolFee`, seeding a zero-amount refundable position. - function test_cross_payer_pays_the_curve_into_fees() public { - string memory label = BASE_LABEL_A; + /// @notice A verified payer sponsoring a NoStatus owner on an open label gives the owner a + /// deposit of the price. + function test_cross_payer_verified_sponsors_nostatus_funds_owner_deposit() public { + _grantPersonhood(leonardo); + // ed left at default NoStatus tier. + _assertCrossPayerDeposit("crosspayr01", leonardo, ed); + } + /// @notice A cross-payer sponsoring a verified owner on a band-gated label gives the owner a + /// deposit of the curve price; personhood buys the owner access, not a discount. + function test_cross_payer_funds_owner_deposit_at_the_curve() public { _grantPersonhood(leonardo); _grantPersonhood(ed); - - uint256 ownPrice = popRules.price(label); - uint256 priorProtocolFees = dotnsNameEscrow.protocolFees(); - uint256 priorReserves = dotnsNameEscrow.reserves(address(0)); - uint256 priorBalance = leonardo.balance; - - uint256 tokenId = _crossPayerRegister(label, leonardo, ed, ownPrice); - - assertEq( - dotnsNameEscrow.protocolFees() - priorProtocolFees, - ownPrice, - "cross-payer charge settles to protocol fees" - ); - assertEq( - dotnsNameEscrow.reserves(address(0)), - priorReserves, - "reserves must not move on the cross-payer path" - ); - assertEq( - priorBalance - leonardo.balance, ownPrice, "payer is debited the cross-payer charge" - ); - - IDotnsNameEscrow.ReleasePosition memory pos = dotnsNameEscrow.getReleasePosition(tokenId); - assertEq(pos.recipient, ed, "position recipient must be the registrant"); - assertEq(pos.amount, 0, "cross-payer registration seeds no refundable deposit"); + _assertCrossPayerDeposit(BASE_LABEL_A, leonardo, ed); } - /// @notice A cross-payer whose tier downgrades into the owner's is still charged only the - /// name's own price, never the owner price plus the downgrade friction. - /// @dev The owner-side price is the curve price and the downgrade friction can at most equal - /// it, so `max(ownerPrice, friction)` collapses to the owner price. The whole charge - /// routes to protocol fees with no refundable deposit. - function test_cross_payer_downgrade_charges_only_owner_price() public { + /// @notice A sponsor whose tier is above the owner's still pays only the name's price, all + /// of it into the owner's deposit. + function test_cross_payer_downgrade_funds_owner_deposit() public { // DEVICE_LABEL_A carries the gateway's separator, which the public path this test drives // rejects, so the label here is a flat one in the same tier. - string memory label = "michael01"; - _grantPersonhood(leonardo); _grantDevicehood(ed); + _assertCrossPayerDeposit("michael01", leonardo, ed); + } - uint256 ownPrice = popRules.price(label); - uint256 priorProtocolFees = dotnsNameEscrow.protocolFees(); - uint256 priorReserves = dotnsNameEscrow.reserves(address(0)); - uint256 priorBalance = leonardo.balance; - - uint256 tokenId = _crossPayerRegister(label, leonardo, ed, ownPrice); + /// @notice The owner of a cross-paid name gets the sponsor's deposit back on release. + function test_cross_payer_deposit_refunds_the_owner() public { + _grantPersonhood(leonardo); + uint256 tokenId = _crossPayerRegister("crosspayr02", leonardo, ed, BASE_DEPOSIT); - assertEq( - dotnsNameEscrow.protocolFees() - priorProtocolFees, - ownPrice, - "downgrade never exceeds the name's own price" - ); - assertEq( - dotnsNameEscrow.reserves(address(0)), - priorReserves, - "reserves stay flat on the cross-payer path" - ); - assertEq( - priorBalance - leonardo.balance, - ownPrice, - "payer is debited the owner-side price, not the sum" - ); + _approveAndRelease(tokenId, ed); + vm.warp(block.timestamp + ESCROW_COOLDOWN + 1); - IDotnsNameEscrow.ReleasePosition memory pos = dotnsNameEscrow.getReleasePosition(tokenId); - assertEq(pos.recipient, ed, "position recipient must be the registrant"); - assertEq(pos.amount, 0, "no refundable deposit seeded for cross-payer registration"); + vm.prank(ed); + dotnsNameEscrow.withdraw(tokenId); + assertEq(dotnsNameEscrow.pendingWithdrawal(ed), BASE_DEPOSIT, "the owner is refunded"); + assertEq(dotnsNameEscrow.pendingWithdrawal(leonardo), 0, "the sponsor is not"); } function test_solvency_after_force_sent_funds() public { diff --git a/test/unit/escrow/DotnsNameEscrowRedeem.t.sol b/test/unit/escrow/DotnsNameEscrowRedeem.t.sol index a479125b7..aaecc76b5 100644 --- a/test/unit/escrow/DotnsNameEscrowRedeem.t.sol +++ b/test/unit/escrow/DotnsNameEscrowRedeem.t.sol @@ -14,14 +14,13 @@ import {IPopRules} from "../../../contracts/pop/IPopRules.sol"; /// and the deposit settlement that makes the second possible without stranding value. /// @dev The defect these cover: reclaim used to gate on the `claimed` flag, which is only set by /// `withdraw`. A holder who released a name and never withdrew removed the label from -/// circulation permanently. For the zero-amount positions seeded by cross-payer registrations +/// circulation permanently. For the zero-amount positions seeded by free registrations /// there is nothing to withdraw, so that was the default outcome rather than an edge case. contract DotnsNameEscrowRedeemTest is BaseDotns { /// @notice 14-char label classifying as NoStatus, so registration seeds a funded position. string internal constant FUNDED_LABEL = "redeemlabela01"; - /// @notice 6-char digit-free label classifying as Personhood, registered on the cross-payer - /// path so its deposit position carries a zero amount. + /// @notice 6-char digit-free label minted with a zero-amount position. string internal constant FREE_LABEL = "redeem"; /// @notice Register `label` for `nameOwner` at `status` and return its tokenId. @@ -37,45 +36,26 @@ contract DotnsNameEscrowRedeemTest is BaseDotns { tokenId = _tokenIdForLabel(label); } - /// @notice Register `label` for `nameOwner` on the cross-payer path (`payer` pays, `nameOwner` - /// receives the name) and return its tokenId. - /// @dev A cross-payer registration routes the whole charge to the protocol fee pot and seeds a - /// zero-amount refundable position keyed to `nameOwner`, which is the registration shape - /// that produces a zero-amount position. `nameOwner` must carry `status`, because the - /// controller prices the owner's tier on this path. - function _registerCrossPayer( + /// @notice Mint `label` to `nameOwner` with a zero-amount position and return its tokenId. + /// @dev A free registration seeds a zero-amount position; every paid one locks the price as a + /// deposit, whoever pays. Mints and seeds directly as the controller, which is all the + /// escrow lifecycle reads. + function _mintWithZeroPosition( string memory label, - address nameOwner, - address payer, - IPopRules.PopStatus status + address nameOwner ) internal returns (uint256 tokenId) { - _setUserPopStatus(nameOwner, status); - - bytes32 secret = keccak256(abi.encodePacked(label, nameOwner, block.timestamp)); - IDotnsRegistrarController.Registration memory registration = - IDotnsRegistrarController.Registration({ - label: label, - owner: nameOwner, - secret: secret, - reserved: true, - maxPrice: type(uint256).max, - pricingVersion: popRules.pricingVersion() - }); - - bytes32 commitment = dotnsRegistrarController.makeCommitment(registration); - vm.prank(payer); - dotnsRegistrarController.commit(commitment); - - vm.warp(block.timestamp + dotnsRegistrarController.minCommitmentAge() + 1); - - uint256 charge = popRules.priceWithoutCheck(label, nameOwner).price; - vm.prank(payer); - dotnsRegistrarController.register{value: charge}(registration); - tokenId = _tokenIdForLabel(label); + vm.startPrank(address(dotnsRegistrarController)); + dotnsRegistrar.register(tokenId, nameOwner, label); + dotnsNameEscrow.deposit( + IDotnsNameEscrow.DepositParams({ + tokenId: tokenId, asset: address(0), amount: 0, recipient: nameOwner + }) + ); + vm.stopPrank(); } /// @notice Approve the escrow for `tokenId` and release it as `caller`. @@ -322,8 +302,7 @@ contract DotnsNameEscrowRedeemTest is BaseDotns { /// right to recover their own name for no consideration whatsoever. `withdraw` is also a /// step a holder has every reason to make before a name can be recycled. function test_zero_amount_withdrawal_does_not_forfeit_the_redeem_right() public { - uint256 tokenId = - _registerCrossPayer(FREE_LABEL, ed, leonardo, IPopRules.PopStatus.Personhood); + uint256 tokenId = _mintWithZeroPosition(FREE_LABEL, ed); assertEq(_positionOf(tokenId).amount, 0, "this case needs a zero-amount position"); _approveAndRelease(tokenId, ed); @@ -435,19 +414,14 @@ contract DotnsNameEscrowRedeemTest is BaseDotns { // the zero-amount case: what the bug actually was - /// @dev The headline regression test. A cross-payer registration seeds a zero-amount position, + /// @dev The headline regression test. A free registration seeds a zero-amount position, /// so its holder has nothing to withdraw and therefore no reason ever to call `withdraw`. /// Under a `released && claimed` reclaim gate that would make the name permanently /// unregisterable. function test_zero_amount_release_becomes_reclaimable_without_any_withdrawal() public { - uint256 tokenId = - _registerCrossPayer(FREE_LABEL, ed, leonardo, IPopRules.PopStatus.Personhood); + uint256 tokenId = _mintWithZeroPosition(FREE_LABEL, ed); - assertEq( - _positionOf(tokenId).amount, - 0, - "a cross-payer registration seeds a zero-amount position" - ); + assertEq(_positionOf(tokenId).amount, 0, "a free registration seeds a zero-amount position"); _approveAndRelease(tokenId, ed); diff --git a/test/unit/pop/PopRules.t.sol b/test/unit/pop/PopRules.t.sol index 131402c5f..c1926c7c9 100644 --- a/test/unit/pop/PopRules.t.sol +++ b/test/unit/pop/PopRules.t.sol @@ -268,14 +268,21 @@ contract PopRulesTests is BaseDotns { } function test_transferFloor_matches_model() public { - // A Personhood sender handing a NoStatus name to a NoStatus recipient pays the name's own - // price, which is the registered model's amount for its base length. - string memory label = "longnamehere"; + // A Personhood sender handing a band-gated name to a NoStatus recipient pays the name's + // own price, which is the registered model's amount for its base length. + string memory label = "lights"; _grantPersonhood(ed); uint256 floor = popRules.transferFloor(label, ed, leonardo); assertEq(floor, costModelRegistry.priceForBaseLength(bytes(label).length)); } + function test_transferFloor_is_zero_for_open_names() public { + // A name open to all moves for free, even when a Personhood holder hands it to a + // NoStatus account. + _grantPersonhood(ed); + assertEq(popRules.transferFloor("longnamehere", ed, leonardo), 0); + } + function test_pricingVersion_matches_registry() public view { assertEq(popRules.pricingVersion(), costModelRegistry.currentVersion()); assertEq(popRules.pricingVersion(), flatPricing.version()); diff --git a/test/unit/registrar/DotnsRegistrar.t.sol b/test/unit/registrar/DotnsRegistrar.t.sol index ed11cfb11..1cacecbc2 100644 --- a/test/unit/registrar/DotnsRegistrar.t.sol +++ b/test/unit/registrar/DotnsRegistrar.t.sol @@ -362,9 +362,9 @@ contract DotnsRegistrarTests is BaseDotns { } function test_transfer_reverts_when_fee_required_but_no_value_attached() public { - string memory label = "feerequired01"; - // Grant personhood to ed so the mint succeeds, but leave leonardo at NoStatus so the - // transfer floor charges a non-zero downgrade fee. + string memory label = "feegated"; + // Grant personhood to ed so the mint succeeds, but leave leonardo at NoStatus, below the + // band of a base length of eight, so the transfer floor charges a non-zero fee. _register(label, ed, IPopRules.PopStatus.Personhood); uint256 tokenId = _tokenIdForLabel(label); diff --git a/test/unit/registrar/DotnsRegistrarControllerLifecycle.t.sol b/test/unit/registrar/DotnsRegistrarControllerLifecycle.t.sol index e58e8c649..d278e26a9 100644 --- a/test/unit/registrar/DotnsRegistrarControllerLifecycle.t.sol +++ b/test/unit/registrar/DotnsRegistrarControllerLifecycle.t.sol @@ -75,7 +75,7 @@ contract DotnsRegistrarControllerLifecycleTest is BaseDotns { assertFalse(isReserved, "no base name reserved by either registration"); } - function test_register_cross_payer_charges_max_not_sum_of_price_and_reach() public { + function test_register_cross_payer_charges_only_the_owner_price() public { string memory label = NOSTATUS_LABEL_A; address payer = leonardo; address nameOwner = ed; @@ -84,11 +84,7 @@ contract DotnsRegistrarControllerLifecycleTest is BaseDotns { _grantNoStatus(nameOwner); uint256 ownerPrice = popRules.priceWithoutCheck(label, nameOwner).price; - uint256 friction = popRules.transferFloor(label, payer, nameOwner); assertGt(ownerPrice, 0, "scenario requires non-zero owner price"); - assertGt(friction, 0, "scenario requires non-zero payer-to-owner friction"); - - uint256 expectedCharge = ownerPrice > friction ? ownerPrice : friction; IDotnsRegistrarController.Registration memory registration = IDotnsRegistrarController.Registration({ @@ -105,21 +101,20 @@ contract DotnsRegistrarControllerLifecycleTest is BaseDotns { dotnsRegistrarController.commit(commitment); vm.warp(block.timestamp + dotnsRegistrarController.minCommitmentAge() + 1); - uint256 protocolFeesBefore = dotnsNameEscrow.protocolFees(); + uint256 payerBalanceBefore = payer.balance; + // Overpay so any extra charge on top of the owner price would show up in the debit. vm.prank(payer); - dotnsRegistrarController.register{value: expectedCharge}(registration); + dotnsRegistrarController.register{value: ownerPrice * 2}(registration); - // Cross-payer charge is the name's own curve price. The whole charge routes to protocol - // fees; the refundable deposit branch is reserved for direct registrants. assertEq( - dotnsNameEscrow.protocolFees() - protocolFeesBefore, - expectedCharge, - "cross-payer must credit the name's own curve price to protocol fees" + payerBalanceBefore - payer.balance, + ownerPrice, + "a payer above the owner's tier is charged only the owner price" ); } - function test_revert_register_cross_payer_when_msg_value_below_max_of_price_and_reach() public { + function test_revert_register_cross_payer_when_msg_value_below_price() public { string memory label = NOSTATUS_LABEL_A; address payer = leonardo; address nameOwner = ed; @@ -128,11 +123,7 @@ contract DotnsRegistrarControllerLifecycleTest is BaseDotns { _grantNoStatus(nameOwner); uint256 ownerPrice = popRules.priceWithoutCheck(label, nameOwner).price; - uint256 friction = popRules.transferFloor(label, payer, nameOwner); assertGt(ownerPrice, 0); - assertGt(friction, 0); - - uint256 expectedCharge = ownerPrice > friction ? ownerPrice : friction; IDotnsRegistrarController.Registration memory registration = IDotnsRegistrarController.Registration({ @@ -151,10 +142,10 @@ contract DotnsRegistrarControllerLifecycleTest is BaseDotns { vm.prank(payer); vm.expectRevert(IDotnsRegistrarController.InsufficientValue.selector); - dotnsRegistrarController.register{value: expectedCharge - 1}(registration); + dotnsRegistrarController.register{value: ownerPrice - 1}(registration); } - function test_register_cross_payer_routes_owner_price_to_protocol_fees() public { + function test_register_cross_payer_locks_the_price_as_the_owner_deposit() public { string memory label = NOSTATUS_LABEL_A; address payer = leonardo; address nameOwner = ed; @@ -180,7 +171,7 @@ contract DotnsRegistrarControllerLifecycleTest is BaseDotns { dotnsRegistrarController.commit(commitment); vm.warp(block.timestamp + dotnsRegistrarController.minCommitmentAge() + 1); - uint256 protocolFeesBefore = dotnsNameEscrow.protocolFees(); + uint256 reservesBefore = dotnsNameEscrow.reserves(address(0)); vm.prank(payer); dotnsRegistrarController.register{value: ownerPrice}(registration); @@ -189,11 +180,12 @@ contract DotnsRegistrarControllerLifecycleTest is BaseDotns { IDotnsNameEscrow.ReleasePosition memory position = dotnsNameEscrow.getReleasePosition(tokenId); - assertEq(position.amount, 0, "cross-payer must not seed a refundable position"); + assertEq(position.recipient, nameOwner, "the owner holds the position"); + assertEq(position.amount, ownerPrice, "the price is the owner's refundable deposit"); assertEq( - dotnsNameEscrow.protocolFees() - protocolFeesBefore, + dotnsNameEscrow.reserves(address(0)) - reservesBefore, ownerPrice, - "cross-payer price must accrue to protocol fees" + "the deposit is reserved for the owner's refund" ); } From 2cd62cd1b728951450c9cc0ea90239336b9e3589 Mon Sep 17 00:00:00 2001 From: giuseppere Date: Tue, 13 Oct 2026 16:23:16 +0200 Subject: [PATCH 2/2] feat(escrow): make every transfer free and let Root withdraw protocol fees --- .github/workflows/4naly3er.yml | 3 - .github/workflows/contract-coverage.yml | 5 +- .github/workflows/deploy-docs.yml | 2 - .github/workflows/gas-report.yml | 39 +--- .github/workflows/push_checking.yml | 5 +- .github/workflows/slither.yaml | 3 - README.md | 20 +- contracts/escrow/DotnsNameEscrow.sol | 111 +++------- contracts/escrow/IDotnsNameEscrow.sol | 107 ++++------ contracts/pop/IPopRules.sol | 26 --- contracts/pop/PopRules.sol | 42 +--- contracts/registrars/DotnsRegistrar.sol | 201 +++--------------- contracts/registrars/IDotnsRegistrar.sol | 73 +------ .../openzeppelin-contracts-upgradeable.patch | 28 --- patches/openzeppelin-contracts.patch | 59 ----- scripts/shell/apply-oz-patches.sh | 54 ----- setup.bash | 6 - .../DotnsRegistrarControllerFuzz.t.sol | 10 +- test/intergration/BasicDotns.t.sol | 3 +- .../NoStatusDepositLifecycle.t.sol | 9 +- test/intergration/PopLifecycleFlow.t.sol | 8 +- test/intergration/StoreIntegration.t.sol | 9 +- .../escrow/DotnsNameEscrowInvariant.t.sol | 25 +-- test/invariant/escrow/EscrowHandler.t.sol | 86 +------- .../DotnsRegistrarControllerInvariant.t.sol | 2 +- .../registrar/PopControllerHandler.t.sol | 11 +- test/unit/escrow/DotnsNameEscrow.t.sol | 143 +++++++++---- test/unit/escrow/DotnsNameEscrowRefunds.t.sol | 116 ++++------ test/unit/pop/PopRules.t.sol | 23 -- test/unit/registrar/DotnsRegistrar.t.sol | 99 +++------ .../registrar/DotnsRegistrarController.t.sol | 48 ++--- 31 files changed, 346 insertions(+), 1030 deletions(-) delete mode 100644 patches/openzeppelin-contracts-upgradeable.patch delete mode 100644 patches/openzeppelin-contracts.patch delete mode 100755 scripts/shell/apply-oz-patches.sh diff --git a/.github/workflows/4naly3er.yml b/.github/workflows/4naly3er.yml index 1a03275b5..47134a519 100644 --- a/.github/workflows/4naly3er.yml +++ b/.github/workflows/4naly3er.yml @@ -42,9 +42,6 @@ jobs: with: python-version: "3.12" - - name: Apply OZ payable patches - run: bash scripts/shell/apply-oz-patches.sh - - env: FOUNDRY_DISABLE_NIGHTLY_WARNING: "1" run: forge install && forge build --build-info diff --git a/.github/workflows/contract-coverage.yml b/.github/workflows/contract-coverage.yml index 324206d44..54b4321ba 100644 --- a/.github/workflows/contract-coverage.yml +++ b/.github/workflows/contract-coverage.yml @@ -66,9 +66,8 @@ jobs: # Forge just recompiles whatever the content hashes say is dirty, so a partial # match via `restore-keys` still saves most of a cold `via_ir` build. This has # to sit after `bun install`: its postinstall (`setup.bash`) force-reclones - # `lib/`'s pinned dependencies and reapplies the OZ patches on top, so hashing - # `lib/` any earlier would key the cache off content these steps are about to - # replace. + # `lib/`'s pinned dependencies, so hashing `lib/` any earlier would key the + # cache off content these steps are about to replace. # # Forge also drops persisted fuzz/invariant/test failure state under # `cache/fuzz`, `cache/invariant`, and `cache/test-failures`, and replays it diff --git a/.github/workflows/deploy-docs.yml b/.github/workflows/deploy-docs.yml index 915aed202..1f57c3fcc 100644 --- a/.github/workflows/deploy-docs.yml +++ b/.github/workflows/deploy-docs.yml @@ -47,8 +47,6 @@ jobs: curl -sSL "https://github.com/rust-lang/mdBook/releases/download/v${MDBOOK_VERSION}/mdbook-v${MDBOOK_VERSION}-x86_64-unknown-linux-gnu.tar.gz" | tar -xz -C /usr/local/bin - run: forge install - - name: Apply OZ payable patches - run: bash scripts/shell/apply-oz-patches.sh - run: forge build - name: Generate docs diff --git a/.github/workflows/gas-report.yml b/.github/workflows/gas-report.yml index 1d23c298a..a9f531a40 100644 --- a/.github/workflows/gas-report.yml +++ b/.github/workflows/gas-report.yml @@ -7,10 +7,6 @@ name: Gas Report # job is gated on `github.event.action == 'closed'` and does NOT check # out PR-head code, so no untrusted code executes under that context. # -# The matrix `generate` shard that pulls the PR head's `patches/` runs -# under the `pull_request` trigger (not `pull_request_target`), so its -# checkout of PR-head code does not see repository secrets. -# # Adding other `pull_request_target` event types or any step that # checks out PR-head code under that trigger would let a malicious PR # exfiltrate repo secrets. Re-review carefully if widened. @@ -89,33 +85,6 @@ jobs: with: python-version: "3.12" - # Sidecar checkout of the PR head's `patches/` and apply script. The - # master-branch matrix shard is checked out at `master`, which does not yet - # carry the OZ payable patches needed for the registrar transferFrom - # overrides. Sourcing them from the PR head keeps the two shards - # building against the same vendored OZ tree. Removed once the patches - # land on master. - - name: Sidecar checkout for OZ patches - if: matrix.target.id == 'master' - uses: actions/checkout@v4 - with: - ref: ${{ github.event.pull_request.head.sha }} - path: pr-head-sidecar - sparse-checkout: | - patches - scripts/shell/apply-oz-patches.sh - sparse-checkout-cone-mode: false - - - name: Apply OZ patches (master shard only) - if: matrix.target.id == 'master' - run: | - rm -rf patches - cp -R pr-head-sidecar/patches ./patches - mkdir -p scripts/shell - cp pr-head-sidecar/scripts/shell/apply-oz-patches.sh scripts/shell/apply-oz-patches.sh - chmod +x scripts/shell/apply-oz-patches.sh - bash scripts/shell/apply-oz-patches.sh - - run: bun install # `cache/` and `out/` are forge's own incremental-compile state: it hashes each @@ -123,11 +92,9 @@ jobs: # restored cache from an unrelated commit is always safe to build on top of. # Forge just recompiles whatever the content hashes say is dirty, so a partial # match via `restore-keys` still saves most of a cold `via_ir` build. This has - # to sit after `bun install`, not just after the master shard's patch step - # above: `bun install`'s postinstall (`setup.bash`) force-reclones `lib/`'s - # pinned dependencies for both shards and reapplies the OZ patches on top, - # so hashing `lib/` any earlier would key the cache off content that step - # is about to replace. + # to sit after `bun install`: its postinstall (`setup.bash`) force-reclones + # `lib/`'s pinned dependencies for both shards, so hashing `lib/` any earlier + # would key the cache off content that step is about to replace. # # Forge also drops persisted fuzz/invariant/test failure state under # `cache/fuzz`, `cache/invariant`, and `cache/test-failures`, and replays it diff --git a/.github/workflows/push_checking.yml b/.github/workflows/push_checking.yml index f65fe10be..6087a089a 100644 --- a/.github/workflows/push_checking.yml +++ b/.github/workflows/push_checking.yml @@ -88,9 +88,8 @@ jobs: # Forge just recompiles whatever the content hashes say is dirty, so a partial # match via `restore-keys` still saves most of a cold `via_ir` build. This has # to sit after `bun install`: its postinstall (`setup.bash`) force-reclones - # `lib/`'s pinned dependencies and reapplies the OZ patches on top, so hashing - # `lib/` any earlier would key the cache off content these steps are about to - # replace. + # `lib/`'s pinned dependencies, so hashing `lib/` any earlier would key the + # cache off content these steps are about to replace. # # Forge also drops persisted fuzz/invariant/test failure state under # `cache/fuzz`, `cache/invariant`, and `cache/test-failures`, and replays it diff --git a/.github/workflows/slither.yaml b/.github/workflows/slither.yaml index e77d642a7..c2fca72ba 100644 --- a/.github/workflows/slither.yaml +++ b/.github/workflows/slither.yaml @@ -41,9 +41,6 @@ jobs: with: python-version: "3.12" - - name: Apply OZ payable patches - run: bash scripts/shell/apply-oz-patches.sh - - env: FOUNDRY_DISABLE_NIGHTLY_WARNING: "1" run: forge install && forge build --build-info diff --git a/README.md b/README.md index a306a07a3..c5e701eeb 100644 --- a/README.md +++ b/README.md @@ -81,7 +81,7 @@ Names of five characters or fewer are never sold on the public path, which rejec ### Worked examples -The path decides whether the amount is a refundable deposit, a non-refundable fee, or nothing at all. Every amount below is the flat launch deposit of 10 DOT. +The path decides whether the amount is a refundable deposit or nothing at all. Every amount below is the flat launch deposit of 10 DOT. | Name | Base length | Path | Amount | Held as | |---|---|---|---|---| @@ -91,19 +91,19 @@ The path decides whether the amount is a refundable deposit, a non-refundable fe | `andrew` | 6 | public, own key (switch on, proof of personhood) | 10 DOT | refundable deposit | | `alicebob42` | 10 | public, own key | 10 DOT | refundable deposit | | any six-to-eight name | 6 to 8 | gateway path | none | no deposit, no fee | -| `andrew`, moved to a wallet that cannot clear its band | 6 | transfer | 10 DOT | protocol fee | +| `andrew`, moved to a wallet that cannot clear its band | 6 | transfer | none | free; the deposit moves with the name | -### Deposits and protocol fees +### Deposits Registering a name under your own key locks a refundable deposit equal to the name's price. The deposit is bound to the name rather than to you, so it travels with the token on every transfer and unlocks only when the current holder releases the name back to escrow. -A name someone else pays for locks the same deposit, owned by the name's owner: a third party who pays for another wallet's registration gives that owner the deposit, which the owner collects on release. A transfer that owes a fee pays a non-refundable fee instead, into a single protocol fee pot. A gateway grant carries neither a deposit nor a fee. The protocol fee pot only ever grows: it backs no refunds, and nothing burns, sweeps, or withdraws from it. Refunds draw solely on the separate per-asset reserve that deposits fund. A holder's own deposit is their money held in trust and is never moved into fees. +A name someone else pays for locks the same deposit, owned by the name's owner: a third party who pays for another wallet's registration gives that owner the deposit, which the owner collects on release. A gateway grant carries no deposit. Refunds draw solely on the per-asset reserve that deposits fund, and a holder's deposit is their money held in trust. The escrow also holds a protocol fee balance from the transfer fees earlier releases charged. Nothing credits it, and only a Root call, `withdrawProtocolFees`, moves it out, to an address governance chooses; deposits, pending withdrawals and refund entries stay out of its reach. -### Transfers re-price at the name's own length +### Transfers are free -Names of nine characters or more are open to all, so they transfer for free whatever the tiers. Any other transferable name charges its own price in two cases: the recipient cannot clear the name's band, or the move is a personhood downgrade, where the recipient's tier is lower than the sender's. Passing a six-character name to a wallet that could never have registered it costs the name's own price, so there is no cheap way to hand a band-gated name to a party who could not have earned it. A move between two wallets that both clear the band, and a move to the same address, cost nothing. The fee, when one is owed, settles into the protocol fee pot. The deposit, when present, rides with the name: the escrow position rebinds to the new holder rather than refunding, and only releasing the name back to escrow unlocks the locked deposit. +Every transferable name moves for free, whatever its length and the holders' tiers, and a transfer that carries value reverts. The bands apply at registration only: once registered, a name can go to any wallet. The deposit, when present, rides with the name: the escrow position rebinds to the new holder rather than refunding, and only releasing the name back to escrow unlocks the locked deposit. -Names minted through the gateway pallet are soulbound: they stay bound to the person who earned them and cannot be transferred at all. Any transfer of a gateway-issued name reverts, and quoting a transfer fee for one reverts rather than returning a price. Everything else about the name works normally, so its owner still sets records, issues subnames, and manages the name. +Names minted through the gateway pallet are soulbound: they stay bound to the person who earned them and cannot be transferred at all. Any transfer of a gateway-issued name reverts. Everything else about the name works normally, so its owner still sets records, issues subnames, and manages the name. A bare transfer does not reset the name's records. Registration and reclaim from escrow both point the registry record back at the default reverse resolver, but a direct ERC-721 `transferFrom` never calls the registry, so a name sold privately arrives still pointing at the seller's resolver. A buyer should overwrite the records they care about rather than assume they start clean. @@ -135,11 +135,11 @@ A pricing model's values are fixed for its life: the flat model's deposit, or th ### What governance controls -Governance chooses a model's values when it deploys one, and the contracts hold them coherent and nothing more: a flat deposit above zero; for the scarcity candidate, D and F above zero, F no greater than D, and D within a ceiling that keeps the multiplication below nine characters from overflowing. Beyond these checks there is no cap on how high or low the amounts go and no limit on how fast they move, so any rate limit or advance notice comes from the governance process rather than from these contracts. Governance also opens or closes the short-name market with the switch, tunes the release cooldown within its one-hour bound, sets the redeem window between 1 and 30 days, and sets the gateway's reservation duration. None of these controls lets governance seize, reassign, or destroy a name anyone already holds. +Governance chooses a model's values when it deploys one, and the contracts hold them coherent and nothing more: a flat deposit above zero; for the scarcity candidate, D and F above zero, F no greater than D, and D within a ceiling that keeps the multiplication below nine characters from overflowing. Beyond these checks there is no cap on how high or low the amounts go and no limit on how fast they move, so any rate limit or advance notice comes from the governance process rather than from these contracts. Governance also opens or closes the short-name market with the switch, tunes the release cooldown within its one-hour bound, sets the redeem window between 1 and 30 days, and sets the gateway's reservation duration. Root can also move the protocol fee balance out of the escrow. None of these controls lets governance seize, reassign, or destroy a name anyone already holds. ### Refund ledgers -The escrow keeps two separate pull-payment ledgers. One has no cooldown and serves as the fallback when a registration overpayment cannot be pushed back to the sender inline. The other gives every credit its own cooldown clock and holds transfer-fee overpayments. A deposit unlocked by releasing a funded name lands on the no-cooldown ledger; its delay comes instead from the position's own `withdrawAvailableAt` stamp, so once `withdraw` lands the credit is immediately pullable. Clients enumerate pending refunds through the escrow's public views, which page under a fixed cap so discovery stays bounded. +The escrow keeps two separate pull-payment ledgers. One has no cooldown and serves as the fallback when a registration overpayment cannot be pushed back to the sender inline. The other gives every entry its own cooldown clock and holds the transfer-fee overpayments earlier releases credited; nothing adds to it. A deposit unlocked by releasing a funded name lands on the no-cooldown ledger; its delay comes instead from the position's own `withdrawAvailableAt` stamp, so once `withdraw` lands the credit is immediately pullable. Clients enumerate pending refunds through the escrow's public views, which page under a fixed cap so discovery stays bounded. Clients that need the exact moment a released name becomes registrable should read `redeemableUntil` from `getReleasePosition` rather than polling `available`. @@ -231,7 +231,7 @@ A separated label reaches the chain only through the gateway path: the public pa Tier assignment is read on every pricing call, not stored: PopRules queries the alias-accounts personhood precompile at DotnsConstants.PERSONHOOD with the dotns context (bytes32("dotns")), and translates the returned status byte into a PopStatus (0=NoStatus, 1=Devicehood, 2=Personhood; the precompile itself names them None, Lite and Full). Unknown tier bytes collapse to NoStatus, so a future precompile addition fails closed rather than silently being treated as a higher tier. There is no on-chain self-attestation; users obtain personhood off-chain through the People-chain ring proof and the alias-accounts pallet propagates the result via XCM. -Classification is not the same thing as effective holder context. A long label such as andrewsays is always a NoStatus-tier label by shape, but it may be held by a Personhood, Devicehood, or NoStatus account. Consumers that need to know what rules apply to that live name should combine three reads: classify the label, query the registrar owner, then query the owner's dotns-context PoP status through the precompile (`PopRules.popStatusOf`). The escrow position is the economic qualifier: if the token has an active release position with a non-zero amount, it came through the paid public path, whoever paid; if no such deposit exists, the name was issued for free, so a verified holder can own the same long label without it being deposit-backed. In other words, andrewsays does not become a Personhood-tier label when a Personhood account owns it, but the owner can still hold personhood for reverse resolution and UI display. A long name transfers for free whatever its holder's tier. +Classification is not the same thing as effective holder context. A long label such as andrewsays is always a NoStatus-tier label by shape, but it may be held by a Personhood, Devicehood, or NoStatus account. Consumers that need to know what rules apply to that live name should combine three reads: classify the label, query the registrar owner, then query the owner's dotns-context PoP status through the precompile (`PopRules.popStatusOf`). The escrow position is the economic qualifier: if the token has an active release position with a non-zero amount, it came through the paid public path, whoever paid; if no such deposit exists, the name was issued for free, so a verified holder can own the same long label without it being deposit-backed. In other words, andrewsays does not become a Personhood-tier label when a Personhood account owns it, but the owner can still hold personhood for reverse resolution and UI display. Any transferable name moves for free whatever its holder's tier. Name grants are the exception path for users or organisations that need to register without satisfying the live PoP tier check. dotNS still does not accept self-attestation: the contracts only consume PoP status from the personhood precompile, and a user cannot set or prove their own status inside dotNS. Instead, `DotnsNameWhitelist` binds a specific label to a specific beneficiary, and registerReserved mints it at zero base cost, bypassing the PoP pricing gate while still using the normal commit-reveal and availability checks. Only governance issues a grant: every admin action on the whitelist requires a substrate Root dispatch, so no key can grant a name, the contract owner included. A Root dispatch can also mint a reserved name directly, without a grant. That covers the whitelist path; the registrar's controller set and the protocol registry's keys are still owner-managed, so an owner retains other routes to the same outcome. diff --git a/contracts/escrow/DotnsNameEscrow.sol b/contracts/escrow/DotnsNameEscrow.sol index e11518f38..576ab4fb4 100644 --- a/contracts/escrow/DotnsNameEscrow.sol +++ b/contracts/escrow/DotnsNameEscrow.sol @@ -16,6 +16,7 @@ import {IDotnsNameEscrow} from "./IDotnsNameEscrow.sol"; import {IDotnsRegistrar} from "../registrars/IDotnsRegistrar.sol"; import {IDotnsProtocolRegistry} from "../registry/IDotnsProtocolRegistry.sol"; import {DotnsConstants} from "../utils/DotnsConstants.sol"; +import {SystemUtils} from "../utils/SystemUtils.sol"; /// @title dotNS Name Escrow /// @notice Holds refundable deposits for registered names and manages the release/reclaim @@ -62,9 +63,7 @@ contract DotnsNameEscrow is /// @notice Delay after release before the deposit withdrawal may be credited. /// @dev Forces a delay between `release` and `withdraw`. It does not bound reclaim: the - /// release-to-reclaim boundary is `redeemWindow`, a separate and longer quantity. Also - /// supplies the per-entry clock for time-locked refund credits, which is why raising it - /// would slow every refund path and not just the deposit one. + /// release-to-reclaim boundary is `redeemWindow`, a separate and longer quantity. uint256 public cooldown; /// @notice Total amount of a specific asset reserved across all positions. @@ -81,9 +80,10 @@ contract DotnsNameEscrow is /// @notice Reverse lookup into `_releasedTokens` (one-based) for O(1) remove-by-swap. mapping(uint256 tokenId => uint256 indexPlusOne) private _releasedIndexPlusOne; - /// @notice Cumulative balance of non-refundable protocol fees; only accumulates. - /// @dev Credited by transfer fees. Never debited: protocol - /// fees do not back refunds, which draw solely on the per-asset reserve. + /// @notice Protocol fee balance held by the escrow. + /// @dev Holds the transfer fees collected by earlier implementations. Nothing credits it, only + /// @custom:function withdrawProtocolFees debits it, and refunds draw solely on the + /// per-asset reserve. uint256 public protocolFees; /// @notice Pull-payment ledger storing each recipient's claimable refund balance. @@ -94,17 +94,18 @@ contract DotnsNameEscrow is mapping(address recipient => uint256 amount) private _pendingWithdrawals; /// @notice Time-locked refund ledger keyed by entryId. - /// @dev Every credit allocates a fresh entryId so per-entry cooldowns are independent and - /// drip-feed credits cannot reset an existing entry's clock. - mapping(uint256 entryId => RefundEntry entry) private _refundEntries; + /// @dev Nothing credits new entries; the claim functions serve the entries already here. + mapping(uint256 entryId => RefundEntry entry) internal _refundEntries; /// @notice Per-recipient list of pending entryIds for paginated enumeration and batch claim. - mapping(address recipient => uint256[] entryIds) private _entriesByRecipient; + mapping(address recipient => uint256[] entryIds) internal _entriesByRecipient; /// @notice Reverse lookup into `_entriesByRecipient` (one-based) for O(1) remove-by-swap. - mapping(uint256 entryId => uint256 indexPlusOne) private _entryIndexPlusOne; + mapping(uint256 entryId => uint256 indexPlusOne) internal _entryIndexPlusOne; - /// @notice Monotonic counter assigning entryIds to new refund credits. + /// @notice Counter that assigned entryIds to refund credits. + /// @dev Nothing credits new entries, so it no longer moves. Kept so every later variable stays + /// at its slot across an upgrade. uint256 private _nextEntryId; /// @notice Period after release during which only the previous holder may act. @@ -297,51 +298,40 @@ contract DotnsNameEscrow is } /// @inheritdoc IDotnsNameEscrow - function chargeTransferFee(ChargeTransferFeeParams calldata params) - external - payable - override - onlyRegistrar - returns (uint256 charged) - { - ReleasePosition storage position = _positions[params.tokenId]; + function rebindPosition(uint256 tokenId, address to) external override onlyRegistrar { + ReleasePosition storage position = _positions[tokenId]; // Released positions are mid-lifecycle in escrow custody and must not be rebound; the // recipient is the original releaser who will claim the refund. The canonical transfer // path never reaches here for released tokens (`_update` short-circuits on escrow-touching // transfers) but the guard hardens the contract against a divergent registrar. - require(!position.released, AlreadyReleased(params.tokenId)); - - address priorRecipient = position.recipient; - - uint256 fee = params.transferFee; - require(msg.value >= fee, InsufficientValue()); + require(!position.released, AlreadyReleased(tokenId)); // Deposits follow the NFT, not the depositor. When the position is funded the locked // deposit travels with the name; when it is a zero-amount lifecycle marker the marker // travels with it. In both cases the position is rebound to the new holder so only // the current holder can later release into escrow and claim the refund. - if (priorRecipient != address(0) && params.to != priorRecipient) { - position.recipient = params.to; + address priorRecipient = position.recipient; + if (priorRecipient != address(0) && to != priorRecipient) { + position.recipient = to; } + } - charged = fee; - - if (fee > 0) { - protocolFees += fee; - emit CrossTierFeePaid( - params.tokenId, - params.payer, - params.to, - fee, - /* isRegistration */ - false - ); - } + /// @inheritdoc IDotnsNameEscrow + function withdrawProtocolFees(address to, uint256 amount) external override nonReentrant { + // `msg.sender` is not consulted: a Root dispatch has no account. + require(SystemUtils.originIsRoot(), NotRoot()); + require(to != address(0), InvalidRecipient()); + require(amount != 0, InvalidAmount()); + uint256 available = protocolFees; + require(amount <= available, ProtocolFeesExceeded(amount, available)); - uint256 overpayment = msg.value - fee; - if (overpayment > 0) { - _creditRefund(params.payer, overpayment, params.tokenId); - } + // Effects before interaction. + protocolFees = available - amount; + + (bool ok,) = payable(to).call{value: amount}(""); + require(ok, ProtocolFeeTransferFailed(to, amount)); + + emit ProtocolFeesWithdrawn(to, amount); } /// @inheritdoc IDotnsNameEscrow @@ -603,37 +593,6 @@ contract DotnsNameEscrow is entry = _refundEntries[entryId]; } - /// @notice Internal helper: allocate a new entryId and credit a refund to `recipient`. - /// @dev Assigns the next monotonic entryId, stores the entry, appends to the recipient's - /// enumeration array, and emits @custom:emits RefundCredited. The cooldown is read from the - /// configured `cooldown` storage value; @custom:constant MAX_COOLDOWN bounds it so the cast to - /// `uint64` cannot truncate for any plausible block timestamp. - function _creditRefund( - address recipient, - uint256 amount, - uint256 tokenId - ) - internal - returns (uint256 entryId) - { - require(recipient != address(0), InvalidRecipient()); - require(amount > 0, InvalidAmount()); - - entryId = ++_nextEntryId; - // forge-lint: disable-next-line(unsafe-typecast) - uint64 availableAt = uint64(block.timestamp + cooldown); - - _refundEntries[entryId] = RefundEntry({ - recipient: recipient, amount: amount, availableAt: availableAt, tokenId: tokenId - }); - - uint256[] storage list = _entriesByRecipient[recipient]; - list.push(entryId); - _entryIndexPlusOne[entryId] = list.length; - - emit RefundCredited(recipient, entryId, amount, availableAt, tokenId); - } - /// @notice Internal helper: delete a refund entry and swap-pop its slot in the recipient's /// enumeration array. function _removeRefundEntry(uint256 entryId, address recipient) internal { diff --git a/contracts/escrow/IDotnsNameEscrow.sol b/contracts/escrow/IDotnsNameEscrow.sol index bffd2d97b..499905f8d 100644 --- a/contracts/escrow/IDotnsNameEscrow.sol +++ b/contracts/escrow/IDotnsNameEscrow.sol @@ -20,23 +20,6 @@ interface IDotnsNameEscrow { address recipient; } - /// @notice Inputs for charging transfer friction and rebinding the escrow position. - /// @dev The fee charged is the name's own price returned by @custom:function - /// PopRules.transferFloor, settled to the protocol fee pot. The deposit, when present, - /// travels with the NFT: the position is rebound to the recipient so the new holder is - /// the only address that can later release into escrow and unlock the locked value. - /// There is no transfer-time refund path. - /// @param tokenId Token whose escrow position is charged and rebound to the recipient. - /// @param transferFee The name's own curve price on a downward or cross-reach transfer. - /// @param payer Original sender of the registrar transfer entrypoint. - /// @param to NFT recipient. Becomes the new position recipient whenever a position exists. - struct ChargeTransferFeeParams { - uint256 tokenId; - uint256 transferFee; - address payer; - address to; - } - /// @notice Canonical escrow state for a token. /// @dev Tracks the phased lifecycle as two flags: `released` flips on `release` (NFT in escrow, /// cooldown started); `claimed` flips on `withdraw` (refund credited to the pull-payment @@ -57,10 +40,10 @@ interface IDotnsNameEscrow { uint64 redeemableUntil; } - /// @notice Time-locked refund entry produced when the protocol owes a recipient value - /// outside the registration-overpayment path. - /// @dev Every credit creates a fresh entry with its own `availableAt`; later credits do not - /// reset earlier entries' clocks. Recipients claim entries individually or in batches. + /// @notice Time-locked refund entry for value a transfer overpaid under earlier + /// implementations. + /// @dev Nothing creates new entries; recipients claim the existing ones individually or in + /// batches once `availableAt` has elapsed. /// @param recipient Address that may claim this entry once `availableAt` has elapsed. /// @param amount Native value credited. /// @param availableAt Earliest block timestamp at which the recipient may claim. @@ -98,20 +81,6 @@ interface IDotnsNameEscrow { /// @notice Emitted when a recipient pulls their accumulated pending refund balance. event WithdrawalClaimed(address indexed recipient, uint256 amount); - /// @notice Emitted when a refund is credited to the time-locked refund ledger. - /// @param recipient Address that may claim the entry once `availableAt` has elapsed. - /// @param entryId Newly-assigned identifier for the credited entry. - /// @param amount Native value credited. - /// @param availableAt Earliest block timestamp at which the recipient may claim. - /// @param tokenId Token associated with the credit, retained for traceability. - event RefundCredited( - address indexed recipient, - uint256 indexed entryId, - uint256 amount, - uint64 availableAt, - uint256 indexed tokenId - ); - /// @notice Emitted when a recipient claims a single refund entry. /// @param recipient Address that pulled the entry. /// @param entryId Identifier of the claimed entry, now deleted. @@ -136,17 +105,10 @@ interface IDotnsNameEscrow { /// @param recipient Address the NFT was returned to, which is also the position recipient. event NameRedeemed(uint256 indexed tokenId, address indexed recipient); - /// @notice Emitted when a transfer fee is paid into the protocol fee pot. - /// @param payer Original `msg.sender` whose value funded the fee. - /// @param isRegistration Always false: a registration locks a refundable deposit and pays no - /// fee, so only @custom:function chargeTransferFee emits this. - event CrossTierFeePaid( - uint256 indexed tokenId, - address indexed payer, - address indexed recipient, - uint256 amount, - bool isRegistration - ); + /// @notice Emitted when governance moves protocol fees out of the escrow. + /// @param to Address that received the value. + /// @param amount Native value moved, debited from the protocol fee balance. + event ProtocolFeesWithdrawn(address indexed to, uint256 amount); /// @notice Emitted when overpayment is refunded to the payer. event OverpaymentRefunded(address indexed payer, uint256 amount); @@ -157,12 +119,18 @@ interface IDotnsNameEscrow { /// @notice Thrown when the caller is not the configured registrar. error NotRegistrar(address caller); + /// @notice Thrown when a governance call does not run under a Root origin. + error NotRoot(); + + /// @notice Thrown when a protocol fee withdrawal asks for more than the balance holds. + error ProtocolFeesExceeded(uint256 amount, uint256 available); + + /// @notice Thrown when the recipient of a protocol fee withdrawal rejects the value. + error ProtocolFeeTransferFailed(address to, uint256 amount); + /// @notice Thrown when the supplied refund recipient is invalid (e.g. zero address). error InvalidRecipient(); - /// @notice Thrown when the attached call value is insufficient to cover the computed charge. - error InsufficientValue(); - /// @notice Thrown when the per-asset reserve cannot cover the refund owed. /// @param available Reserve balance available for the asset. error InsufficientFunds(uint256 tokenId, uint256 owed, uint256 available); @@ -318,27 +286,34 @@ interface IDotnsNameEscrow { /// @param recipient Address whose pending balance should grow by `msg.value`. function creditOverpayment(address recipient) external payable; - /// @notice Charges the transfer fee and rebinds the token's escrow position to the new holder. + /// @notice Rebinds the token's escrow position to the name's new holder. /// @dev Only the configured registrar may call this, otherwise @custom:reverts NotRegistrar. - /// When a fee is owed, the attached value must cover it or @custom:reverts - /// InsufficientValue. Whenever a position exists for the token and the NFT is leaving its - /// prior recipient, the position recipient is rebound to the new holder so the deposit - /// (when funded) and the lifecycle marker (when zero-amount) both follow the NFT. The - /// escrow does not refund anyone at transfer time; the only path back to the locked - /// deposit is for the current holder to release into escrow and wait the cooldown. - /// Emits @custom:emits CrossTierFeePaid (non-registration) when a non-zero fee is credited - /// to the protocol fee pot, and credits any surplus value to the payer on the time-locked - /// refund ledger via @custom:emits RefundCredited. - /// @return charged Amount actually credited to the protocol fee pot. - function chargeTransferFee(ChargeTransferFeeParams calldata params) - external - payable - returns (uint256 charged); - - /// @notice Returns the cumulative protocol fee balance, non-refundable and accumulating. + /// Whenever a position exists for the token and the NFT is leaving its prior recipient, + /// the position recipient becomes `to`, so the deposit (when funded) and the lifecycle + /// marker (when zero-amount) both follow the NFT. The escrow refunds nobody at transfer + /// time; the only path back to the locked deposit is for the current holder to release + /// into escrow and wait the cooldown. A released position @custom:reverts AlreadyReleased. + /// @param tokenId Token whose position follows the transfer. + /// @param to NFT recipient. + function rebindPosition(uint256 tokenId, address to) external; + + /// @notice Returns the protocol fee balance the escrow holds. + /// @dev Nothing credits it: it holds the transfer fees collected by earlier implementations. + /// Only @custom:function withdrawProtocolFees spends it. /// @return balance Current protocol fee balance, in wei. function protocolFees() external view returns (uint256 balance); + /// @notice Moves `amount` of the protocol fee balance to `to`. + /// @dev Callable only under a Root origin, otherwise @custom:reverts NotRoot. Spends the + /// protocol fee balance alone: deposits, pending withdrawals and refund entries belong to + /// their holders and stay out of reach. A zero `to` @custom:reverts InvalidRecipient, a + /// zero `amount` @custom:reverts InvalidAmount, an `amount` above the balance + /// @custom:reverts ProtocolFeesExceeded, and a recipient that rejects the value + /// @custom:reverts ProtocolFeeTransferFailed. Emits @custom:emits ProtocolFeesWithdrawn. + /// @param to Address that receives the value. + /// @param amount Native value to move, in wei. + function withdrawProtocolFees(address to, uint256 amount) external; + /// @notice Releases a token into escrow and starts the withdrawal cooldown. /// @dev First step of the phased lifecycle. The caller must be the current NFT holder and the /// current position recipient (the field is rebound to the holder on every transfer that diff --git a/contracts/pop/IPopRules.sol b/contracts/pop/IPopRules.sol index dd22a9b71..34080fe0e 100644 --- a/contracts/pop/IPopRules.sol +++ b/contracts/pop/IPopRules.sol @@ -311,32 +311,6 @@ interface IPopRules { view returns (PriceWithMeta memory metadata); - /// @notice Transfer-time floor: the greater of the recipient-reach component and the - /// sender-tier-downgrade component, each priced at the name's own length. - /// @dev A name open to all (base length nine or more) always returns zero: any recipient could - /// have registered it, so moving it costs nothing whatever the tiers. Any other name is - /// re-priced at its own length on every move: returns the name's curve price when - /// either (i) the recipient does not meet the label's required tier, or (ii) the - /// recipient's personhood tier is strictly below the sender's, and zero when neither - /// holds. Passing a name to a wallet that could never have registered it therefore costs - /// the name's own curve price. The two components overlap on pure - /// tier mismatches, so the function takes their maximum rather than their sum to avoid - /// double-charging. Consumed by @custom:function DotnsRegistrar.quoteTransferFee. - /// A label that is neither a single lowercase ASCII DNS label nor a device name triggers - /// @custom:reverts PopError. - /// @param name Domain label being transferred. - /// @param from Current holder of the name. - /// @param to Incoming holder of the name. - /// @return floor Transfer-time floor in wei: the name's own curve price, or zero. - function transferFloor( - string calldata name, - address from, - address to - ) - external - view - returns (uint256 floor); - /// @notice Returns whether `name` can key a reservation: a base name with no trailing digits. /// @dev A device name always ends in two digits, so it never qualifies. Non-canonical labels /// trigger @custom:reverts PopError. diff --git a/contracts/pop/PopRules.sol b/contracts/pop/PopRules.sol index 90bfb0977..22a5387e1 100644 --- a/contracts/pop/PopRules.sol +++ b/contracts/pop/PopRules.sol @@ -293,35 +293,6 @@ contract PopRules is return _costModelRegistry().currentVersion(); } - /// @inheritdoc IPopRules - function transferFloor( - string calldata name, - address from, - address to - ) - external - view - override - returns (uint256 floor) - { - _requireLabel(name); - if (from == to) return 0; - (PopStatus required,, uint256 baseLength) = _classifyValidatedName(name); - // A name open to all moves freely: any recipient could have registered it. - if (required == PopStatus.NoStatus) return 0; - uint256 ownPrice = _priceValidatedName(baseLength); - - PopStatus toTier = _personhoodTier(to); - uint256 reachComponent = _meetsReach(required, toTier) ? 0 : ownPrice; - - PopStatus fromTier = _personhoodTier(from); - // `_personhoodTier` never returns Reserved, so users are in {NoStatus, Devicehood, - // Personhood} and enum comparison reflects tier ordering directly. - uint256 downgradeComponent = toTier < fromTier ? ownPrice : 0; - - return reachComponent > downgradeComponent ? reachComponent : downgradeComponent; - } - /// @inheritdoc IPopRules function popStatusOf(address account) external view override returns (PopStatus tier) { return _personhoodTier(account); @@ -343,21 +314,16 @@ contract PopRules is } /// @notice Single canonical "is `userStatus` at reach for `required`?" predicate. - /// @dev Both `priceWithCheck` and `transferFloor` build on this so the tier-eligibility rule - /// lives in exactly one place and the callers cannot disagree about who clears a given label. - /// `_personhoodTier` never returns `Reserved`, so `userStatus` is in `{NoStatus, Devicehood, - /// Personhood}` and the enum comparison reflects tier ordering directly. A `Reserved` - /// `required` (governance label) is unreachable by any verified user, so the comparison returns - /// false and the caller charges the friction fee, providing defence-in-depth if a Reserved - /// label ever enters circulation. + /// @dev `_personhoodTier` never returns `Reserved`, so `userStatus` is in `{NoStatus, + /// Devicehood, Personhood}` and the enum comparison reflects tier ordering directly. A + /// `Reserved` `required` (governance label) is unreachable by any verified user, so the + /// comparison returns false. function _meetsReach(PopStatus required, PopStatus userStatus) private pure returns (bool) { return userStatus >= required; } /// @notice Amount for a base length at the current cost-model version. /// @dev The cost-model registry owns the curve; this contract passes it only the base length. - /// The call is a view because it runs on the ERC721 transfer floor read through - /// @custom:function transferFloor. function _priceValidatedName(uint256 baseLength) internal view returns (uint256 priceValue) { return _costModelRegistry().priceForBaseLength(baseLength); } diff --git a/contracts/registrars/DotnsRegistrar.sol b/contracts/registrars/DotnsRegistrar.sol index a1106534e..9da21165b 100644 --- a/contracts/registrars/DotnsRegistrar.sol +++ b/contracts/registrars/DotnsRegistrar.sol @@ -21,7 +21,6 @@ import {StoreUtils} from "../utils/StoreUtils.sol"; import {LabelUtils} from "../utils/LabelUtils.sol"; import {StringUtils} from "../utils/StringUtils.sol"; import {IDotnsNameEscrow} from "../escrow/IDotnsNameEscrow.sol"; -import {IPopRules} from "../pop/IPopRules.sol"; import {DotnsConstants} from "../utils/DotnsConstants.sol"; /// @title dotNS Registrar @@ -29,8 +28,8 @@ import {DotnsConstants} from "../utils/DotnsConstants.sol"; /// @dev Deliberately policy-free on pricing, reservations, and PoP gating; those live in the /// controllers and @custom:contract IPopRules. The registrar owns transferability itself: publicly /// registered names transfer freely, while names minted through the dotNS gateway pallet are -/// soulbound and revert on transfer. The `_update` hook enforces both the soulbound gate and the -/// fee-on-transfer settlement that consults the escrow. +/// soulbound and revert on transfer. Transfers carry no fee. The `_update` hook enforces the +/// soulbound gate and keeps the escrow position with the holder. /// @custom:security-contact admin@parity.io contract DotnsRegistrar is Initializable, @@ -58,8 +57,7 @@ contract DotnsRegistrar is /// @notice Marks a token as soulbound: minted through the gateway pallet and non-transferable. /// @dev Set at mint by @custom:function register when the caller is the address registered /// under `DotnsConstants.POP_CONTROLLER`. Write-once and never cleared: a name's soulbound - /// state is fixed at registration. Read by the `_update` transfer gate and by - /// @custom:function quoteTransferFee. + /// state is fixed at registration. Read by the `_update` transfer gate. mapping(uint256 tokenId => bool soulbound) private _soulbound; /// @dev Reserved storage space to allow for layout changes in the future. @@ -143,8 +141,8 @@ contract DotnsRegistrar is // Empty labels are an intentional gateway-cold path (substrate Root cannot deploy a // `LabelStore` under `pallet-revive`, so the controller stashes a pending claim and the // user settles via @custom:function IDotnsPopController.claimLabelStore later). Non-empty - // labels must still be canonical so the transfer-floor lookup in `_quoteTransferFee` - // cannot brick the token by reverting on a malformed label. + // labels must still be canonical, since every transfer mirrors the label into the new + // holder's store and @custom:function labelOf reads it back. require(bytes(label).length == 0 || label.isSingleLabel(), InvalidLabel()); _mint(owner, id); // Provenance is verified here rather than trusted from a caller-supplied flag: only the @@ -164,66 +162,6 @@ contract DotnsRegistrar is return LabelUtils.stripTld(protocolRegistry.tld(), _readLabel(tokenId, holder)); } - /// @inheritdoc IDotnsRegistrar - function quoteTransferFee( - uint256 tokenId, - address to - ) - external - view - override - returns (uint256 requiredFee) - { - require(to != address(0), ERC721InvalidReceiver(address(0))); - // A soulbound name cannot be transferred, so it has no transfer price. Revert rather than - // return zero: a zero here would read as "transferable, no fee" to integrators while any - // real transfer reverts in `_update`. - require(!_soulbound[tokenId], NameSoulbound(tokenId)); - - address from = ownerOf(tokenId); - (,, requiredFee) = _quoteTransferFee(from, to, tokenId); - } - - /// @inheritdoc IDotnsRegistrar - function transferFrom( - address from, - address to, - uint256 tokenId - ) - public - payable - override(ERC721Upgradeable, IDotnsRegistrar) - { - super.transferFrom(from, to, tokenId); - } - - /// @inheritdoc IDotnsRegistrar - function safeTransferFrom( - address from, - address to, - uint256 tokenId - ) - public - payable - override(ERC721Upgradeable, IDotnsRegistrar) - { - super.safeTransferFrom(from, to, tokenId, ""); - } - - /// @inheritdoc IDotnsRegistrar - function safeTransferFrom( - address from, - address to, - uint256 tokenId, - bytes memory data - ) - public - payable - override(ERC721Upgradeable, IDotnsRegistrar) - { - super.safeTransferFrom(from, to, tokenId, data); - } - /// @notice Returns the release this network declares it runs, read live from the protocol /// registry so every dotNS contract reports one synchronised value. /// @dev Mirror of `IDotnsProtocolRegistry.protocolVersion`, kept under the historical @@ -275,20 +213,15 @@ contract DotnsRegistrar is return from; } - // Soulbound names are non-transferable, including a move to the sender's own address, which - // keeps this in step with @custom:function quoteTransferFee and the interface contract. It + // Soulbound names are non-transferable, including a move to the sender's own address. It // reverts rather than returning, unwinding the ownership move `super._update` has already // made, and sits before any escrow or store lookup so a soulbound token is rejected even // when the escrow is unconfigured, blocking every custody move including release into // escrow. require(!_soulbound[tokenId], NameSoulbound(tokenId)); - // Self-transfers of a transferable name carry no economic event. Reject attached value for - // the same trapped-funds reason as the mint path above. - if (from == to) { - require(msg.value == 0, UnexpectedValue()); - return from; - } + // Self-transfers of a transferable name change nothing. + if (from == to) return from; // Resolve every registry-sourced dependency once and thread it into the helpers so a // single transfer pays one external lookup per key rather than three. @@ -298,45 +231,27 @@ contract DotnsRegistrar is // `release` is the only caller that moves a name into custody, so any other sender is a // deposit the escrow holds no position for. require(to != escrow || msg.sender == escrow, UnsolicitedEscrowDeposit(tokenId)); - IStoreFactory factory = IStoreFactory(registry.get(DotnsConstants.STORE_FACTORY)); - - bool isEscrowTouching = to == escrow || from == escrow; - // Skip mirroring on escrow-touching paths: release deposits the NFT into custody where - // a `LabelStore` would be wasted and reclaim hands it back to a fresh-mint controller - // that writes the label through its own flow. - if (!isEscrowTouching) { - _syncRecipientStore(factory, to, from, tokenId); - } - (uint256 transferFee, uint256 requiredFee) = - _quoteTransferFeeFor(registry, factory, isEscrowTouching, from, to, tokenId); - if (requiredFee != 0) { - require(msg.value >= requiredFee, TransferFeeRequired(tokenId, to, requiredFee)); - } + // Release and reclaim manage the position and the stores themselves: release deposits the + // NFT into custody where a `LabelStore` would be wasted, reclaim hands it to a controller + // that writes the label through its own flow, and the escrow is mid-call, so its + // non-reentrancy guard would reject a rebind. + if (to == escrow || from == escrow) return from; + + _syncRecipientStore( + IStoreFactory(registry.get(DotnsConstants.STORE_FACTORY)), to, from, tokenId + ); // Deposits follow the NFT, not the depositor: every transfer that moves a name off the // prior position recipient rebinds the escrow position to the new holder so the locked // deposit (when funded) and the lifecycle marker (when zero-amount) both travel with the - // name. Escrow-touching transfers are excluded because the escrow is mid-call and its - // non-reentrancy guard would reject a re-entry; release/reclaim manage the position - // directly. - bool positionSyncNeeded; - if (!isEscrowTouching) { - IDotnsNameEscrow.ReleasePosition memory position = - IDotnsNameEscrow(payable(escrow)).getReleasePosition(tokenId); - positionSyncNeeded = position.recipient != address(0) && to != position.recipient; + // name. + IDotnsNameEscrow.ReleasePosition memory position = + IDotnsNameEscrow(payable(escrow)).getReleasePosition(tokenId); + if (position.recipient != address(0) && to != position.recipient) { + IDotnsNameEscrow(payable(escrow)).rebindPosition(tokenId, to); } - if (requiredFee == 0 && msg.value == 0 && !positionSyncNeeded) { - return from; - } - - IDotnsNameEscrow(payable(escrow)).chargeTransferFee{value: msg.value}( - IDotnsNameEscrow.ChargeTransferFeeParams({ - tokenId: tokenId, transferFee: transferFee, payer: msg.sender, to: to - }) - ); - return from; } @@ -358,9 +273,8 @@ contract DotnsRegistrar is return; } // A slot holding a different string was written by someone else, and `storeLabel` has no - // delete, so mirroring nothing would hand over a name `_quoteTransferFeeFor` rejects on - // every onward transfer. A matching entry is still a no-op, so a transfer back to a prior - // owner passes. + // delete, so mirroring nothing would hand over a name whose label the next transfer could + // not read. A matching entry is still a no-op, so a transfer back to a prior owner passes. factory.writeNewLabel(to, bytes32(tokenId), fullName); } @@ -399,11 +313,6 @@ contract DotnsRegistrar is escrow = protocolRegistry.get(DotnsConstants.NAME_ESCROW); } - /// @notice Resolves the configured PoP rules contract from the protocol registry. - function _popRules() private view returns (IPopRules rules) { - rules = IPopRules(protocolRegistry.get(DotnsConstants.POP_RULES)); - } - /// @notice Resolves the configured store factory from the protocol registry. function _storeFactory() private view returns (IStoreFactory factory) { factory = IStoreFactory(protocolRegistry.get(DotnsConstants.STORE_FACTORY)); @@ -419,68 +328,6 @@ contract DotnsRegistrar is .writeNewLabel(owner, bytes32(tokenId), string.concat(label, protocolRegistry.tld())); } - /// @notice Quotes the friction fee required for a transfer. - /// @dev Required fee is the name's own price returned by @custom:function - /// PopRules.transferFloor. It is paid by the sender on every downward or cross-reach transfer - /// and settles to the - /// protocol fee pot. Any prior deposit travels with the NFT: the escrow rebinds the position to - /// the new holder rather than refunding the sender, so transferring a funded name forfeits the - /// locked deposit to the recipient. Self-transfers and escrow-touching transfers return zero. - function _quoteTransferFee( - address from, - address to, - uint256 tokenId - ) - private - view - returns (address escrow, uint256 transferFee, uint256 requiredFee) - { - if (from == to) return (address(0), 0, 0); - - IDotnsProtocolRegistry registry = protocolRegistry; - escrow = registry.get(DotnsConstants.NAME_ESCROW); - require(escrow != address(0), EscrowNotConfigured()); - - bool isEscrowTouching = to == escrow || from == escrow; - IStoreFactory factory = IStoreFactory(registry.get(DotnsConstants.STORE_FACTORY)); - (transferFee, requiredFee) = - _quoteTransferFeeFor(registry, factory, isEscrowTouching, from, to, tokenId); - } - - /// @notice Quotes the transfer floor reusing a caller-cached registry and store factory. - /// @dev Hot-path variant used by @custom:function _update. Returns `(0, 0)` for any - /// escrow-touching move or when the sender holds no label entry; otherwise reads the canonical - /// label and delegates to @custom:function PopRules.transferFloor. - function _quoteTransferFeeFor( - IDotnsProtocolRegistry registry, - IStoreFactory factory, - bool isEscrowTouching, - address from, - address to, - uint256 tokenId - ) - private - view - returns (uint256 transferFee, uint256 requiredFee) - { - if (isEscrowTouching) return (0, 0); - - string memory fullName = _readLabelFor(factory, tokenId, from); - // No label means there is no label-derived price to charge against; treat as a zero-fee - // move. This is defensive: a gateway name is soulbound and reverts before reaching here, - // and a public name always carries a label, so no reachable transfer hits this branch. - if (bytes(fullName).length == 0) return (0, 0); - // A stored full name always carries the registry TLD suffix, so an empty strip means the - // name is malformed for this registry (a wrong or missing suffix); fail loudly rather than - // mis-pricing the move as zero-fee. - string memory label = LabelUtils.stripTld(registry.tld(), fullName); - require(bytes(label).length != 0, InvalidLabel()); - - transferFee = - IPopRules(registry.get(DotnsConstants.POP_RULES)).transferFloor(label, from, to); - requiredFee = transferFee; - } - /// @inheritdoc UUPSUpgradeable function _authorizeUpgrade(address newImplementation) internal override onlyOwner {} } diff --git a/contracts/registrars/IDotnsRegistrar.sol b/contracts/registrars/IDotnsRegistrar.sol index 6c6779737..42e5ba8ed 100644 --- a/contracts/registrars/IDotnsRegistrar.sol +++ b/contracts/registrars/IDotnsRegistrar.sol @@ -9,7 +9,8 @@ import {IDotnsController} from "./IDotnsController.sol"; /// @notice ERC721-backed ownership for dotNS names with controller-gated registration. /// @dev Intentionally minimal and policy-free. Provides ERC721 ownership for registered name /// token IDs and controller-gated registration; pricing, PoP enforcement, and flow-specific -/// policy live in the controllers. +/// policy live in the controllers. The ERC721 transfer functions carry no fee and are not +/// payable; a soulbound name reverts on every one of them with @custom:reverts NameSoulbound. /// @custom:security-contact admin@parity.io interface IDotnsRegistrar is IERC721 { /// @notice Thrown when a name is already registered. @@ -21,16 +22,12 @@ interface IDotnsRegistrar is IERC721 { /// @notice Thrown when the protocol registry has no escrow address configured. error EscrowNotConfigured(); - /// @notice Thrown when a standard ERC721 transfer is attempted but the recipient - /// tier requires a non-zero transfer fee and the caller forwarded no `msg.value`. - error TransferFeeRequired(uint256 tokenId, address to, uint256 requiredFee); - /// @notice Thrown when @custom:function initialize is called with the zero address as /// the protocol registry. error ProtocolRegistryRequired(); - /// @notice Thrown when a mint, burn, or self-transfer carries `msg.value`. None of those - /// paths forward value onward, so attached value would be permanently trapped. + /// @notice Thrown when a mint carries `msg.value`. Nothing forwards value onward, so attached + /// value would be permanently trapped. error UnexpectedValue(); /// @notice Thrown when @custom:function register is called with the escrow address as @@ -42,10 +39,9 @@ interface IDotnsRegistrar is IERC721 { /// label. error InvalidLabel(); - /// @notice Thrown when a transfer or a transfer-fee quote targets a soulbound name. + /// @notice Thrown when a transfer targets a soulbound name. /// @dev Soulbound names are minted through the dotNS gateway pallet and are permanently - /// non-transferable. Raised by the `_update` transfer gate and by - /// @custom:function quoteTransferFee. + /// non-transferable. Raised by the `_update` transfer gate. error NameSoulbound(uint256 tokenId); /// @notice Thrown when a name is transferred into escrow custody by anything other than the @@ -107,7 +103,7 @@ interface IDotnsRegistrar is IERC721 { /// @notice Returns whether a token is soulbound (PoP-gateway minted and non-transferable). /// @dev Durable on-chain marker set once at mint by @custom:function register and never /// cleared. A `true` result means every transfer overload reverts with - /// @custom:reverts NameSoulbound and @custom:function quoteTransferFee reverts likewise. + /// @custom:reverts NameSoulbound. /// @param tokenId The name's token id. /// @return soulbound True when the name was minted through the gateway pallet. function isSoulbound(uint256 tokenId) external view returns (bool soulbound); @@ -143,59 +139,4 @@ interface IDotnsRegistrar is IERC721 { /// tokenId can resolve the original label in one view call without scanning registration /// events. Returns the empty string when the token does not exist. function labelOf(uint256 tokenId) external view returns (string memory label); - - /// @notice Quotes the additional native fee required to transfer a token to `to`. - /// @dev Returns the fee from @custom:function PopRules.transferFloor: zero for a name open to - /// all (base length nine or more), otherwise the name's own price as the maximum of (i) the - /// reach component charged when the recipient does not meet the label's required tier and - /// (ii) the downgrade component charged when the recipient tier is strictly below the sender - /// tier. Self-transfers and - /// escrow-touching transfers (release into escrow, reclaim out of escrow) return - /// zero. A token whose sender has no stored label also returns zero because there - /// is no label-derived price to charge against. Soulbound names are non-transferable - /// and have no transfer price, so a soulbound `tokenId` reverts with - /// @custom:reverts NameSoulbound rather than returning zero. Rejects a zero `to` with - /// @custom:reverts ERC721InvalidReceiver, an unminted `tokenId` with - /// @custom:reverts ERC721NonexistentToken via the underlying `ownerOf`, and requires - /// the protocol registry to have an escrow configured (otherwise - /// @custom:reverts EscrowNotConfigured). Returns zero when the protocol registry - /// has no `STORE_FACTORY` configured because no label-derived price is reachable. - function quoteTransferFee( - uint256 tokenId, - address to - ) - external - view - returns (uint256 requiredFee); - - /// @inheritdoc IERC721 - /// @dev The registrar's `_update` hook consults @custom:function PopRules.transferFloor - /// to compute the required transfer fee; if the caller does not forward at least that - /// amount as `msg.value`, the transfer reverts with @custom:reverts TransferFeeRequired. - /// A soulbound name is non-transferable and reverts with @custom:reverts NameSoulbound. - /// The `payable` modifier on every transfer overload exists so the fee can be forwarded - /// in the same call. - function safeTransferFrom( - address from, - address to, - uint256 tokenId, - bytes calldata data - ) - external - payable - override; - - /// @inheritdoc IERC721 - /// @dev Subject to the same fee-on-transfer gate as the four-argument overload; reverts with - /// @custom:reverts TransferFeeRequired when the recipient owes a non-zero transfer fee and - /// the caller has not forwarded it as `msg.value`, and with @custom:reverts NameSoulbound when - /// the token is soulbound. - function safeTransferFrom(address from, address to, uint256 tokenId) external payable override; - - /// @inheritdoc IERC721 - /// @dev Subject to the same fee-on-transfer gate as the safe overloads; reverts with - /// @custom:reverts TransferFeeRequired when the recipient owes a non-zero transfer fee and - /// the caller has not forwarded it as `msg.value`, and with @custom:reverts NameSoulbound when - /// the token is soulbound. - function transferFrom(address from, address to, uint256 tokenId) external payable override; } diff --git a/patches/openzeppelin-contracts-upgradeable.patch b/patches/openzeppelin-contracts-upgradeable.patch deleted file mode 100644 index 29f011ce4..000000000 --- a/patches/openzeppelin-contracts-upgradeable.patch +++ /dev/null @@ -1,28 +0,0 @@ -diff --git a/contracts/token/ERC721/ERC721Upgradeable.sol b/contracts/token/ERC721/ERC721Upgradeable.sol -index 17d24dbd..6e903be8 100644 ---- a/contracts/token/ERC721/ERC721Upgradeable.sol -+++ b/contracts/token/ERC721/ERC721Upgradeable.sol -@@ -135,7 +135,7 @@ abstract contract ERC721Upgradeable is Initializable, ContextUpgradeable, ERC165 - } - - /// @inheritdoc IERC721 -- function transferFrom(address from, address to, uint256 tokenId) public virtual { -+ function transferFrom(address from, address to, uint256 tokenId) public virtual payable { - if (to == address(0)) { - revert ERC721InvalidReceiver(address(0)); - } -@@ -148,12 +148,12 @@ abstract contract ERC721Upgradeable is Initializable, ContextUpgradeable, ERC165 - } - - /// @inheritdoc IERC721 -- function safeTransferFrom(address from, address to, uint256 tokenId) public { -+ function safeTransferFrom(address from, address to, uint256 tokenId) public virtual payable { - safeTransferFrom(from, to, tokenId, ""); - } - - /// @inheritdoc IERC721 -- function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public virtual { -+ function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public virtual payable { - transferFrom(from, to, tokenId); - ERC721Utils.checkOnERC721Received(_msgSender(), from, to, tokenId, data); - } diff --git a/patches/openzeppelin-contracts.patch b/patches/openzeppelin-contracts.patch deleted file mode 100644 index c7943bd91..000000000 --- a/patches/openzeppelin-contracts.patch +++ /dev/null @@ -1,59 +0,0 @@ -diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol -index f4783f1e..628bde1e 100644 ---- a/contracts/token/ERC721/ERC721.sol -+++ b/contracts/token/ERC721/ERC721.sol -@@ -112,7 +112,7 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er - } - - /// @inheritdoc IERC721 -- function transferFrom(address from, address to, uint256 tokenId) public virtual { -+ function transferFrom(address from, address to, uint256 tokenId) public virtual payable { - if (to == address(0)) { - revert ERC721InvalidReceiver(address(0)); - } -@@ -125,12 +125,12 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er - } - - /// @inheritdoc IERC721 -- function safeTransferFrom(address from, address to, uint256 tokenId) public { -+ function safeTransferFrom(address from, address to, uint256 tokenId) public virtual payable { - safeTransferFrom(from, to, tokenId, ""); - } - - /// @inheritdoc IERC721 -- function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public virtual { -+ function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public virtual payable { - transferFrom(from, to, tokenId); - ERC721Utils.checkOnERC721Received(_msgSender(), from, to, tokenId, data); - } -diff --git a/contracts/token/ERC721/IERC721.sol b/contracts/token/ERC721/IERC721.sol -index 74982034..1cdfaad9 100644 ---- a/contracts/token/ERC721/IERC721.sol -+++ b/contracts/token/ERC721/IERC721.sol -@@ -52,7 +52,7 @@ interface IERC721 is IERC165 { - * - * Emits a {Transfer} event. - */ -- function safeTransferFrom(address from, address to, uint256 tokenId, bytes calldata data) external; -+ function safeTransferFrom(address from, address to, uint256 tokenId, bytes calldata data) external payable; - - /** - * @dev Safely transfers `tokenId` token from `from` to `to`, checking first that contract recipients -@@ -70,7 +70,7 @@ interface IERC721 is IERC165 { - * - * Emits a {Transfer} event. - */ -- function safeTransferFrom(address from, address to, uint256 tokenId) external; -+ function safeTransferFrom(address from, address to, uint256 tokenId) external payable; - - /** - * @dev Transfers `tokenId` token from `from` to `to`. -@@ -88,7 +88,7 @@ interface IERC721 is IERC165 { - * - * Emits a {Transfer} event. - */ -- function transferFrom(address from, address to, uint256 tokenId) external; -+ function transferFrom(address from, address to, uint256 tokenId) external payable; - - /** - * @dev Gives permission to `to` to transfer `tokenId` token to another account. diff --git a/scripts/shell/apply-oz-patches.sh b/scripts/shell/apply-oz-patches.sh deleted file mode 100755 index bc65ef646..000000000 --- a/scripts/shell/apply-oz-patches.sh +++ /dev/null @@ -1,54 +0,0 @@ -#!/bin/bash -# -# Applies the project's dependency patches under `patches/` to the matching -# `lib//` submodule trees. Idempotent: a patch already in place is detected -# via `git apply --reverse --check` and skipped. -# -# Each patch must be named `lib/.patch` form: e.g. -# `patches/openzeppelin-contracts.patch` applies inside `lib/openzeppelin-contracts/`. -# -# Why this exists: pallet-revive's transferable-name flow needs `payable -# transferFrom`/`safeTransferFrom` on ERC721, which upstream OZ does not ship. -# Vendoring a fork would drift; instead we keep vanilla submodules pinned in -# `setup.bash`'s SUBMODULES list and patch them at install / CI time. -set -euo pipefail - -repo_root="$(cd "$(dirname "$0")/../.." && pwd)" - -if [ ! -d "$repo_root/patches" ]; then - echo " ⊘ No patches/ directory; nothing to apply" - exit 0 -fi - -shopt -s nullglob -patches=("$repo_root"/patches/*.patch) -shopt -u nullglob - -if [ ${#patches[@]} -eq 0 ]; then - echo " ⊘ patches/ is empty; nothing to apply" - exit 0 -fi - -for patch in "${patches[@]}"; do - name="$(basename "$patch" .patch)" - target="$repo_root/lib/$name" - - if [ ! -d "$target" ]; then - echo " ✗ Skipping $(basename "$patch"): target $target does not exist" - continue - fi - - ( - cd "$target" || exit 1 - if git apply --check --reverse "$patch" >/dev/null 2>&1; then - echo " ⊘ $name already patched" - exit 0 - fi - git apply --check "$patch" 2>/dev/null - git apply "$patch" - echo " ✓ $name patched ($(basename "$patch"))" - ) || { - echo " ✗ Failed to apply $(basename "$patch") to $target" - exit 1 - } -done diff --git a/setup.bash b/setup.bash index 7b11d0a24..8505a99e2 100755 --- a/setup.bash +++ b/setup.bash @@ -88,11 +88,6 @@ init_submodules(){ done } -apply_dependency_patches(){ - echo "Applying dependency patches..." - bash scripts/shell/apply-oz-patches.sh -} - check_missing_files(){ echo "Checking for missing source files..." @@ -114,7 +109,6 @@ install_git_hooks(){ } init_submodules -apply_dependency_patches check_missing_files install_git_hooks echo "Setup complete!" diff --git a/test/fuzz/registrar/DotnsRegistrarControllerFuzz.t.sol b/test/fuzz/registrar/DotnsRegistrarControllerFuzz.t.sol index 227bf4c52..79a380041 100644 --- a/test/fuzz/registrar/DotnsRegistrarControllerFuzz.t.sol +++ b/test/fuzz/registrar/DotnsRegistrarControllerFuzz.t.sol @@ -164,9 +164,8 @@ contract DotnsRegistrarControllerFuzzTest is BaseDotns { bytes32 node = _namehash(dotNode, labelhash); uint256 tokenId = uint256(node); - uint256 _xferFee = dotnsRegistrar.quoteTransferFee(tokenId, recipient); vm.prank(sender); - dotnsRegistrar.transferFrom{value: _xferFee}(sender, recipient, tokenId); + dotnsRegistrar.transferFrom(sender, recipient, tokenId); assertEq(dotnsRegistrar.ownerOf(tokenId), recipient); @@ -261,10 +260,8 @@ contract DotnsRegistrarControllerFuzzTest is BaseDotns { uint256 depositorRefundsBefore = dotnsNameEscrow.pendingRefundCount(depositor); uint256 recipientRefundsBefore = dotnsNameEscrow.pendingRefundCount(recipientSeed); - uint256 transferFee = dotnsRegistrar.quoteTransferFee(tokenId, recipientSeed); - vm.prank(depositor); - dotnsRegistrar.transferFrom{value: transferFee}(depositor, recipientSeed, tokenId); + dotnsRegistrar.transferFrom(depositor, recipientSeed, tokenId); assertEq(dotnsRegistrar.ownerOf(tokenId), recipientSeed, "NFT must move to the recipient"); @@ -314,9 +311,8 @@ contract DotnsRegistrarControllerFuzzTest is BaseDotns { assertEq(dotnsReverseResolver.nameOf(sender), string.concat(nameLabel, ".dot")); - uint256 _xferFee = dotnsRegistrar.quoteTransferFee(tokenId, recipient); vm.prank(sender); - dotnsRegistrar.transferFrom{value: _xferFee}(sender, recipient, tokenId); + dotnsRegistrar.transferFrom(sender, recipient, tokenId); assertEq(dotnsReverseResolver.nameOf(sender), ""); } diff --git a/test/intergration/BasicDotns.t.sol b/test/intergration/BasicDotns.t.sol index 31fefc7e3..76d490bfa 100644 --- a/test/intergration/BasicDotns.t.sol +++ b/test/intergration/BasicDotns.t.sol @@ -151,9 +151,8 @@ contract BasicDotnsIntegration is BaseDotns { vm.stopPrank(); assertEq(dotnsContentResolver.contenthash(otherSubnode), CID_B); - uint256 _xferFee = dotnsRegistrar.quoteTransferFee(tokenId, flow.transferTo); vm.startPrank(flow.nameOwner); - dotnsRegistrar.transferFrom{value: _xferFee}(flow.nameOwner, flow.transferTo, tokenId); + dotnsRegistrar.transferFrom(flow.nameOwner, flow.transferTo, tokenId); vm.stopPrank(); assertEq(dotnsRegistrar.ownerOf(tokenId), flow.transferTo); diff --git a/test/intergration/NoStatusDepositLifecycle.t.sol b/test/intergration/NoStatusDepositLifecycle.t.sol index d80397d19..0cd2cab47 100644 --- a/test/intergration/NoStatusDepositLifecycle.t.sol +++ b/test/intergration/NoStatusDepositLifecycle.t.sol @@ -43,14 +43,11 @@ contract NoStatusDepositLifecycle is BaseDotns { "no refund must exist before the transfer" ); - // Transfer the NFT away from the depositor. Same-tier NoStatus floor is - // zero, so msg.value is zero; the escrow is still consulted so the - // position rebinds to the new holder. - uint256 transferFee = dotnsRegistrar.quoteTransferFee(tokenId, recipient); - assertEq(transferFee, 0, "NoStatus to NoStatus transfer floor is zero"); + // Transfer the NFT away from the depositor. Transfers are free; the escrow is + // consulted so the position rebinds to the new holder. vm.prank(depositor); - dotnsRegistrar.transferFrom{value: 0}(depositor, recipient, tokenId); + dotnsRegistrar.transferFrom(depositor, recipient, tokenId); IDotnsNameEscrow.ReleasePosition memory afterTransfer = dotnsNameEscrow.getReleasePosition(tokenId); diff --git a/test/intergration/PopLifecycleFlow.t.sol b/test/intergration/PopLifecycleFlow.t.sol index ec436751f..475580971 100644 --- a/test/intergration/PopLifecycleFlow.t.sol +++ b/test/intergration/PopLifecycleFlow.t.sol @@ -72,13 +72,7 @@ contract PopLifecycleFlow is BaseDotns { bytes32 subnode = _setSubnode(ed, personhoodNode, SUB_LABEL, PERSONHOOD_LABEL_A, leonardo); assertEq(dotnsRegistry.owner(subnode), leonardo); - // It is soulbound: quoting a transfer and attempting one both revert, and ownership - // does not move. - vm.expectRevert( - abi.encodeWithSelector(IDotnsRegistrar.NameSoulbound.selector, personhoodTokenId) - ); - dotnsRegistrar.quoteTransferFee(personhoodTokenId, tiago); - + // It is soulbound: a transfer reverts, and ownership does not move. vm.expectRevert( abi.encodeWithSelector(IDotnsRegistrar.NameSoulbound.selector, personhoodTokenId) ); diff --git a/test/intergration/StoreIntegration.t.sol b/test/intergration/StoreIntegration.t.sol index ece1f0d33..6344ed0d1 100644 --- a/test/intergration/StoreIntegration.t.sol +++ b/test/intergration/StoreIntegration.t.sol @@ -85,9 +85,8 @@ contract StoreIntegrationTest is BaseDotns { bytes32 node = _register(label, ed, IPopRules.PopStatus.NoStatus); uint256 tokenId = uint256(node); - uint256 outboundFee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); vm.prank(ed); - dotnsRegistrar.transferFrom{value: outboundFee}(ed, leonardo, tokenId); + dotnsRegistrar.transferFrom(ed, leonardo, tokenId); address recipientStore = storeFactory.getLabelStore(leonardo); assertTrue(recipientStore != address(0)); @@ -101,12 +100,10 @@ contract StoreIntegrationTest is BaseDotns { bytes32 node = _register(label, ed, IPopRules.PopStatus.NoStatus); uint256 tokenId = uint256(node); - uint256 outboundFee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); vm.prank(ed); - dotnsRegistrar.transferFrom{value: outboundFee}(ed, leonardo, tokenId); - uint256 returnFee = dotnsRegistrar.quoteTransferFee(tokenId, ed); + dotnsRegistrar.transferFrom(ed, leonardo, tokenId); vm.prank(leonardo); - dotnsRegistrar.transferFrom{value: returnFee}(leonardo, ed, tokenId); + dotnsRegistrar.transferFrom(leonardo, ed, tokenId); address edStore = storeFactory.getLabelStore(ed); address leonardoStore = storeFactory.getLabelStore(leonardo); diff --git a/test/invariant/escrow/DotnsNameEscrowInvariant.t.sol b/test/invariant/escrow/DotnsNameEscrowInvariant.t.sol index cbeb04096..4456f90fd 100644 --- a/test/invariant/escrow/DotnsNameEscrowInvariant.t.sol +++ b/test/invariant/escrow/DotnsNameEscrowInvariant.t.sol @@ -29,7 +29,7 @@ contract DotnsNameEscrowInvariantTest is BaseDotns { targetContract(address(handler)); - bytes4[] memory selectors = new bytes4[](12); + bytes4[] memory selectors = new bytes4[](11); selectors[0] = handler.commitRegisterAndDeposit.selector; selectors[1] = handler.registerCrossTier.selector; selectors[2] = handler.releaseToken.selector; @@ -38,12 +38,11 @@ contract DotnsNameEscrowInvariantTest is BaseDotns { selectors[5] = handler.setRandomPopStatus.selector; selectors[6] = handler.reRegisterReclaimed.selector; selectors[7] = handler.transferDeposited.selector; - selectors[8] = handler.transferPayable.selector; - selectors[9] = handler.advanceTime.selector; + selectors[8] = handler.advanceTime.selector; // The two halves of the redeem window. Without both, the fuzzer can only reach reclaim by // way of a withdrawal, which is precisely the assumption the reclaim deadlock rested on. - selectors[10] = handler.redeemReleased.selector; - selectors[11] = handler.reRegisterReleased.selector; + selectors[9] = handler.redeemReleased.selector; + selectors[10] = handler.reRegisterReleased.selector; targetSelector(FuzzSelector({addr: address(handler), selectors: selectors})); excludeContract(address(dotnsRegistrarController)); @@ -57,8 +56,7 @@ contract DotnsNameEscrowInvariantTest is BaseDotns { /// @notice Escrow native balance must always cover the full liability set: tracked /// reserves, the protocol fees, unclaimed pull-payment balances, and the time-locked - /// refund-ledger entries credited by the refund-on-leave path. Under the deposit-binds- - /// to-depositor model these four flows are economically distinct; solvency is only + /// refund-ledger entries. These four flows are economically distinct; solvency is only /// meaningful against their sum. function invariant_solvency() public view { uint256 escrowBalance = address(dotnsNameEscrow).balance; @@ -74,15 +72,10 @@ contract DotnsNameEscrowInvariantTest is BaseDotns { ); } - /// @notice The handler's mirror of protocol-fee inflows must equal the escrow's on-chain - /// protocol-fee balance. Protocol fees only ever accrue, so every inflow the handler - /// tracks (payable transfers) must sum to exactly the balance. - function invariant_protocol_fees_match_tracked_inflows() public view { - assertEq( - handler.ghost_protocolFeesPaidIn(), - dotnsNameEscrow.protocolFees(), - "Tracked protocol-fee inflows must equal on-chain protocol fees" - ); + /// @notice Nothing credits the protocol fee balance: registrations lock deposits and + /// transfers are free, so it stays at zero whatever the handler does. + function invariant_protocol_fees_stay_zero() public view { + assertEq(dotnsNameEscrow.protocolFees(), 0, "Nothing may credit protocol fees"); } /// @notice Sum of all active (deposited but not withdrawn) position amounts must equal diff --git a/test/invariant/escrow/EscrowHandler.t.sol b/test/invariant/escrow/EscrowHandler.t.sol index f8a5577ab..3dc425988 100644 --- a/test/invariant/escrow/EscrowHandler.t.sol +++ b/test/invariant/escrow/EscrowHandler.t.sol @@ -51,12 +51,6 @@ contract EscrowHandler is Test { /// @notice Label used to register each tokenId; required for re-registration after finalise. mapping(uint256 tokenId => string label) public labelByTokenId; - /// @notice Cumulative native amount credited into protocol fees by handler-driven flows. - /// @dev Increments via payable `transferFrom` (`chargeTransferFee`). Protocol fees only - /// ever accrue, so a conservation invariant - /// asserts this equals `escrow.protocolFees()`. - uint256 public ghost_protocolFeesPaidIn; - /// @notice Cumulative native amount credited to recipients via `withdraw()`. uint256 public ghost_pendingCredits; @@ -516,10 +510,9 @@ contract EscrowHandler is Test { } } - /// @notice Transfers a deposited token to a different actor without paying a cross-tier fee. - /// @dev Plain transferFrom path; no fee branch is hit because escrow custody is not - /// involved and tier prices may match. Used to exercise the position-rebind path that - /// moves `position.recipient` to the new holder on every NFT transfer. + /// @notice Transfers a deposited token to a different actor. + /// @dev Plain transferFrom path, free whatever the tiers. Used to exercise the position-rebind + /// path that moves `position.recipient` to the new holder on every NFT transfer. /// @param tokenSeed Seed for selecting which deposited token to transfer. /// @param actorSeed Seed for selecting the recipient. function transferDeposited(uint256 tokenSeed, uint256 actorSeed) external { @@ -528,80 +521,17 @@ contract EscrowHandler is Test { uint256 index = tokenSeed % _depositedTokenIds.length; uint256 tokenId = _depositedTokenIds[index]; - // Zero-value transfer path: skip whenever the registrar's quote returns a - // non-zero fee. The quote folds in both the price-delta path and the - // transfer-fee path, so the handler stays in sync with whatever fee - // branches the contract grows over time. - string memory label = labelByTokenId[tokenId]; - // Read once for documentation continuity; the actual gating check uses the quote. - label; address currentOwner = registrar.ownerOf(tokenId); address recipient = _pickDifferentActor(currentOwner, actorSeed); if (recipient == address(0)) return; - if (registrar.quoteTransferFee(tokenId, recipient) != 0) return; - vm.prank(currentOwner); try registrar.transferFrom(currentOwner, recipient, tokenId) { // Sync the amount as a safety net against future downgrade paths that // might shrink it. Under the deposit-follows-name design the position // rebinds to the new holder on every transfer; the locked amount itself - // travels with the NFT, so on a zero-fee same-tier hop we expect both - // the amount and the per-asset reserves to stay put. - IDotnsNameEscrow.ReleasePosition memory pos = escrow.getReleasePosition(tokenId); - depositAmounts[tokenId] = pos.amount; - } catch { - return; - } - } - - /// @notice Transfers a deposited token using payable `transferFrom`, exercising the - /// cross-tier fee path through `chargeTransferFee`. - /// @dev Funds the call with whatever the registrar's `quoteTransferFee` returns, so the - /// handler stays drift-resistant against future fee additions. Revert-safe: returns - /// early when the recipient's price is zero (no fee path needed) or the inner call - /// reverts. - /// @param tokenIdSeed Seed for selecting which deposited token to transfer. - /// @param fromSeed Unused (current owner is derived on-chain). Retained for fuzzer entropy. - /// @param toSeed Seed for selecting the recipient. - function transferPayable(uint256 tokenIdSeed, uint256 fromSeed, uint256 toSeed) external { - // `fromSeed` injects fuzzer entropy without overriding the registrar's required - // ownership check. Read it once so the parameter is not unused. - fromSeed; - - if (_depositedTokenIds.length == 0 || actors.length < 2) return; - - uint256 index = tokenIdSeed % _depositedTokenIds.length; - uint256 tokenId = _depositedTokenIds[index]; - string memory label = labelByTokenId[tokenId]; - if (bytes(label).length == 0) return; - - address currentOwner = registrar.ownerOf(tokenId); - address to = _pickDifferentActor(currentOwner, toSeed); - if (to == address(0)) return; - - // Use the registrar's own quote so the value attached matches whatever the - // contract actually requires. This includes the transfer-fee branch: when - // the recipient's verification level is below the label's required tier, - // the registrar charges the flat NoStatus deposit even though the - // price-delta path returns zero. Using `quoteTransferFee` makes the handler - // drift-resistant against future fee additions. - uint256 requiredFee = registrar.quoteTransferFee(tokenId, to); - if (requiredFee == 0) return; - - vm.prank(currentOwner); - try registrar.transferFrom{value: requiredFee}(currentOwner, to, tokenId) { - // The transfer fee is credited in full to protocol fees: the position rebinds - // to the new holder rather than refunding, so reserves stay put. Accumulate the - // independently-quoted `requiredFee` so the conservation invariant verifies the - // escrow credited exactly the quoted fee. - ghost_protocolFeesPaidIn += requiredFee; - - // Sync the amount as a safety net against future downgrade paths. - // Under the deposit-follows-name design the leaving-recipient branch - // rebinds the position to the new holder rather than refunding, so - // the locked amount and per-asset reserves stay put while the - // recipient pointer moves. + // travels with the NFT, so we expect both the amount and the per-asset + // reserves to stay put. IDotnsNameEscrow.ReleasePosition memory pos = escrow.getReleasePosition(tokenId); depositAmounts[tokenId] = pos.amount; } catch { @@ -654,10 +584,8 @@ contract EscrowHandler is Test { /// @notice Returns the sum of outstanding time-locked refund entries across all actors. /// @dev Used by the full-solvency invariant to capture overpayment refunds that - /// @custom:function chargeTransferFee credits via @custom:function _creditRefund when the - /// attached value exceeds the transfer fee. Under the deposit-follows-name model the - /// deposit itself never lands on this ledger; only payer overpayments do. Iterates each - /// actor's entry list and sums each entry's amount. + /// earlier implementations credited. Nothing credits new entries, so it stays zero here. + /// Iterates each actor's entry list and sums each entry's amount. /// @return total Aggregate refund-ledger liability owed to the actor set. function totalPendingRefundEntries() external view returns (uint256 total) { uint256 length = actors.length; diff --git a/test/invariant/registrar/DotnsRegistrarControllerInvariant.t.sol b/test/invariant/registrar/DotnsRegistrarControllerInvariant.t.sol index 45467623d..a4ac57b8c 100644 --- a/test/invariant/registrar/DotnsRegistrarControllerInvariant.t.sol +++ b/test/invariant/registrar/DotnsRegistrarControllerInvariant.t.sol @@ -103,7 +103,7 @@ contract DotnsRegistrarControllerInvariantTest is BaseDotns { /// @notice Every unit of native value the escrow holds is accounted for by exactly one ledger. /// @dev The escrow's balance is the sum of three obligations: `tokenReserved`, the deposits - /// held against live positions; `protocolFees`, the accumulated cross-tier fees; and + /// held against live positions; `protocolFees`, which nothing credits; and /// `_pendingWithdrawals`, the amounts credited to recipients and awaiting collection. /// Value only ever moves between these three, never into or out of the set, so their total /// tracks the balance exactly. diff --git a/test/invariant/registrar/PopControllerHandler.t.sol b/test/invariant/registrar/PopControllerHandler.t.sol index 8ebc2c613..1b70985c2 100644 --- a/test/invariant/registrar/PopControllerHandler.t.sol +++ b/test/invariant/registrar/PopControllerHandler.t.sol @@ -468,17 +468,8 @@ contract PopControllerHandler is Test { if (to == from) return; ++transferAttemptCount; - uint256 fee; - try REGISTRAR.quoteTransferFee(tokenId, to) returns (uint256 quoted) { - fee = quoted; - } catch { - // Soulbound names have no transfer price. Send nothing and let the transfer revert. - fee = 0; - } - - vm.deal(from, fee); vm.prank(from); - try REGISTRAR.transferFrom{value: fee}(from, to, tokenId) { + try REGISTRAR.transferFrom(from, to, tokenId) { ++transferSuccessCount; } catch {} } diff --git a/test/unit/escrow/DotnsNameEscrow.t.sol b/test/unit/escrow/DotnsNameEscrow.t.sol index 9f76b23da..7bfcea83a 100644 --- a/test/unit/escrow/DotnsNameEscrow.t.sol +++ b/test/unit/escrow/DotnsNameEscrow.t.sol @@ -7,6 +7,7 @@ import {BaseDotns, IDotnsRegistrarController} from "../../base/BaseDotns.t.sol"; import {IDotnsNameEscrow} from "../../../contracts/escrow/IDotnsNameEscrow.sol"; import {IDotnsRegistrar} from "../../../contracts/registrars/IDotnsRegistrar.sol"; import {IPopRules} from "../../../contracts/pop/IPopRules.sol"; +import {stdStorage, StdStorage} from "forge-std/Test.sol"; /// @title ForceSender /// @notice Self-destructing helper that force-credits its constructor balance to `target`, @@ -29,10 +30,16 @@ contract GhostNft is ERC721 { } } +/// @title ValueRejecter +/// @notice Recipient with no `receive` or `fallback`, so any value sent to it reverts. +contract ValueRejecter {} + /// @title DotnsNameEscrowTest /// @notice Unit tests covering the deposit, release, withdrawal and reclaim lifecycle on /// @custom:contract DotnsNameEscrow, plus the pull-payment and solvency guarantees. contract DotnsNameEscrowTest is BaseDotns { + using stdStorage for StdStorage; + /// @notice Default label used across most tests. /// @dev Eleven characters measured as written, which classifies as NoStatus; the flat deposit /// equals BASE_DEPOSIT. @@ -367,9 +374,6 @@ contract DotnsNameEscrowTest is BaseDotns { assertEq(before.recipient, ed); assertEq(before.amount, BASE_DEPOSIT); - uint256 quotedFee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); - assertEq(quotedFee, 0, "same-tier NoStatus transfer should be free"); - uint256 reservesBefore = dotnsNameEscrow.reserves(address(0)); uint256 edRefundsBefore = dotnsNameEscrow.pendingRefundCount(ed); uint256 leonardoRefundsBefore = dotnsNameEscrow.pendingRefundCount(leonardo); @@ -419,9 +423,6 @@ contract DotnsNameEscrowTest is BaseDotns { assertEq(before.recipient, ed, "funded position starts with registrant"); assertEq(before.amount, BASE_DEPOSIT, "self-registration locks a refundable deposit"); - uint256 quotedFee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); - assertEq(quotedFee, 0, "same-tier transfer costs nothing"); - uint256 edRefundsBefore = dotnsNameEscrow.pendingRefundCount(ed); uint256 leonardoRefundsBefore = dotnsNameEscrow.pendingRefundCount(leonardo); @@ -453,7 +454,9 @@ contract DotnsNameEscrowTest is BaseDotns { assertEq(afterRelease.recipient, leonardo, "release recipient is the current holder"); } - function test_downgrade_transfer_pays_name_price() public { + function test_band_gated_downgrade_transfer_is_free() public { + // A personhood holder moving a name of the personhood band to a device holder, below the + // band, pays nothing and the deposit follows the name. string memory label = "lights01"; _grantPersonhood(ed); @@ -462,22 +465,14 @@ contract DotnsNameEscrowTest is BaseDotns { _commitAndRegister(label, ed, false); uint256 tokenId = _tokenIdForLabel(label); - uint256 ownPrice = popRules.price(label); - uint256 quotedFee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); - - assertEq(quotedFee, ownPrice, "downgrade re-prices at the name's own length"); - uint256 priorProtocolFees = dotnsNameEscrow.protocolFees(); - vm.deal(ed, quotedFee); vm.prank(ed); - dotnsRegistrar.transferFrom{value: quotedFee}(ed, leonardo, tokenId); + dotnsRegistrar.transferFrom(ed, leonardo, tokenId); - assertEq( - dotnsNameEscrow.protocolFees() - priorProtocolFees, - ownPrice, - "downgrade fee settles to protocol fees" - ); + assertEq(dotnsRegistrar.ownerOf(tokenId), leonardo); + assertEq(dotnsNameEscrow.getReleasePosition(tokenId).recipient, leonardo); + assertEq(dotnsNameEscrow.protocolFees(), priorProtocolFees, "no fee reaches the pot"); } function test_downgrade_transfer_is_free_and_rebinds_position() public { @@ -488,8 +483,6 @@ contract DotnsNameEscrowTest is BaseDotns { _grantPersonhood(ed); - assertEq(dotnsRegistrar.quoteTransferFee(tokenId, leonardo), 0, "an open name moves free"); - uint256 reservesBefore = dotnsNameEscrow.reserves(address(0)); uint256 escrowBalanceBefore = address(dotnsNameEscrow).balance; uint256 edRefundsBefore = dotnsNameEscrow.pendingRefundCount(ed); @@ -525,12 +518,11 @@ contract DotnsNameEscrowTest is BaseDotns { } function test_transfer_no_rebind_when_to_equals_position_recipient() public { - // Defensive: if `chargeTransferFee` is ever invoked with `to == position.recipient` - // (only reachable through a future code path or a direct registrar-pranked call), the - // rebind branch must not fire. The position stays funded with the same recipient, no - // refund entry is credited, and the fee leg behaves exactly as for any other payable - // transfer. Drives the call from the registrar's address so the `onlyRegistrar` guard - // passes without trying to perform an NFT move. + // Defensive: if `rebindPosition` is ever invoked with `to == position.recipient` (only + // reachable through a future code path or a direct registrar-pranked call), the rebind + // branch must not fire. The position stays funded with the same recipient and no refund + // entry is credited. Drives the call from the registrar's address so the `onlyRegistrar` + // guard passes without trying to perform an NFT move. uint256 tokenId = _registerNoStatus(LABEL, ed); IDotnsNameEscrow.ReleasePosition memory before = dotnsNameEscrow.getReleasePosition(tokenId); @@ -538,17 +530,10 @@ contract DotnsNameEscrowTest is BaseDotns { assertEq(before.amount, BASE_DEPOSIT, "precondition: deposit at BASE_DEPOSIT"); uint256 reservesBefore = dotnsNameEscrow.reserves(address(0)); - uint256 protocolFeesBefore = dotnsNameEscrow.protocolFees(); uint256 refundCountBefore = dotnsNameEscrow.pendingRefundCount(ed); - uint256 fee = popRules.price(LABEL); - vm.deal(address(dotnsRegistrar), fee); vm.prank(address(dotnsRegistrar)); - dotnsNameEscrow.chargeTransferFee{value: fee}( - IDotnsNameEscrow.ChargeTransferFeeParams({ - tokenId: tokenId, transferFee: fee, payer: leonardo, to: ed - }) - ); + dotnsNameEscrow.rebindPosition(tokenId, ed); IDotnsNameEscrow.ReleasePosition memory pos = dotnsNameEscrow.getReleasePosition(tokenId); assertEq(pos.recipient, ed, "position recipient untouched when to == recipient"); @@ -559,11 +544,6 @@ contract DotnsNameEscrowTest is BaseDotns { reservesBefore, "reserves unchanged when no refund fires" ); - assertEq( - dotnsNameEscrow.protocolFees() - protocolFeesBefore, - fee, - "fee leg still settles to protocol fees" - ); assertEq( dotnsNameEscrow.pendingRefundCount(ed), refundCountBefore, @@ -667,9 +647,7 @@ contract DotnsNameEscrowTest is BaseDotns { assertEq(midPos.recipient, leonardo, "position rebinds to leonardo on the first hop"); assertEq(midPos.amount, BASE_DEPOSIT, "deposit amount untouched on the first hop"); - // Same-tier free transfer; quote must agree this is a zero-fee move that still rebinds. - uint256 quotedFee = dotnsRegistrar.quoteTransferFee(tokenId, tiago); - assertEq(quotedFee, 0, "follow-up same-tier transfer must be free"); + // Same-tier transfer; it still rebinds. vm.prank(leonardo); dotnsRegistrar.transferFrom(leonardo, tiago, tokenId); @@ -850,4 +828,83 @@ contract DotnsNameEscrowTest is BaseDotns { "withdraw should still transfer correct amount" ); } + + // withdrawProtocolFees: governance spends the protocol fee balance, and nothing else + + /// @notice Credits `amount` to the protocol fee balance and backs it with native value, as + /// the transfer fees of earlier implementations left it. + function _seedProtocolFees(uint256 amount) internal { + stdstore.target(address(dotnsNameEscrow)).sig("protocolFees()").checked_write(amount); + vm.deal(address(dotnsNameEscrow), address(dotnsNameEscrow).balance + amount); + } + + function test_withdrawProtocolFees_moves_fees_to_recipient() public { + _seedProtocolFees(5 ether); + address treasury = makeAddr("treasury"); + + _mockOriginIsRoot(true); + vm.expectEmit(true, false, false, true, address(dotnsNameEscrow)); + emit IDotnsNameEscrow.ProtocolFeesWithdrawn(treasury, 3 ether); + dotnsNameEscrow.withdrawProtocolFees(treasury, 3 ether); + _mockOriginIsRoot(false); + + assertEq(treasury.balance, 3 ether, "recipient receives the amount"); + assertEq(dotnsNameEscrow.protocolFees(), 2 ether, "balance is debited by the amount"); + } + + function test_withdrawProtocolFees_reverts_without_root() public { + _seedProtocolFees(1 ether); + + vm.expectRevert(IDotnsNameEscrow.NotRoot.selector); + vm.prank(owner); + dotnsNameEscrow.withdrawProtocolFees(owner, 1 ether); + } + + function test_withdrawProtocolFees_cannot_reach_deposits() public { + // A live deposit backs the escrow balance, but only the protocol fee balance is spendable. + uint256 tokenId = _registerNoStatus(LABEL, ed); + _seedProtocolFees(1 ether); + uint256 reservesBefore = dotnsNameEscrow.reserves(address(0)); + + _mockOriginIsRoot(true); + vm.expectRevert( + abi.encodeWithSelector( + IDotnsNameEscrow.ProtocolFeesExceeded.selector, 1 ether + 1, 1 ether + ) + ); + dotnsNameEscrow.withdrawProtocolFees(owner, 1 ether + 1); + _mockOriginIsRoot(false); + + assertEq(dotnsNameEscrow.reserves(address(0)), reservesBefore); + assertEq(dotnsNameEscrow.getReleasePosition(tokenId).amount, BASE_DEPOSIT); + } + + function test_withdrawProtocolFees_rejects_zero_recipient_and_amount() public { + _seedProtocolFees(1 ether); + _mockOriginIsRoot(true); + + vm.expectRevert(IDotnsNameEscrow.InvalidRecipient.selector); + dotnsNameEscrow.withdrawProtocolFees(address(0), 1 ether); + + vm.expectRevert(IDotnsNameEscrow.InvalidAmount.selector); + dotnsNameEscrow.withdrawProtocolFees(owner, 0); + + _mockOriginIsRoot(false); + } + + function test_withdrawProtocolFees_reverts_when_recipient_rejects_value() public { + _seedProtocolFees(1 ether); + address rejecter = address(new ValueRejecter()); + + _mockOriginIsRoot(true); + vm.expectRevert( + abi.encodeWithSelector( + IDotnsNameEscrow.ProtocolFeeTransferFailed.selector, rejecter, 1 ether + ) + ); + dotnsNameEscrow.withdrawProtocolFees(rejecter, 1 ether); + _mockOriginIsRoot(false); + + assertEq(dotnsNameEscrow.protocolFees(), 1 ether, "a failed payout debits nothing"); + } } diff --git a/test/unit/escrow/DotnsNameEscrowRefunds.t.sol b/test/unit/escrow/DotnsNameEscrowRefunds.t.sol index 03d4963ef..1253cbdd3 100644 --- a/test/unit/escrow/DotnsNameEscrowRefunds.t.sol +++ b/test/unit/escrow/DotnsNameEscrowRefunds.t.sol @@ -8,20 +8,31 @@ import {IDotnsProtocolRegistry} from "../../../contracts/registry/IDotnsProtocol import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; /// @title DotnsNameEscrowRefundHarness -/// @notice Test-only subclass exposing internal refund-ledger primitives so the unit suite can -/// drive `_creditRefund` and `_removeRefundEntry` directly without standing up the full -/// transfer-fee pipeline. +/// @notice Test-only subclass that seeds refund entries straight into storage and exposes +/// `_removeRefundEntry`. Nothing in the escrow credits entries any more, so seeding is the +/// only way to exercise the claim and view functions that serve entries already on the +/// ledger. contract DotnsNameEscrowRefundHarness is DotnsNameEscrow { - function exposeCreditRefund( + uint256 private _seededEntries; + + function seedRefundEntry( address recipient, uint256 amount, uint256 tokenId ) external - payable returns (uint256 entryId) { - entryId = _creditRefund(recipient, amount, tokenId); + entryId = ++_seededEntries; + _refundEntries[entryId] = RefundEntry({ + recipient: recipient, + amount: amount, + availableAt: uint64(block.timestamp + cooldown), + tokenId: tokenId + }); + uint256[] storage list = _entriesByRecipient[recipient]; + list.push(entryId); + _entryIndexPlusOne[entryId] = list.length; } function exposeRemoveRefundEntry(uint256 entryId, address recipient) external { @@ -48,7 +59,8 @@ contract RejectingReceiver { } /// @title DotnsNameEscrowRefundsTest -/// @notice Unit tests for the time-locked refund ledger added to @custom:contract DotnsNameEscrow. +/// @notice Unit tests for claiming entries on the time-locked refund ledger of +/// @custom:contract DotnsNameEscrow. contract DotnsNameEscrowRefundsTest is BaseDotns { DotnsNameEscrowRefundHarness internal harness; @@ -82,15 +94,8 @@ contract DotnsNameEscrowRefundsTest is BaseDotns { otherRecipient = makeAddr("otherRefundRecipient"); } - function _credit( - address to, - uint256 amount, - uint256 tokenId - ) - internal - returns (uint256 entryId) - { - entryId = harness.exposeCreditRefund(to, amount, tokenId); + function _seed(address to, uint256 amount, uint256 tokenId) internal returns (uint256 entryId) { + entryId = harness.seedRefundEntry(to, amount, tokenId); } function _warpPast(uint64 availableAt) internal { @@ -105,53 +110,8 @@ contract DotnsNameEscrowRefundsTest is BaseDotns { assertEq(entry.tokenId, 0, "entry tokenId should be zero"); } - function test_creditRefund_allocatesMonotonicEntryIds() public { - uint256 first = _credit(recipient, 1 ether, TOKEN_ID); - uint256 second = _credit(recipient, 2 ether, TOKEN_ID); - uint256 third = _credit(recipient, 3 ether, TOKEN_ID); - - assertGt(second, first, "second entry id should exceed first"); - assertGt(third, second, "third entry id should exceed second"); - } - - function test_creditRefund_independentCooldowns() public { - uint256 firstId = _credit(recipient, 1 ether, TOKEN_ID); - uint64 firstAvailableAt = harness.refundEntry(firstId).availableAt; - - vm.warp(block.timestamp + 30 minutes); - - uint256 secondId = _credit(recipient, 2 ether, TOKEN_ID); - uint64 secondAvailableAt = harness.refundEntry(secondId).availableAt; - - assertEq( - harness.refundEntry(firstId).availableAt, - firstAvailableAt, - "first entry cooldown must not move when second is credited" - ); - assertGt( - secondAvailableAt, firstAvailableAt, "later entry's clock starts at its credit time" - ); - } - - function test_creditRefund_revertsOnZeroRecipient() public { - vm.expectRevert(IDotnsNameEscrow.InvalidRecipient.selector); - harness.exposeCreditRefund(address(0), 1 ether, TOKEN_ID); - } - - function test_creditRefund_revertsOnZeroAmount() public { - vm.expectRevert(IDotnsNameEscrow.InvalidAmount.selector); - harness.exposeCreditRefund(recipient, 0, TOKEN_ID); - } - - function test_creditRefund_emitsRefundCredited() public { - uint64 expectedAvailableAt = uint64(block.timestamp + DEFAULT_COOLDOWN); - vm.expectEmit(true, true, true, true, address(harness)); - emit IDotnsNameEscrow.RefundCredited(recipient, 1, 1 ether, expectedAvailableAt, TOKEN_ID); - _credit(recipient, 1 ether, TOKEN_ID); - } - function test_claimRefund_transfersAndDeletes() public { - uint256 entryId = _credit(recipient, 1 ether, TOKEN_ID); + uint256 entryId = _seed(recipient, 1 ether, TOKEN_ID); _warpPast(harness.refundEntry(entryId).availableAt); uint256 before = recipient.balance; @@ -165,7 +125,7 @@ contract DotnsNameEscrowRefundsTest is BaseDotns { } function test_claimRefund_revertsWhenCallerIsNotRecipient() public { - uint256 entryId = _credit(recipient, 1 ether, TOKEN_ID); + uint256 entryId = _seed(recipient, 1 ether, TOKEN_ID); _warpPast(harness.refundEntry(entryId).availableAt); vm.prank(otherRecipient); @@ -178,7 +138,7 @@ contract DotnsNameEscrowRefundsTest is BaseDotns { } function test_claimRefund_revertsBeforeCooldown() public { - uint256 entryId = _credit(recipient, 1 ether, TOKEN_ID); + uint256 entryId = _seed(recipient, 1 ether, TOKEN_ID); uint64 availableAt = harness.refundEntry(entryId).availableAt; vm.prank(recipient); @@ -189,7 +149,7 @@ contract DotnsNameEscrowRefundsTest is BaseDotns { } function test_claimRefund_emitsRefundClaimed() public { - uint256 entryId = _credit(recipient, 1 ether, TOKEN_ID); + uint256 entryId = _seed(recipient, 1 ether, TOKEN_ID); _warpPast(harness.refundEntry(entryId).availableAt); vm.expectEmit(true, true, false, true, address(harness)); @@ -200,7 +160,7 @@ contract DotnsNameEscrowRefundsTest is BaseDotns { function test_claimRefund_revertsOnTransferFailure() public { RejectingReceiver rejecter = new RejectingReceiver(); - uint256 entryId = _credit(address(rejecter), 1 ether, TOKEN_ID); + uint256 entryId = _seed(address(rejecter), 1 ether, TOKEN_ID); _warpPast(harness.refundEntry(entryId).availableAt); vm.prank(address(rejecter)); @@ -209,9 +169,9 @@ contract DotnsNameEscrowRefundsTest is BaseDotns { } function test_claimRefundsBatch_aggregatesAndTransfers() public { - uint256 a = _credit(recipient, 1 ether, TOKEN_ID); - uint256 b = _credit(recipient, 2 ether, TOKEN_ID); - uint256 c = _credit(recipient, 3 ether, TOKEN_ID); + uint256 a = _seed(recipient, 1 ether, TOKEN_ID); + uint256 b = _seed(recipient, 2 ether, TOKEN_ID); + uint256 c = _seed(recipient, 3 ether, TOKEN_ID); _warpPast(harness.refundEntry(c).availableAt); uint256[] memory ids = new uint256[](3); @@ -229,12 +189,12 @@ contract DotnsNameEscrowRefundsTest is BaseDotns { } function test_claimRefundsBatch_atomicOnLockedEntry() public { - uint256 a = _credit(recipient, 1 ether, TOKEN_ID); + uint256 a = _seed(recipient, 1 ether, TOKEN_ID); // Warp between credits so entry B's cooldown clock starts late enough that the batch // claim straddles it: A and C are claimable, B is not. vm.warp(block.timestamp + DEFAULT_COOLDOWN); - uint256 b = _credit(recipient, 2 ether, TOKEN_ID); - uint256 c = _credit(recipient, 3 ether, TOKEN_ID); + uint256 b = _seed(recipient, 2 ether, TOKEN_ID); + uint256 c = _seed(recipient, 3 ether, TOKEN_ID); uint64 aAvailableAt = harness.refundEntry(a).availableAt; vm.warp(aAvailableAt + 1); uint64 bAvailableAt = harness.refundEntry(b).availableAt; @@ -266,9 +226,9 @@ contract DotnsNameEscrowRefundsTest is BaseDotns { } function test_pendingRefundIds_paginates() public { - uint256 a = _credit(recipient, 1 ether, TOKEN_ID); - uint256 b = _credit(recipient, 2 ether, TOKEN_ID); - uint256 c = _credit(recipient, 3 ether, TOKEN_ID); + uint256 a = _seed(recipient, 1 ether, TOKEN_ID); + uint256 b = _seed(recipient, 2 ether, TOKEN_ID); + uint256 c = _seed(recipient, 3 ether, TOKEN_ID); uint256[] memory firstPage = harness.pendingRefundIds(recipient, 0, 2); assertEq(firstPage.length, 2); @@ -284,9 +244,9 @@ contract DotnsNameEscrowRefundsTest is BaseDotns { } function test_removeRefundEntry_swapPopMiddlePreservesIndices() public { - uint256 a = _credit(recipient, 1 ether, TOKEN_ID); - uint256 b = _credit(recipient, 2 ether, TOKEN_ID); - uint256 c = _credit(recipient, 3 ether, TOKEN_ID); + uint256 a = _seed(recipient, 1 ether, TOKEN_ID); + uint256 b = _seed(recipient, 2 ether, TOKEN_ID); + uint256 c = _seed(recipient, 3 ether, TOKEN_ID); _warpPast(harness.refundEntry(b).availableAt); vm.prank(recipient); diff --git a/test/unit/pop/PopRules.t.sol b/test/unit/pop/PopRules.t.sol index c1926c7c9..4a71f8074 100644 --- a/test/unit/pop/PopRules.t.sol +++ b/test/unit/pop/PopRules.t.sol @@ -118,13 +118,6 @@ contract PopRulesTests is BaseDotns { assertEq(popRules.priceWithCheck("lights", ed).price, BASE_DEPOSIT); } - function test_transfer_reprices_at_own_length() public { - _grantPersonhood(leonardo); - - assertEq(popRules.transferFloor("lights", leonardo, tiago), BASE_DEPOSIT); - assertEq(popRules.transferFloor("lights", leonardo, leonardo), 0); - } - function test_price_with_check_revert_governance() public { vm.expectRevert( abi.encodeWithSelector(IPopRules.PopError.selector, "Reserved for Governance") @@ -267,22 +260,6 @@ contract PopRulesTests is BaseDotns { assertEq(metadata.price, costModelRegistry.priceForBaseLength(bytes(label).length)); } - function test_transferFloor_matches_model() public { - // A Personhood sender handing a band-gated name to a NoStatus recipient pays the name's - // own price, which is the registered model's amount for its base length. - string memory label = "lights"; - _grantPersonhood(ed); - uint256 floor = popRules.transferFloor(label, ed, leonardo); - assertEq(floor, costModelRegistry.priceForBaseLength(bytes(label).length)); - } - - function test_transferFloor_is_zero_for_open_names() public { - // A name open to all moves for free, even when a Personhood holder hands it to a - // NoStatus account. - _grantPersonhood(ed); - assertEq(popRules.transferFloor("longnamehere", ed, leonardo), 0); - } - function test_pricingVersion_matches_registry() public view { assertEq(popRules.pricingVersion(), costModelRegistry.currentVersion()); assertEq(popRules.pricingVersion(), flatPricing.version()); diff --git a/test/unit/registrar/DotnsRegistrar.t.sol b/test/unit/registrar/DotnsRegistrar.t.sol index 1cacecbc2..e9c673fbb 100644 --- a/test/unit/registrar/DotnsRegistrar.t.sol +++ b/test/unit/registrar/DotnsRegistrar.t.sol @@ -303,19 +303,8 @@ contract DotnsRegistrarTests is BaseDotns { label; } - function test_quote_transfer_fee_reverts_for_zero_recipient() public { - string memory label = "quotezero01"; - _register(label, ed, IPopRules.PopStatus.NoStatus); - uint256 tokenId = _tokenIdForLabel(label); - - vm.expectRevert( - abi.encodeWithSelector(IERC721Errors.ERC721InvalidReceiver.selector, address(0)) - ); - dotnsRegistrar.quoteTransferFee(tokenId, address(0)); - } - - function test_quote_transfer_fee_reverts_when_escrow_unconfigured() public { - string memory label = "quoteneedsescrow01"; + function test_transfer_reverts_when_escrow_unconfigured() public { + string memory label = "needsescrow01"; _register(label, ed, IPopRules.PopStatus.NoStatus); uint256 tokenId = _tokenIdForLabel(label); @@ -329,59 +318,45 @@ contract DotnsRegistrarTests is BaseDotns { ); vm.expectRevert(IDotnsRegistrar.EscrowNotConfigured.selector); - dotnsRegistrar.quoteTransferFee(tokenId, leonardo); - } - - function test_quote_transfer_fee_zero_for_self_transfer() public { - string memory label = "selfquote01"; - _register(label, ed, IPopRules.PopStatus.NoStatus); - uint256 tokenId = _tokenIdForLabel(label); - - assertEq(dotnsRegistrar.quoteTransferFee(tokenId, ed), 0); - } - - function test_quote_transfer_fee_zero_for_escrow_recipient() public { - string memory label = "escrowquote01"; - _register(label, ed, IPopRules.PopStatus.NoStatus); - uint256 tokenId = _tokenIdForLabel(label); - - assertEq(dotnsRegistrar.quoteTransferFee(tokenId, address(dotnsNameEscrow)), 0); + vm.prank(ed); + dotnsRegistrar.transferFrom(ed, leonardo, tokenId); } - function test_quote_transfer_fee_zero_when_no_label_recorded() public { - // Direct-controller registration with an empty label leaves no entry in the holder's - // LabelStore; the quoter has nothing to derive a tier-price from and must return zero. - string memory label = "noquotelabel01"; + function test_transfer_below_band_is_free() public { + string memory label = "feegated"; + // Ed holds personhood and leonardo does not, below the band of a base length of eight: + // the move costs nothing and the deposit follows the name. + _register(label, ed, IPopRules.PopStatus.Personhood); uint256 tokenId = _tokenIdForLabel(label); + uint256 feesBefore = dotnsNameEscrow.protocolFees(); - vm.prank(address(dotnsRegistrarController)); - dotnsRegistrar.register(tokenId, ed, ""); + vm.prank(ed); + dotnsRegistrar.transferFrom(ed, leonardo, tokenId); - assertEq(dotnsRegistrar.quoteTransferFee(tokenId, leonardo), 0); - label; + assertEq(dotnsRegistrar.ownerOf(tokenId), leonardo); + assertEq(dotnsNameEscrow.getReleasePosition(tokenId).recipient, leonardo); + assertEq(dotnsNameEscrow.protocolFees(), feesBefore, "no fee reaches the pot"); } - function test_transfer_reverts_when_fee_required_but_no_value_attached() public { - string memory label = "feegated"; - // Grant personhood to ed so the mint succeeds, but leave leonardo at NoStatus, below the - // band of a base length of eight, so the transfer floor charges a non-zero fee. + function test_transfer_reverts_with_attached_value() public { + string memory label = "valuegated"; _register(label, ed, IPopRules.PopStatus.Personhood); uint256 tokenId = _tokenIdForLabel(label); + vm.deal(ed, 1 ether); - uint256 fee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); - assertGt(fee, 0, "test setup must produce a non-zero transfer floor"); - - vm.expectRevert( - abi.encodeWithSelector( - IDotnsRegistrar.TransferFeeRequired.selector, tokenId, leonardo, fee - ) - ); + // The transfer functions are not payable, so the call fails before `_update` runs. vm.prank(ed); - dotnsRegistrar.transferFrom(ed, leonardo, tokenId); + (bool ok,) = address(dotnsRegistrar).call{value: 1}( + abi.encodeCall(IERC721.transferFrom, (ed, leonardo, tokenId)) + ); + assertFalse(ok, "a transfer carrying value must revert"); + + assertEq(dotnsRegistrar.ownerOf(tokenId), ed); + assertEq(ed.balance, 1 ether, "attached value must be returned on revert"); } - function test_self_transfer_skips_fee_charge() public { - // A `from == to` move must not touch the escrow at all: no fee, no position sync. + function test_self_transfer_skips_escrow() public { + // A `from == to` move must not touch the escrow at all: no position sync. string memory label = "selftransfer01"; _register(label, ed, IPopRules.PopStatus.NoStatus); uint256 tokenId = _tokenIdForLabel(label); @@ -389,7 +364,7 @@ contract DotnsRegistrarTests is BaseDotns { IDotnsNameEscrow.ReleasePosition memory before = dotnsNameEscrow.getReleasePosition(tokenId); vm.prank(ed); - dotnsRegistrar.transferFrom{value: 0}(ed, ed, tokenId); + dotnsRegistrar.transferFrom(ed, ed, tokenId); IDotnsNameEscrow.ReleasePosition memory afterPos = dotnsNameEscrow.getReleasePosition(tokenId); @@ -567,13 +542,6 @@ contract DotnsRegistrarTests is BaseDotns { assertEq(dotnsRegistrar.ownerOf(tokenId), ed); } - function test_soulbound_quote_transfer_fee_reverts() public { - uint256 tokenId = _mintSoulbound("sbquote01", ed); - - vm.expectRevert(abi.encodeWithSelector(IDotnsRegistrar.NameSoulbound.selector, tokenId)); - dotnsRegistrar.quoteTransferFee(tokenId, leonardo); - } - function test_soulbound_cannot_enter_escrow() public { // A soulbound name holds no escrow position, so `release` rejects it before any custody // move is attempted. The name can never be released into escrow. @@ -630,9 +598,11 @@ contract DotnsRegistrarTests is BaseDotns { uint256 tokenId = _mintSoulbound("sbvalue01", ed); vm.deal(ed, 1 ether); - vm.expectRevert(abi.encodeWithSelector(IDotnsRegistrar.NameSoulbound.selector, tokenId)); vm.prank(ed); - dotnsRegistrar.transferFrom{value: 1 ether}(ed, leonardo, tokenId); + (bool ok,) = address(dotnsRegistrar).call{value: 1 ether}( + abi.encodeCall(IERC721.transferFrom, (ed, leonardo, tokenId)) + ); + assertFalse(ok, "a transfer carrying value must revert"); assertEq(dotnsRegistrar.ownerOf(tokenId), ed); assertEq(ed.balance, 1 ether, "attached value must be returned on revert"); @@ -640,8 +610,7 @@ contract DotnsRegistrarTests is BaseDotns { } function test_soulbound_self_transfer_reverts() public { - // A soulbound name reverts even on a same-address move, keeping transferFrom in step with - // quoteTransferFee (which reverts regardless of recipient) and the interface contract. + // A soulbound name reverts even on a same-address move, as the interface contract states. uint256 tokenId = _mintSoulbound("sbself01", ed); vm.expectRevert(abi.encodeWithSelector(IDotnsRegistrar.NameSoulbound.selector, tokenId)); diff --git a/test/unit/registrar/DotnsRegistrarController.t.sol b/test/unit/registrar/DotnsRegistrarController.t.sol index d66ec3f93..63f396c00 100644 --- a/test/unit/registrar/DotnsRegistrarController.t.sol +++ b/test/unit/registrar/DotnsRegistrarController.t.sol @@ -522,9 +522,8 @@ contract DotnsRegistrarControllerTest is BaseDotns { uint256 tokenId = _tokenIdForLabel(nameLabel); - uint256 fee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); vm.prank(ed); - dotnsRegistrar.transferFrom{value: fee}(ed, leonardo, tokenId); + dotnsRegistrar.transferFrom(ed, leonardo, tokenId); assertEq(dotnsRegistrar.ownerOf(tokenId), leonardo); @@ -544,13 +543,11 @@ contract DotnsRegistrarControllerTest is BaseDotns { uint256 tokenId = _tokenIdForLabel(nameLabel); - uint256 outboundFee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); vm.prank(ed); - dotnsRegistrar.transferFrom{value: outboundFee}(ed, leonardo, tokenId); + dotnsRegistrar.transferFrom(ed, leonardo, tokenId); - uint256 returnFee = dotnsRegistrar.quoteTransferFee(tokenId, ed); vm.prank(leonardo); - dotnsRegistrar.transferFrom{value: returnFee}(leonardo, ed, tokenId); + dotnsRegistrar.transferFrom(leonardo, ed, tokenId); ILabelStore edStore = ILabelStore(storeFactory.getLabelStore(ed)); assertEq(edStore.getLabel(bytes32(tokenId)), string.concat(nameLabel, ".dot")); @@ -567,9 +564,8 @@ contract DotnsRegistrarControllerTest is BaseDotns { uint256 tokenId = _tokenIdForLabel(nameLabel); assertEq(dotnsReverseResolver.nameOf(ed), "primarymove01.dot"); - uint256 _xferFee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); vm.prank(ed); - dotnsRegistrar.transferFrom{value: _xferFee}(ed, leonardo, tokenId); + dotnsRegistrar.transferFrom(ed, leonardo, tokenId); assertEq(dotnsRegistrar.ownerOf(tokenId), leonardo); assertEq(dotnsReverseResolver.nameOf(ed), ""); @@ -596,9 +592,8 @@ contract DotnsRegistrarControllerTest is BaseDotns { uint256 tokenId = _tokenIdForLabel(nameLabel); - uint256 _xferFee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); vm.prank(ed); - dotnsRegistrar.safeTransferFrom{value: _xferFee}(ed, leonardo, tokenId); + dotnsRegistrar.safeTransferFrom(ed, leonardo, tokenId); assertEq(dotnsRegistrar.ownerOf(tokenId), leonardo); @@ -633,9 +628,7 @@ contract DotnsRegistrarControllerTest is BaseDotns { vm.warp(block.timestamp + dotnsRegistrarController.minCommitmentAge() + 1); - uint256 priced = popRules.priceWithoutCheck(popfullLabel, ownerAddr).price; - uint256 friction = popRules.transferFloor(popfullLabel, payer, ownerAddr); - uint256 charge = priced > friction ? priced : friction; + uint256 charge = popRules.priceWithoutCheck(popfullLabel, ownerAddr).price; vm.deal(payer, charge); vm.prank(payer); @@ -660,10 +653,8 @@ contract DotnsRegistrarControllerTest is BaseDotns { vm.prank(ed); dotnsRegistrar.setApprovalForAll(tiago, true); - uint256 xferFee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); - vm.deal(tiago, xferFee); vm.prank(tiago); - dotnsRegistrar.transferFrom{value: xferFee}(ed, leonardo, tokenId); + dotnsRegistrar.transferFrom(ed, leonardo, tokenId); assertEq(dotnsRegistrar.ownerOf(tokenId), leonardo); @@ -685,9 +676,8 @@ contract DotnsRegistrarControllerTest is BaseDotns { assertEq(storeFactory.getLabelStore(leonardo), address(0)); - uint256 _xferFee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); vm.prank(ed); - dotnsRegistrar.transferFrom{value: _xferFee}(ed, leonardo, tokenId); + dotnsRegistrar.transferFrom(ed, leonardo, tokenId); assertEq(dotnsRegistrar.ownerOf(tokenId), leonardo); // Label-less mints (gateway-cold PoP path) carry no label to mirror, so the registrar @@ -696,15 +686,14 @@ contract DotnsRegistrarControllerTest is BaseDotns { assertEq(storeFactory.getLabelStore(leonardo), address(0)); } - /// @notice A zero-fee NoStatus to NoStatus transfer must still rebind the escrow - /// position to the new holder, because the deposit follows the NFT and - /// only the current holder can later release it. - /// @dev Exercises the position-sync path in the registrar: with the transfer floor - /// at zero (NoStatus to NoStatus), the registrar still consults the escrow - /// whenever a live position points at a recipient other than the destination, + /// @notice A NoStatus to NoStatus transfer must rebind the escrow position to the new + /// holder, because the deposit follows the NFT and only the current holder can + /// later release it. + /// @dev Exercises the position-sync path in the registrar: the registrar consults the + /// escrow whenever a live position points at a recipient other than the destination, /// so the position rebinds. No reserve movement, no refund entry, and no /// transfer-time payout fires. - function test_transfer_zero_fee_rebinds_position_to_new_holder() public { + function test_transfer_rebinds_position_to_new_holder() public { string memory nameLabel = NOSTATUS_LABEL_A; _register(nameLabel, ed, IPopRules.PopStatus.NoStatus); @@ -714,15 +703,12 @@ contract DotnsRegistrarControllerTest is BaseDotns { assertEq(before.amount, BASE_DEPOSIT, "NoStatus mint must seed BASE_DEPOSIT deposit"); assertEq(before.recipient, ed, "deposit recipient must be original registrant at mint"); - uint256 transferFee = dotnsRegistrar.quoteTransferFee(tokenId, leonardo); - assertEq(transferFee, 0, "NoStatus to NoStatus transfer floor is zero"); - uint256 priorReserve = dotnsNameEscrow.reserves(address(0)); uint256 priorPendingRefundsEd = dotnsNameEscrow.pendingRefundCount(ed); uint256 priorPendingRefundsLeonardo = dotnsNameEscrow.pendingRefundCount(leonardo); vm.prank(ed); - dotnsRegistrar.transferFrom{value: 0}(ed, leonardo, tokenId); + dotnsRegistrar.transferFrom(ed, leonardo, tokenId); IDotnsNameEscrow.ReleasePosition memory after_ = dotnsNameEscrow.getReleasePosition(tokenId); assertEq(after_.amount, BASE_DEPOSIT, "deposit amount must travel with the NFT"); @@ -768,7 +754,7 @@ contract DotnsRegistrarControllerTest is BaseDotns { // Outbound leg ed to leonardo rebinds the position to leonardo. vm.prank(ed); - dotnsRegistrar.transferFrom{value: 0}(ed, leonardo, tokenId); + dotnsRegistrar.transferFrom(ed, leonardo, tokenId); IDotnsNameEscrow.ReleasePosition memory between = dotnsNameEscrow.getReleasePosition(tokenId); @@ -777,7 +763,7 @@ contract DotnsRegistrarControllerTest is BaseDotns { // Return leg leonardo back to ed rebinds the position back to ed. vm.prank(leonardo); - dotnsRegistrar.transferFrom{value: 0}(leonardo, ed, tokenId); + dotnsRegistrar.transferFrom(leonardo, ed, tokenId); IDotnsNameEscrow.ReleasePosition memory after_ = dotnsNameEscrow.getReleasePosition(tokenId); assertEq(after_.amount, BASE_DEPOSIT, "deposit must travel through the return hop");