From 27514ba678538371c3c887c881d7dedd3c065a3d Mon Sep 17 00:00:00 2001 From: Gustavo Gonzalez Date: Fri, 15 Nov 2024 07:12:05 +0700 Subject: [PATCH] feat: Ownable2Step (#352) Resolves #290 - Introduce a new Ownable2Step contract - Make `onlyOwner()` internal in Ownable and implement `MethodError` to be able to use the error in our new contract(we've already seen we will need this for others) - Propose we use nextest instead of vainilla `cargo test` in the CI. It is faster(not as relevant right now) and the output is so much nicer #### PR Checklist - [x] Unit Tests - [x] e2e Tests - [x] Documentation --------- Co-authored-by: Daniel Bigos --- .github/pull_request_template.md | 1 + .github/workflows/test.yml | 20 +- CHANGELOG.md | 6 + Cargo.lock | 17 +- Cargo.toml | 2 + contracts/src/access/mod.rs | 1 + contracts/src/access/ownable.rs | 128 +++++-- contracts/src/access/ownable_two_step.rs | 353 ++++++++++++++++++ examples/basic/script/Cargo.toml | 2 +- examples/ownable-two-step/Cargo.toml | 24 ++ examples/ownable-two-step/src/constructor.sol | 28 ++ examples/ownable-two-step/src/lib.rs | 35 ++ examples/ownable-two-step/tests/abi/mod.rs | 21 ++ .../tests/ownable_two_step.rs | 331 ++++++++++++++++ examples/ownable/Cargo.toml | 2 +- lib/e2e/README.md | 3 +- 16 files changed, 926 insertions(+), 48 deletions(-) create mode 100644 contracts/src/access/ownable_two_step.rs create mode 100644 examples/ownable-two-step/Cargo.toml create mode 100644 examples/ownable-two-step/src/constructor.sol create mode 100644 examples/ownable-two-step/src/lib.rs create mode 100644 examples/ownable-two-step/tests/abi/mod.rs create mode 100644 examples/ownable-two-step/tests/ownable_two_step.rs diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 385188f3a..3cd298a57 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -19,3 +19,4 @@ Some of the items may not apply. - [ ] Tests - [ ] Documentation +- [ ] Changelog diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 65c612399..63db2e44d 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -11,7 +11,7 @@ permissions: contents: read on: push: - branches: [ main, release/* ] + branches: [main, release/*] paths-ignore: - "**.md" - "**.adoc" @@ -32,7 +32,7 @@ jobs: matrix: # Run on stable and beta to ensure that tests won't break on the next # version of the rust toolchain. - toolchain: [ stable, beta ] + toolchain: [stable, beta] steps: - uses: actions/checkout@v4 with: @@ -44,6 +44,11 @@ jobs: toolchain: ${{ matrix.toolchain }} rustflags: "" + - name: "Install nextest" + uses: taiki-e/install-action@v2 + with: + tool: cargo-nextest + - name: Cargo generate-lockfile # Enable this ci template to run regardless of whether the lockfile is # checked in or not. @@ -52,7 +57,7 @@ jobs: # https://twitter.com/jonhoo/status/1571290371124260865 - name: Run unit tests - run: cargo test --locked --features std --all-targets + run: cargo nextest run --locked --features std --all-targets # https://github.com/rust-lang/cargo/issues/6669 - name: Run doc tests @@ -64,7 +69,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ macos-latest ] + os: [macos-latest] # Windows fails because of `stylus-proc`. # os: [macos-latest, windows-latest] steps: @@ -78,12 +83,17 @@ jobs: toolchain: stable rustflags: "" + - name: "Install nextest" + uses: taiki-e/install-action@v2 + with: + tool: cargo-nextest + - name: Cargo generate-lockfile if: hashFiles('Cargo.lock') == '' run: cargo generate-lockfile - name: Run unit tests - run: cargo test --locked --features std --all-targets + run: cargo nextest run --locked --features std --all-targets coverage: # Use llvm-cov to build and collect coverage and outputs in a format that # is compatible with codecov.io. diff --git a/CHANGELOG.md b/CHANGELOG.md index 94cc5d580..64c3524a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - ERC-1155 Multi Token Standard. #275 - `SafeErc20` Utility. #289 - Finite Fields arithmetics. #376 +- `Ownable2Step` contract. #352 +- `IOwnable` trait. #352 + +### Changed(breaking) + +- Removed `only_owner` from the public interface of `Ownable`. #352 ### Changed diff --git a/Cargo.lock b/Cargo.lock index 26a4862b2..ec00212a8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -801,7 +801,7 @@ dependencies = [ [[package]] name = "basic-example-script" -version = "0.0.0" +version = "0.1.1" dependencies = [ "alloy", "alloy-primitives", @@ -2644,7 +2644,20 @@ dependencies = [ [[package]] name = "ownable-example" -version = "0.0.0" +version = "0.1.1" +dependencies = [ + "alloy", + "alloy-primitives", + "e2e", + "eyre", + "openzeppelin-stylus", + "stylus-sdk", + "tokio", +] + +[[package]] +name = "ownable-two-step" +version = "0.1.1" dependencies = [ "alloy", "alloy-primitives", diff --git a/Cargo.toml b/Cargo.toml index 99def5a6d..f62589f1f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,6 +19,7 @@ members = [ "examples/basic/token", "examples/basic/script", "examples/ecdsa", + "examples/ownable-two-step", "examples/safe-erc20", "benches", ] @@ -38,6 +39,7 @@ default-members = [ "examples/safe-erc20", "examples/merkle-proofs", "examples/ownable", + "examples/ownable-two-step", "examples/access-control", "examples/basic/token", "examples/ecdsa", diff --git a/contracts/src/access/mod.rs b/contracts/src/access/mod.rs index fb0063d21..ec5c62350 100644 --- a/contracts/src/access/mod.rs +++ b/contracts/src/access/mod.rs @@ -1,3 +1,4 @@ //! Contracts implementing access control mechanisms. pub mod control; pub mod ownable; +pub mod ownable_two_step; diff --git a/contracts/src/access/ownable.rs b/contracts/src/access/ownable.rs index 157457fd1..50692973f 100644 --- a/contracts/src/access/ownable.rs +++ b/contracts/src/access/ownable.rs @@ -10,13 +10,18 @@ //! to the owner. use alloy_primitives::Address; use alloy_sol_types::sol; +use openzeppelin_stylus_proc::interface_id; use stylus_sdk::{ + call::MethodError, evm, msg, stylus_proc::{public, sol_storage, SolidityError}, }; sol! { /// Emitted when ownership gets transferred between accounts. + /// + /// * `previous_owner` - Address of the previous owner. + /// * `new_owner` - Address of the new owner. #[allow(missing_docs)] event OwnershipTransferred(address indexed previous_owner, address indexed new_owner); } @@ -45,6 +50,12 @@ pub enum Error { InvalidOwner(OwnableInvalidOwner), } +impl MethodError for Error { + fn encode(self) -> alloc::vec::Vec { + self.into() + } +} + sol_storage! { /// State of an `Ownable` contract. pub struct Ownable { @@ -53,32 +64,21 @@ sol_storage! { } } -#[public] -impl Ownable { - /// Returns the address of the current owner. - pub fn owner(&self) -> Address { - self._owner.get() - } +/// Interface for an [`Ownable`] contract. +#[interface_id] +pub trait IOwnable { + /// The error type associated to the trait implementation. + type Error: Into>; - /// Checks if the [`msg::sender`] is set as the owner. + /// Returns the address of the current owner. /// - /// # Errors + /// # Arguments /// - /// If called by any account other than the owner, then the error - /// [`Error::UnauthorizedAccount`] is returned. - pub fn only_owner(&self) -> Result<(), Error> { - let account = msg::sender(); - if self.owner() != account { - return Err(Error::UnauthorizedAccount( - OwnableUnauthorizedAccount { account }, - )); - } + /// * `&self` - Read access to the contract's state. + fn owner(&self) -> Address; - Ok(()) - } - - /// Transfers ownership of the contract to a new account (`new_owner`). Can - /// only be called by the current owner. + /// Transfers ownership of the contract to a new account (`new_owner`). + /// Can only be called by the current owner. /// /// # Arguments /// @@ -89,13 +89,52 @@ impl Ownable { /// /// If `new_owner` is the zero address, then the error /// [`OwnableInvalidOwner`] is returned. - pub fn transfer_ownership( + /// + /// # Events + /// + /// Emits a [`OwnershipTransferred`] event. + fn transfer_ownership( + &mut self, + new_owner: Address, + ) -> Result<(), Self::Error>; + + /// Leaves the contract without owner. It will not be possible to call + /// functions that require `only_owner`. Can only be called by the current + /// owner. + /// + /// NOTE: Renouncing ownership will leave the contract without an owner, + /// thereby disabling any functionality that is only available to the owner. + /// + /// # Arguments + /// + /// * `&mut self` - Write access to the contract's state. + /// + /// # Errors + /// + /// If not called by the owner, then the error + /// [`Error::UnauthorizedAccount`] is returned. + /// + /// # Events + /// + /// Emits a [`OwnershipTransferred`] event. + fn renounce_ownership(&mut self) -> Result<(), Self::Error>; +} + +#[public] +impl IOwnable for Ownable { + type Error = Error; + + fn owner(&self) -> Address { + self._owner.get() + } + + fn transfer_ownership( &mut self, new_owner: Address, - ) -> Result<(), Error> { + ) -> Result<(), Self::Error> { self.only_owner()?; - if new_owner == Address::ZERO { + if new_owner.is_zero() { return Err(Error::InvalidOwner(OwnableInvalidOwner { owner: Address::ZERO, })); @@ -106,31 +145,46 @@ impl Ownable { Ok(()) } - /// Leaves the contract without owner. It will not be possible to call - /// [`Self::only_owner`] functions. Can only be called by the current owner. + fn renounce_ownership(&mut self) -> Result<(), Self::Error> { + self.only_owner()?; + self._transfer_ownership(Address::ZERO); + Ok(()) + } +} + +impl Ownable { + /// Checks if the [`msg::sender`] is set as the owner. /// - /// NOTE: Renouncing ownership will leave the contract without an owner, - /// thereby disabling any functionality that is only available to the owner. + /// # Arguments + /// + /// * `&self` - Read access to the contract's state. /// /// # Errors /// - /// If not called by the owner, then the error + /// If called by any account other than the owner, then the error /// [`Error::UnauthorizedAccount`] is returned. - pub fn renounce_ownership(&mut self) -> Result<(), Error> { - self.only_owner()?; - self._transfer_ownership(Address::ZERO); + pub fn only_owner(&self) -> Result<(), Error> { + let account = msg::sender(); + if self.owner() != account { + return Err(Error::UnauthorizedAccount( + OwnableUnauthorizedAccount { account }, + )); + } + Ok(()) } -} -impl Ownable { /// Transfers ownership of the contract to a new account (`new_owner`). /// Internal function without access restriction. /// /// # Arguments /// /// * `&mut self` - Write access to the contract's state. - /// * `new_owner` - Account that's gonna be the next owner. + /// * `new_owner` - Account that is going to be the next owner. + /// + /// # Events + /// + /// Emits a [`OwnershipTransferred`] event. pub fn _transfer_ownership(&mut self, new_owner: Address) { let previous_owner = self._owner.get(); self._owner.set(new_owner); @@ -143,7 +197,7 @@ mod tests { use alloy_primitives::{address, Address}; use stylus_sdk::msg; - use super::{Error, Ownable}; + use super::{Error, IOwnable, Ownable}; const ALICE: Address = address!("A11CEacF9aa32246d767FCCD72e02d6bCbcC375d"); diff --git a/contracts/src/access/ownable_two_step.rs b/contracts/src/access/ownable_two_step.rs new file mode 100644 index 000000000..f5d9a2be1 --- /dev/null +++ b/contracts/src/access/ownable_two_step.rs @@ -0,0 +1,353 @@ +//! Contract module which provides an access control mechanism, where +//! there is an account (an owner) that can be granted exclusive access to +//! specific functions. +//! +//! This extension of the `Ownable` contract includes a two-step mechanism to +//! transfer ownership, where the new owner must call +//! [`Ownable2Step::accept_ownership`] in order to replace the old one. This can +//! help prevent common mistakes, such as transfers of ownership to +//! incorrect accounts, or to contracts that are unable to interact with the +//! permission system. +//! +//! The initial owner is set to the address provided by the deployer. This can +//! later be changed with [`Ownable2Step::transfer_ownership`] and +//! [`Ownable2Step::accept_ownership`]. +//! +//! This module uses [`Ownable`] as a member, and makes all its public functions +//! available. + +use alloy_primitives::Address; +use alloy_sol_types::sol; +use stylus_sdk::{ + evm, msg, + stylus_proc::{public, sol_storage, SolidityError}, +}; + +use crate::access::ownable::{ + Error as OwnableError, IOwnable, Ownable, OwnableUnauthorizedAccount, +}; + +sol! { + /// Emitted when ownership transfer starts. + /// + /// * `previous_owner` - Address of the previous owner. + /// * `new_owner` - Address of the new owner, to which the ownership + /// will be transferred. + event OwnershipTransferStarted( + address indexed previous_owner, + address indexed new_owner + ); + +} + +/// An error that occurred in the implementation of an [`Ownable2Step`] +/// contract. +#[derive(SolidityError, Debug)] +pub enum Error { + /// Error type from [`Ownable`] contract. + Ownable(OwnableError), +} + +sol_storage! { + /// State of an `Ownable2Step` contract. + pub struct Ownable2Step { + /// [`Ownable`] contract. + Ownable _ownable; + /// Pending owner of the contract. + address _pending_owner; + } +} + +/// Interface for an [`Ownable2Step`] contract. +pub trait IOwnable2Step { + /// The error type associated to the trait implementation. + type Error: Into>; + + /// Returns the address of the current owner. + /// + /// Re-export of [`Ownable::owner`]. + /// + /// # Arguments + /// + /// * `&self` - Read access to the contract's state. + fn owner(&self) -> Address; + + /// Returns the address of the pending owner. + /// + /// # Arguments + /// + /// * `&self` - Read access to the contract's state. + fn pending_owner(&self) -> Address; + + /// Starts the ownership transfer of the contract to a new account. + /// Replaces the pending transfer if there is one. Can only be called by the + /// current owner. + /// + /// Setting `new_owner` to `Address::ZERO` is allowed; this can be used + /// to cancel an initiated ownership transfer. + /// + /// # Arguments + /// + /// * `&mut self` - Write access to the contract's state. + /// * `new_owner` - The next owner of this contract. + /// + /// # Errors + /// + /// If called by any account other than the owner, then the error + /// [`OwnableError::UnauthorizedAccount`] is returned. + /// + /// # Events + /// + /// Emits a [`OwnershipTransferStarted`] event. + fn transfer_ownership( + &mut self, + new_owner: Address, + ) -> Result<(), Self::Error>; + + /// Accepts the ownership of the contract. + /// Can only be called by the pending owner. + /// + /// # Arguments + /// + /// * `&mut self` - Write access to the contract's state. + /// + /// # Errors + /// + /// If called by any account other than the pending owner, then the error + /// [`OwnableError::UnauthorizedAccount`] is returned. + /// + /// # Events + /// + /// Emits a [`crate::access::ownable::OwnershipTransferred`] event. + fn accept_ownership(&mut self) -> Result<(), Self::Error>; + + /// Leaves the contract without owner. It will not be possible to call + /// [`Ownable::only_owner`] functions. Can only be called by the current + /// owner. + /// + /// NOTE: Renouncing ownership will leave the contract without an owner, + /// thereby disabling any functionality that is only available to the owner. + /// + /// # Arguments + /// # Errors + /// + /// If not called by the owner, then the error + /// [`OwnableError::UnauthorizedAccount`] is returned. + /// + /// # Events + /// + /// Emits a [`crate::access::ownable::OwnershipTransferred`] event. + fn renounce_ownership(&mut self) -> Result<(), Self::Error>; +} + +#[public] +impl IOwnable2Step for Ownable2Step { + type Error = Error; + + fn owner(&self) -> Address { + self._ownable.owner() + } + + fn pending_owner(&self) -> Address { + self._pending_owner.get() + } + + fn transfer_ownership( + &mut self, + new_owner: Address, + ) -> Result<(), Self::Error> { + self._ownable.only_owner()?; + self._pending_owner.set(new_owner); + + let current_owner = self.owner(); + evm::log(OwnershipTransferStarted { + previous_owner: current_owner, + new_owner, + }); + Ok(()) + } + + fn accept_ownership(&mut self) -> Result<(), Self::Error> { + let sender = msg::sender(); + let pending_owner = self.pending_owner(); + if sender != pending_owner { + return Err(OwnableError::UnauthorizedAccount( + OwnableUnauthorizedAccount { account: sender }, + ) + .into()); + } + self._transfer_ownership(sender); + Ok(()) + } + + fn renounce_ownership(&mut self) -> Result<(), Error> { + self._ownable.only_owner()?; + self._transfer_ownership(Address::ZERO); + Ok(()) + } +} + +impl Ownable2Step { + /// Transfers ownership of the contract to a new account (`new_owner`) and + /// sets [`Self::pending_owner`] to `Address::ZERO` to avoid situations + /// where the transfer has been completed or the current owner renounces, + /// but [`Self::pending_owner`] can still accept ownership. + /// + /// Internal function without access restriction. + /// + /// # Arguments + /// + /// * `&mut self` - Write access to the contract's state. + /// * `new_owner` - Account that's gonna be the next owner. + /// + /// # Events + /// + /// Emits a [`crate::access::ownable::OwnershipTransferred`] event. + fn _transfer_ownership(&mut self, new_owner: Address) { + self._pending_owner.set(Address::ZERO); + self._ownable._transfer_ownership(new_owner); + } +} + +#[cfg(all(test, feature = "std"))] +mod tests { + use alloy_primitives::{address, Address}; + use stylus_sdk::msg; + + use super::{Error, IOwnable2Step, Ownable2Step, OwnableError}; + + const ALICE: Address = address!("A11CEacF9aa32246d767FCCD72e02d6bCbcC375d"); + const BOB: Address = address!("B0B0cB49ec2e96DF5F5fFB081acaE66A2cBBc2e2"); + + #[motsu::test] + fn reads_owner(contract: Ownable2Step) { + contract._ownable._owner.set(msg::sender()); + let owner = contract.owner(); + assert_eq!(owner, msg::sender()); + } + + #[motsu::test] + fn reads_pending_owner(contract: Ownable2Step) { + contract._pending_owner.set(ALICE); + let pending_owner = contract.pending_owner(); + assert_eq!(pending_owner, ALICE); + } + + #[motsu::test] + fn initiates_ownership_transfer(contract: Ownable2Step) { + contract._ownable._owner.set(msg::sender()); + + contract + .transfer_ownership(ALICE) + .expect("should initiate ownership transfer"); + let pending_owner = contract._pending_owner.get(); + assert_eq!(pending_owner, ALICE); + assert_eq!(contract.owner(), msg::sender()); + } + + #[motsu::test] + fn prevents_non_owners_from_initiating_transfer(contract: Ownable2Step) { + contract._ownable._owner.set(ALICE); + + let err = contract.transfer_ownership(BOB).unwrap_err(); + assert!(matches!( + err, + Error::Ownable(OwnableError::UnauthorizedAccount(_)) + )); + } + + #[motsu::test] + fn accepts_ownership(contract: Ownable2Step) { + contract._ownable._owner.set(ALICE); + contract._pending_owner.set(msg::sender()); + + contract.accept_ownership().expect("should accept ownership"); + assert_eq!(contract.owner(), msg::sender()); + assert_eq!(contract.pending_owner(), Address::ZERO); + } + + #[motsu::test] + fn prevents_non_pending_owner_from_accepting(contract: Ownable2Step) { + contract._ownable._owner.set(ALICE); + contract._pending_owner.set(BOB); + + let err = contract.accept_ownership().unwrap_err(); + assert!(matches!( + err, + Error::Ownable(OwnableError::UnauthorizedAccount(_)) + )); + } + + #[motsu::test] + fn completes_two_step_ownership_transfer(contract: Ownable2Step) { + contract._ownable._owner.set(msg::sender()); + + contract + .transfer_ownership(ALICE) + .expect("should initiate ownership transfer"); + assert_eq!(contract.pending_owner(), ALICE); + + // Simulate ALICE accepting ownership, since we cannot set `msg::sender` + // in tests yet. + contract._pending_owner.set(msg::sender()); + contract.accept_ownership().expect("should accept ownership"); + + assert_eq!(contract.owner(), msg::sender()); + assert_eq!(contract.pending_owner(), Address::ZERO); + } + + #[motsu::test] + fn renounces_ownership(contract: Ownable2Step) { + contract._ownable._owner.set(msg::sender()); + + contract.renounce_ownership().expect("should renounce ownership"); + assert_eq!(contract.owner(), Address::ZERO); + } + + #[motsu::test] + fn prevents_non_owners_from_renouncing(contract: Ownable2Step) { + contract._ownable._owner.set(ALICE); + + let err = contract.renounce_ownership().unwrap_err(); + assert!(matches!( + err, + Error::Ownable(OwnableError::UnauthorizedAccount(_)) + )); + } + + #[motsu::test] + fn cancels_transfer_on_renounce(contract: Ownable2Step) { + contract._ownable._owner.set(msg::sender()); + contract._pending_owner.set(ALICE); + + contract.renounce_ownership().expect("should renounce ownership"); + assert_eq!(contract.owner(), Address::ZERO); + assert_eq!(contract.pending_owner(), Address::ZERO); + } + + #[motsu::test] + fn allows_owner_to_cancel_transfer(contract: Ownable2Step) { + contract._ownable._owner.set(msg::sender()); + contract._pending_owner.set(ALICE); + + contract + .transfer_ownership(Address::ZERO) + .expect("should cancel transfer"); + assert_eq!(contract.pending_owner(), Address::ZERO); + assert_eq!(contract.owner(), msg::sender()); + } + + #[motsu::test] + fn allows_owner_to_overwrite_transfer(contract: Ownable2Step) { + contract._ownable._owner.set(msg::sender()); + + contract + .transfer_ownership(ALICE) + .expect("should initiate ownership transfer"); + assert_eq!(contract.pending_owner(), ALICE); + + contract.transfer_ownership(BOB).expect("should overwrite transfer"); + assert_eq!(contract.pending_owner(), BOB); + assert_eq!(contract.owner(), msg::sender()); + } +} diff --git a/examples/basic/script/Cargo.toml b/examples/basic/script/Cargo.toml index 55a9d05d3..7b81d8e91 100644 --- a/examples/basic/script/Cargo.toml +++ b/examples/basic/script/Cargo.toml @@ -4,7 +4,7 @@ edition.workspace = true license.workspace = true repository.workspace = true publish = false -version = "0.0.0" +version.workspace = true [dependencies] basic-example = { path = "../token" } diff --git a/examples/ownable-two-step/Cargo.toml b/examples/ownable-two-step/Cargo.toml new file mode 100644 index 000000000..172c38d8b --- /dev/null +++ b/examples/ownable-two-step/Cargo.toml @@ -0,0 +1,24 @@ +[package] +name = "ownable-two-step" +edition.workspace = true +license.workspace = true +repository.workspace = true +publish = false +version.workspace = true + +[dependencies] +openzeppelin-stylus.workspace = true +alloy-primitives.workspace = true +stylus-sdk.workspace = true + +[dev-dependencies] +alloy.workspace = true +e2e.workspace = true +tokio.workspace = true +eyre.workspace = true + +[lib] +crate-type = ["lib", "cdylib"] + +[features] +e2e = [] diff --git a/examples/ownable-two-step/src/constructor.sol b/examples/ownable-two-step/src/constructor.sol new file mode 100644 index 000000000..15ad2af6f --- /dev/null +++ b/examples/ownable-two-step/src/constructor.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.21; + +contract Ownable2StepExample { + mapping(address => uint256) _balances; + mapping(address => mapping(address => uint256)) _allowances; + uint256 _totalSupply; + + address private _owner; + address private _pendingOwner; + + error OwnableInvalidOwner(address owner); + + event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); + + constructor(address initialOwner) { + if (initialOwner == address(0)) { + revert OwnableInvalidOwner(address(0)); + } + _transferOwnership(initialOwner); + } + + function _transferOwnership(address newOwner) internal virtual { + address oldOwner = _owner; + _owner = newOwner; + emit OwnershipTransferred(oldOwner, newOwner); + } +} diff --git a/examples/ownable-two-step/src/lib.rs b/examples/ownable-two-step/src/lib.rs new file mode 100644 index 000000000..11d9583ea --- /dev/null +++ b/examples/ownable-two-step/src/lib.rs @@ -0,0 +1,35 @@ +#![cfg_attr(not(test), no_main)] +extern crate alloc; + +use alloc::vec::Vec; + +use alloy_primitives::{Address, U256}; +use openzeppelin_stylus::{ + access::ownable_two_step::Ownable2Step, + token::erc20::{Erc20, IErc20}, +}; +use stylus_sdk::prelude::{entrypoint, public, sol_storage}; + +sol_storage! { + #[entrypoint] + struct Ownable2StepExample { + #[borrow] + Erc20 erc20; + #[borrow] + Ownable2Step ownable; + } +} + +#[public] +#[inherit(Erc20, Ownable2Step)] +impl Ownable2StepExample { + pub fn transfer( + &mut self, + to: Address, + value: U256, + ) -> Result<(), Vec> { + self.ownable._ownable.only_owner()?; + self.erc20.transfer(to, value)?; + Ok(()) + } +} diff --git a/examples/ownable-two-step/tests/abi/mod.rs b/examples/ownable-two-step/tests/abi/mod.rs new file mode 100644 index 000000000..ad958ef02 --- /dev/null +++ b/examples/ownable-two-step/tests/abi/mod.rs @@ -0,0 +1,21 @@ +#![allow(dead_code)] +use alloy::sol; + +sol!( + #[sol(rpc)] + contract Ownable2Step { + error OwnableUnauthorizedAccount(address account); + error OwnableInvalidOwner(address owner); + + #[derive(Debug, PartialEq)] + event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); + #[derive(Debug, PartialEq)] + event OwnershipTransferStarted(address indexed previousOwner, address indexed newOwner); + + function owner() public view virtual returns (address owner); + function pendingOwner() public view returns (address pendingOwner); + function renounceOwnership() public virtual onlyOwner; + function transferOwnership(address newOwner) public virtual; + function acceptOwnership() public virtual; + } +); diff --git a/examples/ownable-two-step/tests/ownable_two_step.rs b/examples/ownable-two-step/tests/ownable_two_step.rs new file mode 100644 index 000000000..0e2216231 --- /dev/null +++ b/examples/ownable-two-step/tests/ownable_two_step.rs @@ -0,0 +1,331 @@ +#![cfg(feature = "e2e")] + +use abi::{ + Ownable2Step, + Ownable2Step::{OwnershipTransferStarted, OwnershipTransferred}, +}; +use alloy::{primitives::Address, sol}; +use e2e::{receipt, send, Account, EventExt, ReceiptExt, Revert}; +use eyre::Result; + +use crate::Ownable2StepExample::constructorCall; + +mod abi; + +sol!("src/constructor.sol"); + +fn ctr(owner: Address) -> constructorCall { + constructorCall { initialOwner: owner } +} + +// ============================================================================ +// Integration Tests: Ownable2Step +// ============================================================================ + +#[e2e::test] +async fn constructs(alice: Account) -> Result<()> { + let alice_addr = alice.address(); + let receipt = + alice.as_deployer().with_constructor(ctr(alice_addr)).deploy().await?; + let contract = Ownable2Step::new(receipt.address()?, &alice.wallet); + + assert!(receipt.emits(OwnershipTransferred { + previousOwner: Address::ZERO, + newOwner: alice_addr, + })); + + let Ownable2Step::ownerReturn { owner } = contract.owner().call().await?; + assert_eq!(owner, alice_addr); + + let Ownable2Step::pendingOwnerReturn { pendingOwner } = + contract.pendingOwner().call().await?; + assert_eq!(pendingOwner, Address::ZERO); + + Ok(()) +} + +#[e2e::test] +async fn construct_reverts_when_owner_is_zero_address( + alice: Account, +) -> Result<()> { + let err = alice + .as_deployer() + .with_constructor(ctr(Address::ZERO)) + .deploy() + .await + .expect_err("should not deploy due to `OwnableInvalidOwner`"); + + assert!(err.reverted_with(Ownable2Step::OwnableInvalidOwner { + owner: Address::ZERO + })); + + Ok(()) +} + +#[e2e::test] +async fn transfer_ownership_initiates_transfer( + alice: Account, + bob: Account, +) -> Result<()> { + let alice_addr = alice.address(); + let bob_addr = bob.address(); + + let contract_addr = alice + .as_deployer() + .with_constructor(ctr(alice_addr)) + .deploy() + .await? + .address()?; + let contract = Ownable2Step::new(contract_addr, &alice.wallet); + + let receipt = receipt!(contract.transferOwnership(bob_addr))?; + assert!(receipt.emits(OwnershipTransferStarted { + previousOwner: alice_addr, + newOwner: bob_addr, + })); + + // Current owner is still Alice + let Ownable2Step::ownerReturn { owner } = contract.owner().call().await?; + assert_eq!(owner, alice_addr); + + // Pending owner is Bob + let Ownable2Step::pendingOwnerReturn { pendingOwner } = + contract.pendingOwner().call().await?; + assert_eq!(pendingOwner, bob_addr); + + Ok(()) +} + +#[e2e::test] +async fn transfer_ownership_reverts_when_not_owner( + alice: Account, + bob: Account, +) -> Result<()> { + let alice_addr = alice.address(); + let bob_addr = bob.address(); + + let contract_addr = alice + .as_deployer() + .with_constructor(ctr(bob_addr)) + .deploy() + .await? + .address()?; + let contract = Ownable2Step::new(contract_addr, &alice.wallet); + + let err = send!(contract.transferOwnership(bob_addr)) + .expect_err("should not transfer when not owned"); + err.reverted_with(Ownable2Step::OwnableUnauthorizedAccount { + account: alice_addr, + }); + + Ok(()) +} + +#[e2e::test] +async fn accept_ownership(alice: Account, bob: Account) -> Result<()> { + let alice_addr = alice.address(); + let bob_addr = bob.address(); + + let contract_addr = alice + .as_deployer() + .with_constructor(ctr(alice_addr)) + .deploy() + .await? + .address()?; + let contract = Ownable2Step::new(contract_addr, &alice.wallet); + receipt!(contract.transferOwnership(bob_addr))?; + + // Connect as Bob and accept ownership + let contract = Ownable2Step::new(contract_addr, &bob.wallet); + let receipt = receipt!(contract.acceptOwnership())?; + assert!(receipt.emits(OwnershipTransferred { + previousOwner: alice_addr, + newOwner: bob_addr, + })); + + let Ownable2Step::ownerReturn { owner } = contract.owner().call().await?; + assert_eq!(owner, bob_addr); + + let Ownable2Step::pendingOwnerReturn { pendingOwner } = + contract.pendingOwner().call().await?; + assert_eq!(pendingOwner, Address::ZERO); + + Ok(()) +} + +#[e2e::test] +async fn transfer_ownership_cancel_transfer( + alice: Account, + bob: Account, +) -> Result<()> { + let alice_addr = alice.address(); + let bob_addr = bob.address(); + + let contract_addr = alice + .as_deployer() + .with_constructor(ctr(alice_addr)) + .deploy() + .await? + .address()?; + let contract = Ownable2Step::new(contract_addr, &alice.wallet); + receipt!(contract.transferOwnership(bob_addr))?; + + let receipt = receipt!(contract.transferOwnership(Address::ZERO))?; + assert!(receipt.emits(OwnershipTransferStarted { + previousOwner: alice_addr, + newOwner: Address::ZERO, + })); + + let Ownable2Step::pendingOwnerReturn { pendingOwner } = + contract.pendingOwner().call().await?; + assert_eq!(pendingOwner, Address::ZERO); + + Ok(()) +} + +#[e2e::test] +async fn overwrite_previous_transfer_ownership( + alice: Account, + bob: Account, + charlie: Account, +) -> Result<()> { + let alice_addr = alice.address(); + let bob_addr = bob.address(); + let charlie_addr = charlie.address(); + + let contract_addr = alice + .as_deployer() + .with_constructor(ctr(alice_addr)) + .deploy() + .await? + .address()?; + + let contract = Ownable2Step::new(contract_addr, &alice.wallet); + + let receipt = receipt!(contract.transferOwnership(bob_addr))?; + assert!(receipt.emits(OwnershipTransferStarted { + previousOwner: alice_addr, + newOwner: bob_addr, + })); + + let receipt = receipt!(contract.transferOwnership(charlie_addr))?; + assert!(receipt.emits(OwnershipTransferStarted { + previousOwner: alice_addr, + newOwner: charlie_addr, + })); + + let Ownable2Step::pendingOwnerReturn { pendingOwner } = + contract.pendingOwner().call().await?; + assert_eq!(pendingOwner, charlie_addr); + + // Connect as Bob and try to accept ownership + let contract = Ownable2Step::new(contract_addr, &bob.wallet); + let err = send!(contract.acceptOwnership()) + .expect_err("should not accept when not pending owner"); + + err.reverted_with(Ownable2Step::OwnableUnauthorizedAccount { + account: bob_addr, + }); + + // Connect as Charlie and accept ownership + let contract = Ownable2Step::new(contract_addr, &charlie.wallet); + let receipt = receipt!(contract.acceptOwnership())?; + assert!(receipt.emits(OwnershipTransferred { + previousOwner: alice_addr, + newOwner: charlie_addr, + })); + + let Ownable2Step::pendingOwnerReturn { pendingOwner } = + contract.pendingOwner().call().await?; + assert_eq!(pendingOwner, Address::ZERO); + + let Ownable2Step::ownerReturn { owner } = contract.owner().call().await?; + assert_eq!(owner, charlie_addr); + + Ok(()) +} + +#[e2e::test] +async fn accept_ownership_reverts_when_not_pending_owner( + alice: Account, + bob: Account, + charlie: Account, +) -> Result<()> { + let alice_addr = alice.address(); + let bob_addr = bob.address(); + let charlie_addr = charlie.address(); + let contract_addr = alice + .as_deployer() + .with_constructor(ctr(alice_addr)) + .deploy() + .await? + .address()?; + let contract = Ownable2Step::new(contract_addr, &alice.wallet); + receipt!(contract.transferOwnership(bob_addr))?; + + // Connect as Charlie and attempt to accept ownership + let contract = Ownable2Step::new(contract_addr, &charlie.wallet); + let err = send!(contract.acceptOwnership()) + .expect_err("should not accept when not pending owner"); + err.reverted_with(Ownable2Step::OwnableUnauthorizedAccount { + account: charlie_addr, + }); + + Ok(()) +} + +#[e2e::test] +async fn renounce_ownership(alice: Account) -> Result<()> { + let alice_addr = alice.address(); + let contract_addr = alice + .as_deployer() + .with_constructor(ctr(alice_addr)) + .deploy() + .await? + .address()?; + let contract = Ownable2Step::new(contract_addr, &alice.wallet); + + let receipt = receipt!(contract.renounceOwnership())?; + assert!(receipt.emits(OwnershipTransferred { + previousOwner: alice_addr, + newOwner: Address::ZERO, + })); + + let Ownable2Step::ownerReturn { owner } = contract.owner().call().await?; + assert_eq!(owner, Address::ZERO); + + // Pending owner is set to zero + let Ownable2Step::pendingOwnerReturn { pendingOwner } = + contract.pendingOwner().call().await?; + assert_eq!(pendingOwner, Address::ZERO); + + Ok(()) +} + +#[e2e::test] +async fn renounce_ownership_reverts_when_not_owner( + alice: Account, + bob: Account, +) -> Result<()> { + let alice_addr = alice.address(); + let bob_addr = bob.address(); + + let contract_addr = alice + .as_deployer() + .with_constructor(ctr(alice_addr)) + .deploy() + .await? + .address()?; + let contract = Ownable2Step::new(contract_addr, &bob.wallet); + + let err = send!(contract.renounceOwnership()) + .expect_err("should prevent non-owner from renouncing"); + err.reverted_with(Ownable2Step::OwnableUnauthorizedAccount { + account: bob_addr, + }); + + let Ownable2Step::ownerReturn { owner } = contract.owner().call().await?; + assert_eq!(owner, alice_addr); + + Ok(()) +} diff --git a/examples/ownable/Cargo.toml b/examples/ownable/Cargo.toml index 3234d5fd0..03409625e 100644 --- a/examples/ownable/Cargo.toml +++ b/examples/ownable/Cargo.toml @@ -4,7 +4,7 @@ edition.workspace = true license.workspace = true repository.workspace = true publish = false -version = "0.0.0" +version.workspace = true [dependencies] openzeppelin-stylus.workspace = true diff --git a/lib/e2e/README.md b/lib/e2e/README.md index dbd5c850f..79a222399 100644 --- a/lib/e2e/README.md +++ b/lib/e2e/README.md @@ -31,7 +31,7 @@ async fn accounts_are_funded(alice: Account) -> eyre::Result<()> { } ``` -A `Account` is a thin wrapper over a [`PrivateKeySigner`] and an `alloy` provider with a +An `Account` is a thin wrapper over a [`PrivateKeySigner`] and an `alloy` provider with a [`WalletFiller`]. Both of them are connected to the RPC endpoint defined by the `RPC_URL` environment variable. This means that a `Account` is the main proxy between the RPC and the test code. @@ -47,7 +47,6 @@ async fn foo(alice: Account, bob: Account) -> eyre::Result<()> { } ``` -[`LocalWallet`]: https://github.com/alloy-rs/alloy/blob/8aa54828c025a99bbe7e2d4fc9768605d172cc6d/crates/signer-local/src/lib.rs#L37 [`WalletFiller`]: https://github.com/alloy-rs/alloy/blob/8aa54828c025a99bbe7e2d4fc9768605d172cc6d/crates/provider/src/fillers/wallet.rs#L30