diff --git a/packages/contracts/scripts/e2e-validate.ts b/packages/contracts/scripts/e2e-validate.ts index eeea0e9..8d3e1e6 100644 --- a/packages/contracts/scripts/e2e-validate.ts +++ b/packages/contracts/scripts/e2e-validate.ts @@ -437,48 +437,60 @@ async function main() { console.log(" ✓ adminship rotated + last-admin guard enforced"); // ════════════════════════════════════════════════════════════════════ - // ⑫b 2-step proxyAdmin transfer (versioned proxy level). - // deployer is still proxyAdmin here — removing from whitelistedAdmins - // in ⑫ doesn't affect the versioned-proxy admin slot. Transfer it to - // userA (already the sole whitelistedAdmin after ⑫) so one address - // controls both surfaces going forward. + // ⑫b proxyAdmin whitelist rotation (versioned proxy level). + // deployer is still the sole proxyAdmin here — removing it from + // whitelistedAdmins in ⑫ doesn't affect the versioned-proxy admin + // list. Grant userA, then revoke deployer so one address controls + // both surfaces going forward. // ════════════════════════════════════════════════════════════════════ - console.log("\n⑫b 2-step proxyAdmin transfer: deployer → userA …"); + console.log("\n⑫b proxyAdmin whitelist rotation: deployer → userA …"); assertEq( - (await versioned.proxyAdmin()).toLowerCase(), - deployer.address.toLowerCase(), - "proxyAdmin still = deployer (whitelist revoke didn't touch versioned admin)", + await versioned.isProxyAdmin(deployer.address), + true, + "deployer still proxyAdmin (whitelist revoke didn't touch versioned admin)", ); - await (await versioned.connect(deployer).transferProxyAdmin(userA.address)).wait(); + await (await versioned.connect(deployer).setProxyAdmin(userA.address, true)).wait(); assertEq( - (await versioned.pendingProxyAdmin()).toLowerCase(), - userA.address.toLowerCase(), - "pendingProxyAdmin = userA", + await versioned.isProxyAdmin(userA.address), + true, + "userA granted proxyAdmin", + ); + assertEq( + await versioned.proxyAdminCount(), + 2n, + "proxyAdminCount = 2 after grant", ); - // Guard: only the pending address can accept + // Guard: non-admin cannot mutate the list await expectRevertWithName( - () => versioned.connect(nonAdmin).acceptProxyAdmin(), - "VersionedFeeProxy_NotPendingProxyAdmin", - "non-pending accept blocked", + () => versioned.connect(nonAdmin).setProxyAdmin(nonAdmin.address, true), + "VersionedFeeProxy_NotProxyAdmin", + "non-admin setProxyAdmin blocked", ); - await (await versioned.connect(userA).acceptProxyAdmin()).wait(); + // Revoke deployer — userA becomes the sole admin + await (await versioned.connect(deployer).setProxyAdmin(deployer.address, false)).wait(); assertEq( - (await versioned.proxyAdmin()).toLowerCase(), - userA.address.toLowerCase(), - "proxyAdmin now = userA", + await versioned.isProxyAdmin(deployer.address), + false, + "deployer revoked", ); assertEq( - await versioned.pendingProxyAdmin(), - ethers.ZeroAddress, - "pendingProxyAdmin cleared", + await versioned.proxyAdminCount(), + 1n, + "proxyAdminCount = 1 after revoke", ); // Old proxyAdmin locked out await expectRevertWithName( - () => versioned.connect(deployer).transferProxyAdmin(deployer.address), + () => versioned.connect(deployer).setProxyAdmin(deployer.address, true), "VersionedFeeProxy_NotProxyAdmin", "old proxyAdmin locked out", ); - console.log(" ✓ proxyAdmin rotated via 2-step, old admin locked out"); + // Last-admin guard — userA cannot self-revoke + await expectRevertWithName( + () => versioned.connect(userA).setProxyAdmin(userA.address, false), + "VersionedFeeProxy_LastProxyAdmin", + "last proxyAdmin cannot self-revoke", + ); + console.log(" ✓ proxyAdmin rotated via whitelist, last-admin guard enforced"); // ════════════════════════════════════════════════════════════════════ // ⑬ Final withdrawAll by userA (now sole admin) diff --git a/packages/contracts/src/IntuitionFeeProxyFactory.sol b/packages/contracts/src/IntuitionFeeProxyFactory.sol index a94b380..f4e2a40 100644 --- a/packages/contracts/src/IntuitionFeeProxyFactory.sol +++ b/packages/contracts/src/IntuitionFeeProxyFactory.sol @@ -212,10 +212,13 @@ contract IntuitionFeeProxyFactory is (ethMultiVault, depositFixedFee, depositPercentageFee, initialAdmins) ); - address proxyAdmin_ = initialAdmins.length > 0 ? initialAdmins[0] : address(0); - + // Pass the full initialAdmins array to the proxy's Role 1 whitelist. + // Both Role 1 (proxyAdmins, upgrade authority) and Role 2 + // (whitelistedAdmins, fees) start from the same set — admins can + // diverge the two later via setProxyAdmin / setWhitelistedAdmin + // if their team / safe layout needs it. proxy = address( - new IntuitionVersionedFeeProxy(proxyAdmin_, version, impl, initData, name) + new IntuitionVersionedFeeProxy(initialAdmins, version, impl, initData, name) ); proxiesByDeployer[msg.sender].push(proxy); diff --git a/packages/contracts/src/IntuitionVersionedFeeProxy.sol b/packages/contracts/src/IntuitionVersionedFeeProxy.sol index fcfdeee..f61aa45 100644 --- a/packages/contracts/src/IntuitionVersionedFeeProxy.sol +++ b/packages/contracts/src/IntuitionVersionedFeeProxy.sol @@ -16,9 +16,11 @@ import {Errors} from "./libraries/Errors.sol"; /// with the logic implementation's regular storage. /// - No `receive()`: direct ETH transfers revert. All legitimate fee flows /// carry calldata, so a bare transfer would only be a mis-send. -/// - Admin gating is a single `proxyAdmin` address; users should point it at a -/// multisig. Transferable via `transferProxyAdmin`. -/// - ⚠️ **`name` is admin-controlled metadata, NOT a trust anchor.** The +/// - Admin gating is a **whitelist of proxyAdmins** (any of them can act). +/// Grant / revoke via `setProxyAdmin`. The last remaining admin cannot +/// self-revoke (a runtime guard prevents accidental lock-out). Recommended +/// setup: at least one Gnosis Safe in the list. +/// - ⚠️ **`name` is admin-controlled metadata, NOT a trust anchor.** Any /// proxy-admin can rename the proxy at any time — including to mimic a /// known brand. Consumers MUST derive identity / "official" status from /// the proxy address itself (e.g. the Factory's `isProxyFromFactory` @@ -52,14 +54,18 @@ contract IntuitionVersionedFeeProxy is IIntuitionVersionedFeeProxy { bytes32[] versionList; mapping(bytes32 => address) implementations; mapping(bytes32 => bool) versionExists; - address proxyAdmin; + // Multi-admin whitelist for Role 1 (upgrade authority). Any admin in + // the mapping can register versions, set default, rename, and grant / + // revoke other proxyAdmins. Mirrors the Role 2 (fee-admin) shape so + // both rotation flows feel symmetric. The pre-rotation single-slot + // 2-step model (proxyAdmin + pendingProxyAdmin) was dropped — the + // safety it provided is delivered by the recommendation to put a + // Safe multisig in the list, which gives N-of-M signing semantics. + mapping(address => bool) proxyAdmins; + // Count tracked alongside the mapping so the last-admin guard is O(1). + // Replaces the 2-step pending-admin pattern. + uint256 proxyAdminCount; bytes32 name; - // 2-step admin transfer: `pendingProxyAdmin` holds the candidate set by - // the current admin via `transferProxyAdmin`. Only that address can - // promote itself via `acceptProxyAdmin`. Prevents fat-fingered - // transfers to lost / wrong addresses. Appended — ERC-7201 namespaced - // slot mask (~0xff) reserves 256 slots, plenty of room. - address pendingProxyAdmin; } function _layout() private pure returns (Layout storage s) { @@ -72,7 +78,7 @@ contract IntuitionVersionedFeeProxy is IIntuitionVersionedFeeProxy { // ============ Modifiers ============ modifier onlyProxyAdmin() { - if (msg.sender != _layout().proxyAdmin) { + if (!_layout().proxyAdmins[msg.sender]) { revert Errors.VersionedFeeProxy_NotProxyAdmin(); } _; @@ -80,19 +86,22 @@ contract IntuitionVersionedFeeProxy is IIntuitionVersionedFeeProxy { // ============ Constructor ============ - /// @param admin Proxy-admin authorized for version management (recommend: Safe) + /// @param initialProxyAdmins Initial whitelist (at least one non-zero, no + /// duplicates). For production: include at least one Safe multisig. /// @param initialVersion Identifier for the initial registered version (e.g. bytes32("v2.0.0")) /// @param initialImpl Address of the initial logic implementation /// @param initData Calldata forwarded via delegatecall to initialize the logic - /// @param initialName Optional human-readable name (bytes32 — empty for none, editable by proxyAdmin via setName) + /// @param initialName Optional human-readable name (bytes32 — empty for none, editable by a proxyAdmin via setName) constructor( - address admin, + address[] memory initialProxyAdmins, bytes32 initialVersion, address initialImpl, bytes memory initData, bytes32 initialName ) { - if (admin == address(0)) revert Errors.IntuitionFeeProxy_ZeroAddress(); + if (initialProxyAdmins.length == 0) { + revert Errors.IntuitionFeeProxy_NoAdminsProvided(); + } if (initialVersion == bytes32(0)) revert Errors.VersionedFeeProxy_InvalidVersion(); if (initialImpl == address(0) || initialImpl.code.length == 0) { revert Errors.VersionedFeeProxy_InvalidImplementation(); @@ -107,7 +116,23 @@ contract IntuitionVersionedFeeProxy is IIntuitionVersionedFeeProxy { } Layout storage s = _layout(); - s.proxyAdmin = admin; + + // Seed the proxyAdmin whitelist. Reject zero addresses and dedupe + // (silently — duplicates in the list would otherwise inflate the + // count and break the last-admin guard). + uint256 added; + uint256 len = initialProxyAdmins.length; + for (uint256 i = 0; i < len; i++) { + address a = initialProxyAdmins[i]; + if (a == address(0)) revert Errors.IntuitionFeeProxy_ZeroAddress(); + if (!s.proxyAdmins[a]) { + s.proxyAdmins[a] = true; + emit ProxyAdminGranted(a); + unchecked { ++added; } + } + } + s.proxyAdminCount = added; + s.implementations[initialVersion] = initialImpl; s.versionExists[initialVersion] = true; s.versionList.push(initialVersion); @@ -116,7 +141,6 @@ contract IntuitionVersionedFeeProxy is IIntuitionVersionedFeeProxy { _mirrorEip1967(initialImpl); - emit ProxyAdminTransferred(address(0), admin); emit VersionRegistered(initialVersion, initialImpl); emit DefaultVersionChanged(bytes32(0), initialVersion); if (initialName != bytes32(0)) { @@ -213,11 +237,11 @@ contract IntuitionVersionedFeeProxy is IIntuitionVersionedFeeProxy { /// `receiver == msg.sender || approvals[receiver][msg.sender] & DEPOSIT`, /// and `msg.sender` from the MultiVault's POV is this proxy. A new /// impl could legally route deposits to any address that has approved - /// this proxy on the MultiVault. The trust model relies on - /// `proxyAdmin` being a Safe multisig (M-of-N, M ≥ 3 recommended). - /// Users who want to be insulated from default-version switches MUST - /// pin a specific version via `executeAtVersion(version, …)` rather - /// than relying on the fallback. + /// this proxy on the MultiVault. The trust model relies on the + /// `proxyAdmins` whitelist including a Safe multisig (M-of-N, M ≥ 3 + /// recommended). Users who want to be insulated from default-version + /// switches MUST pin a specific version via `executeAtVersion(version, …)` + /// rather than relying on the fallback. function setDefaultVersion(bytes32 version) external onlyProxyAdmin { Layout storage s = _layout(); if (!s.versionExists[version]) revert Errors.VersionedFeeProxy_VersionNotFound(); @@ -229,22 +253,25 @@ contract IntuitionVersionedFeeProxy is IIntuitionVersionedFeeProxy { } /// @inheritdoc IIntuitionVersionedFeeProxy - function transferProxyAdmin(address newAdmin) external onlyProxyAdmin { - if (newAdmin == address(0)) revert Errors.IntuitionFeeProxy_ZeroAddress(); - Layout storage s = _layout(); - s.pendingProxyAdmin = newAdmin; - emit ProxyAdminTransferStarted(s.proxyAdmin, newAdmin); - } - - /// @inheritdoc IIntuitionVersionedFeeProxy - function acceptProxyAdmin() external { + function setProxyAdmin(address admin, bool status) external onlyProxyAdmin { + if (admin == address(0)) revert Errors.IntuitionFeeProxy_ZeroAddress(); Layout storage s = _layout(); - address pending = s.pendingProxyAdmin; - if (msg.sender != pending) revert Errors.VersionedFeeProxy_NotPendingProxyAdmin(); - address old = s.proxyAdmin; - s.proxyAdmin = pending; - delete s.pendingProxyAdmin; - emit ProxyAdminTransferred(old, pending); + bool current = s.proxyAdmins[admin]; + if (current == status) revert Errors.VersionedFeeProxy_ProxyAdminAlreadySet(); + // Last-admin guard: refuse to revoke the only remaining proxyAdmin, + // otherwise the role would be permanently lost (no one can grant it + // back). Mirrors the Role 2 (fee-admin) `adminCount > 0` invariant. + if (!status && s.proxyAdminCount == 1) { + revert Errors.VersionedFeeProxy_LastProxyAdmin(); + } + s.proxyAdmins[admin] = status; + if (status) { + unchecked { ++s.proxyAdminCount; } + emit ProxyAdminGranted(admin); + } else { + unchecked { --s.proxyAdminCount; } + emit ProxyAdminRevoked(admin); + } } /// @inheritdoc IIntuitionVersionedFeeProxy @@ -279,13 +306,13 @@ contract IntuitionVersionedFeeProxy is IIntuitionVersionedFeeProxy { } /// @inheritdoc IIntuitionVersionedFeeProxy - function proxyAdmin() external view returns (address) { - return _layout().proxyAdmin; + function isProxyAdmin(address candidate) external view returns (bool) { + return _layout().proxyAdmins[candidate]; } - /// @notice The candidate admin awaiting acceptance, or address(0) if none. - function pendingProxyAdmin() external view returns (address) { - return _layout().pendingProxyAdmin; + /// @inheritdoc IIntuitionVersionedFeeProxy + function proxyAdminCount() external view returns (uint256) { + return _layout().proxyAdminCount; } // ============ ERC-165 ============ @@ -355,10 +382,6 @@ contract IntuitionVersionedFeeProxy is IIntuitionVersionedFeeProxy { } } - // NOTE: no `receive()` — ETH transfers without calldata revert. Fee flows - // all come with calldata (createAtoms / deposit / …), so bare transfers - // would only be mis-sends. - // ============ Internal ============ /// @dev Writes `impl` into the EIP-1967 implementation slot and emits diff --git a/packages/contracts/src/interfaces/IIntuitionVersionedFeeProxy.sol b/packages/contracts/src/interfaces/IIntuitionVersionedFeeProxy.sol index b3b1427..048f7f3 100644 --- a/packages/contracts/src/interfaces/IIntuitionVersionedFeeProxy.sol +++ b/packages/contracts/src/interfaces/IIntuitionVersionedFeeProxy.sol @@ -13,10 +13,11 @@ interface IIntuitionVersionedFeeProxy { event VersionRegistered(bytes32 indexed version, address indexed implementation); event VersionRemoved(bytes32 indexed version); event DefaultVersionChanged(bytes32 indexed oldVersion, bytes32 indexed newVersion); - event ProxyAdminTransferred(address indexed oldAdmin, address indexed newAdmin); - /// @notice Emitted when a new proxy-admin transfer is initiated (pending the new admin's acceptance). - event ProxyAdminTransferStarted(address indexed currentAdmin, address indexed pendingAdmin); + /// @notice Emitted when an address gains the proxyAdmin role. + event ProxyAdminGranted(address indexed admin); + /// @notice Emitted when an address loses the proxyAdmin role. + event ProxyAdminRevoked(address indexed admin); /// @notice Emitted when the proxy's human-readable name is set or changed. event NameChanged(bytes32 indexed oldName, bytes32 indexed newName); @@ -27,17 +28,20 @@ interface IIntuitionVersionedFeeProxy { function removeVersion(bytes32 version) external; function setDefaultVersion(bytes32 version) external; - /// @notice Initiate a proxy-admin transfer. The new admin takes over only - /// after they call `acceptProxyAdmin`. Passing `address(0)` reverts. - /// Calling again before acceptance overwrites the pending admin. - function transferProxyAdmin(address newAdmin) external; - - /// @notice Finalize a pending proxy-admin transfer. Only callable by the - /// address set as `pendingProxyAdmin` via `transferProxyAdmin`. - function acceptProxyAdmin() external; + /// @notice Grant or revoke the proxyAdmin role for an address. + /// `status = true` adds; `status = false` removes. Idempotent + /// calls (status already matches) revert with + /// `ProxyAdminAlreadySet`. The last remaining admin cannot + /// self-revoke (revert `LastProxyAdmin`). + /// @dev Any current proxyAdmin can call this. For production, the + /// recommended setup is to keep at least one Gnosis Safe + /// multisig in the whitelist — the Safe's internal quorum + /// provides the safety net the previous 2-step transfer flow + /// used to enforce. + function setProxyAdmin(address admin, bool status) external; /// @notice Set or rename the proxy's human-readable label. Pass bytes32(0) to clear. - /// @dev ⚠️ **The name is NOT a trust anchor.** The proxy-admin can + /// @dev ⚠️ **The name is NOT a trust anchor.** Any proxy-admin can /// rename the proxy at any time — including to mimic a known /// brand. Frontends MUST NOT use `name` to derive an "official" /// / "verified" badge. Use the Factory's `isProxyFromFactory` @@ -50,8 +54,13 @@ interface IIntuitionVersionedFeeProxy { function getImplementation(bytes32 version) external view returns (address); function getDefaultVersion() external view returns (bytes32); function getVersions() external view returns (bytes32[] memory); - function proxyAdmin() external view returns (address); - function pendingProxyAdmin() external view returns (address); + + /// @notice Returns true if `candidate` is currently a proxyAdmin. + function isProxyAdmin(address candidate) external view returns (bool); + + /// @notice Returns the number of addresses currently holding the proxyAdmin role. + function proxyAdminCount() external view returns (uint256); + /// @notice Returns the proxy's current human-readable label. /// @dev ⚠️ See the warning on `setName` — a name is admin-controlled /// metadata, never a source of trust. diff --git a/packages/contracts/src/libraries/Errors.sol b/packages/contracts/src/libraries/Errors.sol index 8765e18..5fffc97 100644 --- a/packages/contracts/src/libraries/Errors.sol +++ b/packages/contracts/src/libraries/Errors.sol @@ -99,8 +99,16 @@ library Errors { /// @notice Delegatecall into the versioned implementation failed without returndata error VersionedFeeProxy_DelegateCallFailed(); - /// @notice `acceptProxyAdmin` called by an address that is not the pending admin - error VersionedFeeProxy_NotPendingProxyAdmin(); + /// @notice `setProxyAdmin` called with status matching the current state + /// (granting an already-admin or revoking a non-admin). Idempotent + /// calls revert so the on-chain log of grant/revoke events stays + /// truthful (every emit corresponds to a real state change). + error VersionedFeeProxy_ProxyAdminAlreadySet(); + + /// @notice `setProxyAdmin(admin, false)` would leave the proxy with zero + /// admins, permanently locking the role. The last remaining + /// proxyAdmin cannot self-revoke. + error VersionedFeeProxy_LastProxyAdmin(); /// @notice The new impl's `STORAGE_COMPAT_ID` differs from the proxy's /// reference (default version's impl). Registering an incompatible diff --git a/packages/contracts/test/IntuitionFeeProxyFactory.test.ts b/packages/contracts/test/IntuitionFeeProxyFactory.test.ts index d299599..841b63d 100644 --- a/packages/contracts/test/IntuitionFeeProxyFactory.test.ts +++ b/packages/contracts/test/IntuitionFeeProxyFactory.test.ts @@ -162,7 +162,11 @@ describe("IntuitionFeeProxyFactory (UUPS)", function () { "IntuitionVersionedFeeProxy", proxyAddr, )) as unknown as IntuitionVersionedFeeProxy; - expect(await versioned.proxyAdmin()).to.equal(admin1.address); // = initialAdmins[0] + // Factory now passes the full initialAdmins array to Role 1 — every + // initial admin is also a proxyAdmin. proxyAdminCount mirrors Role 2. + expect(await versioned.isProxyAdmin(admin1.address)).to.equal(true); + expect(await versioned.isProxyAdmin(admin2.address)).to.equal(true); + expect(await versioned.proxyAdminCount()).to.equal(2n); expect(await versioned.getDefaultVersion()).to.equal(V2); expect(await versioned.getVersions()).to.deep.equal([V2]); expect(await versioned.getImplementation(V2)).to.equal(await implV2.getAddress()); diff --git a/packages/contracts/test/IntuitionFeeProxyV2.test.ts b/packages/contracts/test/IntuitionFeeProxyV2.test.ts index 11e3d50..2118ced 100644 --- a/packages/contracts/test/IntuitionFeeProxyV2.test.ts +++ b/packages/contracts/test/IntuitionFeeProxyV2.test.ts @@ -43,7 +43,7 @@ describe("IntuitionFeeProxyV2", function () { // delegating the initializer call on deployment. const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); const proxyDeployment = await VersionedFactory.deploy( - admin1.address, // proxy-admin = first admin in V2 admins list + [admin1.address], // proxyAdmins whitelist (post Role 1 multi-admin) INITIAL_VERSION, await impl.getAddress(), initData, @@ -125,7 +125,7 @@ describe("IntuitionFeeProxyV2", function () { ]); const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); await expect( - VersionedFactory.deploy(admin1.address, INITIAL_VERSION, await impl.getAddress(), initData, ethers.ZeroHash) + VersionedFactory.deploy([admin1.address], INITIAL_VERSION, await impl.getAddress(), initData, ethers.ZeroHash) ).to.be.revertedWithCustomError(impl, "IntuitionFeeProxy_InvalidMultiVaultAddress"); }); @@ -141,7 +141,7 @@ describe("IntuitionFeeProxyV2", function () { ]); const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); await expect( - VersionedFactory.deploy(admin1.address, INITIAL_VERSION, await impl.getAddress(), initData, ethers.ZeroHash) + VersionedFactory.deploy([admin1.address], INITIAL_VERSION, await impl.getAddress(), initData, ethers.ZeroHash) ).to.be.revertedWithCustomError(impl, "IntuitionFeeProxy_FeePercentageTooHigh"); }); @@ -157,7 +157,7 @@ describe("IntuitionFeeProxyV2", function () { ]); const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); await expect( - VersionedFactory.deploy(admin1.address, INITIAL_VERSION, await impl.getAddress(), initData, ethers.ZeroHash) + VersionedFactory.deploy([admin1.address], INITIAL_VERSION, await impl.getAddress(), initData, ethers.ZeroHash) ).to.be.revertedWithCustomError(impl, "IntuitionFeeProxy_FixedFeeTooHigh"); }); @@ -173,7 +173,7 @@ describe("IntuitionFeeProxyV2", function () { ]); const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); await expect( - VersionedFactory.deploy(admin1.address, INITIAL_VERSION, await impl.getAddress(), initData, ethers.ZeroHash) + VersionedFactory.deploy([admin1.address], INITIAL_VERSION, await impl.getAddress(), initData, ethers.ZeroHash) ).to.be.revertedWithCustomError(impl, "IntuitionFeeProxy_NoAdminsProvided"); }); @@ -189,7 +189,7 @@ describe("IntuitionFeeProxyV2", function () { ]); const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); await expect( - VersionedFactory.deploy(admin1.address, INITIAL_VERSION, await impl.getAddress(), initData, ethers.ZeroHash) + VersionedFactory.deploy([admin1.address], INITIAL_VERSION, await impl.getAddress(), initData, ethers.ZeroHash) ).to.be.revertedWithCustomError(impl, "IntuitionFeeProxy_NoAdminsProvided"); }); @@ -206,7 +206,7 @@ describe("IntuitionFeeProxyV2", function () { ]); const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); const p = await VersionedFactory.deploy( - alice.address, + [alice.address], INITIAL_VERSION, await impl.getAddress(), initData, diff --git a/packages/contracts/test/IntuitionFeeProxyV2Sponsored.test.ts b/packages/contracts/test/IntuitionFeeProxyV2Sponsored.test.ts index d8cf46e..94db77b 100644 --- a/packages/contracts/test/IntuitionFeeProxyV2Sponsored.test.ts +++ b/packages/contracts/test/IntuitionFeeProxyV2Sponsored.test.ts @@ -39,7 +39,7 @@ describe("IntuitionFeeProxyV2Sponsored (B1 full-sponsorship)", function () { const VerFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); const vp = await VerFactory.deploy( - admin1.address, + [admin1.address], INITIAL_VERSION, await impl.getAddress(), initData, diff --git a/packages/contracts/test/IntuitionFeeProxyV2_1.test.ts b/packages/contracts/test/IntuitionFeeProxyV2_1.test.ts index 1d8f03e..fd52a71 100644 --- a/packages/contracts/test/IntuitionFeeProxyV2_1.test.ts +++ b/packages/contracts/test/IntuitionFeeProxyV2_1.test.ts @@ -52,7 +52,7 @@ describe("IntuitionFeeProxyV2_1 — canonical versioning demo", function () { const Versioned = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); const proxyDeployment = (await Versioned.deploy( - admin.address, + [admin.address], V2_LABEL, await v2Impl.getAddress(), initData, @@ -270,7 +270,7 @@ describe("IntuitionFeeProxyV2_1Sponsored — sponsored versioning demo", functio const Versioned = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); const proxyDeployment = await Versioned.deploy( - admin.address, + [admin.address], V2_SPONSORED_LABEL, await v2sImpl.getAddress(), initData, diff --git a/packages/contracts/test/IntuitionVersionedFeeProxy.test.ts b/packages/contracts/test/IntuitionVersionedFeeProxy.test.ts index a9db3a8..59bf477 100644 --- a/packages/contracts/test/IntuitionVersionedFeeProxy.test.ts +++ b/packages/contracts/test/IntuitionVersionedFeeProxy.test.ts @@ -35,7 +35,7 @@ describe("IntuitionVersionedFeeProxy (ERC-7936)", function () { const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); const versioned = (await VersionedFactory.deploy( - proxyAdmin.address, + [proxyAdmin.address, admin2.address, admin3.address], V2, await implV2.getAddress(), initData, @@ -75,10 +75,13 @@ describe("IntuitionVersionedFeeProxy (ERC-7936)", function () { describe("Construction", function () { it("registers initial version, sets default and proxy-admin, runs initializer", async function () { - const { versioned, proxyAdmin, implV2, proxyAsV2, mv } = + const { versioned, proxyAdmin, admin2, admin3, implV2, proxyAsV2, mv } = await loadFixture(deployFixture); - expect(await versioned.proxyAdmin()).to.equal(proxyAdmin.address); + expect(await versioned.isProxyAdmin(proxyAdmin.address)).to.equal(true); + expect(await versioned.isProxyAdmin(admin2.address)).to.equal(true); + expect(await versioned.isProxyAdmin(admin3.address)).to.equal(true); + expect(await versioned.proxyAdminCount()).to.equal(3n); expect(await versioned.getDefaultVersion()).to.equal(V2); expect(await versioned.getVersions()).to.deep.equal([V2]); expect(await versioned.getImplementation(V2)).to.equal(await implV2.getAddress()); @@ -89,11 +92,19 @@ describe("IntuitionVersionedFeeProxy (ERC-7936)", function () { expect(await proxyAsV2.adminCount()).to.equal(3n); }); - it("reverts on zero admin", async function () { + it("reverts on empty initial proxyAdmins array", async function () { + const { implV2 } = await loadFixture(deployFixture); + const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); + await expect( + VersionedFactory.deploy([], V2, await implV2.getAddress(), "0x01", ethers.ZeroHash), + ).to.be.revertedWithCustomError(VersionedFactory, "IntuitionFeeProxy_NoAdminsProvided"); + }); + + it("reverts on zero address in initial proxyAdmins", async function () { const { implV2 } = await loadFixture(deployFixture); const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); await expect( - VersionedFactory.deploy(ethers.ZeroAddress, V2, await implV2.getAddress(), "0x", ethers.ZeroHash), + VersionedFactory.deploy([ethers.ZeroAddress], V2, await implV2.getAddress(), "0x01", ethers.ZeroHash), ).to.be.revertedWithCustomError(VersionedFactory, "IntuitionFeeProxy_ZeroAddress"); }); @@ -102,10 +113,10 @@ describe("IntuitionVersionedFeeProxy (ERC-7936)", function () { const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); await expect( VersionedFactory.deploy( - proxyAdmin.address, + [proxyAdmin.address], ethers.ZeroHash, await implV2.getAddress(), - "0x", + "0x01", ethers.ZeroHash, ), ).to.be.revertedWithCustomError(VersionedFactory, "VersionedFeeProxy_InvalidVersion"); @@ -115,7 +126,7 @@ describe("IntuitionVersionedFeeProxy (ERC-7936)", function () { const [, , , , , , eoa] = await ethers.getSigners(); const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); await expect( - VersionedFactory.deploy(eoa.address, V2, eoa.address, "0x", ethers.ZeroHash), + VersionedFactory.deploy([eoa.address], V2, eoa.address, "0x01", ethers.ZeroHash), ).to.be.revertedWithCustomError(VersionedFactory, "VersionedFeeProxy_InvalidImplementation"); }); @@ -131,7 +142,7 @@ describe("IntuitionVersionedFeeProxy (ERC-7936)", function () { ]); const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); await expect( - VersionedFactory.deploy(proxyAdmin.address, V2, await impl.getAddress(), badInit, ethers.ZeroHash), + VersionedFactory.deploy([proxyAdmin.address], V2, await impl.getAddress(), badInit, ethers.ZeroHash), ).to.be.revertedWithCustomError(impl, "IntuitionFeeProxy_FeePercentageTooHigh"); }); @@ -143,7 +154,7 @@ describe("IntuitionVersionedFeeProxy (ERC-7936)", function () { const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); await expect( VersionedFactory.deploy( - proxyAdmin.address, + [proxyAdmin.address], V2, await implV2.getAddress(), "0x", @@ -456,14 +467,13 @@ describe("IntuitionVersionedFeeProxy (ERC-7936)", function () { "registerVersion(bytes32,address)", "removeVersion(bytes32)", "setDefaultVersion(bytes32)", - "transferProxyAdmin(address)", - "acceptProxyAdmin()", + "setProxyAdmin(address,bool)", "setName(bytes32)", "getImplementation(bytes32)", "getDefaultVersion()", "getVersions()", - "proxyAdmin()", - "pendingProxyAdmin()", + "isProxyAdmin(address)", + "proxyAdminCount()", "getName()", "executeAtVersion(bytes32,bytes)", ]; @@ -482,86 +492,95 @@ describe("IntuitionVersionedFeeProxy (ERC-7936)", function () { }); }); - // ============ transferProxyAdmin ============ + // ============ setProxyAdmin ============ - describe("transferProxyAdmin", function () { - it("2-step transfer: pending is set, accept finalises, old admin loses powers", async function () { + describe("setProxyAdmin", function () { + it("grant: adds an address to the whitelist + increments count + emits event", async function () { const { versioned, proxyAdmin, newAdmin } = await loadFixture(deployFixture); const v3Addr = await deployV3Impl(); - // Step 1 — current admin initiates transfer (pending only) - await expect(versioned.connect(proxyAdmin).transferProxyAdmin(newAdmin.address)) - .to.emit(versioned, "ProxyAdminTransferStarted") - .withArgs(proxyAdmin.address, newAdmin.address); - expect(await versioned.proxyAdmin()).to.equal(proxyAdmin.address); - expect(await versioned.pendingProxyAdmin()).to.equal(newAdmin.address); + expect(await versioned.isProxyAdmin(newAdmin.address)).to.equal(false); + const countBefore = await versioned.proxyAdminCount(); - // Before acceptance: old admin still has powers - await expect(versioned.connect(proxyAdmin).registerVersion(V3, v3Addr)) + await expect(versioned.connect(proxyAdmin).setProxyAdmin(newAdmin.address, true)) + .to.emit(versioned, "ProxyAdminGranted") + .withArgs(newAdmin.address); + + expect(await versioned.isProxyAdmin(newAdmin.address)).to.equal(true); + expect(await versioned.proxyAdminCount()).to.equal(countBefore + 1n); + + // New admin can now act + await expect(versioned.connect(newAdmin).registerVersion(V3, v3Addr)) .to.emit(versioned, "VersionRegistered") .withArgs(V3, v3Addr); - // And new admin cannot yet act - await expect( - versioned.connect(newAdmin).registerVersion(ethers.encodeBytes32String("v99"), v3Addr), - ).to.be.revertedWithCustomError(versioned, "VersionedFeeProxy_NotProxyAdmin"); + }); + + it("revoke: removes an address + decrements count + emits event", async function () { + const { versioned, proxyAdmin, admin2 } = await loadFixture(deployFixture); + const countBefore = await versioned.proxyAdminCount(); + + await expect(versioned.connect(proxyAdmin).setProxyAdmin(admin2.address, false)) + .to.emit(versioned, "ProxyAdminRevoked") + .withArgs(admin2.address); - // Step 2 — pending admin accepts - await expect(versioned.connect(newAdmin).acceptProxyAdmin()) - .to.emit(versioned, "ProxyAdminTransferred") - .withArgs(proxyAdmin.address, newAdmin.address); - expect(await versioned.proxyAdmin()).to.equal(newAdmin.address); - expect(await versioned.pendingProxyAdmin()).to.equal(ethers.ZeroAddress); + expect(await versioned.isProxyAdmin(admin2.address)).to.equal(false); + expect(await versioned.proxyAdminCount()).to.equal(countBefore - 1n); - // Old admin now reverts + // Revoked admin can no longer act + const v3Addr = await deployV3Impl(); await expect( - versioned.connect(proxyAdmin).registerVersion(ethers.encodeBytes32String("v100"), v3Addr), + versioned.connect(admin2).registerVersion(V3, v3Addr), ).to.be.revertedWithCustomError(versioned, "VersionedFeeProxy_NotProxyAdmin"); }); - it("transferProxyAdmin reverts on zero address", async function () { + it("setProxyAdmin reverts on zero address", async function () { const { versioned, proxyAdmin } = await loadFixture(deployFixture); await expect( - versioned.connect(proxyAdmin).transferProxyAdmin(ethers.ZeroAddress), + versioned.connect(proxyAdmin).setProxyAdmin(ethers.ZeroAddress, true), ).to.be.revertedWithCustomError(versioned, "IntuitionFeeProxy_ZeroAddress"); }); - it("non-admin cannot initiate transfer", async function () { + it("non-admin cannot grant or revoke", async function () { const { versioned, nonAdmin, newAdmin } = await loadFixture(deployFixture); await expect( - versioned.connect(nonAdmin).transferProxyAdmin(newAdmin.address), + versioned.connect(nonAdmin).setProxyAdmin(newAdmin.address, true), ).to.be.revertedWithCustomError(versioned, "VersionedFeeProxy_NotProxyAdmin"); }); - it("acceptProxyAdmin reverts when called by non-pending address", async function () { - const { versioned, proxyAdmin, newAdmin, nonAdmin } = await loadFixture(deployFixture); - - // No pending admin yet → anyone should revert + it("idempotent grant (already admin) reverts with ProxyAdminAlreadySet", async function () { + const { versioned, proxyAdmin, admin2 } = await loadFixture(deployFixture); await expect( - versioned.connect(newAdmin).acceptProxyAdmin(), - ).to.be.revertedWithCustomError(versioned, "VersionedFeeProxy_NotPendingProxyAdmin"); + versioned.connect(proxyAdmin).setProxyAdmin(admin2.address, true), + ).to.be.revertedWithCustomError(versioned, "VersionedFeeProxy_ProxyAdminAlreadySet"); + }); - // After transferProxyAdmin, only `newAdmin` can accept - await versioned.connect(proxyAdmin).transferProxyAdmin(newAdmin.address); - await expect( - versioned.connect(nonAdmin).acceptProxyAdmin(), - ).to.be.revertedWithCustomError(versioned, "VersionedFeeProxy_NotPendingProxyAdmin"); + it("idempotent revoke (not an admin) reverts with ProxyAdminAlreadySet", async function () { + const { versioned, proxyAdmin, nonAdmin } = await loadFixture(deployFixture); await expect( - versioned.connect(proxyAdmin).acceptProxyAdmin(), - ).to.be.revertedWithCustomError(versioned, "VersionedFeeProxy_NotPendingProxyAdmin"); + versioned.connect(proxyAdmin).setProxyAdmin(nonAdmin.address, false), + ).to.be.revertedWithCustomError(versioned, "VersionedFeeProxy_ProxyAdminAlreadySet"); }); - it("transferProxyAdmin overwrites a pending candidate", async function () { - const { versioned, proxyAdmin, newAdmin, nonAdmin } = await loadFixture(deployFixture); - await versioned.connect(proxyAdmin).transferProxyAdmin(newAdmin.address); - expect(await versioned.pendingProxyAdmin()).to.equal(newAdmin.address); + it("last-admin guard: cannot revoke the only remaining proxyAdmin", async function () { + const { versioned, proxyAdmin, admin2, admin3 } = await loadFixture(deployFixture); - await versioned.connect(proxyAdmin).transferProxyAdmin(nonAdmin.address); - expect(await versioned.pendingProxyAdmin()).to.equal(nonAdmin.address); + // Revoke admin2 + admin3 — proxyAdmin is now the only one left + await versioned.connect(proxyAdmin).setProxyAdmin(admin2.address, false); + await versioned.connect(proxyAdmin).setProxyAdmin(admin3.address, false); + expect(await versioned.proxyAdminCount()).to.equal(1n); - // Old pending can no longer accept + // The last admin self-revoking would lock the role permanently await expect( - versioned.connect(newAdmin).acceptProxyAdmin(), - ).to.be.revertedWithCustomError(versioned, "VersionedFeeProxy_NotPendingProxyAdmin"); + versioned.connect(proxyAdmin).setProxyAdmin(proxyAdmin.address, false), + ).to.be.revertedWithCustomError(versioned, "VersionedFeeProxy_LastProxyAdmin"); + }); + + it("grant + revoke roundtrip on the same address", async function () { + const { versioned, proxyAdmin, newAdmin } = await loadFixture(deployFixture); + await versioned.connect(proxyAdmin).setProxyAdmin(newAdmin.address, true); + expect(await versioned.isProxyAdmin(newAdmin.address)).to.equal(true); + await versioned.connect(proxyAdmin).setProxyAdmin(newAdmin.address, false); + expect(await versioned.isProxyAdmin(newAdmin.address)).to.equal(false); }); }); @@ -584,7 +603,7 @@ describe("IntuitionVersionedFeeProxy (ERC-7936)", function () { const VersionedFactory = await ethers.getContractFactory("IntuitionVersionedFeeProxy"); const NAME = ethers.encodeBytes32String("My DAO Fees"); const deployed = await VersionedFactory.deploy( - proxyAdmin.address, + [proxyAdmin.address], V2, await implV2.getAddress(), initData, diff --git a/packages/safe-tx/src/cli/op-registry.ts b/packages/safe-tx/src/cli/op-registry.ts index 39c4ec1..a990762 100644 --- a/packages/safe-tx/src/cli/op-registry.ts +++ b/packages/safe-tx/src/cli/op-registry.ts @@ -2,6 +2,7 @@ import { getAddress, stringToHex, type Address, type Hex } from 'viem' import * as factory from '../ops/factory.js' import * as uups from '../ops/uups-upgrade.js' import * as v2 from '../ops/v2-admin.js' +import * as versionedProxy from '../ops/versioned-proxy.js' import type { AdminOp } from '../types.js' /** @@ -21,7 +22,7 @@ export type OpFlag = { export type OpRegistration = { name: string - category: 'v2-admin' | 'factory' | 'uups' + category: 'v2-admin' | 'factory' | 'uups' | 'versioned-proxy' description: string flags: OpFlag[] build: (args: Record) => AdminOp @@ -133,6 +134,21 @@ export const OP_REGISTRY: OpRegistration[] = [ build: ({ factory: f }) => factory.acceptOwnership(f as Address), }, + // ----- versioned-proxy (Role 1) ----- + { + name: 'set-proxy-admin', + category: 'versioned-proxy', + description: + 'Add or remove an address from the versioned proxy Role 1 (proxyAdmins) whitelist', + flags: [ + { name: 'proxy', type: 'address', description: 'Versioned proxy address', required: true }, + { name: 'admin', type: 'address', description: 'Admin address to toggle', required: true }, + { name: 'status', type: 'bool', description: 'true = grant, false = revoke', required: true }, + ], + build: ({ proxy, admin, status }) => + versionedProxy.setProxyAdmin(proxy as Address, admin as Address, status as boolean), + }, + // ----- uups ----- { name: 'upgrade-to-and-call', diff --git a/packages/safe-tx/src/ops/versioned-proxy.ts b/packages/safe-tx/src/ops/versioned-proxy.ts index 1c56548..a971029 100644 --- a/packages/safe-tx/src/ops/versioned-proxy.ts +++ b/packages/safe-tx/src/ops/versioned-proxy.ts @@ -4,22 +4,22 @@ import type { AdminOp } from '../types.js' /** * Role 1 (proxyAdmin) operations on `IntuitionVersionedFeeProxy`. * - * `transferProxyAdmin` is 2-step: it sets `pendingProxyAdmin`, the - * target then has to call `acceptProxyAdmin` from their own wallet - * (or via their Safe propose flow if they're a Safe) to finalize. + * Role 1 is a whitelist (post 2-step retirement) — `setProxyAdmin` + * grants or revokes instantly. The contract enforces idempotent + * reject + last-admin guard. + * + * `registerVersion` and `setDefaultVersion` are the version-registry + * mutators — they live on the same role-1 surface so the same admins + * that rotate the whitelist also manage the implementation directory. */ const VERSIONED_PROXY_ABI = [ { type: 'function', - name: 'transferProxyAdmin', - inputs: [{ name: 'newAdmin', type: 'address' }], - outputs: [], - stateMutability: 'nonpayable', - }, - { - type: 'function', - name: 'acceptProxyAdmin', - inputs: [], + name: 'setProxyAdmin', + inputs: [ + { name: 'admin', type: 'address' }, + { name: 'status', type: 'bool' }, + ], outputs: [], stateMutability: 'nonpayable', }, @@ -42,29 +42,20 @@ const VERSIONED_PROXY_ABI = [ }, ] as const -export function transferProxyAdmin(proxy: Address, newAdmin: Address): AdminOp { - return { - to: proxy, - value: 0n, - data: encodeFunctionData({ - abi: VERSIONED_PROXY_ABI, - functionName: 'transferProxyAdmin', - args: [newAdmin], - }), - description: `transferProxyAdmin(-> ${newAdmin}) on proxy ${proxy}`, - } -} - -export function acceptProxyAdmin(proxy: Address): AdminOp { +export function setProxyAdmin( + proxy: Address, + admin: Address, + status: boolean, +): AdminOp { return { to: proxy, value: 0n, data: encodeFunctionData({ abi: VERSIONED_PROXY_ABI, - functionName: 'acceptProxyAdmin', - args: [], + functionName: 'setProxyAdmin', + args: [admin, status], }), - description: `acceptProxyAdmin() on proxy ${proxy}`, + description: `setProxyAdmin(${admin}, ${status}) on ${proxy}`, } } diff --git a/packages/safe-tx/test/fixtures/constants.ts b/packages/safe-tx/test/fixtures/constants.ts index 41339bc..8117c0f 100644 --- a/packages/safe-tx/test/fixtures/constants.ts +++ b/packages/safe-tx/test/fixtures/constants.ts @@ -12,7 +12,13 @@ export const INTUITION_RPC = process.env.INTUITION_RPC ?? 'https://rpc.intuition export const FORK_BLOCK = 3_250_000 -export const ANVIL_PORT = Number(process.env.ANVIL_PORT ?? 8545) +// 8546 by default (not 8545) so the test suite doesn't collide with a +// Hardhat / localhost node a developer commonly keeps running on 8545 +// from another package. Anvil silently fails to bind a busy port and the +// fixture's RPC-ready probe would otherwise talk to the foreign node, +// which doesn't expose the `anvil_*` namespace and produces opaque +// "Method not supported" failures in unrelated tests. +export const ANVIL_PORT = Number(process.env.ANVIL_PORT ?? 8546) export const ANVIL_HOST = '127.0.0.1' export const ANVIL_RPC = `http://${ANVIL_HOST}:${ANVIL_PORT}` diff --git a/packages/safe-tx/test/integration/api-kit.test.ts b/packages/safe-tx/test/integration/api-kit.test.ts index d6df8c4..b765ce3 100644 --- a/packages/safe-tx/test/integration/api-kit.test.ts +++ b/packages/safe-tx/test/integration/api-kit.test.ts @@ -13,7 +13,7 @@ const PK_B = '0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d const SAFE = getAddress('0xf10D442D0fB934D4037DC30769a6EfCf2f54F7B6') const PROXY = getAddress('0x29fcB43b46531BcA003ddC8FCB67FFE91900C762') -const PORT = 8889 // distinct from anvil-sanity (8545) and direct-sign (8546) +const PORT = 8889 // distinct from anvil-sanity (8546) and direct-sign (8547) describe('api-kit mode against mock STS', () => { let mock: MockSts diff --git a/packages/safe-tx/test/integration/direct-sign.test.ts b/packages/safe-tx/test/integration/direct-sign.test.ts index d4555e7..744b620 100644 --- a/packages/safe-tx/test/integration/direct-sign.test.ts +++ b/packages/safe-tx/test/integration/direct-sign.test.ts @@ -20,8 +20,8 @@ import { } from '../fixtures/constants.js' import { impersonateAndFund } from '../fixtures/impersonate.js' -// Distinct port from anvil-sanity (8545) to allow vitest parallelism. -const PORT = 8546 +// Distinct port from anvil-sanity (8546) to allow vitest parallelism. +const PORT = 8547 // Anvil's default first account is well-known and pre-funded on the // fork — used as the executor (no impersonation needed). diff --git a/packages/safe-tx/test/unit/cli/op-registry.test.ts b/packages/safe-tx/test/unit/cli/op-registry.test.ts index d7cab1f..33c57d7 100644 --- a/packages/safe-tx/test/unit/cli/op-registry.test.ts +++ b/packages/safe-tx/test/unit/cli/op-registry.test.ts @@ -8,13 +8,15 @@ import { } from '../../../src/cli/op-registry.js' describe('OP_REGISTRY', () => { - it('contains 10 ops total (5 v2-admin + 4 factory + 1 uups)', () => { - expect(OP_REGISTRY).toHaveLength(10) + it('contains 11 ops total (5 v2-admin + 4 factory + 1 versioned-proxy + 1 uups)', () => { + expect(OP_REGISTRY).toHaveLength(11) const v2 = OP_REGISTRY.filter((o) => o.category === 'v2-admin') const fac = OP_REGISTRY.filter((o) => o.category === 'factory') + const vp = OP_REGISTRY.filter((o) => o.category === 'versioned-proxy') const up = OP_REGISTRY.filter((o) => o.category === 'uups') expect(v2).toHaveLength(5) expect(fac).toHaveLength(4) + expect(vp).toHaveLength(1) expect(up).toHaveLength(1) }) diff --git a/packages/safe-tx/test/unit/ops/versioned-proxy.test.ts b/packages/safe-tx/test/unit/ops/versioned-proxy.test.ts index fa236ad..15febe3 100644 --- a/packages/safe-tx/test/unit/ops/versioned-proxy.test.ts +++ b/packages/safe-tx/test/unit/ops/versioned-proxy.test.ts @@ -3,40 +3,36 @@ import { decodeFunctionData, getAddress, stringToHex, toFunctionSelector } from import * as vp from '../../../src/ops/versioned-proxy.js' const PROXY = getAddress('0xf10D442D0fB934D4037DC30769a6EfCf2f54F7B6') -const NEW_ADMIN = getAddress('0xc634457aD68b037E2D5aA1C10c3930d7e4E2d551') +const ADMIN = getAddress('0xc634457aD68b037E2D5aA1C10c3930d7e4E2d551') const NEW_IMPL = getAddress('0x29fcB43b46531BcA003ddC8FCB67FFE91900C762') const VERSION_BYTES32 = stringToHex('v3.0.0', { size: 32 }) -const SELECTOR_TRANSFER = toFunctionSelector('transferProxyAdmin(address)') -const SELECTOR_ACCEPT = toFunctionSelector('acceptProxyAdmin()') +const SELECTOR_SET_PROXY_ADMIN = toFunctionSelector('setProxyAdmin(address,bool)') const SELECTOR_REGISTER = toFunctionSelector('registerVersion(bytes32,address)') const SELECTOR_SET_DEFAULT = toFunctionSelector('setDefaultVersion(bytes32)') const ABI = [ - { type: 'function', name: 'transferProxyAdmin', inputs: [{ type: 'address' }], outputs: [], stateMutability: 'nonpayable' }, - { type: 'function', name: 'acceptProxyAdmin', inputs: [], outputs: [], stateMutability: 'nonpayable' }, + { type: 'function', name: 'setProxyAdmin', inputs: [{ type: 'address' }, { type: 'bool' }], outputs: [], stateMutability: 'nonpayable' }, { type: 'function', name: 'registerVersion', inputs: [{ type: 'bytes32' }, { type: 'address' }], outputs: [], stateMutability: 'nonpayable' }, { type: 'function', name: 'setDefaultVersion', inputs: [{ type: 'bytes32' }], outputs: [], stateMutability: 'nonpayable' }, ] as const describe('versioned-proxy AdminOp builders', () => { - it('transferProxyAdmin', () => { - const op = vp.transferProxyAdmin(PROXY, NEW_ADMIN) + it('setProxyAdmin grant', () => { + const op = vp.setProxyAdmin(PROXY, ADMIN, true) expect(op.to).toBe(PROXY) expect(op.value).toBe(0n) - expect(op.data.slice(0, 10)).toBe(SELECTOR_TRANSFER) + expect(op.data.slice(0, 10)).toBe(SELECTOR_SET_PROXY_ADMIN) + expect(op.description).toContain('setProxyAdmin') const decoded = decodeFunctionData({ abi: ABI, data: op.data }) - expect(decoded.functionName).toBe('transferProxyAdmin') - expect(decoded.args).toEqual([NEW_ADMIN]) + expect(decoded.functionName).toBe('setProxyAdmin') + expect(decoded.args).toEqual([ADMIN, true]) }) - it('acceptProxyAdmin (no args, selector only)', () => { - const op = vp.acceptProxyAdmin(PROXY) - expect(op.data).toBe(SELECTOR_ACCEPT) - expect(op.data.length).toBe(10) + it('setProxyAdmin revoke', () => { + const op = vp.setProxyAdmin(PROXY, ADMIN, false) const decoded = decodeFunctionData({ abi: ABI, data: op.data }) - expect(decoded.functionName).toBe('acceptProxyAdmin') - expect(decoded.args ?? []).toEqual([]) + expect(decoded.args).toEqual([ADMIN, false]) }) it('registerVersion', () => { @@ -57,8 +53,7 @@ describe('versioned-proxy AdminOp builders', () => { it('all builders target the proxy address with value 0n', () => { const ops = [ - vp.transferProxyAdmin(PROXY, NEW_ADMIN), - vp.acceptProxyAdmin(PROXY), + vp.setProxyAdmin(PROXY, ADMIN, true), vp.registerVersion(PROXY, VERSION_BYTES32, NEW_IMPL), vp.setDefaultVersion(PROXY, VERSION_BYTES32), ] diff --git a/packages/safe-tx/test/unit/signers/factory.test.ts b/packages/safe-tx/test/unit/signers/factory.test.ts index 414264d..0a34cb1 100644 --- a/packages/safe-tx/test/unit/signers/factory.test.ts +++ b/packages/safe-tx/test/unit/signers/factory.test.ts @@ -26,13 +26,16 @@ describe('getSigner factory', () => { }, 5000) it('rejects with an actionable error when trezor has no bridge + no deps', async () => { - // Same dual-path as ledger: + // Same dual-path as ledger, with one extra branch: // - deps missing -> "trezor signer requires optional dep" // - deps installed but no Trezor Bridge running -> init timeout + // - deps installed AND init succeeded but no device present at + // getAddress time -> "trezor getAddress failed. Transport is missing" + // All four are user-actionable and resolve quickly. await expect( getSigner('trezor', { trezor: { initTimeoutMs: 1500 } }), ).rejects.toThrow( - /trezor signer requires optional dep|trezor init failed|trezor connect init timed out/i, + /trezor signer requires optional dep|trezor init failed|trezor connect init timed out|trezor getAddress failed/i, ) }, 5000) diff --git a/packages/sdk/src/readers.ts b/packages/sdk/src/readers.ts index 38e58fc..71362d1 100644 --- a/packages/sdk/src/readers.ts +++ b/packages/sdk/src/readers.ts @@ -190,12 +190,16 @@ export async function readSponsoredMetrics( } } -/** Registered version labels + current default + current + pending proxy admin. +/** Registered version labels + current default + size of the proxyAdmin whitelist. * * Hybrid dispatch: Multicall3 with `allowFailure: true` on chains that - * support it (per-field revert tolerant — older impls missing - * `pendingProxyAdmin` don't break the read), parallel per-field reads + * support it (per-field revert tolerant), parallel per-field reads * otherwise. Both return identical shape. + * + * Role 1 became a whitelist post 2-step retirement — callers that need + * the full admin list should reduce the `ProxyAdminGranted` / + * `ProxyAdminRevoked` event log (no on-chain getter is exposed for the + * full set). */ export async function readProxyVersions( client: PublicClient, @@ -203,41 +207,36 @@ export async function readProxyVersions( ): Promise<{ versions: readonly `0x${string}`[] defaultVersion: `0x${string}` | undefined - proxyAdmin: Address | undefined - pendingProxyAdmin: Address | undefined + proxyAdminCount: bigint }> { const abi = IntuitionVersionedFeeProxyABI as any const contracts = [ { abi, address: proxy, functionName: 'getVersions' }, { abi, address: proxy, functionName: 'getDefaultVersion' }, - { abi, address: proxy, functionName: 'proxyAdmin' }, - { abi, address: proxy, functionName: 'pendingProxyAdmin' }, + { abi, address: proxy, functionName: 'proxyAdminCount' }, ] as const try { - const [versions, defaultVersion, proxyAdmin, pendingProxyAdmin] = + const [versions, defaultVersion, proxyAdminCount] = await client.multicall({ allowFailure: true, contracts: contracts as any }) return { versions: versions.status === 'success' ? (versions.result as readonly `0x${string}`[]) : [], defaultVersion: defaultVersion.status === 'success' ? (defaultVersion.result as `0x${string}`) : undefined, - proxyAdmin: proxyAdmin.status === 'success' ? (proxyAdmin.result as Address) : undefined, - pendingProxyAdmin: pendingProxyAdmin.status === 'success' ? (pendingProxyAdmin.result as Address) : undefined, + proxyAdminCount: proxyAdminCount.status === 'success' ? (proxyAdminCount.result as bigint) : 0n, } } catch { // Fallback — no Multicall3 on this chain. Per-field try-catch so a // single-field revert doesn't poison the whole read. const safe = (p: Promise): Promise => p.catch(() => undefined) - const [versions, defaultVersion, proxyAdmin, pendingProxyAdmin] = await Promise.all([ + const [versions, defaultVersion, proxyAdminCount] = await Promise.all([ safe(client.readContract(contracts[0] as any) as Promise), safe(client.readContract(contracts[1] as any) as Promise<`0x${string}`>), - safe(client.readContract(contracts[2] as any) as Promise
), - safe(client.readContract(contracts[3] as any) as Promise
), + safe(client.readContract(contracts[2] as any) as Promise), ]) return { versions: versions ?? [], defaultVersion, - proxyAdmin, - pendingProxyAdmin, + proxyAdminCount: proxyAdminCount ?? 0n, } } } diff --git a/packages/webapp/src/components/AdminsTab.tsx b/packages/webapp/src/components/AdminsTab.tsx index 53352c7..606223a 100644 --- a/packages/webapp/src/components/AdminsTab.tsx +++ b/packages/webapp/src/components/AdminsTab.tsx @@ -7,8 +7,6 @@ import { UpgradeAuthorityPanel } from './UpgradeAuthorityPanel' interface Props { proxy: Address - proxyAdmin: Address | undefined - pendingProxyAdmin: Address | undefined account: Address | undefined isFeeAdmin: boolean isVersionsFetching: boolean @@ -17,8 +15,6 @@ interface Props { export function AdminsTab({ proxy, - proxyAdmin, - pendingProxyAdmin, account, isFeeAdmin, isVersionsFetching, @@ -33,8 +29,6 @@ export function AdminsTab({

void isRefreshing: boolean } +/** + * Role 1 panel — multi-admin whitelist (post 2-step retirement). Mirrors + * `AdminsPanel` (Role 2) in structure: list + per-row revoke + grant + * form. Direct write for current proxyAdmins, "Propose via Safe" path + * when at least one Safe sits in the whitelist. + * + * Default visible: the list and grant form. + * Advanced (collapsible, closed by default): the "grant both roles in + * one click" convenience + the Role 1 vs Role 2 explainer. + */ export function UpgradeAuthorityPanel({ proxy, - proxyAdmin, - pendingProxyAdmin, account, isConnectedFeeAdmin, onTransferred, isRefreshing, }: Props) { - const isYou = Boolean( - account && - proxyAdmin && - account.toLowerCase() === proxyAdmin.toLowerCase(), - ) - const hasPending = Boolean( - pendingProxyAdmin && pendingProxyAdmin.toLowerCase() !== ZERO, + const { admins: proxyAdmins, isLoading, refetch } = useProxyAdmins(proxy) + const adminStatuses = useSafeStatuses(proxyAdmins) + const safeAdmin = proxyAdmins.find( + (a) => adminStatuses[a.toLowerCase()]?.kind === 'safe', ) - const isPendingYou = Boolean( + const safePropose = useSafePropose({ safeAddress: safeAdmin }) + + const isYou = Boolean( account && - pendingProxyAdmin && - account.toLowerCase() === pendingProxyAdmin.toLowerCase(), + proxyAdmins.some( + (a) => account.toLowerCase() === a.toLowerCase(), + ), ) - const hasBothRoles = isYou && isConnectedFeeAdmin - - // Detect if proxyAdmin is itself a Safe — then we can propose - // transferProxyAdmin / acceptProxyAdmin via that Safe. - const proxyAdminStatus = useSafeStatus(proxyAdmin) - const proxyAdminSafe = proxyAdminStatus.kind === 'safe' ? proxyAdmin : undefined - // Same for the pending admin — useful when the new owner is a Safe - // and needs to acceptProxyAdmin via that Safe's quorum. - const pendingStatus = useSafeStatus(pendingProxyAdmin) - const pendingIsSafe = pendingStatus.kind === 'safe' - const safePropose = useSafePropose({ - safeAddress: proxyAdminSafe ?? (pendingIsSafe ? pendingProxyAdmin : undefined), - }) - - // Standalone "Grant Role 1" — separate write instance from the rotation. - const [newAdminInput, setNewAdminInput] = useState('') - const { - transferAdmin, - hash: transferHash, - isPending: transferPending, - error: transferError, - reset: resetTransfer, - } = useTransferProxyAdmin(proxy) - const transferReceipt = useWaitForTransactionReceipt({ hash: transferHash }) - - // Step 2 — pending admin accepts - const { - acceptAdmin, - hash: acceptHash, - isPending: acceptPending, - error: acceptError, - reset: resetAccept, - } = useAcceptProxyAdmin(proxy) - const acceptReceipt = useWaitForTransactionReceipt({ hash: acceptHash }) - - // Bridges the window between "tx mined" and "useProxyVersions reflects - // the new on-chain state" — drives the card-title spinner. - const postTx = usePostTxRefreshing(isRefreshing) - // Rotation state machine (grant + transfer + external-accept detection). - const rotation = useProxyAdminRotation({ - proxy, - proxyAdmin, - account, - onWriteSuccess: () => { - onTransferred() - postTx.start() - }, - }) + const { + setProxyAdmin, + hash, + isPending, + error: writeError, + reset, + } = useSetProxyAdmin(proxy) + const receipt = useWaitForTransactionReceipt({ hash }) - // Transient success flash after acceptProxyAdmin lands — tells the - // new admin visually that they now hold the role. Auto-dismisses. - const [acceptedFlash, setAcceptedFlash] = useState(false) + const [addDraft, setAddDraft] = useState('') + const [pendingTarget, setPendingTarget] = useState
(null) + const postTx = usePostTxRefreshing(isRefreshing) - // Standalone transfer: clear input, refetch parent, bridge spinner. useEffect(() => { - if (transferReceipt.isSuccess) { - setNewAdminInput('') - resetTransfer() + if (receipt.isSuccess) { + refetch() + reset() + setAddDraft('') onTransferred() postTx.start() } - }, [transferReceipt.isSuccess]) + }, [hash, receipt.isSuccess]) - // Accept: refetch parent, flash success, bridge spinner. useEffect(() => { - if (acceptReceipt.isSuccess) { - resetAccept() - onTransferred() - postTx.start() - setAcceptedFlash(true) - const t = setTimeout(() => setAcceptedFlash(false), 6000) - return () => clearTimeout(t) + if ( + !isPending && + !receipt.isLoading && + !receipt.isSuccess && + pendingTarget + ) { + setPendingTarget(null) } - }, [acceptReceipt.isSuccess]) + }, [isPending, receipt.isLoading, receipt.isSuccess, writeError]) + + const addValid = + isAddress(addDraft) && + !proxyAdmins.some((a) => a.toLowerCase() === addDraft.toLowerCase()) - const inputValid = isAddress(newAdminInput.trim()) + const busy = isPending || receipt.isLoading || safePropose.isProposing - const titleBusy = - postTx.active || - transferPending || - transferReceipt.isLoading || - acceptPending || - acceptReceipt.isLoading || - rotation.busy + async function onGrant() { + if (!addValid) return + setPendingTarget('ADD') + try { + await setProxyAdmin(addDraft as Address, true) + } catch (e) { + console.error(e) + setPendingTarget(null) + } + } + + async function onRevoke(addr: Address) { + setPendingTarget(addr) + try { + await setProxyAdmin(addr, false) + } catch (e) { + console.error(e) + setPendingTarget(null) + } + } + + async function onProposeGrant() { + if (!addValid || !safeAdmin) return + safePropose.reset() + try { + await safePropose.propose( + ops.versionedProxy.setProxyAdmin(proxy, addDraft as Address, true), + ) + } catch (e) { + console.error(e) + } + } + + async function onProposeRevoke(addr: Address) { + if (!safeAdmin) return + safePropose.reset() + try { + await safePropose.propose( + ops.versionedProxy.setProxyAdmin(proxy, addr, false), + ) + } catch (e) { + console.error(e) + } + } + + const canInteract = isYou || Boolean(safeAdmin) return (
@@ -145,294 +151,317 @@ export function UpgradeAuthorityPanel({ Role 1

- Upgrade authority (proxyAdmin) - {titleBusy && } + Upgrade authority (proxyAdmins) + {(postTx.active || (isLoading && proxyAdmins.length > 0)) && ( + + )}

- Single slot · 2-step grant + {proxyAdmins.length} address{proxyAdmins.length === 1 ? '' : 'es'} · + instant grant/revoke

- Single slot. Controls implementation registration only — fees and - withdrawals are Role 2. Use a Safe for production. + Controls implementation registration and default version. Mirrors + Role 2's pattern (whitelist + last-admin guard). Use a Safe for + production — its internal quorum is the safety net we used to + get from the 2-step transfer.

- + - {acceptedFlash && ( -
- - You are now proxyAdmin — transfer finalised - on-chain. - + {isLoading && proxyAdmins.length === 0 && ( +
+
+
)} -
- - current - - {proxyAdmin ? ( - <> - - - {isYou && ( - - you - - )} - - ) : ( - — - )} -
- - {hasPending && ( -
-
- - pending - - - {isPendingYou && ( - - you - - )} -
-

- Pending must call{' '} - acceptProxyAdmin() to - finalise. Current admin can still overwrite. -

- {isPendingYou && ( - - )} - {pendingIsSafe && pendingProxyAdmin && ( - - )} - {acceptError && ( -

- {acceptError.message.split('\n')[0]} -

- )} -
+ {proxyAdmins.length > 0 && ( +
    + {proxyAdmins.map((addr) => { + const isSelf = + account && addr.toLowerCase() === account.toLowerCase() + const isLast = proxyAdmins.length === 1 + const canDirectRevoke = isYou && !isLast + const canProposeRevoke = Boolean(safeAdmin) && !isLast + return ( +
  • +
    + + + {isSelf && ( + + you + + )} +
    +
    + {canDirectRevoke && ( + + )} + {canProposeRevoke && ( + + )} + {isLast && ( + + Last admin — cannot revoke + + )} +
    +
    • + ) + })} +
)} - {(isYou || proxyAdminSafe) && ( -
-
-
) } /** - * Subcomponent for the "grant both roles" convenience flow. Pure renderer - * over the rotation state machine — no local state of its own. + * Collapsible "Advanced" section — closed by default. + * + * Houses two things: + * 1. The grant-both-roles convenience (only enabled when the caller is + * a direct admin on BOTH roles — granting both via Safe is just two + * separate ProposeViaSafe flows, no point bundling). + * 2. The Role 1 vs Role 2 explainer paragraph (operational reference, + * not needed in the main flow once the user is familiar). + */ +function AdvancedSection({ + proxy, + canGrantBoth, + onWriteDone, +}: { + proxy: Address + canGrantBoth: boolean + onWriteDone: () => void +}) { + const [open, setOpen] = useState(false) + + return ( +
+ + + {open && ( +
+ {canGrantBoth && } +

+ Role 1 vs Role 2.{' '} + Role 1 (proxyAdmins) controls which logic the proxy + delegates to — register new implementations, change default + version, rename. It cannot touch fees, + withdrawals, or the sponsor pool; those are Role 2 below. + Both roles use the same whitelist model with last-admin + guard. Keep them disjoint if your dev team and ops team + are distinct. +

+
+ )} +
+ ) +} + +/** + * Convenience: grant Role 1 + Role 2 to a new address in two back-to-back + * transactions. Only available when the caller currently holds both roles + * (direct admin on each). For Safe-mediated grants, use the per-role + * Propose via Safe buttons — each goes through its own quorum anyway. */ -function RotateBothRolesForm({ - rotation, +function GrantBothRolesForm({ + proxy, + onDone, }: { - rotation: ReturnType + proxy: Address + onDone: () => void }) { - const buttonLabel = (() => { - switch (rotation.stage) { - case 'grant': - return rotation.grantPending ? 'Sign Role 2…' : 'Granting Role 2…' - case 'transfer': - return rotation.transferPending ? 'Sign Role 1…' : 'Granting Role 1…' - case 'done': - return 'Waiting for accept…' - default: - return 'Grant both roles' + const [input, setInput] = useState('') + const [stage, setStage] = useState<'idle' | 'role1' | 'role2'>('idle') + + const role1 = useSetProxyAdmin(proxy) + const role1Receipt = useWaitForTransactionReceipt({ hash: role1.hash }) + const role2 = useSetWhitelistedAdmin(proxy) + const role2Receipt = useWaitForTransactionReceipt({ hash: role2.hash }) + + const inputValid = isAddress(input.trim()) + + useEffect(() => { + if (stage === 'role1' && role1Receipt.isSuccess) { + setStage('role2') + role2.setAdmin(input.trim() as Address, true).catch(() => setStage('idle')) } - })() + }, [stage, role1Receipt.isSuccess]) - const stepDone = (s: 'grant' | 'transfer' | 'accept') => { - if (s === 'grant') - return rotation.grantConfirmed || rotation.stage === 'transfer' || rotation.stage === 'done' - if (s === 'transfer') return rotation.stage === 'done' - return false + useEffect(() => { + if (stage === 'role2' && role2Receipt.isSuccess) { + setStage('idle') + setInput('') + onDone() + } + }, [stage, role2Receipt.isSuccess]) + + const busy = stage !== 'idle' + + function start() { + if (!inputValid) return + setStage('role1') + role1.setProxyAdmin(input.trim() as Address, true).catch(() => setStage('idle')) } + const label = (() => { + if (stage === 'role1') { + return role1.isPending ? 'Sign Role 1…' : 'Granting Role 1…' + } + if (stage === 'role2') { + return role2.isPending ? 'Sign Role 2…' : 'Granting Role 2…' + } + return 'Grant both roles' + })() + return ( -
+
+
+ + Convenience + + + Grant both roles to a single address + +

- Two signatures: setWhitelistedAdmin{' '} - then transferProxyAdmin. The - new admin then calls{' '} - acceptProxyAdmin(). + You currently hold both roles. This runs{' '} + setProxyAdmin(new, true){' '} + then{' '} + setWhitelistedAdmin(new, true){' '} + back-to-back (2 signatures). Both grants are instant — no + 2-step ceremony.

- {rotation.stage !== 'complete' && ( -
- rotation.setInput(e.target.value)} - disabled={rotation.stage !== 'idle' && rotation.stage !== 'done'} - className="input flex-1 min-w-[18rem] font-mono text-xs" - /> - -
- )} - {rotation.stage !== 'idle' && rotation.stage !== 'complete' && ( -
    -
  1. - Grant fee admin (immediate) -
    2. -
  2. - Transfer proxyAdmin (pending) -
    3. -
  3. - New admin calls{' '} - acceptProxyAdmin() -
    4. -
- )} - {rotation.stage === 'complete' && ( -
-

- Rotation complete. Optionally revoke your fee admin rights from - the new admin's wallet. -

- -
- )} - {rotation.error && ( +
+ setInput(e.target.value)} + disabled={busy} + className="input flex-1 min-w-[18rem] font-mono text-xs" + /> + +
+ {(role1.error || role2.error) && (

- {rotation.error.split('\n')[0]} + {(role1.error ?? role2.error)!.message.split('\n')[0]}

)}
diff --git a/packages/webapp/src/hooks/useProxyAdminRotation.ts b/packages/webapp/src/hooks/useProxyAdminRotation.ts deleted file mode 100644 index d119073..0000000 --- a/packages/webapp/src/hooks/useProxyAdminRotation.ts +++ /dev/null @@ -1,165 +0,0 @@ -import { useEffect, useRef, useState } from 'react' -import { isAddress, type Address } from 'viem' -import { useWaitForTransactionReceipt } from 'wagmi' - -import { useSetWhitelistedAdmin } from './useProxy' -import { useTransferProxyAdmin } from './useVersionedProxy' -import type { AdminRotateStage } from '../types' - -export interface ProxyAdminRotation { - /** Input address for the target admin. */ - input: string - setInput: (v: string) => void - /** Where we are in the 5-state machine. */ - stage: AdminRotateStage - /** User-facing error from any step of the chain. */ - error: string | null - /** True while any signature/mining inside the rotation is in flight. */ - busy: boolean - /** Receipts — exposed so the caller can show step-by-step progress. */ - grantConfirmed: boolean - transferConfirmed: boolean - /** Granular flags for per-step button text ("Sign Role 2…" etc). */ - grantPending: boolean - transferPending: boolean - /** True when the trimmed input is a valid hex address. */ - isValid: boolean - /** Start the 2-tx chain. Precondition: isValid. */ - start: () => Promise - /** Clear all state — run manually once the user acknowledges completion. */ - reset: () => void -} - -/** - * Encapsulates the "grant both roles" 5-state machine + the 2 writes it - * drives (setWhitelistedAdmin + transferProxyAdmin) and the external - * acceptance detection. Keeps UpgradeAuthorityPanel free of state-machine - * wiring — the panel just renders based on `stage`. - * - * Callbacks: - * - `onWriteSuccess` — fired when EITHER write mines. Callers refetch - * `useProxyVersions` to pick up the new pending/current admin. - */ -export function useProxyAdminRotation({ - proxy, - proxyAdmin, - account, - onWriteSuccess, -}: { - proxy: Address - proxyAdmin: Address | undefined - account: Address | undefined - onWriteSuccess: () => void -}): ProxyAdminRotation { - const { - setAdmin: grantFeeAdmin, - hash: grantHash, - isPending: grantPending, - reset: resetGrant, - } = useSetWhitelistedAdmin(proxy) - const grantReceipt = useWaitForTransactionReceipt({ hash: grantHash }) - - const { - transferAdmin, - hash: transferHash, - isPending: transferPending, - reset: resetTransfer, - } = useTransferProxyAdmin(proxy) - const transferReceipt = useWaitForTransactionReceipt({ hash: transferHash }) - - const [input, setInput] = useState('') - const [stage, setStage] = useState('idle') - const [error, setError] = useState(null) - - function resetAll() { - setStage('idle') - setInput('') - setError(null) - resetGrant() - resetTransfer() - } - - // Fire proxyAdmin transfer once the fee-admin grant confirms. - useEffect(() => { - if (stage === 'grant' && grantReceipt.isSuccess) { - resetGrant() - setStage('transfer') - transferAdmin(input.trim() as Address).catch((e: any) => { - setError(e?.message ?? 'transferProxyAdmin failed') - setStage('idle') - }) - } - }, [stage, grantReceipt.isSuccess, resetGrant, input, transferAdmin]) - - // Transfer mined → move to "done" (awaiting external accept). - useEffect(() => { - if (transferReceipt.isSuccess) { - resetTransfer() - onWriteSuccess() - if (stage === 'transfer') setStage('done') - } - }, [transferReceipt.isSuccess, resetTransfer, onWriteSuccess, stage]) - - // Refresh parent reads when grant mines too (not strictly needed for - // versions, but keeps behaviour symmetric with the panel's onTransferred). - useEffect(() => { - if (grantReceipt.isSuccess) onWriteSuccess() - }, [grantReceipt.isSuccess, onWriteSuccess]) - - // Detect external acceptance: on-chain proxyAdmin flipped to our target. - useEffect(() => { - if (stage !== 'done' || !proxyAdmin || !input) return - if (proxyAdmin.toLowerCase() === input.trim().toLowerCase()) { - setStage('complete') - } - }, [stage, proxyAdmin, input]) - - // Fresh wallet = fresh rotation form. Previous wallet's in-flight state - // would be confusing for the new principal. - const prevAccount = useRef(account) - useEffect(() => { - if (prevAccount.current !== account) { - resetAll() - prevAccount.current = account - } - // `resetAll` is intentionally omitted — it's stable for our purposes - // and listing it would require a useCallback dance for no gain. - - }, [account]) - - const isValid = isAddress(input.trim()) - - async function start() { - setError(null) - setStage('grant') - try { - await grantFeeAdmin(input.trim() as Address, true) - } catch (e: any) { - setError(e?.message ?? 'setWhitelistedAdmin failed') - setStage('idle') - } - } - - const busy = - grantPending || - grantReceipt.isLoading || - transferPending || - transferReceipt.isLoading || - stage === 'grant' || - stage === 'transfer' - - return { - input, - setInput, - stage, - error, - busy, - grantConfirmed: grantReceipt.isSuccess, - transferConfirmed: transferReceipt.isSuccess, - grantPending, - transferPending, - isValid, - start, - reset: resetAll, - } -} diff --git a/packages/webapp/src/hooks/useProxyRoles.ts b/packages/webapp/src/hooks/useProxyRoles.ts index 9b6180c..fd4666f 100644 --- a/packages/webapp/src/hooks/useProxyRoles.ts +++ b/packages/webapp/src/hooks/useProxyRoles.ts @@ -1,9 +1,12 @@ +import { useReadContract } from 'wagmi' import type { Address } from 'viem' +import { IntuitionVersionedFeeProxyABI } from '@intuition-fee-proxy/sdk' + export interface ProxyRoles { /** True when the connected wallet is a whitelisted fee admin (Role 2). */ isFeeAdmin: boolean - /** True when the connected wallet is the on-chain proxyAdmin (Role 1). */ + /** True when the connected wallet is in the proxyAdmins whitelist (Role 1). */ isProxyAdmin: boolean /** True when the connected wallet holds both roles simultaneously. */ hasBothRoles: boolean @@ -12,21 +15,30 @@ export interface ProxyRoles { } /** - * Derives the four role booleans from `account` + `proxyAdmin` + a precomputed - * `isFeeAdmin`. Pure — no side effects, no wagmi calls. + * Derives the four role booleans from `account` + on-chain + * `isProxyAdmin(account)` lookup + a precomputed `isFeeAdmin`. + * + * Role 1 became a whitelist (post 2-step retirement), so a single + * `proxyAdmin` address comparison is no longer enough — we hit the + * contract's `isProxyAdmin(addr)` view. */ export function useProxyRoles({ + proxy, account, - proxyAdmin, isFeeAdmin, }: { + proxy: Address | undefined account: Address | undefined - proxyAdmin: Address | undefined isFeeAdmin: boolean }): ProxyRoles { - const isProxyAdmin = Boolean( - account && proxyAdmin && account.toLowerCase() === proxyAdmin.toLowerCase(), - ) + const result = useReadContract({ + abi: IntuitionVersionedFeeProxyABI as any, + address: proxy, + functionName: 'isProxyAdmin', + args: account ? [account] : undefined, + query: { enabled: Boolean(proxy && account) }, + }) + const isProxyAdmin = Boolean(result.data) const hasBothRoles = isProxyAdmin && isFeeAdmin const isViewer = !isFeeAdmin && !isProxyAdmin diff --git a/packages/webapp/src/hooks/useVersionedProxy.ts b/packages/webapp/src/hooks/useVersionedProxy.ts index 4a8fed1..9937268 100644 --- a/packages/webapp/src/hooks/useVersionedProxy.ts +++ b/packages/webapp/src/hooks/useVersionedProxy.ts @@ -1,5 +1,18 @@ -import { useReadContract, useReadContracts, useWriteContract } from 'wagmi' -import { hexToString, stringToHex, type Address, type Hex } from 'viem' +import { useEffect, useState } from 'react' +import { + useBlockNumber, + usePublicClient, + useReadContract, + useReadContracts, + useWriteContract, +} from 'wagmi' +import { + getAddress, + hexToString, + stringToHex, + type Address, + type Hex, +} from 'viem' import { IntuitionVersionedFeeProxyABI } from '@intuition-fee-proxy/sdk' @@ -10,15 +23,13 @@ export function useProxyVersions(proxy: Address | undefined) { contracts: [ { abi, address: proxy, functionName: 'getVersions' }, { abi, address: proxy, functionName: 'getDefaultVersion' }, - { abi, address: proxy, functionName: 'proxyAdmin' }, - { abi, address: proxy, functionName: 'pendingProxyAdmin' }, + { abi, address: proxy, functionName: 'proxyAdminCount' }, ], allowFailure: false, query: { enabled: Boolean(proxy), - // Auto-poll so `proxyAdmin` / `pendingProxyAdmin` reflect - // acceptance that happens from another wallet or tab without - // forcing the user to refresh. + // Auto-poll so the admin count reflects grants/revokes happening + // from another wallet or tab without forcing the user to refresh. refetchInterval: 10_000, }, }) @@ -27,15 +38,14 @@ export function useProxyVersions(proxy: Address | undefined) { ...result, versions: (result.data?.[0] as Hex[] | undefined) ?? [], defaultVersion: result.data?.[1] as Hex | undefined, - proxyAdmin: result.data?.[2] as Address | undefined, - pendingProxyAdmin: result.data?.[3] as Address | undefined, + proxyAdminCount: (result.data?.[2] as bigint | undefined) ?? 0n, } } /** * Cheap 1-read hook for pages that only need the currently-active version - * label (Explore card, etc.). Avoids the 3-read overhead of - * `useProxyVersions` when the versions list / proxyAdmin aren't needed. + * label (Explore card, etc.). Avoids the overhead of `useProxyVersions` + * when the versions list / admin count aren't needed. * * Decodes the bytes32 to a human-readable label ("v2.0.0"). Empty string * if the proxy has no default set yet (shouldn't happen — Factory always @@ -108,44 +118,130 @@ export function useSetDefaultVersion(proxy: Address | undefined) { } /** - * Step 1 of the 2-step proxy-admin transfer. Only callable by the current - * `proxyAdmin`. Sets `pendingProxyAdmin = newAdmin`; the target must then - * call `acceptProxyAdmin()` from their own wallet to finalise. Passing a - * wrong address is recoverable — just call again with the correct one. + * Grant or revoke the Role 1 (proxyAdmin) whitelist for an address. + * Mirrors the Role 2 `setWhitelistedAdmin` pattern — instant, no 2-step + * ceremony. The contract enforces: + * - idempotent reject (revert if status already matches) + * - last-admin guard (revert if revoke would empty the whitelist) */ -export function useTransferProxyAdmin(proxy: Address | undefined) { +export function useSetProxyAdmin(proxy: Address | undefined) { const { writeContractAsync, data, isPending, error, reset } = useWriteContract() - function transferAdmin(newAdmin: Address) { + function setProxyAdmin(admin: Address, status: boolean) { if (!proxy) throw new Error('Proxy address missing') return writeContractAsync({ abi, address: proxy, - functionName: 'transferProxyAdmin', - args: [newAdmin], + functionName: 'setProxyAdmin', + args: [admin, status], }) } - return { transferAdmin, hash: data, isPending, error, reset } + return { setProxyAdmin, hash: data, isPending, error, reset } } /** - * Step 2 of the 2-step proxy-admin transfer. Must be called by the address - * currently set as `pendingProxyAdmin`. Promotes caller to `proxyAdmin`. + * Reconstruct the current proxyAdmin whitelist from the on-chain + * `ProxyAdminGranted` / `ProxyAdminRevoked` event log. Mirrors the + * Role 2 `useAdmins` pattern — the contract doesn't expose a getter + * for the full list, so we reduce events. */ -export function useAcceptProxyAdmin(proxy: Address | undefined) { - const { writeContractAsync, data, isPending, error, reset } = useWriteContract() +export function useProxyAdmins(proxy: Address | undefined) { + const publicClient = usePublicClient() + const { data: currentBlock } = useBlockNumber({ watch: true }) + const [admins, setAdmins] = useState([]) + const [isLoading, setIsLoading] = useState(false) + const [error, setError] = useState(null) + const [refreshKey, setRefreshKey] = useState(0) - function acceptAdmin() { - if (!proxy) throw new Error('Proxy address missing') - return writeContractAsync({ - abi, - address: proxy, - functionName: 'acceptProxyAdmin', - }) - } + useEffect(() => { + if (!publicClient || !proxy || !currentBlock) return + let cancelled = false + setIsLoading(true) + setError(null) + Promise.all([ + publicClient.getLogs({ + address: proxy, + event: { + type: 'event', + name: 'ProxyAdminGranted', + inputs: [{ type: 'address', name: 'admin', indexed: true }], + }, + fromBlock: 0n, + toBlock: currentBlock, + }), + publicClient.getLogs({ + address: proxy, + event: { + type: 'event', + name: 'ProxyAdminRevoked', + inputs: [{ type: 'address', name: 'admin', indexed: true }], + }, + fromBlock: 0n, + toBlock: currentBlock, + }), + ]) + .then(([grants, revokes]) => { + if (cancelled) return + type LogShape = { args: { admin: Address }; blockNumber: bigint; logIndex: number } + // Order all events by (block, logIndex) and replay them. + const events: Array<{ admin: Address; granted: boolean; bn: bigint; li: number }> = + [ + ...grants.map((l) => { + const x = l as unknown as LogShape + return { + admin: x.args.admin, + granted: true, + bn: x.blockNumber, + li: x.logIndex, + } + }), + ...revokes.map((l) => { + const x = l as unknown as LogShape + return { + admin: x.args.admin, + granted: false, + bn: x.blockNumber, + li: x.logIndex, + } + }), + ].sort((a, b) => (a.bn === b.bn ? a.li - b.li : a.bn < b.bn ? -1 : 1)) + + const set = new Set() + for (const e of events) { + const a = getAddress(e.admin) + if (e.granted) set.add(a) + else set.delete(a) + } + setAdmins(Array.from(set) as Address[]) + setIsLoading(false) + }) + .catch((e) => { + if (cancelled) return + setError(e as Error) + setIsLoading(false) + }) + return () => { + cancelled = true + } + }, [publicClient, proxy, currentBlock ? Number(currentBlock) : 0, refreshKey]) - return { acceptAdmin, hash: data, isPending, error, reset } + return { admins, isLoading, error, refetch: () => setRefreshKey((k) => k + 1) } +} + +/** Read whether a given address is currently a proxyAdmin. */ +export function useIsProxyAdmin( + proxy: Address | undefined, + candidate: Address | undefined, +) { + const result = useReadContract({ + abi, + address: proxy, + functionName: 'isProxyAdmin', + args: candidate ? [candidate] : undefined, + query: { enabled: Boolean(proxy && candidate) }, + }) + return { ...result, isProxyAdmin: Boolean(result.data) } } /** Read the proxy's human-readable name (bytes32, decoded to string). */ diff --git a/packages/webapp/src/pages/Docs.tsx b/packages/webapp/src/pages/Docs.tsx index 22b2085..c62096a 100644 --- a/packages/webapp/src/pages/Docs.tsx +++ b/packages/webapp/src/pages/Docs.tsx @@ -504,8 +504,8 @@ function Architecture() { } /> -

Proxy admin (single address / Safe)

+

Proxy admins (whitelisted)

- Both admin roles are rotatable, but the mechanics differ on - purpose — the slower path covers the higher-impact role. + Both admin roles are whitelists with the same shape — N addresses, + instant grant/revoke, last-admin guard. The 2-step ceremony Role + 1 used to have was retired in favour of the multi-admin model + (any current admin can grant a replacement, then the old admin + revokes itself).

- Current admin calls transferProxyAdmin(newAdmin), - which only sets pendingProxyAdmin — no powers - move. The target must then sign{' '} - acceptProxyAdmin() from their own wallet to - finalise. Until then the outgoing admin keeps full powers - and can overwrite the pending candidate. + Whitelist of upgrade-authority addresses.{' '} + setProxyAdmin(addr, true/false) grants or + revokes in a single tx. Any current proxyAdmin can mutate + the list. The contract reverts if status already matches + and refuses to revoke the last remaining admin. } /> @@ -986,10 +988,10 @@ function AdminRotation() { term="Role 2 — fee admins (instant, N addresses)" desc={ <> - Whitelist-style. setWhitelistedAdmin(addr, true/false){' '} - adds or removes in a single tx. Any fee admin can grant or - revoke any other — except the last one cannot self-revoke. - Multiple fee admins can coexist; all share the same powers. + Same shape as Role 1, different surface.{' '} + setWhitelistedAdmin(addr, true/false) mutates + the fee-admin whitelist. Any fee admin can grant or revoke + any other — except the last one cannot self-revoke. } /> @@ -997,15 +999,13 @@ function AdminRotation() { term="Convenience — Grant both roles" desc={ <> - When a single wallet holds both roles and wants to hand the - whole proxy off, the webapp exposes a combined flow that - fires setWhitelistedAdmin(new, true) then{' '} - transferProxyAdmin(new) back-to-back (2 sigs, - 1 click). The target still has to{' '} - acceptProxyAdmin() from their side; acceptance - is auto-detected via the on-chain state poll. The outgoing - admin optionally revokes themselves as fee admin from the - new owner's wallet afterwards. + When a single wallet holds both roles and wants to hand + the whole proxy off, the webapp exposes a combined flow + that fires setProxyAdmin(new, true) then{' '} + setWhitelistedAdmin(new, true) back-to-back + (2 sigs, 1 click). Both grants are instant. The outgoing + admin optionally revokes itself from each whitelist + afterwards. } /> diff --git a/packages/webapp/src/pages/ProxyDetail.tsx b/packages/webapp/src/pages/ProxyDetail.tsx index 6fcd16b..4f67531 100644 --- a/packages/webapp/src/pages/ProxyDetail.tsx +++ b/packages/webapp/src/pages/ProxyDetail.tsx @@ -59,8 +59,6 @@ function ProxyDetail({ proxy }: { proxy: Address }) { const { versions, defaultVersion, - proxyAdmin, - pendingProxyAdmin, refetch: refetchVersions, isFetching: isVersionsFetching, } = useProxyVersions(proxy) @@ -78,8 +76,8 @@ function ProxyDetail({ proxy }: { proxy: Address }) { const family: ProxyFamily = channel === 'sponsored' ? 'sponsored' : 'standard' const { isProxyAdmin, isViewer } = useProxyRoles({ + proxy, account, - proxyAdmin, isFeeAdmin, }) @@ -173,8 +171,6 @@ function ProxyDetail({ proxy }: { proxy: Address }) { {tab === 'admins' && (