Feat/clarity (#81)

* cleaned up PCR

* versions

* slight refactoring

* visibility

* typo

README.md

@@ -7,7 +7,7 @@
 
 # Solidity Library for Savvies by Distributed Lab
 
-The library consists of modules and utilities that are built with a help of [Openzeppelin Contracts](https://github.com/OpenZeppelin/openzeppelin-contracts) (4.9.2) and **go far beyond mediocre solidity**.
+The library consists of modules and utilities that are built with a help of [Openzeppelin Contracts](https://github.com/OpenZeppelin/openzeppelin-contracts) (4.9.5) and **go far beyond mediocre solidity**.
 
 - Implementation of [**Contracts Registry**](https://eips.ethereum.org/EIPS/eip-6224) pattern
 - Versatile **RBAC** and **MultiOwnable** smart contracts

contracts/contracts-registry/AbstractContractsRegistry.sol

@@ -4,7 +4,7 @@ pragma solidity ^0.8.4;
 import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
 
-import {ProxyUpgrader} from "./proxy/ProxyUpgrader.sol";
+import {TransparentProxyUpgrader} from "../proxy/transparent/TransparentProxyUpgrader.sol";
 import {AbstractDependant} from "./AbstractDependant.sol";
 
 /**
@@ -36,7 +36,7 @@ import {AbstractDependant} from "./AbstractDependant.sol";
  * The management is simplified because all of the contracts are now located in a single place.
  */
 abstract contract AbstractContractsRegistry is Initializable {
-    ProxyUpgrader private _proxyUpgrader;
+    TransparentProxyUpgrader private _proxyUpgrader;
 
     mapping(string => address) private _contracts;
     mapping(address => bool) private _isProxy;
@@ -50,7 +50,7 @@ abstract contract AbstractContractsRegistry is Initializable {
      * @notice The proxy initializer function
      */
     function __ContractsRegistry_init() internal onlyInitializing {
-        _proxyUpgrader = new ProxyUpgrader();
+        _proxyUpgrader = new TransparentProxyUpgrader();
     }
 
     /**

contracts/contracts-registry/pools/AbstractPoolContractsRegistry.sol

@@ -9,7 +9,7 @@ import {Paginator} from "../../libs/arrays/Paginator.sol";
 
 import {AbstractDependant} from "../../contracts-registry/AbstractDependant.sol";
 
-import {ProxyBeacon} from "./proxy/ProxyBeacon.sol";
+import {ProxyBeacon} from "../../proxy/beacon/ProxyBeacon.sol";
 
 /**
  * @notice The PoolContractsRegistry module
@@ -48,6 +48,16 @@ abstract contract AbstractPoolContractsRegistry is Initializable, AbstractDepend
         _contractsRegistry = contractsRegistry_;
     }
 
+    /**
+     * @notice The function to add new pools into the registry. Gets called from PoolFactory
+     *
+     * Proper only factory access control must be added in descending contracts + `_addProxyPool()` should be called inside.
+     *
+     * @param name_ the pool's associated name
+     * @param poolAddress_ the proxy address of the pool
+     */
+    function addProxyPool(string memory name_, address poolAddress_) public virtual;
+
     /**
      * @notice The function to get implementation of the specific pools
      * @param name_ the name of the pools

contracts/contracts-registry/pools/pool-factory/AbstractPoolFactory.sol

@@ -6,7 +6,7 @@ import {Create2} from "@openzeppelin/contracts/utils/Create2.sol";
 import {AbstractDependant} from "../../../contracts-registry/AbstractDependant.sol";
 import {AbstractPoolContractsRegistry} from "../AbstractPoolContractsRegistry.sol";
 
-import {PublicBeaconProxy} from "./proxy/PublicBeaconProxy.sol";
+import {PublicBeaconProxy} from "../../../proxy/beacon/PublicBeaconProxy.sol";
 
 /**
  * @notice The PoolContractsRegistry module
@@ -74,11 +74,7 @@ abstract contract AbstractPoolFactory is AbstractDependant {
         string memory poolType_,
         address poolProxy_
     ) internal virtual {
-        (bool success, ) = poolRegistry_.call(
-            abi.encodeWithSignature("addProxyPool(string,address)", poolType_, poolProxy_)
-        );
-
-        require(success, "AbstractPoolFactory: failed to register contract");
+        AbstractPoolContractsRegistry(poolRegistry_).addProxyPool(poolType_, poolProxy_);
     }
 
     /**

contracts/contracts-registry/pools/presets/MultiOwnablePoolContractsRegistry.sol

@@ -39,6 +39,4 @@ abstract contract MultiOwnablePoolContractsRegistry is
     ) external onlyOwner {
         _injectDependenciesToExistingPoolsWithData(name_, data_, offset_, limit_);
     }
-
-    function addProxyPool(string memory name_, address poolAddress_) public virtual;
 }

contracts/contracts-registry/pools/presets/OwnablePoolContractsRegistry.sol

@@ -40,6 +40,4 @@ abstract contract OwnablePoolContractsRegistry is
     ) external onlyOwner {
         _injectDependenciesToExistingPoolsWithData(name_, data_, offset_, limit_);
     }
-
-    function addProxyPool(string memory name_, address poolAddress_) public virtual;
 }

contracts/package.json

@@ -1,6 +1,6 @@
 {
   "name": "@solarity/solidity-lib",
-  "version": "2.6.11",
+  "version": "2.6.12",
   "license": "MIT",
   "author": "Distributed Lab",
   "readme": "README.md",
@@ -21,8 +21,8 @@
     "!mock/**/*"
   ],
   "dependencies": {
-    "@openzeppelin/contracts": "4.9.2",
-    "@openzeppelin/contracts-upgradeable": "4.9.2",
+    "@openzeppelin/contracts": "4.9.5",
+    "@openzeppelin/contracts-upgradeable": "4.9.5",
     "@uniswap/v2-core": "1.0.1",
     "@uniswap/v2-periphery": "1.1.0-beta.0",
     "@uniswap/v3-core": "1.0.1",

contracts/contracts-registry/pools/proxy/ProxyBeacon.sol → contracts/proxy/beacon/ProxyBeacon.sol

@@ -5,9 +5,9 @@ import {IBeacon} from "@openzeppelin/contracts/proxy/beacon/IBeacon.sol";
 import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 
 /**
- * @notice The PoolContractsRegistry module
+ * @notice The proxies module
  *
- * This is a utility lightweighted ProxyBeacon contract this is used as a beacon that BeaconProxies point to.
+ * This is a lightweight utility ProxyBeacon contract that may be used as a beacon that BeaconProxies point to.
  */
 contract ProxyBeacon is IBeacon {
     using Address for address;
@@ -27,15 +27,15 @@ contract ProxyBeacon is IBeacon {
         _OWNER = msg.sender;
     }
 
-    function upgradeTo(address newImplementation_) external onlyOwner {
+    function upgradeTo(address newImplementation_) external virtual onlyOwner {
         require(newImplementation_.isContract(), "ProxyBeacon: not a contract");
 
         _implementation = newImplementation_;
 
         emit Upgraded(newImplementation_);
     }
 
-    function implementation() public view override returns (address) {
+    function implementation() public view virtual override returns (address) {
         return _implementation;
     }
 }

contracts/contracts-registry/pools/pool-factory/proxy/PublicBeaconProxy.sol → contracts/proxy/beacon/PublicBeaconProxy.sol

@@ -4,11 +4,12 @@ pragma solidity ^0.8.4;
 import {BeaconProxy} from "@openzeppelin/contracts/proxy/beacon/BeaconProxy.sol";
 
 /**
- * @notice The PoolContractsRegistry module
+ * @notice The proxies module
  *
- * The helper BeaconProxy that get deployed by the PoolFactory. Note that the external
- * `implementation()` function is added to the contract to provide compatability with the
- * Etherscan. This means that the implementation must not have such a function declared.
+ * The helper BeaconProxy that can be deployed by the factories.
+ *
+ * Note that the external `implementation()` function is added to the contract to provide compatability with
+ * Etherscan. This means that the implementation contract must not have such a function declared.
  */
 contract PublicBeaconProxy is BeaconProxy {
     constructor(address beacon_, bytes memory data_) payable BeaconProxy(beacon_, data_) {}
@@ -17,7 +18,7 @@ contract PublicBeaconProxy is BeaconProxy {
      * @notice The function that returns implementation contract this proxy points to
      * @return address the implementation address
      */
-    function implementation() external view virtual returns (address) {
+    function implementation() public view virtual returns (address) {
         return _implementation();
     }
 }

contracts/contracts-registry/proxy/ProxyUpgrader.sol → contracts/proxy/transparent/TransparentProxyUpgrader.sol

@@ -5,43 +5,38 @@ import {ITransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transp
 import {Address} from "@openzeppelin/contracts/utils/Address.sol";
 
 /**
- * @notice The ContractsRegistry module
+ * @notice The proxies module
  *
- * This is the helper contract that is used by an AbstractContractsRegistry as a proxy admin.
- * It is essential to distinguish between the admin and the registry due to the Transparent proxies nature
+ * This is the lightweight helper contract that may be used as a transparent proxy admin.
  */
-contract ProxyUpgrader {
+contract TransparentProxyUpgrader {
     using Address for address;
 
     address private immutable _OWNER;
 
     modifier onlyOwner() {
-        _onlyOwner();
+        require(_OWNER == msg.sender, "TransparentProxyUpgrader: not an owner");
         _;
     }
 
     constructor() {
         _OWNER = msg.sender;
     }
 
-    function upgrade(address what_, address to_, bytes calldata data_) external onlyOwner {
+    function upgrade(address what_, address to_, bytes calldata data_) external virtual onlyOwner {
         if (data_.length > 0) {
             ITransparentUpgradeableProxy(payable(what_)).upgradeToAndCall(to_, data_);
         } else {
             ITransparentUpgradeableProxy(payable(what_)).upgradeTo(to_);
         }
     }
 
-    function getImplementation(address what_) external view onlyOwner returns (address) {
+    function getImplementation(address what_) public view virtual returns (address) {
         // bytes4(keccak256("implementation()")) == 0x5c60da1b
         (bool success_, bytes memory returndata_) = address(what_).staticcall(hex"5c60da1b");
 
-        require(success_, "ProxyUpgrader: not a proxy");
+        require(success_, "TransparentProxyUpgrader: not a proxy");
 
         return abi.decode(returndata_, (address));
     }
-
-    function _onlyOwner() internal view {
-        require(_OWNER == msg.sender, "ProxyUpgrader: not an owner");
-    }
 }

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@solarity/solidity-lib",
-  "version": "2.6.11",
+  "version": "2.6.12",
   "license": "MIT",
   "author": "Distributed Lab",
   "description": "Solidity Library by Distributed Lab",
@@ -29,8 +29,8 @@
     "publish-to-npm": "npm run lint-fix && bash ./scripts/publish.sh"
   },
   "dependencies": {
-    "@openzeppelin/contracts": "4.9.2",
-    "@openzeppelin/contracts-upgradeable": "4.9.2",
+    "@openzeppelin/contracts": "4.9.5",
+    "@openzeppelin/contracts-upgradeable": "4.9.5",
     "@uniswap/v2-core": "1.0.1",
     "@uniswap/v2-periphery": "1.1.0-beta.0",
     "@uniswap/v3-core": "1.0.1",

test/contracts-registry/pools/pool-factory/PoolFactory.test.ts

@@ -107,7 +107,7 @@ describe("PoolFactory", () => {
         await contractsRegistry.addContract(await contractsRegistry.POOL_FACTORY_NAME(), OWNER);
         await contractsRegistry.injectDependencies(await contractsRegistry.POOL_CONTRACTS_REGISTRY_NAME());
 
-        await expect(poolFactory.deployPool()).to.be.revertedWith("AbstractPoolFactory: failed to register contract");
+        await expect(poolFactory.deployPool()).to.be.revertedWith("PoolContractsRegistry: not a factory");
       });
 
       it("should deploy several pools", async () => {

test/proxy/beacon/PublicBeaconProxy.test.ts

@@ -0,0 +1,41 @@
+import { ethers } from "hardhat";
+import { SignerWithAddress } from "@nomicfoundation/hardhat-ethers/signers";
+import { expect } from "chai";
+import { Reverter } from "@/test/helpers/reverter";
+
+import { PublicBeaconProxy, ProxyBeacon, ERC20Mock } from "@ethers-v6";
+
+describe("ProxyBeacon", () => {
+  const reverter = new Reverter();
+
+  let OWNER: SignerWithAddress;
+
+  let proxyBeacon: ProxyBeacon;
+  let token: ERC20Mock;
+
+  before("setup", async () => {
+    [OWNER] = await ethers.getSigners();
+
+    const ProxyBeacon = await ethers.getContractFactory("ProxyBeacon");
+    const ERC20Mock = await ethers.getContractFactory("ERC20Mock");
+
+    proxyBeacon = await ProxyBeacon.deploy();
+    token = await ERC20Mock.deploy("mock", "mock", 18);
+
+    await reverter.snapshot();
+  });
+
+  afterEach(reverter.revert);
+
+  describe("functions", () => {
+    it("should get implementation", async () => {
+      await proxyBeacon.upgradeTo(await token.getAddress());
+
+      const PublicBeaconProxy = await ethers.getContractFactory("PublicBeaconProxy");
+      const proxy: PublicBeaconProxy = await PublicBeaconProxy.deploy(proxyBeacon, "0x");
+
+      expect(await proxyBeacon.implementation()).to.equal(await token.getAddress());
+      expect(await proxy.implementation()).to.equal(await token.getAddress());
+    });
+  });
+});