use OpenZeppelin Ownable instead of custom logic, add more best practice thingies

Author: hougangdev

.gitmodules

@@ -1,3 +1,6 @@
 [submodule "lib/forge-std"]
 	path = lib/forge-std
 	url = https://github.com/foundry-rs/forge-std
+[submodule "lib/openzeppelin-contracts"]
+	path = lib/openzeppelin-contracts
+	url = https://github.com/OpenZeppelin/openzeppelin-contracts

foundry.toml

@@ -4,5 +4,8 @@ out = "out"
 libs = ["lib"]
 solc = "0.8.25"
 evm_version = "cancun"
+remappings = [
+    "@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/"
+]
 
 # See more config options https://github.com/foundry-rs/foundry/blob/master/crates/config/README.md#all-options

lib/openzeppelin-contracts

@@ -1,26 +1,28 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.25;
 
+import "@openzeppelin/contracts/access/Ownable.sol";
+import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
+
 /**
  * @title LimitOrderBook
  * @dev A contract for managing limit orders with off-chain execution
  * @dev This contract is designed to be used on HyperEVM
  * @notice Users can place limit orders which are executed by off-chain bots when price conditions are met
  * @notice Executors can be authorized to mark orders as executed
  */
-contract LimitOrderBook {
+contract LimitOrderBook is Ownable, ReentrancyGuard {
     /*//////////////////////////////////////////////////////////////
                                 ERRORS
     //////////////////////////////////////////////////////////////*/
 
-    error InvalidPrice();
-    error InvalidAmount();
-    error OrderNotFound();
-    error NotOrderOwner();
-    error OrderAlreadyExecuted();
-    error OrderNotExecutable();
-    error UnauthorizedExecutor();
-    error NotOwner();
+    error LimitOrderBook_InvalidPrice();
+    error LimitOrderBook_InvalidAmount();
+    error LimitOrderBook_OrderNotFound();
+    error LimitOrderBook_NotOrderOwner();
+    error LimitOrderBook_OrderAlreadyExecuted();
+    error LimitOrderBook_OrderNotExecutable();
+    error LimitOrderBook_UnauthorizedExecutor();
 
     /*//////////////////////////////////////////////////////////////
                                 TYPES
@@ -42,7 +44,6 @@ contract LimitOrderBook {
 
     // Access control for executors
     mapping(address => bool) public s_authorizedExecutors;
-    address public s_owner;
 
     /*//////////////////////////////////////////////////////////////
                                 EVENTS
@@ -58,27 +59,21 @@ contract LimitOrderBook {
                                 MODIFIERS
     //////////////////////////////////////////////////////////////*/
 
-    modifier onlyOwner() {
-        if (msg.sender != s_owner) revert NotOwner();
-        _;
-    }
-
     modifier onlyAuthorizedExecutor() {
-        if (!s_authorizedExecutors[msg.sender]) revert UnauthorizedExecutor();
+        if (!s_authorizedExecutors[msg.sender]) revert LimitOrderBook_UnauthorizedExecutor();
         _;
     }
 
     modifier validOrder(uint256 orderId) {
-        if (s_orders[orderId].user == address(0)) revert OrderNotFound();
+        if (s_orders[orderId].user == address(0)) revert LimitOrderBook_OrderNotFound();
         _;
     }
 
     /*//////////////////////////////////////////////////////////////
                                 CONSTRUCTOR
     //////////////////////////////////////////////////////////////*/
 
-    constructor() {
-        s_owner = msg.sender;
+    constructor(address initialOwner) Ownable(initialOwner) {
         // Initially, allow anyone to execute (can be changed later)
         // This supports the permissionless bot execution model
         s_authorizedExecutors[address(0)] = true; // Flag to indicate permissionless execution
@@ -94,10 +89,10 @@ contract LimitOrderBook {
      * @param amount The amount/quantity for the order
      * @return orderId The unique identifier for the placed order
      */
-    function placeOrder(uint256 price, uint256 amount) external returns (uint256 orderId) {
+    function placeOrder(uint256 price, uint256 amount) external nonReentrant returns (uint256 orderId) {
         // Validation
-        if (price == 0) revert InvalidPrice();
-        if (amount == 0) revert InvalidAmount();
+        if (price == 0) revert LimitOrderBook_InvalidPrice();
+        if (amount == 0) revert LimitOrderBook_InvalidAmount();
 
         // Create order
         orderId = s_nextOrderId;
@@ -113,14 +108,14 @@ contract LimitOrderBook {
      * @notice Cancel an existing order
      * @param orderId The ID of the order to cancel
      */
-    function cancelOrder(uint256 orderId) external validOrder(orderId) {
+    function cancelOrder(uint256 orderId) external nonReentrant validOrder(orderId) {
         Order storage order = s_orders[orderId];
 
         // Verify ownership
-        if (order.user != msg.sender) revert NotOrderOwner();
+        if (order.user != msg.sender) revert LimitOrderBook_NotOrderOwner();
 
         // Verify not already executed
-        if (order.executed) revert OrderAlreadyExecuted();
+        if (order.executed) revert LimitOrderBook_OrderAlreadyExecuted();
 
         // Delete the order
         delete s_orders[orderId];
@@ -133,16 +128,16 @@ contract LimitOrderBook {
      * @param orderId The ID of the order to execute
      * @dev This function can be called by anyone initially, but access can be restricted
      */
-    function markExecuted(uint256 orderId) external validOrder(orderId) {
+    function markExecuted(uint256 orderId) external nonReentrant validOrder(orderId) {
         Order storage order = s_orders[orderId];
 
         // Verify not already executed
-        if (order.executed) revert OrderAlreadyExecuted();
+        if (order.executed) revert LimitOrderBook_OrderAlreadyExecuted();
 
         // If executor authorization is enabled, check permission
         // address(0) flag indicates if permissionless execution is allowed
         if (!s_authorizedExecutors[address(0)] && !s_authorizedExecutors[msg.sender]) {
-            revert UnauthorizedExecutor();
+            revert LimitOrderBook_UnauthorizedExecutor();
         }
 
         // Mark as executed
@@ -169,14 +164,6 @@ contract LimitOrderBook {
         emit ExecutorRevoked(executor);
     }
 
-    /**
-     * @notice Transfer ownership of the contract
-     * @param newOwner The new owner address
-     */
-    function transferOwnership(address newOwner) external onlyOwner {
-        s_owner = newOwner;
-    }
-
     /**
      * @notice Enable or disable executor authorization requirement
      * @param requireAuth True to require authorization, false for permissionless execution

src/LimitOrderBook.sol

@@ -16,7 +16,7 @@ contract LimitOrderBookTest is Test {
     event OrderCancelled(uint256 indexed orderId);
 
     function setUp() public {
-        orderBook = new LimitOrderBook();
+        orderBook = new LimitOrderBook(address(this)); // Deploy with test contract as initial owner
     }
 
     function test_PlaceOrder() public {
@@ -41,13 +41,13 @@ contract LimitOrderBookTest is Test {
 
     function test_PlaceOrder_InvalidPrice() public {
         vm.prank(user1);
-        vm.expectRevert(LimitOrderBook.InvalidPrice.selector);
+        vm.expectRevert(LimitOrderBook.LimitOrderBook_InvalidPrice.selector);
         orderBook.placeOrder(0, 100);
     }
 
     function test_PlaceOrder_InvalidAmount() public {
         vm.prank(user1);
-        vm.expectRevert(LimitOrderBook.InvalidAmount.selector);
+        vm.expectRevert(LimitOrderBook.LimitOrderBook_InvalidAmount.selector);
         orderBook.placeOrder(1000, 0);
     }
 
@@ -64,7 +64,7 @@ contract LimitOrderBookTest is Test {
         orderBook.cancelOrder(orderId);
 
         // Order should be deleted
-        vm.expectRevert(LimitOrderBook.OrderNotFound.selector);
+        vm.expectRevert(LimitOrderBook.LimitOrderBook_OrderNotFound.selector);
         orderBook.getOrder(orderId);
     }
 
@@ -75,7 +75,7 @@ contract LimitOrderBookTest is Test {
 
         // Try to cancel as user2
         vm.prank(user2);
-        vm.expectRevert(LimitOrderBook.NotOrderOwner.selector);
+        vm.expectRevert(LimitOrderBook.LimitOrderBook_NotOrderOwner.selector);
         orderBook.cancelOrder(orderId);
     }
 
@@ -90,7 +90,7 @@ contract LimitOrderBookTest is Test {
 
         // Try to cancel executed order
         vm.prank(user1);
-        vm.expectRevert(LimitOrderBook.OrderAlreadyExecuted.selector);
+        vm.expectRevert(LimitOrderBook.LimitOrderBook_OrderAlreadyExecuted.selector);
         orderBook.cancelOrder(orderId);
     }
 
@@ -122,7 +122,7 @@ contract LimitOrderBookTest is Test {
 
         // Try to execute again
         vm.prank(executor);
-        vm.expectRevert(LimitOrderBook.OrderAlreadyExecuted.selector);
+        vm.expectRevert(LimitOrderBook.LimitOrderBook_OrderAlreadyExecuted.selector);
         orderBook.markExecuted(orderId);
     }
 
@@ -155,7 +155,7 @@ contract LimitOrderBookTest is Test {
     }
 
     function test_AuthorizeExecutor() public {
-        address owner = orderBook.s_owner();
+        address owner = orderBook.owner();
 
         vm.prank(owner);
         vm.expectEmit(true, false, false, true);
@@ -166,7 +166,7 @@ contract LimitOrderBookTest is Test {
     }
 
     function test_RevokeExecutor() public {
-        address owner = orderBook.s_owner();
+        address owner = orderBook.owner();
 
         // First authorize
         vm.prank(owner);
@@ -183,15 +183,15 @@ contract LimitOrderBookTest is Test {
 
     function test_OnlyOwnerFunctions() public {
         vm.prank(user1);
-        vm.expectRevert(LimitOrderBook.NotOwner.selector);
+        vm.expectRevert(); // OpenZeppelin's Ownable will revert with OwnableUnauthorizedAccount
         orderBook.authorizeExecutor(executor);
 
         vm.prank(user1);
-        vm.expectRevert(LimitOrderBook.NotOwner.selector);
+        vm.expectRevert(); // OpenZeppelin's Ownable will revert with OwnableUnauthorizedAccount
         orderBook.revokeExecutor(executor);
 
         vm.prank(user1);
-        vm.expectRevert(LimitOrderBook.NotOwner.selector);
+        vm.expectRevert(); // OpenZeppelin's Ownable will revert with OwnableUnauthorizedAccount
         orderBook.transferOwnership(user2);
     }