Fix generation of NPM package including the JS

.github/workflows/tests.yaml

@@ -15,11 +15,16 @@ jobs:
           node-version: "22"
           cache: "npm"
       - run: npm ci
-      - run: node scripts/generateAMPSkips.js 1 5 10 24
+      - run: node scripts/generateAMPSkips.js 1 5 10 24 40
       - run: npx hardhat compile
       - run: npx hardhat size-contracts
       - run: npm run solhint
       - run: npx hardhat test
         env:
           REPORT_GAS: "1"
+      - name: Clean to avoid sol-coverage error with AccessManagedProxyS40
+        run: |
+          npx hardhat clean
+          rm contracts/amps/*.sol
+          node scripts/generateAMPSkips.js 1 5 10 24
       - run: npx hardhat coverage

.solcover.js

@@ -4,5 +4,9 @@ module.exports = {
   //   instrumentedDir: path.join(process.cwd(), "instrumented"),
   //   client: client,
   skipFiles: ["dependencies/", "mock/"],
+  mocha: {
+    grep: "AccessManagedProxyS40", // Find everything with this tag
+    invert: true, // Run the grep's inverse set.
+  },
   //   logger: console,
 };

README.md

@@ -3,8 +3,44 @@
 This repository contains the smart contract AccessManagedProxy, a proxy with built-in access control, delegating the
 permissions to an OpenZeppelin's 5.x AccessManager.
 
-See https://forum.openzeppelin.com/t/accessmanagedproxy-is-a-good-idea/41917/3 for a discussion on the advantages of
-using it.
+## 🧐 Motivation
+
+OZ 5.x introduces the AccessManager contract and AccessManaged base contract to move the access control decisions
+from code to configuration. This is a step forward compared to the 4.x AccessControl authorization framework, where
+you had to make decisions in the contract mapping methods to role names.
+
+But the AccessManaged approach falls short, because you still have to make the decision (at coding time) of which
+methods to decorate with the restricted modifier.
+
+Implementing the access control with a modifier in the methods affects the observability of the access control
+configuration and its formal verification.
+
+It also increases the test effort and makes it harder to achieve 100% branch coverage in the contracts. Finally,
+this has an effect on the contract size of the implementation contracts.
+
+The widespread use of proxy contracts (mainly for upgradeable contracts) gives us the opportunity to move the
+implementation of the access control delegation logic to the AccessManager contract (that is, in the end, what the
+restricted modifier does) to the proxy contract.
+
+In this way, BEFORE doing the delegatecall to the implementation contract, we will check if the call is enabled by
+calling ACCESS_MANAGER.canCall(msg.sender, address(this), selector).
+
+For gas-optimization or other reasons, we can define a list of methods (probably the views) that will be excluded
+from calling the AccessManager, reducing the overhead for non-restricted method calls to the minimum.
+
+Another advantage of this approach is the run-time observability of the access control configuration, by checking if
+a given method is included in those that skip the access control, or otherwise, we will check the access manager
+configuration for that method.
+
+More details on the motivation of this idea here: https://forum.openzeppelin.com/t/accessmanagedproxy-is-a-good-idea/41917
+
+## 📝 Details
+
+The package includes the AccessManagedProxy contract, and AccessManagedProxyS1 to AccessManagedProxyS24 that are
+versions that accept from 1 to 24 methods (selectors) that will be skipped of the access manager check, to reduce gas
+usage or to access immutability on some methods.
+
+## Development
 
 Try running some of the following tasks:
 

contracts/AMPUtils.sol

@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import {AccessManagedProxy} from "./AccessManagedProxy.sol";
+
+/**
+ * @title AMPUtils
+ * @dev Utility functions for doing custom access control rules, for contracts deployed
+ *      with AccessManagedProxy
+ * @author Ensuro
+ */
+library AMPUtils {
+  // Error copied from IAccessManaged
+  error AccessManagedUnauthorized(address caller);
+
+  /**
+   * @dev Checks if the user can call a particular selector, assuming the calling contract was deployed as an AMP.
+   *
+   * @param user The user for which you want to check the access, typically msg.sender
+   * @param selector The selector of the method called (or a fake selector generated with makeSelector or another way)
+   */
+  function checkCanCall(address user, bytes4 selector) internal view {
+    (bool immediate, ) = AccessManagedProxy(payable(address(this))).ACCESS_MANAGER().canCall(
+      user,
+      address(this),
+      selector
+    );
+    require(immediate, AccessManagedUnauthorized(user));
+  }
+
+  /**
+   * @dev Standard way of creating "fake selectors" (not necessarily tied to a method call) for specific permissions
+   */
+  function makeSelector(bytes memory something) internal pure returns (bytes4) {
+    return bytes4(keccak256(something));
+  }
+}

