♻️ Make Ownable2Step Module-Friendly (#219)

### 🕓 Changelog This PR refactors the `Ownable2Step` contract to make it module-friendly and ready for the breaking `0.4.0` release. Furthermore, I enhance the `VyperDeployer` to display an informative message if the locally installed Vyper version is not compatible with the Vyper contract to be compiled. --------- Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>
pcaversaccio · Mar 27, 2024 · 87cabde · 87cabde
1 parent ac7f64f
commit 87cabde
Show file tree

Hide file tree

Showing 7 changed files with 761 additions and 636 deletions.
diff --git a/.gas-snapshot b/.gas-snapshot
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,9 @@
 - **Authentication**
   - [`AccessControl`](https://github.com/pcaversaccio/snekmate/blob/v0.1.0/src/snekmate/auth/AccessControl.vy): Make `AccessControl` module-friendly. ([#216](https://github.com/pcaversaccio/snekmate/pull/216))
   - [`Ownable`](https://github.com/pcaversaccio/snekmate/blob/v0.1.0/src/snekmate/auth/Ownable.vy): Make `Ownable` module-friendly. ([#218](https://github.com/pcaversaccio/snekmate/pull/218))
+  - [`Ownable2Step`](https://github.com/pcaversaccio/snekmate/blob/v0.1.0/src/snekmate/auth/Ownable2Step.vy): Make `Ownable2Step` module-friendly. ([#219](https://github.com/pcaversaccio/snekmate/pull/219))
+- **Vyper Contract Deployer**
+  - [`VyperDeployer`](https://github.com/pcaversaccio/snekmate/blob/v0.1.0/lib/utils/VyperDeployer.sol): Improve error message in the event of a Vyper compilation error. ([#219](https://github.com/pcaversaccio/snekmate/pull/219))
 
 ### 👀 Full Changelog
 

diff --git a/GUIDELINES.md b/GUIDELINES.md
@@ -74,6 +74,7 @@ def _as_singleton_array(element: uint256) -> DynArray[uint256, 1]:
   - Vyper built-in interface imports
   - Custom interface imports
   - Module imports
+  - Module exports
   - `public` constants
   - `internal` constants
   - `public` immutables

diff --git a/lib/utils/VyperDeployer.sol b/lib/utils/VyperDeployer.sol
@@ -2,6 +2,13 @@
 pragma solidity ^0.8.25;
 
 import {Create} from "create-util/Create.sol";
+import {console, StdStyle} from "forge-std/Test.sol";
+
+/**
+ * @dev Error that occurs when compiling a contract has failed.
+ * @param emitter The contract that emits the error.
+ */
+error CompilationFailed(address emitter);
 
 /**
  * @dev Error that occurs when deploying a contract has failed.
@@ -67,6 +74,20 @@ contract VyperDeployer is Create {
          */
         bytes memory bytecode = cheatCodes.ffi(cmds);
 
+        /**
+         * @dev Revert if the Vyper compilation failed or is a zero-length
+         * contract creation bytecode.
+         */
+        if (bytecode.length == 0) {
+            // solhint-disable-next-line no-console
+            console.log(
+                StdStyle.red(
+                    "Vyper compilation failed! Please ensure that you have a valid Vyper version installed."
+                )
+            );
+            revert CompilationFailed({emitter: self});
+        }
+
         /**
          * @dev Deploy the bytecode with the `CREATE` instruction.
          */
@@ -110,6 +131,20 @@ contract VyperDeployer is Create {
          */
         bytes memory bytecode = cheatCodes.ffi(cmds);
 
+        /**
+         * @dev Revert if the Vyper compilation failed or is a zero-length
+         * contract creation bytecode.
+         */
+        if (bytecode.length == 0) {
+            // solhint-disable-next-line no-console
+            console.log(
+                StdStyle.red(
+                    "Vyper compilation failed! Please ensure that you have a valid Vyper version installed."
+                )
+            );
+            revert CompilationFailed({emitter: self});
+        }
+
         /**
          * @dev Add the ABI-encoded constructor arguments to the
          * deployment bytecode.
@@ -163,6 +198,20 @@ contract VyperDeployer is Create {
          */
         bytes memory bytecode = cheatCodes.ffi(cmds);
 
+        /**
+         * @dev Revert if the Vyper compilation failed or is a zero-length
+         * contract creation bytecode.
+         */
+        if (bytecode.length == 0) {
+            // solhint-disable-next-line no-console
+            console.log(
+                StdStyle.red(
+                    "Vyper compilation failed! Please ensure that you have a valid Vyper version installed."
+                )
+            );
+            revert CompilationFailed({emitter: self});
+        }
+
         /**
          * @dev Deploy the bytecode with the `CREATE` instruction.
          */
@@ -214,6 +263,20 @@ contract VyperDeployer is Create {
          */
         bytes memory bytecode = cheatCodes.ffi(cmds);
 
+        /**
+         * @dev Revert if the Vyper compilation failed or is a zero-length
+         * contract creation bytecode.
+         */
+        if (bytecode.length == 0) {
+            // solhint-disable-next-line no-console
+            console.log(
+                StdStyle.red(
+                    "Vyper compilation failed! Please ensure that you have a valid Vyper version installed."
+                )
+            );
+            revert CompilationFailed({emitter: self});
+        }
+
         /**
          * @dev Add the ABI-encoded constructor arguments to the
          * deployment bytecode.

diff --git a/src/snekmate/auth/Ownable2Step.vy b/src/snekmate/auth/Ownable2Step.vy
@@ -15,14 +15,26 @@
 """
 
 
-# @dev Returns the address of the current owner.
-# @notice If you declare a variable as `public`,
-# Vyper automatically generates an `external`
-# getter function for the variable.
-owner: public(address)
+# @dev We import and use the `Ownable` module.
+from . import Ownable as ownable
+uses: ownable
+
+
+# @dev We export (i.e. the runtime bytecode exposes these
+# functions externally, allowing them to be called using
+# the ABI encoding specification) the `external` getter
+# function `owner` from the `Ownable` module.
+# @notice Please note that you must always also export (if
+# required by the contract logic) `public` declared `constant`,
+# `immutable`, and state variables, for which Vyper automatically
+# generates an `external` getter function for the variable.
+exports: ownable.owner
 
 
 # @dev Returns the address of the pending owner.
+# @notice If you declare a variable as `public`,
+# Vyper automatically generates an `external`
+# getter function for the variable.
 pending_owner: public(address)
 
 
@@ -33,24 +45,19 @@ event OwnershipTransferStarted:
     new_owner: indexed(address)
 
 
-# @dev Emitted when the ownership is transferred
-# from `previous_owner` to `new_owner`.
-event OwnershipTransferred:
-    previous_owner: indexed(address)
-    new_owner: indexed(address)
-
-
 @deploy
 @payable
 def __init__():
     """
     @dev To omit the opcodes for checking the `msg.value`
          in the creation-time EVM bytecode, the constructor
          is declared as `payable`.
-    @notice The `owner` role will be assigned to
-            the `msg.sender`.
+    @notice At initialisation time, the `owner` role will
+            be assigned to the `msg.sender` since we `uses`
+            the `Ownable` module, which implements the
+            aforementioned logic at contract creation time.
     """
-    self._transfer_ownership(msg.sender)
+    pass
 
 
 @external
@@ -68,9 +75,9 @@ def transfer_ownership(new_owner: address):
             there is one.
     @param new_owner The 20-byte address of the new owner.
     """
-    self._check_owner()
+    ownable._check_owner()
     self.pending_owner = new_owner
-    log OwnershipTransferStarted(self.owner, new_owner)
+    log OwnershipTransferStarted(ownable.owner, new_owner)
 
 
 @external
@@ -87,22 +94,16 @@ def accept_ownership():
 @external
 def renounce_ownership():
     """
-    @dev Sourced from {Ownable-renounce_ownership}.
-    @notice See {Ownable-renounce_ownership} for
-            the function docstring.
+    @dev Leaves the contract without an owner.
+    @notice Renouncing ownership will leave the
+            contract without an owner, thereby
+            removing any functionality that is
+            only available to the owner.
     """
-    self._check_owner()
+    ownable._check_owner()
     self._transfer_ownership(empty(address))
 
 
-@internal
-def _check_owner():
-    """
-    @dev Throws if the sender is not the owner.
-    """
-    assert msg.sender == self.owner, "Ownable2Step: caller is not the owner"
-
-
 @internal
 def _transfer_ownership(new_owner: address):
     """
@@ -114,6 +115,4 @@ def _transfer_ownership(new_owner: address):
     @param new_owner The 20-byte address of the new owner.
     """
     self.pending_owner = empty(address)
-    old_owner: address = self.owner
-    self.owner = new_owner
-    log OwnershipTransferred(old_owner, new_owner)
+    ownable._transfer_ownership(new_owner)
diff --git a/src/snekmate/auth/mocks/Ownable2StepMock.vy b/src/snekmate/auth/mocks/Ownable2StepMock.vy
@@ -0,0 +1,50 @@
+# pragma version ~=0.4.0b5
+"""
+@title Ownable2Step Module Reference Implementation
+@custom:contract-name Ownable2StepMock
+@license GNU Affero General Public License v3.0 only
+@author pcaversaccio
+"""
+
+
+# @dev We import and initialise the `Ownable` module.
+from .. import Ownable as ow
+initializes: ow
+
+
+# @dev We import and initialise the `Ownable2Step` module.
+from .. import Ownable2Step as o2
+initializes: o2[ownable := ow]
+
+
+# @dev We export (i.e. the runtime bytecode exposes these
+# functions externally, allowing them to be called using
+# the ABI encoding specification) all `external` functions
+# from the `Ownable2Step` module.
+# @notice Please note that you must always also export (if
+# required by the contract logic) `public` declared `constant`,
+# `immutable`, and state variables, for which Vyper automatically
+# generates an `external` getter function for the variable.
+exports: (
+    o2.owner,
+    o2.pending_owner,
+    o2.transfer_ownership,
+    o2.accept_ownership,
+    o2.renounce_ownership,
+)
+
+
+@deploy
+@payable
+def __init__():
+    """
+    @dev To omit the opcodes for checking the `msg.value`
+         in the creation-time EVM bytecode, the constructor
+         is declared as `payable`.
+    @notice The `owner` role will be assigned to
+            the `msg.sender`.
+    """
+    # The following line assigns the `owner`
+    # to the `msg.sender`.
+    ow.__init__()
+    o2.__init__()
diff --git a/test/auth/Ownable2Step.t.sol b/test/auth/Ownable2Step.t.sol
@@ -17,7 +17,10 @@ contract Ownable2StepTest is Test {
 
     function setUp() public {
         ownable2Step = IOwnable2Step(
-            vyperDeployer.deployContract("src/snekmate/auth/", "Ownable2Step")
+            vyperDeployer.deployContract(
+                "src/snekmate/auth/mocks/",
+                "Ownable2StepMock"
+            )
         );
     }
 
@@ -28,7 +31,10 @@ contract Ownable2StepTest is Test {
         vm.expectEmit(true, true, false, false);
         emit IOwnable2Step.OwnershipTransferred(zeroAddress, deployer);
         ownable2StepInitialEvent = IOwnable2Step(
-            vyperDeployer.deployContract("src/snekmate/auth/", "Ownable2Step")
+            vyperDeployer.deployContract(
+                "src/snekmate/auth/mocks/",
+                "Ownable2StepMock"
+            )
         );
         assertEq(ownable2StepInitialEvent.owner(), deployer);
         assertEq(ownable2StepInitialEvent.pending_owner(), zeroAddress);
@@ -52,7 +58,7 @@ contract Ownable2StepTest is Test {
     }
 
     function testTransferOwnershipNonOwner() public {
-        vm.expectRevert(bytes("Ownable2Step: caller is not the owner"));
+        vm.expectRevert(bytes("Ownable: caller is not the owner"));
         ownable2Step.transfer_ownership(makeAddr("newOwner"));
     }
 
@@ -104,7 +110,7 @@ contract Ownable2StepTest is Test {
     }
 
     function testRenounceOwnershipNonOwner() public {
-        vm.expectRevert(bytes("Ownable2Step: caller is not the owner"));
+        vm.expectRevert(bytes("Ownable: caller is not the owner"));
         ownable2Step.renounce_ownership();
     }
 
@@ -158,7 +164,7 @@ contract Ownable2StepTest is Test {
     ) public {
         vm.assume(nonOwner != deployer);
         vm.prank(nonOwner);
-        vm.expectRevert(bytes("Ownable2Step: caller is not the owner"));
+        vm.expectRevert(bytes("Ownable: caller is not the owner"));
         ownable2Step.transfer_ownership(newOwner);
     }
 
@@ -245,7 +251,7 @@ contract Ownable2StepTest is Test {
     function testFuzzRenounceOwnershipNonOwner(address nonOwner) public {
         vm.assume(nonOwner != deployer);
         vm.prank(nonOwner);
-        vm.expectRevert(bytes("Ownable2Step: caller is not the owner"));
+        vm.expectRevert(bytes("Ownable: caller is not the owner"));
         ownable2Step.renounce_ownership();
     }
 
@@ -285,7 +291,10 @@ contract Ownable2StepInvariants is Test {
 
     function setUp() public {
         ownable2Step = IOwnable2Step(
-            vyperDeployer.deployContract("src/snekmate/auth/", "Ownable2Step")
+            vyperDeployer.deployContract(
+                "src/snekmate/auth/mocks/",
+                "Ownable2StepMock"
+            )
         );
         owner2StepHandler = new Owner2StepHandler(
             ownable2Step,
@@ -295,11 +304,11 @@ contract Ownable2StepInvariants is Test {
         targetContract(address(owner2StepHandler));
     }
 
-    function invariantOwner() public view {
+    function statefulFuzzOwner() public view {
         assertEq(ownable2Step.owner(), owner2StepHandler.owner());
     }
 
-    function invariantPendingOwner() public view {
+    function statefulFuzzPendingOwner() public view {
         assertEq(
             ownable2Step.pending_owner(),
             owner2StepHandler.pending_owner()