From 1be2959ae3d2e2d84bca642b5ccffe27e4d50761 Mon Sep 17 00:00:00 2001
From: Oliver Anyanwu <oliver@devrally.tech>
Date: Sun, 17 May 2026 12:41:42 +0100
Subject: [PATCH] feat: add two-step Ownable mixin with tests

Pending-owner must accept before ownership transfers - avoids the
classic foot-gun where a typo'd address ends up as owner. Renounce stays
single-step but zeroes pendingOwner too. Custom errors throughout.

Concrete OwnableHarness in the test file lets the abstract base be
deployed for unit tests.
---
 src/Ownable.sol    | 46 +++++++++++++++++++++++++
 test/Ownable.t.sol | 85 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 131 insertions(+)
 create mode 100644 src/Ownable.sol
 create mode 100644 test/Ownable.t.sol

diff --git a/src/Ownable.sol b/src/Ownable.sol
new file mode 100644
index 0000000..2327740
--- /dev/null
+++ b/src/Ownable.sol
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+/// Minimal two-step Ownable. Pending owner must accept - no foot-gun transfers
+/// to an address that can't sign for itself.
+abstract contract Ownable {
+    address public owner;
+    address public pendingOwner;
+
+    event OwnershipTransferStarted(address indexed previousOwner, address indexed newOwner);
+    event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
+
+    error NotOwner();
+    error NotPendingOwner();
+
+    modifier onlyOwner() {
+        if (msg.sender != owner) revert NotOwner();
+        _;
+    }
+
+    constructor() {
+        owner = msg.sender;
+        emit OwnershipTransferred(address(0), msg.sender);
+    }
+
+    function transferOwnership(address newOwner) external onlyOwner {
+        pendingOwner = newOwner;
+        emit OwnershipTransferStarted(owner, newOwner);
+    }
+
+    function acceptOwnership() external {
+        if (msg.sender != pendingOwner) revert NotPendingOwner();
+        address previous = owner;
+        owner = pendingOwner;
+        delete pendingOwner;
+        emit OwnershipTransferred(previous, owner);
+    }
+
+    /// Owner can renounce immediately - no two-step. Use carefully.
+    function renounceOwnership() external onlyOwner {
+        address previous = owner;
+        delete owner;
+        delete pendingOwner;
+        emit OwnershipTransferred(previous, address(0));
+    }
+}
diff --git a/test/Ownable.t.sol b/test/Ownable.t.sol
new file mode 100644
index 0000000..507fd63
--- /dev/null
+++ b/test/Ownable.t.sol
@@ -0,0 +1,85 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+import {Test} from "forge-std/Test.sol";
+import {Ownable} from "../src/Ownable.sol";
+
+/// Concrete impl so we can deploy the abstract Ownable in tests
+contract OwnableHarness is Ownable {
+    uint256 public protectedValue;
+
+    function setProtectedValue(uint256 v) external onlyOwner {
+        protectedValue = v;
+    }
+}
+
+contract OwnableTest is Test {
+    OwnableHarness public h;
+    address deployer;
+    address alice = makeAddr("alice");
+    address bob = makeAddr("bob");
+
+    function setUp() public {
+        deployer = address(this);
+        h = new OwnableHarness();
+    }
+
+    function test_InitialOwnerIsDeployer() public view {
+        assertEq(h.owner(), deployer);
+        assertEq(h.pendingOwner(), address(0));
+    }
+
+    function test_OnlyOwner_CanCallProtected() public {
+        h.setProtectedValue(1);
+        assertEq(h.protectedValue(), 1);
+
+        vm.expectRevert(Ownable.NotOwner.selector);
+        vm.prank(alice);
+        h.setProtectedValue(2);
+    }
+
+    function test_TwoStepTransfer() public {
+        h.transferOwnership(alice);
+        // Owner unchanged until accepted
+        assertEq(h.owner(), deployer);
+        assertEq(h.pendingOwner(), alice);
+
+        vm.prank(alice);
+        h.acceptOwnership();
+        assertEq(h.owner(), alice);
+        assertEq(h.pendingOwner(), address(0));
+    }
+
+    function test_Accept_RevertWhenNotPending() public {
+        h.transferOwnership(alice);
+        vm.expectRevert(Ownable.NotPendingOwner.selector);
+        vm.prank(bob);
+        h.acceptOwnership();
+    }
+
+    function test_Transfer_RevertWhenNotOwner() public {
+        vm.expectRevert(Ownable.NotOwner.selector);
+        vm.prank(alice);
+        h.transferOwnership(bob);
+    }
+
+    function test_Renounce_ZerosOwner() public {
+        h.renounceOwnership();
+        assertEq(h.owner(), address(0));
+
+        // Nobody can call protected anymore
+        vm.expectRevert(Ownable.NotOwner.selector);
+        h.setProtectedValue(1);
+    }
+
+    function test_Transfer_OverwritesPendingOwner() public {
+        h.transferOwnership(alice);
+        h.transferOwnership(bob);
+        assertEq(h.pendingOwner(), bob);
+
+        // Alice (no longer pending) cannot accept
+        vm.expectRevert(Ownable.NotPendingOwner.selector);
+        vm.prank(alice);
+        h.acceptOwnership();
+    }
+}