js/deployProxy.js

@@ -44,14 +44,32 @@ async function deployAMPProxy(contractFactory, initializeArgs = [], opts = {}) {
     deployFunction = async (hre_, opts, factory, ...args) => ozUpgradesDeploy(hre_, opts, factory, ...args, acMgr);
   }
 
-  return hre.upgrades.deployProxy(contractFactory, [], {
+  return hre.upgrades.deployProxy(contractFactory, initializeArgs, {
     ...opts,
     kind: "uups",
     proxyFactory,
     deployFunction,
   });
 }
 
+async function attachAsAMP(contract, ampContractFactory = undefined) {
+  ampContractFactory = ampContractFactory || (await ethers.getContractFactory("AccessManagedProxyS1"));
+  return ampContractFactory.attach(contract);
+}
+
+async function getAccessManager(contract, ampContractFactory = undefined, accessManagerFactory="AccessManager") {
+  const contractAsAMP = await attachAsAMP(contract, ampContractFactory);
+  return ethers.getContractAt(accessManagerFactory, await contractAsAMP.ACCESS_MANAGER());
+}
+
+function makeSelector(role) {
+  return ethers.keccak256(ethers.toUtf8Bytes(role)).slice(0, 10);
+}
+
+
 module.exports = {
   deployAMPProxy,
+  attachAsAMP,
+  getAccessManager,
+  makeSelector,
 };

scripts/make-npm-package.sh

@@ -19,13 +19,13 @@ rm -fr $TARGET_DIR 2>/dev/null
 
 mkdir -p $TARGET_DIR
 
-# Generate 1 to 24 skip method contracts
-node scripts/generateAMPSkips.js $(seq -s" " 1 24)
+# Generate 1 to 40 skip method contracts
+node scripts/generateAMPSkips.js $(seq -s" " 1 40)
 
 npx hardhat clean
 env COMPILE_MODE=production npx hardhat compile
 
-git archive --format tar HEAD README.md contracts/ | tar xv -C $TARGET_DIR
+git archive --format tar HEAD README.md contracts/ js/ | tar xv -C $TARGET_DIR
 cp -r contracts/amps $TARGET_DIR/contracts
 # rm -fR $TARGET_DIR/contracts/mocks/
 

test/test-access-managed-proxy.js

@@ -4,8 +4,7 @@ const hre = require("hardhat");
 const { ethers } = hre;
 const { tagitVariant, setupAMRole } = require("@ensuro/utils/js/utils");
 const helpers = require("@nomicfoundation/hardhat-network-helpers");
-const { deployAMPProxy } = require("../js/deployProxy");
-const { deploy: ozUpgradesDeploy } = require("@openzeppelin/hardhat-upgrades/dist/utils");
+const { deployAMPProxy, attachAsAMP } = require("../js/deployProxy");
 
 async function setUpCommon() {
   const [admin, anon] = await ethers.getSigners();
@@ -118,6 +117,13 @@ const variants = [
     hasAC: true,
     hasSkippedMethod: true,
   },
+  {
+    name: "AccessManagedProxyS40",
+    fixture: async () => setUpAMPSkip(40),
+    method: "callThruAMPNonSkippedMethod",
+    hasAC: true,
+    hasSkippedMethod: true,
+  },
   {
     name: "AccessManagedProxyS1",
     fixture: async () => setUpAMPSkip(1),
@@ -206,7 +212,7 @@ variants.forEach((variant) => {
         expect(await dummy.connect(anon).viewMethod()).to.equal(anon);
         expect(await dummy.connect(anon).pureMethod()).to.equal(123456n);
       } else {
-        const dummyAsAMP = AccessManagedProxy.attach(dummy);
+        const dummyAsAMP = await attachAsAMP(dummy);
         await expect(dummy.connect(anon).viewMethod())
           .to.be.revertedWithCustomError(dummyAsAMP, "AccessManagedUnauthorized")
           .withArgs(anon);