From d94dcc7f53cd2eb717f8dc438a129d15fa1b89e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8r=E2=88=82=C2=A1?= Date: Tue, 5 May 2026 17:05:00 +0000 Subject: [PATCH 01/31] Amending the order of evalRule where who --- .../condition/extensions/RuledCondition.sol | 2 +- test/other/RuledCondition.t.sol | 277 ++++++++++++++++++ 2 files changed, 278 insertions(+), 1 deletion(-) create mode 100644 test/other/RuledCondition.t.sol diff --git a/src/common/permission/condition/extensions/RuledCondition.sol b/src/common/permission/condition/extensions/RuledCondition.sol index af59068ac..2f9954b00 100644 --- a/src/common/permission/condition/extensions/RuledCondition.sol +++ b/src/common/permission/condition/extensions/RuledCondition.sol @@ -178,7 +178,7 @@ abstract contract RuledCondition is PermissionConditionUpgradeable { uint32 ruleIndexOnSuccess, uint32 ruleIndexOnFailure ) = decodeRuleValue(uint256(_rule.value)); - bool result = _evalRule(currentRuleIndex, _who, _where, _permissionId, _compareList); + bool result = _evalRule(currentRuleIndex, _where, _who, _permissionId, _compareList); return _evalRule( diff --git a/test/other/RuledCondition.t.sol b/test/other/RuledCondition.t.sol new file mode 100644 index 000000000..abc2c20e8 --- /dev/null +++ b/test/other/RuledCondition.t.sol @@ -0,0 +1,277 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; + +import {RuledCondition} from "../../src/common/permission/condition/extensions/RuledCondition.sol"; +import {PermissionCondition} from "../../src/common/permission/condition/PermissionCondition.sol"; + +/// @notice Concrete `RuledCondition` for tests. Exposes `_updateRules` and +/// drives `_evalRule(0, ...)` from a public entrypoint. +contract RuledConditionHarness is RuledCondition { + function setRules(Rule[] memory _rules) external { + _updateRules(_rules); + } + + function isGranted( + address _where, + address _who, + bytes32 _permissionId, + bytes calldata _data + ) external view returns (bool) { + uint256[] memory compareList; + if (_data.length > 0) { + compareList = abi.decode(_data, (uint256[])); + } + return _evalRule(0, _where, _who, _permissionId, compareList); + } +} + +/// @notice Returns true only when `(_where, _who)` matches the configured pair. +/// Asymmetry is the point: any swap in upstream evaluation flips the verdict. +contract AddressCheckConditionMock is PermissionCondition { + address public expectedWhere; + address public expectedWho; + + function setExpected( + address _expectedWhere, + address _expectedWho + ) external { + expectedWhere = _expectedWhere; + expectedWho = _expectedWho; + } + + function isGranted( + address _where, + address _who, + bytes32, + bytes memory + ) external view returns (bool) { + return _where == expectedWhere && _who == expectedWho; + } +} + +contract RuledConditionEvalLogicTest is Test { + // RuledCondition's id/op constants are private; redeclare with the exact + // values from `RuledCondition.sol` rather than reach into private storage. + // A renumbering upstream would itself be a bug worth catching. + uint8 internal constant CONDITION_RULE_ID = 202; + uint8 internal constant LOGIC_OP_RULE_ID = 203; + uint8 internal constant VALUE_RULE_ID = 204; + + uint8 internal constant OP_EQ = 1; // Op.EQ + uint8 internal constant OP_RET = 7; // Op.RET + uint8 internal constant OP_AND = 9; // Op.AND + uint8 internal constant OP_IF_ELSE = 12; // Op.IF_ELSE + + RuledConditionHarness internal harness; + AddressCheckConditionMock internal predicate; + + address internal expectedWhere = makeAddr("expectedWhere"); + address internal expectedWho = makeAddr("expectedWho"); + address internal someoneElse = makeAddr("someoneElse"); + + bytes32 internal constant PERM = keccak256("SOME_PERMISSION"); + + function setUp() public { + harness = new RuledConditionHarness(); + predicate = new AddressCheckConditionMock(); + predicate.setExpected(expectedWhere, expectedWho); + } + + /// @notice Loads an IF_ELSE rule whose predicate is the asymmetric + /// AddressCheckConditionMock. + /// Rules: + /// [0] IF_ELSE(predicate=1, success=2, failure=3) + /// [1] CONDITION(predicate) op=EQ -- predicate's verdict cast against `comparedTo == 1` + /// [2] VALUE(1) op=RET -- success branch returns true + /// [3] VALUE(0) op=RET -- failure branch returns false + function _loadIfElseRules() internal { + RuledCondition.Rule[] memory rules = new RuledCondition.Rule[](4); + rules[0] = RuledCondition.Rule({ + id: LOGIC_OP_RULE_ID, + op: OP_IF_ELSE, + value: harness.encodeIfElse(1, 2, 3), + permissionId: PERM + }); + rules[1] = RuledCondition.Rule({ + id: CONDITION_RULE_ID, + op: OP_EQ, + value: uint240(uint160(address(predicate))), + permissionId: PERM + }); + rules[2] = RuledCondition.Rule({ + id: VALUE_RULE_ID, + op: OP_RET, + value: 1, + permissionId: PERM + }); + rules[3] = RuledCondition.Rule({ + id: VALUE_RULE_ID, + op: OP_RET, + value: 0, + permissionId: PERM + }); + harness.setRules(rules); + } + + // ------------------------------------------------------------------------- + // C-1 regression + // ------------------------------------------------------------------------- + + /// @notice With the args in the correct order, the predicate matches and + /// the success branch fires. Sanity check that the rules are wired. + function test_C1_IfElsePredicate_RoutesToSuccessOnMatch() public { + _loadIfElseRules(); + assertTrue( + harness.isGranted(expectedWhere, expectedWho, PERM, bytes("")), + "predicate matches: success branch should return true" + ); + } + + /// @notice With a non-matching `_who`, the predicate fails and the + /// failure branch fires. Sanity check. + function test_C1_IfElsePredicate_RoutesToFailureOnMismatch() public { + _loadIfElseRules(); + assertFalse( + harness.isGranted(expectedWhere, someoneElse, PERM, bytes("")), + "predicate does not match: failure branch should return false" + ); + } + + /// @notice C-1: with `(_where, _who)` swapped at the call site, the + /// predicate must NOT match. Pre-fix, `_evalLogic` swaps them again + /// internally (line 181) and the asymmetric predicate sees the original + /// pair, returning true and routing to the success branch. Post-fix the + /// swap is gone and the predicate correctly sees `(expectedWho, expectedWhere)`, + /// which does not match. + function test_C1_IfElsePredicate_SwappedArgsMustNotMatch() public { + _loadIfElseRules(); + assertFalse( + harness.isGranted( + expectedWho, // _where -- intentionally swapped + expectedWhere, // _who -- intentionally swapped + PERM, + bytes("") + ), + "swapped args must not satisfy the asymmetric predicate" + ); + } + + // ------------------------------------------------------------------------- + // C-1 propagation through an intermediate AND/OR layer in the predicate. + // AND/OR don't introduce their own swap, so the bug carries through. + // ------------------------------------------------------------------------- + + function _loadIfElseThroughAndRules() internal { + // Rules: + // [0] IF_ELSE(predicate=1, success=2, failure=3) + // [1] AND(rule=4, rule=5) -- predicate via AND + // [2] VALUE(1) op=RET -- success + // [3] VALUE(0) op=RET -- failure + // [4] CONDITION(predicate) op=EQ + // [5] VALUE(1) op=RET -- AND collapses to rule 4 + RuledCondition.Rule[] memory rules = new RuledCondition.Rule[](6); + rules[0] = RuledCondition.Rule({ + id: LOGIC_OP_RULE_ID, + op: OP_IF_ELSE, + value: harness.encodeIfElse(1, 2, 3), + permissionId: PERM + }); + rules[1] = RuledCondition.Rule({ + id: LOGIC_OP_RULE_ID, + op: OP_AND, + value: harness.encodeLogicalOperator(4, 5), + permissionId: PERM + }); + rules[2] = RuledCondition.Rule({ + id: VALUE_RULE_ID, + op: OP_RET, + value: 1, + permissionId: PERM + }); + rules[3] = RuledCondition.Rule({ + id: VALUE_RULE_ID, + op: OP_RET, + value: 0, + permissionId: PERM + }); + rules[4] = RuledCondition.Rule({ + id: CONDITION_RULE_ID, + op: OP_EQ, + value: uint240(uint160(address(predicate))), + permissionId: PERM + }); + rules[5] = RuledCondition.Rule({ + id: VALUE_RULE_ID, + op: OP_RET, + value: 1, + permissionId: PERM + }); + harness.setRules(rules); + } + + function test_C1_IfElseThroughAnd_RoutesToSuccessOnMatch() public { + _loadIfElseThroughAndRules(); + assertTrue( + harness.isGranted(expectedWhere, expectedWho, PERM, bytes("")), + "AND-wrapped predicate matches: success branch should return true" + ); + } + + function test_C1_IfElseThroughAnd_SwappedArgsMustNotMatch() public { + _loadIfElseThroughAndRules(); + assertFalse( + harness.isGranted(expectedWho, expectedWhere, PERM, bytes("")), + "C-1 (through AND): swapped args must not satisfy the asymmetric predicate" + ); + } + + function test_C1_BranchEvaluation_UsesCorrectArgsForRecursion() public { + // Predicate is always-true (VALUE 1 RET), so the success branch + // always runs. The success branch is the asymmetric CONDITION; its + // verdict is the test's signal. + // + // Rules: + // [0] IF_ELSE(predicate=1, success=2, failure=3) + // [1] VALUE(1) op=RET -- predicate is always true + // [2] CONDITION(predicate) op=EQ -- success branch + // [3] VALUE(0) op=RET -- failure branch (unused) + RuledCondition.Rule[] memory rules = new RuledCondition.Rule[](4); + rules[0] = RuledCondition.Rule({ + id: LOGIC_OP_RULE_ID, + op: OP_IF_ELSE, + value: harness.encodeIfElse(1, 2, 3), + permissionId: PERM + }); + rules[1] = RuledCondition.Rule({ + id: VALUE_RULE_ID, + op: OP_RET, + value: 1, + permissionId: PERM + }); + rules[2] = RuledCondition.Rule({ + id: CONDITION_RULE_ID, + op: OP_EQ, + value: uint240(uint160(address(predicate))), + permissionId: PERM + }); + rules[3] = RuledCondition.Rule({ + id: VALUE_RULE_ID, + op: OP_RET, + value: 0, + permissionId: PERM + }); + harness.setRules(rules); + + assertTrue( + harness.isGranted(expectedWhere, expectedWho, PERM, bytes("")), + "success branch: predicate must see (expectedWhere, expectedWho)" + ); + assertFalse( + harness.isGranted(expectedWho, expectedWhere, PERM, bytes("")), + "success branch: swapped args must not satisfy the predicate" + ); + } +} From 6f010ac807e3fd945bf29636ec3fdbcd330707cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8r=E2=88=82=C2=A1?= Date: Wed, 6 May 2026 09:11:20 +0000 Subject: [PATCH 02/31] Consistent behaviour with ANY_ADDR --- src/framework/plugin/repo/PluginRepo.sol | 11 ++ test/framework/plugin/repo/PluginRepo.t.sol | 153 ++++++++++++++++++++ 2 files changed, 164 insertions(+) create mode 100644 test/framework/plugin/repo/PluginRepo.t.sol diff --git a/src/framework/plugin/repo/PluginRepo.sol b/src/framework/plugin/repo/PluginRepo.sol index 7271f3af5..ff14fb5fd 100644 --- a/src/framework/plugin/repo/PluginRepo.sol +++ b/src/framework/plugin/repo/PluginRepo.sol @@ -262,6 +262,17 @@ contract PluginRepo is address ) internal virtual override auth(UPGRADE_REPO_PERMISSION_ID) {} + /// @inheritdoc PermissionManager + /// @dev Mirrors DAO.sol — block ANY_ADDR grants for permissions whose + /// compromise propagates to every consumer DAO of this repo. + function isPermissionRestrictedForAnyAddr( + bytes32 _permissionId + ) internal pure override returns (bool) { + return + _permissionId == MAINTAINER_PERMISSION_ID || + _permissionId == UPGRADE_REPO_PERMISSION_ID; + } + /// @notice Checks if this or the parent contract supports an interface by its ID. /// @param _interfaceId The ID of the interface. /// @return Returns `true` if the interface is supported. diff --git a/test/framework/plugin/repo/PluginRepo.t.sol b/test/framework/plugin/repo/PluginRepo.t.sol new file mode 100644 index 000000000..0722e0bac --- /dev/null +++ b/test/framework/plugin/repo/PluginRepo.t.sol @@ -0,0 +1,153 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; + +import {PluginRepo} from "../../../../src/framework/plugin/repo/PluginRepo.sol"; +import {PermissionManager} from "../../../../src/core/permission/PermissionManager.sol"; +import {IPermissionCondition} from "../../../../src/common/permission/condition/IPermissionCondition.sol"; +import {PermissionConditionMock} from "../../../mocks/permission/PermissionConditionMock.sol"; + +/// @notice Regression coverage for finding C-2: `PluginRepo` must override +/// `isPermissionRestrictedForAnyAddr` so that the dangerous `MAINTAINER` and +/// `UPGRADE_REPO` permissions cannot be granted to `ANY_ADDR`. Mirrors the +/// `DAO.sol` defense-in-depth pattern. +contract PluginRepoAnyAddrRestrictionTest is Test { + address internal constant ANY_ADDR = address(type(uint160).max); + + bytes32 internal constant ROOT_PERMISSION_ID = keccak256("ROOT_PERMISSION"); + bytes32 internal constant MAINTAINER_PERMISSION_ID = keccak256("MAINTAINER_PERMISSION"); + bytes32 internal constant UPGRADE_REPO_PERMISSION_ID = keccak256("UPGRADE_REPO_PERMISSION"); + // An arbitrary permission that is NOT in the restricted set — used to + // confirm the override is selective rather than a blanket lock-out. + bytes32 internal constant CUSTOM_PERMISSION_ID = keccak256("CUSTOM_PERMISSION"); + + PluginRepo internal repo; + + address internal maintainer = address(0xBEEF); + address internal upgrader = address(0xC0FFEE); + address internal stranger = address(0xBAD); + + function setUp() public { + // Deploy the repo behind a UUPS proxy with this test contract as the + // initial owner. `PluginRepo.initialize` grants ROOT, MAINTAINER, and + // UPGRADE_REPO to the initial owner — so this contract can call + // `grant`/`grantWithCondition` directly. + PluginRepo impl = new PluginRepo(); + repo = PluginRepo( + address(new ERC1967Proxy(address(impl), abi.encodeCall(PluginRepo.initialize, (address(this))))) + ); + } + + // ------------------------------------------------------------------------- + // C-2 regression — bare grants to ANY_ADDR + // ------------------------------------------------------------------------- + + function test_C2_GrantMaintainerToAnyAddr_Reverts() public { + vm.expectRevert(PermissionManager.PermissionsForAnyAddressDisallowed.selector); + repo.grant(address(repo), ANY_ADDR, MAINTAINER_PERMISSION_ID); + } + + function test_C2_GrantUpgradeRepoToAnyAddr_Reverts() public { + vm.expectRevert(PermissionManager.PermissionsForAnyAddressDisallowed.selector); + repo.grant(address(repo), ANY_ADDR, UPGRADE_REPO_PERMISSION_ID); + } + + // ------------------------------------------------------------------------- + // C-2 regression — conditional grants to ANY_ADDR + // ------------------------------------------------------------------------- + // + // The same restriction applies to `grantWithCondition` because + // `PermissionManager._grantWithCondition` consults the same + // `isPermissionRestrictedForAnyAddr` hook (see `PermissionManager.sol:412-419`). + + function test_C2_GrantWithConditionMaintainerToAnyAddr_Reverts() public { + PermissionConditionMock cond = new PermissionConditionMock(); + vm.expectRevert(PermissionManager.PermissionsForAnyAddressDisallowed.selector); + repo.grantWithCondition(address(repo), ANY_ADDR, MAINTAINER_PERMISSION_ID, IPermissionCondition(address(cond))); + } + + function test_C2_GrantWithConditionUpgradeRepoToAnyAddr_Reverts() public { + PermissionConditionMock cond = new PermissionConditionMock(); + vm.expectRevert(PermissionManager.PermissionsForAnyAddressDisallowed.selector); + repo.grantWithCondition( + address(repo), ANY_ADDR, UPGRADE_REPO_PERMISSION_ID, IPermissionCondition(address(cond)) + ); + } + + // ------------------------------------------------------------------------- + // ROOT to ANY_ADDR is already blocked by the base `PermissionManager._grant` + // — guard against an accidental loosening when the override is added. + // ------------------------------------------------------------------------- + + function test_C2_GrantRootToAnyAddr_StillReverts() public { + vm.expectRevert(PermissionManager.PermissionsForAnyAddressDisallowed.selector); + repo.grant(address(repo), ANY_ADDR, ROOT_PERMISSION_ID); + } + + // ------------------------------------------------------------------------- + // The override is selective: permissions outside the restricted set must + // continue to support `ANY_ADDR` (otherwise the override would be a + // blanket lock-out and would break legitimate downstream patterns). + // ------------------------------------------------------------------------- + + function test_C2_GrantOtherPermissionToAnyAddr_StillSucceeds() public { + repo.grant(address(repo), ANY_ADDR, CUSTOM_PERMISSION_ID); + // `isGranted` against a stranger should now return true via the + // `ANY_ADDR` lookup path in `PermissionManager.isGranted`. + assertTrue( + repo.isGranted(address(repo), stranger, CUSTOM_PERMISSION_ID, bytes("")), + "Non-restricted permission must still flow through ANY_ADDR." + ); + } + + // ------------------------------------------------------------------------- + // Happy path: specific-address grants of the now-restricted permissions + // continue to work. Confirms the fix only narrows ANY_ADDR, not specific + // grants — otherwise legitimate maintainer onboarding would break. + // ------------------------------------------------------------------------- + + function test_C2_GrantMaintainerToSpecificAddress_StillSucceeds() public { + repo.grant(address(repo), maintainer, MAINTAINER_PERMISSION_ID); + assertTrue( + repo.isGranted(address(repo), maintainer, MAINTAINER_PERMISSION_ID, bytes("")), + "Specific MAINTAINER grant must succeed." + ); + // Non-grantee must NOT inherit the permission via ANY_ADDR. + assertFalse( + repo.isGranted(address(repo), stranger, MAINTAINER_PERMISSION_ID, bytes("")), + "Stranger must not have MAINTAINER without an explicit grant." + ); + } + + function test_C2_GrantUpgradeRepoToSpecificAddress_StillSucceeds() public { + repo.grant(address(repo), upgrader, UPGRADE_REPO_PERMISSION_ID); + assertTrue( + repo.isGranted(address(repo), upgrader, UPGRADE_REPO_PERMISSION_ID, bytes("")), + "Specific UPGRADE_REPO grant must succeed." + ); + assertFalse( + repo.isGranted(address(repo), stranger, UPGRADE_REPO_PERMISSION_ID, bytes("")), + "Stranger must not have UPGRADE_REPO without an explicit grant." + ); + } + + function test_C2_GrantWithConditionMaintainerToSpecificAddress_StillSucceeds() public { + PermissionConditionMock cond = new PermissionConditionMock(); + cond.setAnswer(true); + repo.grantWithCondition( + address(repo), maintainer, MAINTAINER_PERMISSION_ID, IPermissionCondition(address(cond)) + ); + assertTrue( + repo.isGranted(address(repo), maintainer, MAINTAINER_PERMISSION_ID, bytes("")), + "Specific conditional MAINTAINER grant must succeed when the condition returns true." + ); + cond.setAnswer(false); + assertFalse( + repo.isGranted(address(repo), maintainer, MAINTAINER_PERMISSION_ID, bytes("")), + "Specific conditional MAINTAINER grant must respect the condition." + ); + } +} From f1d38fee63514888b9b6f76022cc8cdad2aebc39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8r=E2=88=82=C2=A1?= Date: Wed, 6 May 2026 09:33:07 +0000 Subject: [PATCH 03/31] Updated doc --- docs/modules/api/pages/framework.adoc | 8 ++++++-- test/framework/plugin/repo/PluginRepo.t.sol | 6 +++--- test/other/RuledCondition.t.sol | 10 +++------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/modules/api/pages/framework.adoc b/docs/modules/api/pages/framework.adoc index c39e86cb4..d3eac6a08 100644 --- a/docs/modules/api/pages/framework.adoc +++ b/docs/modules/api/pages/framework.adoc @@ -107,6 +107,7 @@ :xref-PluginRepo-buildCount-uint8-: xref:framework.adoc#PluginRepo-buildCount-uint8- :xref-PluginRepo-tagHash-struct-PluginRepo-Tag-: xref:framework.adoc#PluginRepo-tagHash-struct-PluginRepo-Tag- :xref-PluginRepo-_authorizeUpgrade-address-: xref:framework.adoc#PluginRepo-_authorizeUpgrade-address- +:xref-PluginRepo-isPermissionRestrictedForAnyAddr-bytes32-: xref:framework.adoc#PluginRepo-isPermissionRestrictedForAnyAddr-bytes32- :xref-PluginRepo-supportsInterface-bytes4-: xref:framework.adoc#PluginRepo-supportsInterface-bytes4- :xref-PermissionManager-__PermissionManager_init-address-: xref:framework.adoc#PermissionManager-__PermissionManager_init-address- :xref-PermissionManager-grant-address-address-bytes32-: xref:framework.adoc#PermissionManager-grant-address-address-bytes32- @@ -122,7 +123,6 @@ :xref-PermissionManager-_revoke-address-address-bytes32-: xref:framework.adoc#PermissionManager-_revoke-address-address-bytes32- :xref-PermissionManager-_auth-bytes32-: xref:framework.adoc#PermissionManager-_auth-bytes32- :xref-PermissionManager-permissionHash-address-address-bytes32-: xref:framework.adoc#PermissionManager-permissionHash-address-address-bytes32- -:xref-PermissionManager-isPermissionRestrictedForAnyAddr-bytes32-: xref:framework.adoc#PermissionManager-isPermissionRestrictedForAnyAddr-bytes32- :xref-PluginRepo-MAINTAINER_PERMISSION_ID-bytes32: xref:framework.adoc#PluginRepo-MAINTAINER_PERMISSION_ID-bytes32 :xref-PluginRepo-UPGRADE_REPO_PERMISSION_ID-bytes32: xref:framework.adoc#PluginRepo-UPGRADE_REPO_PERMISSION_ID-bytes32 :xref-PluginRepo-buildsPerRelease-mapping-uint8----uint16-: xref:framework.adoc#PluginRepo-buildsPerRelease-mapping-uint8----uint16- @@ -1077,6 +1077,7 @@ The plugin repository contract required for managing and publishing different pl * {xref-PluginRepo-buildCount-uint8-}[`++buildCount(_release)++`] * {xref-PluginRepo-tagHash-struct-PluginRepo-Tag-}[`++tagHash(_tag)++`] * {xref-PluginRepo-_authorizeUpgrade-address-}[`++_authorizeUpgrade()++`] +* {xref-PluginRepo-isPermissionRestrictedForAnyAddr-bytes32-}[`++isPermissionRestrictedForAnyAddr(_permissionId)++`] * {xref-PluginRepo-supportsInterface-bytes4-}[`++supportsInterface(_interfaceId)++`] [.contract-subindex-inherited] @@ -1095,7 +1096,6 @@ The plugin repository contract required for managing and publishing different pl * {xref-PermissionManager-_revoke-address-address-bytes32-}[`++_revoke(_where, _who, _permissionId)++`] * {xref-PermissionManager-_auth-bytes32-}[`++_auth(_permissionId)++`] * {xref-PermissionManager-permissionHash-address-address-bytes32-}[`++permissionHash(_where, _who, _permissionId)++`] -* {xref-PermissionManager-isPermissionRestrictedForAnyAddr-bytes32-}[`++isPermissionRestrictedForAnyAddr(_permissionId)++`] [.contract-subindex-inherited] .ProtocolVersion @@ -1376,6 +1376,10 @@ Gets the total number of builds for a given release number. [[PluginRepo-_authorizeUpgrade-address-]] ==== `[.contract-item-name]#++_authorizeUpgrade++#++(address)++` [.item-kind]#internal# +[.contract-item] +[[PluginRepo-isPermissionRestrictedForAnyAddr-bytes32-]] +==== `[.contract-item-name]#++isPermissionRestrictedForAnyAddr++#++(bytes32 _permissionId) → bool++` [.item-kind]#internal# + [.contract-item] [[PluginRepo-supportsInterface-bytes4-]] ==== `[.contract-item-name]#++supportsInterface++#++(bytes4 _interfaceId) → bool++` [.item-kind]#public# diff --git a/test/framework/plugin/repo/PluginRepo.t.sol b/test/framework/plugin/repo/PluginRepo.t.sol index 0722e0bac..ac3c0995a 100644 --- a/test/framework/plugin/repo/PluginRepo.t.sol +++ b/test/framework/plugin/repo/PluginRepo.t.sol @@ -10,7 +10,7 @@ import {PermissionManager} from "../../../../src/core/permission/PermissionManag import {IPermissionCondition} from "../../../../src/common/permission/condition/IPermissionCondition.sol"; import {PermissionConditionMock} from "../../../mocks/permission/PermissionConditionMock.sol"; -/// @notice Regression coverage for finding C-2: `PluginRepo` must override +/// @notice Regression coverage: `PluginRepo` must override /// `isPermissionRestrictedForAnyAddr` so that the dangerous `MAINTAINER` and /// `UPGRADE_REPO` permissions cannot be granted to `ANY_ADDR`. Mirrors the /// `DAO.sol` defense-in-depth pattern. @@ -42,7 +42,7 @@ contract PluginRepoAnyAddrRestrictionTest is Test { } // ------------------------------------------------------------------------- - // C-2 regression — bare grants to ANY_ADDR + // bare grants to ANY_ADDR // ------------------------------------------------------------------------- function test_C2_GrantMaintainerToAnyAddr_Reverts() public { @@ -56,7 +56,7 @@ contract PluginRepoAnyAddrRestrictionTest is Test { } // ------------------------------------------------------------------------- - // C-2 regression — conditional grants to ANY_ADDR + // conditional grants to ANY_ADDR // ------------------------------------------------------------------------- // // The same restriction applies to `grantWithCondition` because diff --git a/test/other/RuledCondition.t.sol b/test/other/RuledCondition.t.sol index abc2c20e8..b68278e2f 100644 --- a/test/other/RuledCondition.t.sol +++ b/test/other/RuledCondition.t.sol @@ -116,10 +116,6 @@ contract RuledConditionEvalLogicTest is Test { harness.setRules(rules); } - // ------------------------------------------------------------------------- - // C-1 regression - // ------------------------------------------------------------------------- - /// @notice With the args in the correct order, the predicate matches and /// the success branch fires. Sanity check that the rules are wired. function test_C1_IfElsePredicate_RoutesToSuccessOnMatch() public { @@ -140,7 +136,7 @@ contract RuledConditionEvalLogicTest is Test { ); } - /// @notice C-1: with `(_where, _who)` swapped at the call site, the + /// @notice Parameters swapped at the call site, the /// predicate must NOT match. Pre-fix, `_evalLogic` swaps them again /// internally (line 181) and the asymmetric predicate sees the original /// pair, returning true and routing to the success branch. Post-fix the @@ -160,7 +156,7 @@ contract RuledConditionEvalLogicTest is Test { } // ------------------------------------------------------------------------- - // C-1 propagation through an intermediate AND/OR layer in the predicate. + // Propagation through an intermediate AND/OR layer in the predicate. // AND/OR don't introduce their own swap, so the bug carries through. // ------------------------------------------------------------------------- @@ -224,7 +220,7 @@ contract RuledConditionEvalLogicTest is Test { _loadIfElseThroughAndRules(); assertFalse( harness.isGranted(expectedWho, expectedWhere, PERM, bytes("")), - "C-1 (through AND): swapped args must not satisfy the asymmetric predicate" + "(through AND): swapped args must not satisfy the asymmetric predicate" ); } From da9b236199e0ac1944eee3a429887e7f35e99eca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8r=E2=88=82=C2=A1?= Date: Mon, 18 May 2026 16:22:55 +0000 Subject: [PATCH 04/31] Starting to port the TypeScript tests into solidity --- .../extensions/membership/IMembership.t.sol | 36 +++ test/common/utils/deployment/ProxyLib.t.sol | 164 ++++++++++ test/common/utils/math/BitMap.t.sol | 139 +++++++++ test/common/utils/math/Ratio.t.sol | 129 ++++++++ test/common/utils/math/UncheckedMath.t.sol | 79 +++++ .../utils/versioning/ProtocolVersion.t.sol | 79 +++++ .../versioning/VersionComparisonLib.t.sol | 283 ++++++++++++++++++ 7 files changed, 909 insertions(+) create mode 100644 test/common/plugin/extensions/membership/IMembership.t.sol create mode 100644 test/common/utils/deployment/ProxyLib.t.sol create mode 100644 test/common/utils/math/BitMap.t.sol create mode 100644 test/common/utils/math/Ratio.t.sol create mode 100644 test/common/utils/math/UncheckedMath.t.sol create mode 100644 test/common/utils/versioning/ProtocolVersion.t.sol create mode 100644 test/common/utils/versioning/VersionComparisonLib.t.sol diff --git a/test/common/plugin/extensions/membership/IMembership.t.sol b/test/common/plugin/extensions/membership/IMembership.t.sol new file mode 100644 index 000000000..29fe15ca2 --- /dev/null +++ b/test/common/plugin/extensions/membership/IMembership.t.sol @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {IMembership} from "../../../../../src/common/plugin/extensions/membership/IMembership.sol"; + +/// @notice Direct tests for the `IMembership` interface in +/// `src/common/plugin/extensions/membership/IMembership.sol`. +/// +/// Ports `osx-commons/contracts/test/plugin/extensions/membership.ts`. The +/// single TS test compared `getInterfaceId(IMembership__factory)` against +/// the v1.0.0 frozen ID via a typechain import; here we replace that +/// dependency with an inline literal — drift in either direction (renamed +/// function, added function, changed signature) breaks compilation +/// of `type(IMembership).interfaceId` or the equality assertion. +contract IMembershipTest is Test { + /// Frozen iface ID introduced in v1.0.0. `IMembership` has a single + /// external function `isMember(address)`; its ERC-165 ID is that + /// function's selector. + /// Computed via `cast sig "isMember(address)"`. + bytes4 internal constant IMEMBERSHIP_V1_0_0_INTERFACE_ID = 0xa230c524; + + function test_IMembership_hasSameInterfaceIdAsV1_0_0() public pure { + assertEq(type(IMembership).interfaceId, IMEMBERSHIP_V1_0_0_INTERFACE_ID); + } + + function test_IMembership_interfaceIdIsNotEmpty() public pure { + assertTrue(type(IMembership).interfaceId != bytes4(0)); + } + + function test_IMembership_interfaceIdIsNotIERC165() public pure { + assertTrue(type(IMembership).interfaceId != type(IERC165).interfaceId); + } +} diff --git a/test/common/utils/deployment/ProxyLib.t.sol b/test/common/utils/deployment/ProxyLib.t.sol new file mode 100644 index 000000000..ecd4f090f --- /dev/null +++ b/test/common/utils/deployment/ProxyLib.t.sol @@ -0,0 +1,164 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test, Vm} from "forge-std/Test.sol"; +import {ProxyFactory} from "../../../../src/common/utils/deployment/ProxyFactory.sol"; +import {IDAO} from "../../../../src/common/dao/IDAO.sol"; +import {PluginUUPSUpgradeableMockBuild1} from "../../../mocks/commons/plugin/PluginUUPSUpgradeableMock.sol"; +import {PluginCloneableMockBuild1} from "../../../mocks/commons/plugin/PluginCloneableMock.sol"; + +/// @notice Direct tests for `ProxyFactory` and the `ProxyLib` library it +/// delegates to (`src/common/utils/deployment/`). +/// +/// Ports `osx-commons/contracts/test/utils/deployment/proxy-lib.ts` and +/// closes the gaps from `TESTS.md` §7: explicit `ProxyCreated` event +/// emission, immutable `implementation()` getter, init-revert propagation +/// (closes central flaw log F15), and distinct addresses for consecutive +/// deploys. +contract ProxyLibTest is Test { + /// A non-zero address used as the `IDAO` argument to `initialize` to + /// distinguish "uninitialized" (dao = address(0)) from "initialized" + /// (dao = fakeDao). Value chosen for visual clarity in traces. + address internal constant FAKE_DAO = 0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF; + + PluginUUPSUpgradeableMockBuild1 internal uupsImpl; + PluginCloneableMockBuild1 internal cloneableImpl; + ProxyFactory internal uupsFactory; + ProxyFactory internal cloneableFactory; + bytes internal initCalldata; + + function setUp() public { + uupsImpl = new PluginUUPSUpgradeableMockBuild1(); + cloneableImpl = new PluginCloneableMockBuild1(); + uupsFactory = new ProxyFactory(address(uupsImpl)); + cloneableFactory = new ProxyFactory(address(cloneableImpl)); + + // Both mocks expose `initialize(IDAO)` with identical signatures; the same + // calldata works for either factory. + initCalldata = abi.encodeCall(PluginUUPSUpgradeableMockBuild1.initialize, (IDAO(FAKE_DAO))); + } + + // ------------------------------------------------------------------------- + // implementation() — immutable getter + // ------------------------------------------------------------------------- + + function test_implementation_returnsConstructorArg() public view { + assertEq(uupsFactory.implementation(), address(uupsImpl)); + assertEq(cloneableFactory.implementation(), address(cloneableImpl)); + } + + // ------------------------------------------------------------------------- + // deployUUPSProxy + // ------------------------------------------------------------------------- + + function test_deployUUPSProxy_initializedWhenCalldataProvided() public { + address proxyAddr = uupsFactory.deployUUPSProxy(initCalldata); + PluginUUPSUpgradeableMockBuild1 proxy = PluginUUPSUpgradeableMockBuild1(proxyAddr); + + assertEq(proxy.implementation(), uupsFactory.implementation()); + assertEq(proxy.implementation(), address(uupsImpl)); + assertEq(address(proxy.dao()), FAKE_DAO); + assertEq(proxy.state1(), 1); + } + + function test_deployUUPSProxy_uninitializedWhenNoCalldata() public { + address proxyAddr = uupsFactory.deployUUPSProxy(""); + PluginUUPSUpgradeableMockBuild1 proxy = PluginUUPSUpgradeableMockBuild1(proxyAddr); + + assertEq(proxy.implementation(), address(uupsImpl)); + assertEq(address(proxy.dao()), address(0)); + assertEq(proxy.state1(), 0); + } + + function test_deployUUPSProxy_emitsProxyCreatedWithDeployedAddress() public { + vm.recordLogs(); + address proxy = uupsFactory.deployUUPSProxy(""); + Vm.Log[] memory logs = vm.getRecordedLogs(); + + bytes32 expectedTopic = keccak256("ProxyCreated(address)"); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].topics[0] == expectedTopic && logs[i].emitter == address(uupsFactory)) { + address emittedProxy = abi.decode(logs[i].data, (address)); + assertEq(emittedProxy, proxy); + found = true; + break; + } + } + assertTrue(found, "ProxyCreated not emitted by uupsFactory"); + } + + function test_deployUUPSProxy_revertsIfInitCalldataReverts() public { + // F15: an init that targets a non-existent selector reverts inside the + // proxy's constructor delegatecall — the entire deployUUPSProxy call + // must revert (no silent partial proxy creation). + bytes memory bogus = abi.encodeWithSelector(bytes4(keccak256("doesNotExist()"))); + vm.expectRevert(); + uupsFactory.deployUUPSProxy(bogus); + } + + function test_deployUUPSProxy_consecutiveDeploysProduceDistinctAddresses() public { + address a = uupsFactory.deployUUPSProxy(""); + address b = uupsFactory.deployUUPSProxy(""); + address c = uupsFactory.deployUUPSProxy(initCalldata); + assertTrue(a != b); + assertTrue(b != c); + assertTrue(a != c); + } + + // ------------------------------------------------------------------------- + // deployMinimalProxy + // ------------------------------------------------------------------------- + + function test_deployMinimalProxy_initializedWhenCalldataProvided() public { + address proxyAddr = cloneableFactory.deployMinimalProxy(initCalldata); + PluginCloneableMockBuild1 proxy = PluginCloneableMockBuild1(proxyAddr); + + assertEq(address(proxy.dao()), FAKE_DAO); + assertEq(proxy.state1(), 1); + } + + function test_deployMinimalProxy_uninitializedWhenNoCalldata() public { + address proxyAddr = cloneableFactory.deployMinimalProxy(""); + PluginCloneableMockBuild1 proxy = PluginCloneableMockBuild1(proxyAddr); + + assertEq(address(proxy.dao()), address(0)); + assertEq(proxy.state1(), 0); + } + + function test_deployMinimalProxy_emitsProxyCreatedWithDeployedAddress() public { + vm.recordLogs(); + address proxy = cloneableFactory.deployMinimalProxy(""); + Vm.Log[] memory logs = vm.getRecordedLogs(); + + bytes32 expectedTopic = keccak256("ProxyCreated(address)"); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].topics[0] == expectedTopic && logs[i].emitter == address(cloneableFactory)) { + address emittedProxy = abi.decode(logs[i].data, (address)); + assertEq(emittedProxy, proxy); + found = true; + break; + } + } + assertTrue(found, "ProxyCreated not emitted by cloneableFactory"); + } + + function test_deployMinimalProxy_revertsIfInitCalldataReverts() public { + // F15: bogus selector in init calldata reverts inside the post-clone + // functionCall — the entire deployMinimalProxy reverts. + bytes memory bogus = abi.encodeWithSelector(bytes4(0xdeadbeef)); + vm.expectRevert(); + cloneableFactory.deployMinimalProxy(bogus); + } + + function test_deployMinimalProxy_consecutiveDeploysProduceDistinctAddresses() public { + address a = cloneableFactory.deployMinimalProxy(""); + address b = cloneableFactory.deployMinimalProxy(""); + address c = cloneableFactory.deployMinimalProxy(initCalldata); + assertTrue(a != b); + assertTrue(b != c); + assertTrue(a != c); + } +} diff --git a/test/common/utils/math/BitMap.t.sol b/test/common/utils/math/BitMap.t.sol new file mode 100644 index 000000000..8d348039a --- /dev/null +++ b/test/common/utils/math/BitMap.t.sol @@ -0,0 +1,139 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {hasBit, flipBit} from "../../../../src/common/utils/math/BitMap.sol"; + +/// @notice Direct tests for the `hasBit` / `flipBit` file-level functions +/// in `src/common/utils/math/BitMap.sol`. +/// +/// Ports `osx-commons/contracts/test/utils/math/bitmap.ts` and adds the +/// gap-analysis items recorded in `TESTS.md` §1 (boundary at index 255, +/// bit-position isolation, and three cross-function invariants expressed +/// as fuzz tests). +contract BitMapTest is Test { + uint256 internal constant ZEROS = 0; + uint256 internal constant ONES = type(uint256).max; + + // ------------------------------------------------------------------------- + // hasBit + // ------------------------------------------------------------------------- + + function test_hasBit_exampleBitmapValue6() public pure { + // 6 == 0b0110 — only indices 1 and 2 are set. + uint256 bm = 6; + assertFalse(hasBit(bm, 0)); + assertTrue(hasBit(bm, 1)); + assertTrue(hasBit(bm, 2)); + assertFalse(hasBit(bm, 3)); + } + + function test_hasBit_allZeros() public pure { + // Exhaustive — `index` is `uint8`, so 256 values cover the entire input space. + for (uint256 i = 0; i < 256; i++) { + assertFalse(hasBit(ZEROS, uint8(i))); + } + } + + function test_hasBit_allOnes() public pure { + for (uint256 i = 0; i < 256; i++) { + assertTrue(hasBit(ONES, uint8(i))); + } + } + + function test_hasBit_maxIndex255() public pure { + // Boundary at uint8(255) — the MSB of a 256-bit word. + uint256 onlyTopBit = uint256(1) << 255; + assertTrue(hasBit(onlyTopBit, 255)); + assertFalse(hasBit(onlyTopBit, 0)); + assertFalse(hasBit(onlyTopBit, 1)); + assertFalse(hasBit(onlyTopBit, 254)); + } + + function test_hasBit_isolatesIndividualBits() public pure { + // 0xAA == 0b10101010 — alternating bits over indices 0..7. + uint256 bm = 0xAA; + assertFalse(hasBit(bm, 0)); + assertTrue(hasBit(bm, 1)); + assertFalse(hasBit(bm, 2)); + assertTrue(hasBit(bm, 3)); + assertFalse(hasBit(bm, 4)); + assertTrue(hasBit(bm, 5)); + assertFalse(hasBit(bm, 6)); + assertTrue(hasBit(bm, 7)); + // Indices above the populated nibble are zero. + assertFalse(hasBit(bm, 8)); + assertFalse(hasBit(bm, 255)); + } + + // ------------------------------------------------------------------------- + // flipBit + // ------------------------------------------------------------------------- + + function test_flipBit_zerosToOnes() public pure { + uint256 bm = ZEROS; + + assertFalse(hasBit(bm, 0)); + bm = flipBit(bm, 0); + assertEq(bm, 1); + assertTrue(hasBit(bm, 0)); + + assertFalse(hasBit(bm, 1)); + bm = flipBit(bm, 1); + assertEq(bm, 3); + assertTrue(hasBit(bm, 1)); + } + + function test_flipBit_onesToZeros() public pure { + // Start at 3 (0b11), clear bits 0 and 1 in order. + uint256 bm = 3; + + assertTrue(hasBit(bm, 0)); + bm = flipBit(bm, 0); + assertEq(bm, 2); + assertFalse(hasBit(bm, 0)); + + assertTrue(hasBit(bm, 1)); + bm = flipBit(bm, 1); + assertEq(bm, 0); + assertFalse(hasBit(bm, 1)); + } + + function test_flipBit_maxIndex255() public pure { + // Flip the MSB on, then off. + uint256 bm = flipBit(0, 255); + assertEq(bm, uint256(1) << 255); + assertTrue(hasBit(bm, 255)); + + bm = flipBit(bm, 255); + assertEq(bm, 0); + assertFalse(hasBit(bm, 255)); + } + + // ------------------------------------------------------------------------- + // Cross-function invariants (fuzzed) + // ------------------------------------------------------------------------- + + /// `flipBit` toggles the queried bit, every time, for any bitmap and index. + /// Closes gap #1 from `TESTS.md` §1: `hasBit(flipBit(bm, i), i) != hasBit(bm, i)`. + function testFuzz_flipBitTogglesQueriedBit(uint256 bm, uint8 idx) public pure { + bool before = hasBit(bm, idx); + assertEq(hasBit(flipBit(bm, idx), idx), !before); + } + + /// `flipBit` is its own inverse: `flipBit(flipBit(bm, i), i) == bm`. + /// Closes gap #2 from `TESTS.md` §1. + function testFuzz_flipBitIsItsOwnInverse(uint256 bm, uint8 idx) public pure { + assertEq(flipBit(flipBit(bm, idx), idx), bm); + } + + /// `flipBit(bm, i)` leaves every bit other than `i` untouched. Verified by + /// checking an independent index `j != i` against the original bitmap. + /// This is the bit-position isolation invariant — a localized version of + /// the gap test `test_hasBit_isolatesIndividualBits` above. + function testFuzz_flipBitOnlyAffectsQueriedBit(uint256 bm, uint8 idx, uint8 other) public pure { + vm.assume(idx != other); + assertEq(hasBit(flipBit(bm, idx), other), hasBit(bm, other)); + } +} diff --git a/test/common/utils/math/Ratio.t.sol b/test/common/utils/math/Ratio.t.sol new file mode 100644 index 000000000..4cf302bd2 --- /dev/null +++ b/test/common/utils/math/Ratio.t.sol @@ -0,0 +1,129 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test, stdError} from "forge-std/Test.sol"; +import {RATIO_BASE, _applyRatioCeiled, RatioOutOfBounds} from "../../../../src/common/utils/math/Ratio.sol"; + +/// @notice Direct tests for `RATIO_BASE` and `_applyRatioCeiled` in +/// `src/common/utils/math/Ratio.sol`. +/// +/// Ports `osx-commons/contracts/test/utils/math/ratio.ts` and adds the +/// gap-analysis items from `TESTS.md` §2: identity case (ratio == base), +/// zero-value, zero-ratio, distinct revert paths for `RatioOutOfBounds` +/// vs arithmetic overflow, and the inclusive-boundary property. +contract RatioTest is Test { + /// 50% in `RATIO_BASE` units. Replaces the TS `pctToRatio(50)` SDK helper. + uint256 internal constant HALF = RATIO_BASE / 2; + + /// External wrapper used to force `_applyRatioCeiled` calls into a child + /// frame so `vm.expectRevert` can intercept them. File-level functions + /// otherwise inline into the test contract and reverts collapse to the + /// test's own frame. + function applyRatioCeiledExt(uint256 value, uint256 ratio) external pure returns (uint256) { + return _applyRatioCeiled(value, ratio); + } + + // ------------------------------------------------------------------------- + // RATIO_BASE + // ------------------------------------------------------------------------- + + function test_RATIO_BASE_is10ToThe6() public pure { + assertEq(RATIO_BASE, 10 ** 6); + assertEq(RATIO_BASE, 1_000_000); + } + + // ------------------------------------------------------------------------- + // _applyRatioCeiled — happy paths + // ------------------------------------------------------------------------- + + function test_applyRatioCeiled_noRemainderDoesNotCeil() public pure { + // 32 * 0.5 == 16 exactly — no ceiling applied. + assertEq(_applyRatioCeiled(32, HALF), 16); + } + + function test_applyRatioCeiled_remainderCeils() public pure { + // 33 * 0.5 == 16.5 → ceils to 17. + assertEq(_applyRatioCeiled(33, HALF), 17); + } + + function test_applyRatioCeiled_ratioEqualsBaseReturnsValueUnchanged() public pure { + // GAP: ratio == RATIO_BASE is the identity case. + assertEq(_applyRatioCeiled(123, RATIO_BASE), 123); + assertEq(_applyRatioCeiled(0, RATIO_BASE), 0); + assertEq(_applyRatioCeiled(1, RATIO_BASE), 1); + assertEq(_applyRatioCeiled(type(uint128).max, RATIO_BASE), type(uint128).max); + } + + function test_applyRatioCeiled_zeroValueReturnsZero() public pure { + // GAP: value == 0 collapses to 0 regardless of ratio. + assertEq(_applyRatioCeiled(0, 0), 0); + assertEq(_applyRatioCeiled(0, 1), 0); + assertEq(_applyRatioCeiled(0, HALF), 0); + assertEq(_applyRatioCeiled(0, RATIO_BASE), 0); + } + + function test_applyRatioCeiled_zeroRatioReturnsZero() public pure { + // GAP: ratio == 0 collapses to 0 regardless of value. + assertEq(_applyRatioCeiled(0, 0), 0); + assertEq(_applyRatioCeiled(1, 0), 0); + assertEq(_applyRatioCeiled(type(uint128).max, 0), 0); + } + + // ------------------------------------------------------------------------- + // _applyRatioCeiled — revert paths + // ------------------------------------------------------------------------- + + function test_applyRatioCeiled_revertsIfRatioExceedsBase() public { + uint256 tooBig = RATIO_BASE + 1; + vm.expectRevert(abi.encodeWithSelector(RatioOutOfBounds.selector, RATIO_BASE, tooBig)); + this.applyRatioCeiledExt(123, tooBig); + } + + function test_applyRatioCeiled_revertEncodesActualRatio() public { + // GAP: the custom error must carry the exact actual ratio in its second arg, + // not just any too-large value. + uint256 max = type(uint256).max; + vm.expectRevert(abi.encodeWithSelector(RatioOutOfBounds.selector, RATIO_BASE, max)); + this.applyRatioCeiledExt(0, max); + } + + function test_applyRatioCeiled_ratioAtBaseDoesNotRevert() public pure { + // Boundary is inclusive on the success side: ratio == RATIO_BASE is allowed. + _applyRatioCeiled(123, RATIO_BASE); + } + + function test_applyRatioCeiled_revertsOnArithmeticOverflow() public { + // GAP: distinct revert path from `RatioOutOfBounds`. Ratio is in-bounds; the + // overflow happens inside `_value * _ratio`. + uint256 overflowValue = type(uint256).max / RATIO_BASE + 1; + vm.expectRevert(stdError.arithmeticError); + this.applyRatioCeiledExt(overflowValue, RATIO_BASE); + } + + function test_applyRatioCeiled_justBelowOverflowDoesNotRevert() public pure { + // The largest value that does NOT overflow `_value * RATIO_BASE`. + uint256 borderValue = type(uint256).max / RATIO_BASE; + _applyRatioCeiled(borderValue, RATIO_BASE); + } + + // ------------------------------------------------------------------------- + // Fuzzed invariants + // ------------------------------------------------------------------------- + + /// For any in-bounds ratio, the ceiled result never exceeds `value`. Even with + /// ceiling, applying a ratio in `[0, 1]` to `value` cannot grow it. + /// `value` bounded to `uint128` to stay inside the no-overflow envelope + /// (`uint128 << uint256.max / RATIO_BASE`). + function testFuzz_applyRatioCeiled_resultNeverExceedsValue(uint128 value, uint256 ratio) public pure { + ratio = bound(ratio, 0, RATIO_BASE); + assertLe(_applyRatioCeiled(uint256(value), ratio), uint256(value)); + } + + /// `ratio == RATIO_BASE` is the identity function on `value` for any value + /// that does not overflow `value * RATIO_BASE`. + function testFuzz_applyRatioCeiled_baseRatioIsIdentity(uint256 value) public pure { + value = bound(value, 0, type(uint256).max / RATIO_BASE); + assertEq(_applyRatioCeiled(value, RATIO_BASE), value); + } +} diff --git a/test/common/utils/math/UncheckedMath.t.sol b/test/common/utils/math/UncheckedMath.t.sol new file mode 100644 index 000000000..954edbf4a --- /dev/null +++ b/test/common/utils/math/UncheckedMath.t.sol @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {_uncheckedAdd, _uncheckedSub} from "../../../../src/common/utils/math/UncheckedMath.sol"; + +/// @notice Direct tests for `_uncheckedAdd` and `_uncheckedSub` in +/// `src/common/utils/math/UncheckedMath.sol`. +/// +/// No upstream TS coverage existed for these helpers — entire surface is +/// the gap recorded in `TESTS.md` §3. Boundary cases verify wrap-around +/// (no panic); fuzz tests verify each helper agrees with the same expression +/// inside an explicit `unchecked` block, pinning behavioural equivalence. +contract UncheckedMathTest is Test { + // ------------------------------------------------------------------------- + // _uncheckedAdd + // ------------------------------------------------------------------------- + + function test_uncheckedAdd_normalInputs() public pure { + assertEq(_uncheckedAdd(0, 0), 0); + assertEq(_uncheckedAdd(1, 2), 3); + assertEq(_uncheckedAdd(type(uint256).max, 0), type(uint256).max); + assertEq(_uncheckedAdd(0, type(uint256).max), type(uint256).max); + } + + function test_uncheckedAdd_wrapsOnOverflow() public pure { + // `type(uint256).max + 1` wraps to 0 — a checked `+` would panic 0x11. + assertEq(_uncheckedAdd(type(uint256).max, 1), 0); + // `type(uint256).max + 2` wraps to 1. + assertEq(_uncheckedAdd(type(uint256).max, 2), 1); + // Two large operands wrap predictably. + assertEq(_uncheckedAdd(type(uint256).max, type(uint256).max), type(uint256).max - 1); + } + + // ------------------------------------------------------------------------- + // _uncheckedSub + // ------------------------------------------------------------------------- + + function test_uncheckedSub_normalInputs() public pure { + assertEq(_uncheckedSub(0, 0), 0); + assertEq(_uncheckedSub(3, 2), 1); + assertEq(_uncheckedSub(type(uint256).max, type(uint256).max), 0); + assertEq(_uncheckedSub(type(uint256).max, 0), type(uint256).max); + } + + function test_uncheckedSub_wrapsOnUnderflow() public pure { + // `0 - 1` wraps to `type(uint256).max` — a checked `-` would panic 0x11. + assertEq(_uncheckedSub(0, 1), type(uint256).max); + // `0 - 2` wraps to `max - 1`. + assertEq(_uncheckedSub(0, 2), type(uint256).max - 1); + // Subtracting more than the minuend wraps proportionally. + assertEq(_uncheckedSub(5, 10), type(uint256).max - 4); + } + + // ------------------------------------------------------------------------- + // Fuzzed equivalence to inline `unchecked` blocks + // ------------------------------------------------------------------------- + + /// `_uncheckedAdd(a, b)` agrees with `unchecked { a + b }` for any inputs. + /// Pins the helper to the canonical wrap-on-overflow semantics so a future + /// refactor that accidentally re-introduces checked arithmetic fails here. + function testFuzz_uncheckedAdd_equalsInlineUnchecked(uint256 a, uint256 b) public pure { + uint256 expected; + unchecked { + expected = a + b; + } + assertEq(_uncheckedAdd(a, b), expected); + } + + /// `_uncheckedSub(a, b)` agrees with `unchecked { a - b }` for any inputs. + function testFuzz_uncheckedSub_equalsInlineUnchecked(uint256 a, uint256 b) public pure { + uint256 expected; + unchecked { + expected = a - b; + } + assertEq(_uncheckedSub(a, b), expected); + } +} diff --git a/test/common/utils/versioning/ProtocolVersion.t.sol b/test/common/utils/versioning/ProtocolVersion.t.sol new file mode 100644 index 000000000..f982f4c79 --- /dev/null +++ b/test/common/utils/versioning/ProtocolVersion.t.sol @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {IProtocolVersion} from "../../../../src/common/utils/versioning/IProtocolVersion.sol"; +import {ProtocolVersionMock} from "../../../mocks/commons/utils/versioning/ProtocolVersionMock.sol"; + +/// @notice Direct tests for the abstract `ProtocolVersion` base contract and +/// the `IProtocolVersion` interface in `src/common/utils/versioning/`. +/// +/// Ports `osx-commons/contracts/test/utils/versioning/protocol-version.ts` +/// and closes the gaps from `TESTS.md` §5: exact return-value check against +/// the inline `[1, 4, 0]` constant (replacing the TS dependency on +/// `package.json`), stateless / deterministic across calls. +contract ProtocolVersionTest is Test { + /// Frozen iface ID introduced in v1.3.0. `IProtocolVersion` has a single + /// function `protocolVersion()`, so its ERC-165 ID equals that function's + /// selector. If the interface ever changes (added/removed/renamed + /// function), this literal stops matching and the test fails — exactly + /// the drift detection the TS `getInterfaceId(...) == initial` did. + bytes4 internal constant IPROTOCOL_VERSION_V1_3_0_INTERFACE_ID = 0x2ae9c600; + + /// The production protocol version that the absorbed `ProtocolVersion.sol` + /// returns. Lifted to a constant so a change in the source forces a deliberate + /// edit here too. + uint8 internal constant MAJOR = 1; + uint8 internal constant MINOR = 4; + uint8 internal constant PATCH = 0; + + ProtocolVersionMock internal mock; + + function setUp() public { + mock = new ProtocolVersionMock(); + } + + // ------------------------------------------------------------------------- + // IProtocolVersion — interface ID + // ------------------------------------------------------------------------- + + function test_IProtocolVersion_hasSameInterfaceIdAsV1_3_0() public pure { + assertEq(type(IProtocolVersion).interfaceId, IPROTOCOL_VERSION_V1_3_0_INTERFACE_ID); + } + + function test_IProtocolVersion_interfaceIdIsNotEmpty() public pure { + // Sanity: the selector XOR for a single-function interface is the + // function selector itself, which must be non-zero. + assertTrue(type(IProtocolVersion).interfaceId != bytes4(0)); + } + + function test_IProtocolVersion_interfaceIdIsNotIERC165() public pure { + // Cross-check: `IProtocolVersion` is a distinct interface, not an alias + // of `IERC165`. + assertTrue(type(IProtocolVersion).interfaceId != type(IERC165).interfaceId); + } + + // ------------------------------------------------------------------------- + // ProtocolVersion — concrete value + // ------------------------------------------------------------------------- + + function test_protocolVersion_returnsCurrentProductionVersion() public view { + uint8[3] memory v = mock.protocolVersion(); + assertEq(v[0], MAJOR); + assertEq(v[1], MINOR); + assertEq(v[2], PATCH); + } + + function test_protocolVersion_isDeterministicAcrossCalls() public view { + // GAP: function is `pure` — repeated calls must return the identical + // tuple. Locks against accidental refactor to a state-touching + // implementation. + uint8[3] memory a = mock.protocolVersion(); + uint8[3] memory b = mock.protocolVersion(); + assertEq(a[0], b[0]); + assertEq(a[1], b[1]); + assertEq(a[2], b[2]); + } +} diff --git a/test/common/utils/versioning/VersionComparisonLib.t.sol b/test/common/utils/versioning/VersionComparisonLib.t.sol new file mode 100644 index 000000000..ccb4d0709 --- /dev/null +++ b/test/common/utils/versioning/VersionComparisonLib.t.sol @@ -0,0 +1,283 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {VersionComparisonLib} from "../../../../src/common/utils/versioning/VersionComparisonLib.sol"; + +/// @notice Direct tests for `VersionComparisonLib` in +/// `src/common/utils/versioning/VersionComparisonLib.sol`. +/// +/// Ports `osx-commons/contracts/test/utils/versioning/version-comparison-lib.ts` +/// and adds the gap items from `TESTS.md` §4: +/// - boundary versions [0,0,0] / [255,255,255] / asymmetric extremes, +/// - the logical-consistency invariant `!lt(a,b) && !eq(a,b) ⇔ gt(a,b)` +/// plus the lte/gte duals (closes central flaw log F16), +/// - transitivity of `lt`. +/// +/// The TS suite uses three matrix helpers (`eqChecks`, `ltChecks`, `gtChecks`) +/// shared across all 6 operators. The same DRY shape is preserved here via +/// internal helpers that take a function reference to a per-op wrapper. +contract VersionComparisonLibTest is Test { + using VersionComparisonLib for uint8[3]; + + // ------------------------------------------------------------------------- + // Per-operator wrappers + // + // Internal function references to library functions are not addressable + // directly through `using`; each operator gets a tiny wrapper on the test + // contract so we can pass it by reference into the matrix helpers below. + // ------------------------------------------------------------------------- + + function _opEq(uint8[3] memory l, uint8[3] memory r) internal pure returns (bool) { + return l.eq(r); + } + + function _opNeq(uint8[3] memory l, uint8[3] memory r) internal pure returns (bool) { + return l.neq(r); + } + + function _opLt(uint8[3] memory l, uint8[3] memory r) internal pure returns (bool) { + return l.lt(r); + } + + function _opLte(uint8[3] memory l, uint8[3] memory r) internal pure returns (bool) { + return l.lte(r); + } + + function _opGt(uint8[3] memory l, uint8[3] memory r) internal pure returns (bool) { + return l.gt(r); + } + + function _opGte(uint8[3] memory l, uint8[3] memory r) internal pure returns (bool) { + return l.gte(r); + } + + function _v(uint8 a, uint8 b, uint8 c) internal pure returns (uint8[3] memory v) { + v[0] = a; + v[1] = b; + v[2] = c; + } + + // ------------------------------------------------------------------------- + // Matrix helpers — mirror the TS `eqChecks` / `ltChecks` / `gtChecks` shape + // ------------------------------------------------------------------------- + + /// 8 pairs where `lhs == rhs`. Covers every "all-zero subset" combination + /// of the three semver components, including the all-zero and all-one vectors. + function _runEqualPairs(function(uint8[3] memory, uint8[3] memory) internal pure returns (bool) op, bool expected) + internal + pure + { + assertEq(op(_v(1, 1, 1), _v(1, 1, 1)), expected); + assertEq(op(_v(0, 1, 1), _v(0, 1, 1)), expected); + assertEq(op(_v(1, 0, 1), _v(1, 0, 1)), expected); + assertEq(op(_v(1, 1, 0), _v(1, 1, 0)), expected); + assertEq(op(_v(1, 0, 0), _v(1, 0, 0)), expected); + assertEq(op(_v(0, 1, 0), _v(0, 1, 0)), expected); + assertEq(op(_v(0, 0, 1), _v(0, 0, 1)), expected); + assertEq(op(_v(0, 0, 0), _v(0, 0, 0)), expected); + } + + /// 16 pairs where `lhs < rhs` — exercises every single-component step-up + /// as well as multi-component variations. + function _runLtPairs(function(uint8[3] memory, uint8[3] memory) internal pure returns (bool) op, bool expected) + internal + pure + { + // Single-component bumps from [1,1,1]. + assertEq(op(_v(1, 1, 1), _v(2, 1, 1)), expected); + assertEq(op(_v(1, 1, 1), _v(1, 2, 1)), expected); + assertEq(op(_v(1, 1, 1), _v(1, 1, 2)), expected); + // Two-component bumps. + assertEq(op(_v(1, 1, 1), _v(1, 2, 2)), expected); + assertEq(op(_v(1, 1, 1), _v(2, 1, 2)), expected); + assertEq(op(_v(1, 1, 1), _v(2, 2, 1)), expected); + // Three-component bump. + assertEq(op(_v(1, 1, 1), _v(2, 2, 2)), expected); + // Patch = 0 on both sides — ensure trailing zeros don't break ordering. + assertEq(op(_v(1, 1, 0), _v(1, 2, 0)), expected); + assertEq(op(_v(1, 1, 0), _v(2, 1, 0)), expected); + assertEq(op(_v(1, 1, 0), _v(2, 2, 0)), expected); + // Major = 0 — pre-1.x versions still compare on minor/patch. + assertEq(op(_v(0, 1, 1), _v(0, 1, 2)), expected); + assertEq(op(_v(0, 1, 1), _v(0, 2, 1)), expected); + assertEq(op(_v(0, 1, 1), _v(0, 2, 2)), expected); + // GAP: per-component isolation — only one slot non-zero. + assertEq(op(_v(1, 0, 0), _v(2, 0, 0)), expected); + assertEq(op(_v(0, 1, 0), _v(0, 2, 0)), expected); + assertEq(op(_v(0, 0, 1), _v(0, 0, 2)), expected); + } + + /// Mirror of `_runLtPairs` with sides swapped. + function _runGtPairs(function(uint8[3] memory, uint8[3] memory) internal pure returns (bool) op, bool expected) + internal + pure + { + assertEq(op(_v(2, 1, 1), _v(1, 1, 1)), expected); + assertEq(op(_v(1, 2, 1), _v(1, 1, 1)), expected); + assertEq(op(_v(1, 1, 2), _v(1, 1, 1)), expected); + assertEq(op(_v(1, 2, 2), _v(1, 1, 1)), expected); + assertEq(op(_v(2, 1, 2), _v(1, 1, 1)), expected); + assertEq(op(_v(2, 2, 1), _v(1, 1, 1)), expected); + assertEq(op(_v(2, 2, 2), _v(1, 1, 1)), expected); + assertEq(op(_v(1, 2, 0), _v(1, 1, 0)), expected); + assertEq(op(_v(2, 1, 0), _v(1, 1, 0)), expected); + assertEq(op(_v(2, 2, 0), _v(1, 1, 0)), expected); + assertEq(op(_v(0, 1, 2), _v(0, 1, 1)), expected); + assertEq(op(_v(0, 2, 1), _v(0, 1, 1)), expected); + assertEq(op(_v(0, 2, 2), _v(0, 1, 1)), expected); + assertEq(op(_v(2, 0, 0), _v(1, 0, 0)), expected); + assertEq(op(_v(0, 2, 0), _v(0, 1, 0)), expected); + assertEq(op(_v(0, 0, 2), _v(0, 0, 1)), expected); + } + + // ------------------------------------------------------------------------- + // eq + // ------------------------------------------------------------------------- + + function test_eq_returnsTrueIfLhsEqualsRhs() public pure { + _runEqualPairs(_opEq, true); + } + + function test_eq_returnsFalseIfLhsDoesNotEqualRhs() public pure { + _runLtPairs(_opEq, false); + _runGtPairs(_opEq, false); + } + + // ------------------------------------------------------------------------- + // neq + // ------------------------------------------------------------------------- + + function test_neq_returnsTrueIfLhsDoesNotEqualRhs() public pure { + _runLtPairs(_opNeq, true); + _runGtPairs(_opNeq, true); + } + + function test_neq_returnsFalseIfLhsEqualsRhs() public pure { + _runEqualPairs(_opNeq, false); + } + + // ------------------------------------------------------------------------- + // lt + // ------------------------------------------------------------------------- + + function test_lt_returnsTrueIfLhsLessThanRhs() public pure { + _runLtPairs(_opLt, true); + } + + function test_lt_returnsFalseIfLhsNotLessThanRhs() public pure { + _runGtPairs(_opLt, false); + _runEqualPairs(_opLt, false); + } + + // ------------------------------------------------------------------------- + // lte + // ------------------------------------------------------------------------- + + function test_lte_returnsTrueIfLhsLessThanOrEqualRhs() public pure { + _runLtPairs(_opLte, true); + _runEqualPairs(_opLte, true); + } + + function test_lte_returnsFalseIfLhsGreaterThanRhs() public pure { + _runGtPairs(_opLte, false); + } + + // ------------------------------------------------------------------------- + // gt + // ------------------------------------------------------------------------- + + function test_gt_returnsTrueIfLhsGreaterThanRhs() public pure { + _runGtPairs(_opGt, true); + } + + function test_gt_returnsFalseIfLhsNotGreaterThanRhs() public pure { + _runLtPairs(_opGt, false); + _runEqualPairs(_opGt, false); + } + + // ------------------------------------------------------------------------- + // gte + // ------------------------------------------------------------------------- + + function test_gte_returnsTrueIfLhsGreaterThanOrEqualRhs() public pure { + _runGtPairs(_opGte, true); + _runEqualPairs(_opGte, true); + } + + function test_gte_returnsFalseIfLhsLessThanRhs() public pure { + _runLtPairs(_opGte, false); + } + + // ------------------------------------------------------------------------- + // Boundary versions (GAP) + // ------------------------------------------------------------------------- + + function test_boundary_minAndMaxVersionsCompareCorrectly() public pure { + uint8[3] memory zero = _v(0, 0, 0); + uint8[3] memory max = _v(255, 255, 255); + + // Equality reflexive at boundaries. + assertTrue(zero.eq(zero)); + assertTrue(max.eq(max)); + + // zero < max in every operator that orders strictly. + assertTrue(zero.lt(max)); + assertFalse(zero.gt(max)); + assertTrue(zero.lte(max)); + assertFalse(zero.gte(max)); + assertTrue(zero.neq(max)); + + // Asymmetric extremes — [255,0,0] vs [0,0,255]: major dominates. + assertTrue(_v(255, 0, 0).gt(_v(0, 0, 255))); + assertFalse(_v(255, 0, 0).lt(_v(0, 0, 255))); + + // [0,0,255] vs [0,1,0]: minor dominates over patch. + assertTrue(_v(0, 1, 0).gt(_v(0, 0, 255))); + assertFalse(_v(0, 1, 0).lt(_v(0, 0, 255))); + } + + // ------------------------------------------------------------------------- + // Logical-consistency invariants (GAP — closes central log F16) + // ------------------------------------------------------------------------- + + /// Exactly one of `lt`, `eq`, `gt` is true for any pair. The TS suite never + /// asserted the full trichotomy across the 6 operators; locking it in here + /// catches a class of subtle operator-drift bugs. + function testFuzz_trichotomy(uint8[3] memory a, uint8[3] memory b) public pure { + bool isLt = a.lt(b); + bool isEq = a.eq(b); + bool isGt = a.gt(b); + + // Exactly one of the three holds. + uint256 trueCount = (isLt ? 1 : 0) + (isEq ? 1 : 0) + (isGt ? 1 : 0); + assertEq(trueCount, 1); + } + + /// `neq` is the negation of `eq`. + function testFuzz_neqIsNegationOfEq(uint8[3] memory a, uint8[3] memory b) public pure { + assertEq(a.neq(b), !a.eq(b)); + } + + /// `lte` is `lt || eq`. Same shape for `gte`. + function testFuzz_lteEqualsLtOrEq(uint8[3] memory a, uint8[3] memory b) public pure { + assertEq(a.lte(b), a.lt(b) || a.eq(b)); + } + + function testFuzz_gteEqualsGtOrEq(uint8[3] memory a, uint8[3] memory b) public pure { + assertEq(a.gte(b), a.gt(b) || a.eq(b)); + } + + /// `!lt(a,b) && !eq(a,b) ⇔ gt(a,b)` — the consistency invariant called out + /// in `TESTS.md` F16. + function testFuzz_consistencyAcrossOperators(uint8[3] memory a, uint8[3] memory b) public pure { + assertEq(!a.lt(b) && !a.eq(b), a.gt(b)); + } + + /// `lt` is transitive: `lt(a, b) && lt(b, c) ⇒ lt(a, c)`. + function testFuzz_ltIsTransitive(uint8[3] memory a, uint8[3] memory b, uint8[3] memory c) public pure { + vm.assume(a.lt(b) && b.lt(c)); + assertTrue(a.lt(c)); + } +} From d06b829a8829fa060f03aca4549397e50cf5604c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8r=E2=88=82=C2=A1?= Date: Mon, 18 May 2026 16:29:11 +0000 Subject: [PATCH 05/31] Permission and metadata tests --- .../permission/auth/DaoAuthorizable.t.sol | 94 +++++++++ .../condition/PermissionCondition.t.sol | 96 +++++++++ .../utils/metadata/MetadataExtension.t.sol | 189 ++++++++++++++++++ 3 files changed, 379 insertions(+) create mode 100644 test/common/permission/auth/DaoAuthorizable.t.sol create mode 100644 test/common/permission/condition/PermissionCondition.t.sol create mode 100644 test/common/utils/metadata/MetadataExtension.t.sol diff --git a/test/common/permission/auth/DaoAuthorizable.t.sol b/test/common/permission/auth/DaoAuthorizable.t.sol new file mode 100644 index 000000000..24dc62927 --- /dev/null +++ b/test/common/permission/auth/DaoAuthorizable.t.sol @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {IDAO} from "../../../../src/common/dao/IDAO.sol"; +import {DaoUnauthorized} from "../../../../src/common/permission/auth/auth.sol"; +import {DaoAuthorizableMock} from "../../../mocks/commons/permission/auth/DaoAuthorizableMock.sol"; +import { + DaoAuthorizableUpgradeableMock +} from "../../../mocks/commons/permission/auth/DaoAuthorizableUpgradeableMock.sol"; +import {DAOMock} from "../../../mocks/commons/dao/DAOMock.sol"; + +/// @dev Minimal shape both `DaoAuthorizableMock` and `DaoAuthorizableUpgradeableMock` expose. +/// Lets the shared base test contract call into either variant through one typed reference. +interface IDaoAuthorizableMock { + function dao() external view returns (IDAO); + function authorizedFunc() external; +} + +/// @notice Shared behaviour tests for `DaoAuthorizable` and `DaoAuthorizableUpgradeable`. +/// +/// Ports `osx-commons/contracts/test/permission/auth/dao-authorizable.ts`. The +/// TS suite's `daoAuthorizableBaseTests(fixture)` DRY pattern is reproduced +/// via this abstract base + two concrete derivations (one per source contract). +/// Gaps closed from `TESTS.md` §8: `DaoUnauthorized` error carries all four +/// fields, auth guard reverts (no silent fallthrough), and `dao()` returns the +/// exact stored address. +abstract contract DaoAuthorizableSharedTest is Test { + bytes32 internal constant PERM_ID = keccak256("AUTHORIZED_FUNC_PERMISSION"); + + DAOMock internal daoMock; + IDaoAuthorizableMock internal target; + address internal bob; + + /// Concrete subclasses construct one variant of the mock and return it. + function _deployTarget() internal virtual returns (IDaoAuthorizableMock); + + function setUp() public virtual { + bob = makeAddr("bob"); + daoMock = new DAOMock(); + target = _deployTarget(); + } + + function test_dao_returnsConstructorOrInitDao() public view { + assertEq(address(target.dao()), address(daoMock)); + } + + function test_authorizedFunc_revertsIfPermissionNotGranted() public { + // The mock DAO defaults to `hasPermission == false`. + assertFalse(daoMock.hasPermissionReturnValueMock()); + + vm.expectRevert( + abi.encodeWithSelector(DaoUnauthorized.selector, address(daoMock), address(target), bob, PERM_ID) + ); + vm.prank(bob); + target.authorizedFunc(); + } + + function test_authorizedFunc_succeedsIfPermissionGranted() public { + daoMock.setHasPermissionReturnValueMock(true); + vm.prank(bob); + target.authorizedFunc(); + } +} + +/// @notice Constructable variant: `DaoAuthorizable` is set via constructor. +contract DaoAuthorizableTest is DaoAuthorizableSharedTest { + function _deployTarget() internal override returns (IDaoAuthorizableMock) { + return IDaoAuthorizableMock(address(new DaoAuthorizableMock(IDAO(address(daoMock))))); + } +} + +/// @notice Upgradeable variant: `DaoAuthorizableUpgradeable` is set via initializer. +/// Adds the two TS-side tests for the initializer guard. +contract DaoAuthorizableUpgradeableTest is DaoAuthorizableSharedTest { + function _deployTarget() internal override returns (IDaoAuthorizableMock) { + DaoAuthorizableUpgradeableMock impl = new DaoAuthorizableUpgradeableMock(); + impl.initialize(IDAO(address(daoMock))); + return IDaoAuthorizableMock(address(impl)); + } + + function test_initialize_revertsIfCalledTwice() public { + DaoAuthorizableUpgradeableMock m = DaoAuthorizableUpgradeableMock(address(target)); + vm.expectRevert("Initializable: contract is already initialized"); + m.initialize(IDAO(address(daoMock))); + } + + function test_initInternal_revertsIfCalledOutsideInitializer() public { + DaoAuthorizableUpgradeableMock m = DaoAuthorizableUpgradeableMock(address(target)); + vm.expectRevert("Initializable: contract is not initializing"); + m.notAnInitializer(IDAO(address(daoMock))); + } +} diff --git a/test/common/permission/condition/PermissionCondition.t.sol b/test/common/permission/condition/PermissionCondition.t.sol new file mode 100644 index 000000000..806d1cb5a --- /dev/null +++ b/test/common/permission/condition/PermissionCondition.t.sol @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {IPermissionCondition} from "../../../../src/common/permission/condition/IPermissionCondition.sol"; +import {IProtocolVersion} from "../../../../src/common/utils/versioning/IProtocolVersion.sol"; +import {PermissionConditionMock} from "../../../mocks/commons/permission/condition/PermissionConditionMock.sol"; +import { + PermissionConditionUpgradeableMock +} from "../../../mocks/commons/permission/condition/PermissionConditionUpgradeableMock.sol"; + +/// @dev Minimal shape both `PermissionConditionMock` and +/// `PermissionConditionUpgradeableMock` expose. Lets the shared base call into +/// either variant through one typed reference. +interface IPermissionConditionMock { + function supportsInterface(bytes4) external view returns (bool); + function protocolVersion() external view returns (uint8[3] memory); +} + +/// @notice Direct tests for `PermissionCondition` and +/// `PermissionConditionUpgradeable` in `src/common/permission/condition/`. +/// +/// Ports `osx-commons/contracts/test/permission/condition/permission-condition.ts` +/// (the TS describe was labelled `IProposal` due to a copy-paste error — fixed +/// here). Closes the gaps from `TESTS.md` §9: negative `supportsInterface` +/// case, exact `protocolVersion()` return value, frozen `IPermissionCondition` +/// iface ID matches v1.0.0. +abstract contract PermissionConditionSharedTest is Test { + /// Frozen `IPermissionCondition` interface ID introduced in v1.0.0. + /// Single function `isGranted(address,address,bytes32,bytes)`. + /// Computed via `cast sig "isGranted(address,address,bytes32,bytes)"`. + bytes4 internal constant IPERMISSION_CONDITION_V1_0_0_INTERFACE_ID = 0x2675fdd0; + + IPermissionConditionMock internal conditionMock; + + function _deployConditionMock() internal virtual returns (IPermissionConditionMock); + + function setUp() public virtual { + conditionMock = _deployConditionMock(); + } + + // ------------------------------------------------------------------------- + // IPermissionCondition iface ID (drift detector vs v1.0.0) + // ------------------------------------------------------------------------- + + function test_IPermissionCondition_hasSameInterfaceIdAsV1_0_0() public pure { + assertEq(type(IPermissionCondition).interfaceId, IPERMISSION_CONDITION_V1_0_0_INTERFACE_ID); + } + + // ------------------------------------------------------------------------- + // Protocol version + // ------------------------------------------------------------------------- + + function test_protocolVersion_returnsCurrentProductionVersion() public view { + uint8[3] memory v = conditionMock.protocolVersion(); + assertEq(v[0], 1); + assertEq(v[1], 4); + assertEq(v[2], 0); + } + + // ------------------------------------------------------------------------- + // ERC-165 + // ------------------------------------------------------------------------- + + function test_supportsInterface_ERC165() public view { + assertTrue(conditionMock.supportsInterface(type(IERC165).interfaceId)); + } + + function test_supportsInterface_IPermissionCondition() public view { + assertTrue(conditionMock.supportsInterface(type(IPermissionCondition).interfaceId)); + } + + function test_supportsInterface_IProtocolVersion() public view { + assertTrue(conditionMock.supportsInterface(type(IProtocolVersion).interfaceId)); + } + + /// GAP: negative case — an unrelated random selector returns false. + function test_supportsInterface_returnsFalseForUnknownInterface() public view { + assertFalse(conditionMock.supportsInterface(0xdeadbeef)); + assertFalse(conditionMock.supportsInterface(0x00000000)); + } +} + +contract PermissionConditionTest is PermissionConditionSharedTest { + function _deployConditionMock() internal override returns (IPermissionConditionMock) { + return IPermissionConditionMock(address(new PermissionConditionMock())); + } +} + +contract PermissionConditionUpgradeableTest is PermissionConditionSharedTest { + function _deployConditionMock() internal override returns (IPermissionConditionMock) { + return IPermissionConditionMock(address(new PermissionConditionUpgradeableMock())); + } +} diff --git a/test/common/utils/metadata/MetadataExtension.t.sol b/test/common/utils/metadata/MetadataExtension.t.sol new file mode 100644 index 000000000..de97b647c --- /dev/null +++ b/test/common/utils/metadata/MetadataExtension.t.sol @@ -0,0 +1,189 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test, Vm} from "forge-std/Test.sol"; +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {IDAO} from "../../../../src/common/dao/IDAO.sol"; +import {DaoUnauthorized} from "../../../../src/common/permission/auth/auth.sol"; +import {MetadataExtensionMock} from "../../../mocks/commons/utils/metadata/MetadataExtensionMock.sol"; +import { + MetadataExtensionUpgradeableMock +} from "../../../mocks/commons/utils/metadata/MetadataExtensionUpgradeableMock.sol"; +import {DAOMock} from "../../../mocks/commons/dao/DAOMock.sol"; + +/// @dev Minimal shape that both `MetadataExtensionMock` and +/// `MetadataExtensionUpgradeableMock` expose. Used both as a function-selector +/// source for the ERC-165 ID and as a typed handle in the shared tests. +interface IMetadataExtension { + function setMetadata(bytes calldata _metadata) external; + function getMetadata() external view returns (bytes memory); + function supportsInterface(bytes4 _interfaceId) external view returns (bool); +} + +/// @notice Shared behaviour tests for `MetadataExtension` and +/// `MetadataExtensionUpgradeable` in `src/common/utils/metadata/`. +/// +/// Ports `osx-commons/contracts/test/utils/metadata.ts` and closes the gaps +/// from `TESTS.md` §10: empty / large payload roundtrips, explicit XOR +/// selector check, auth guard verified via `DaoUnauthorized`, event-after- +/// state-change ordering, and (Upgradeable only) hard-coded storage slot +/// isolation via `vm.load`. +abstract contract MetadataExtensionSharedTest is Test { + DAOMock internal daoMock; + IMetadataExtension internal target; + address internal bob; + + /// `MetadataExtension`'s `supportsInterface` returns true for this XOR + /// (per source); pre-computed here so the assertion is double-anchored. + bytes4 internal immutable METADATA_SELECTOR_INTERFACE_ID = + IMetadataExtension.setMetadata.selector ^ IMetadataExtension.getMetadata.selector; + + function _deployTarget() internal virtual returns (IMetadataExtension); + + function setUp() public virtual { + bob = makeAddr("bob"); + daoMock = new DAOMock(); + target = _deployTarget(); + // Default: any caller passes the auth check unless a test flips this back. + daoMock.setHasPermissionReturnValueMock(true); + } + + // ------------------------------------------------------------------------- + // ERC-165 + // ------------------------------------------------------------------------- + + function test_supportsInterface_ERC165() public view { + assertTrue(target.supportsInterface(type(IERC165).interfaceId)); + } + + function test_supportsInterface_metadataXorSelector() public view { + assertTrue(target.supportsInterface(METADATA_SELECTOR_INTERFACE_ID)); + // Sanity: confirm the precomputed XOR equals the inline computation + // anyone could verify with `cast sig`. + assertEq(METADATA_SELECTOR_INTERFACE_ID, bytes4(0x940cac36)); + } + + function test_supportsInterface_returnsFalseForUnknownInterface() public view { + assertFalse(target.supportsInterface(0xdeadbeef)); + } + + // ------------------------------------------------------------------------- + // setMetadata / getMetadata + // ------------------------------------------------------------------------- + + function test_setMetadata_revertsIfCallerLacksPermission() public { + daoMock.setHasPermissionReturnValueMock(false); + // Match only the selector — the four-arg payload is exercised by + // `DaoAuthorizable.t.sol` and need not be re-verified here. + vm.expectPartialRevert(DaoUnauthorized.selector); + vm.prank(bob); + target.setMetadata(hex"11"); + } + + function test_setMetadata_emitsMetadataSetWithExactPayload() public { + bytes memory payload = hex"11"; + vm.recordLogs(); + target.setMetadata(payload); + Vm.Log[] memory logs = vm.getRecordedLogs(); + + bytes32 expectedTopic = keccak256("MetadataSet(bytes)"); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].topics[0] == expectedTopic && logs[i].emitter == address(target)) { + bytes memory decoded = abi.decode(logs[i].data, (bytes)); + assertEq(decoded, payload); + found = true; + break; + } + } + assertTrue(found, "MetadataSet not emitted by target"); + } + + function test_getMetadata_returnsLastSet() public { + bytes memory payload = hex"11"; + target.setMetadata(payload); + assertEq(target.getMetadata(), payload); + } + + function test_getMetadata_handlesPayloadLargerThan32Bytes() public { + // Stress sstore/sload semantics for `bytes` storage: a 50-byte payload + // straddles two storage slots. Repeats the TS suite's check. + bytes memory big = new bytes(50); + for (uint256 i = 0; i < 50; i++) { + big[i] = 0x11; + } + target.setMetadata(big); + assertEq(target.getMetadata(), big); + } + + function test_getMetadata_emptyPayloadRoundtrips() public { + // GAP: empty bytes accepted and retrievable. + target.setMetadata(""); + assertEq(target.getMetadata().length, 0); + } + + function test_setMetadata_overwritesPreviousValue() public { + target.setMetadata(hex"11"); + target.setMetadata(hex"22"); + assertEq(target.getMetadata(), hex"22"); + } + + function test_setMetadata_stateUpdatedBeforeEvent() public { + // GAP: event emitted *after* the state change. We can't observe two + // states from outside a single transaction, but we can confirm the + // emitted bytes match the value `getMetadata()` returns immediately + // after — which proves the storage write happened by then. + bytes memory payload = hex"deadbeef"; + vm.recordLogs(); + target.setMetadata(payload); + Vm.Log[] memory logs = vm.getRecordedLogs(); + assertEq(target.getMetadata(), payload); + + bytes32 expectedTopic = keccak256("MetadataSet(bytes)"); + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].topics[0] == expectedTopic && logs[i].emitter == address(target)) { + assertEq(abi.decode(logs[i].data, (bytes)), payload); + return; + } + } + revert("MetadataSet not emitted"); + } +} + +/// @notice Constructable variant. +contract MetadataExtensionTest is MetadataExtensionSharedTest { + function _deployTarget() internal override returns (IMetadataExtension) { + return IMetadataExtension(address(new MetadataExtensionMock(IDAO(address(daoMock))))); + } +} + +/// @notice Upgradeable variant — adds the hard-coded storage-slot isolation +/// test (closes the `MetadataExtensionUpgradeable` storage-slot gap in §10). +contract MetadataExtensionUpgradeableTest is MetadataExtensionSharedTest { + /// `keccak256(abi.encode(uint256(keccak256("osx-commons.storage.MetadataExtension")) - 1)) & ~bytes32(uint256(0xff))` + /// — duplicated verbatim from `MetadataExtensionUpgradeable.sol`. + bytes32 internal constant METADATA_STORAGE_SLOT = + 0x47ff9796f72d439c6e5c30a24b9fad985a00c85a9f2258074c400a94f8746b00; + + function _deployTarget() internal override returns (IMetadataExtension) { + MetadataExtensionUpgradeableMock impl = new MetadataExtensionUpgradeableMock(); + impl.initialize(IDAO(address(daoMock))); + return IMetadataExtension(address(impl)); + } + + function test_setMetadata_writesToHardcodedStorageSlot() public { + bytes memory payload = hex"42"; + target.setMetadata(payload); + + // The `bytes` struct member sits at slot `METADATA_STORAGE_SLOT` (struct + // has a single field, so it's the first slot). For a short bytes value + // (< 32 bytes), OZ packs `value | (length * 2)` into the slot. + bytes32 raw = vm.load(address(target), METADATA_STORAGE_SLOT); + // The low byte encodes `length * 2` for short bytes. payload is 1 byte + // long, so the low byte is 2. + assertEq(uint256(raw) & 0xff, 2, "short-bytes length encoding mismatch"); + // The high byte holds the payload itself. + assertEq(uint256(raw) >> 248, 0x42, "short-bytes value mismatch"); + } +} From 498c563dfdd7ff27094bb8bdbcd169ce182b6154 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8r=E2=88=82=C2=A1?= Date: Mon, 18 May 2026 16:34:57 +0000 Subject: [PATCH 06/31] Extension tests --- .../extensions/governance/Addresslist.t.sol | 299 ++++++++++++++++++ .../plugin/extensions/proposal/Proposal.t.sol | 154 +++++++++ 2 files changed, 453 insertions(+) create mode 100644 test/common/plugin/extensions/governance/Addresslist.t.sol create mode 100644 test/common/plugin/extensions/proposal/Proposal.t.sol diff --git a/test/common/plugin/extensions/governance/Addresslist.t.sol b/test/common/plugin/extensions/governance/Addresslist.t.sol new file mode 100644 index 000000000..4b1f3f5c1 --- /dev/null +++ b/test/common/plugin/extensions/governance/Addresslist.t.sol @@ -0,0 +1,299 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {Addresslist} from "../../../../../src/common/plugin/extensions/governance/Addresslist.sol"; +import {AddresslistMock} from "../../../../mocks/commons/plugin/extensions/governance/AddresslistMock.sol"; + +/// @notice Direct tests for `Addresslist` in +/// `src/common/plugin/extensions/governance/Addresslist.sol`. +/// +/// Ports `osx-commons/contracts/test/plugin/extensions/governance/addresslist.ts`. +/// Each TS step that advanced the chain via `evm_mine` is reproduced with +/// `vm.roll(block.number + 1)` so the checkpoint reads target distinct blocks. +/// Closes the gaps from `TESTS.md` §11: large-array behaviour, exact-block +/// precision, off-by-one window around the boundary block. +contract AddresslistTest is Test { + AddresslistMock internal addresslist; + address internal alice; + address internal bob; + address internal carol; + + function setUp() public { + alice = makeAddr("alice"); + bob = makeAddr("bob"); + carol = makeAddr("carol"); + addresslist = new AddresslistMock(); + // Start at a block far from genesis so we can read `block.number - 1` + // without falling off the chain (some Foundry versions choke at 0). + vm.roll(100); + } + + // ------------------------------------------------------------------------- + // addresslistLength + // ------------------------------------------------------------------------- + + function test_addresslistLength_growsWithAdditions() public { + assertEq(addresslist.addresslistLength(), 0); + + _add(alice); + assertEq(addresslist.addresslistLength(), 1); + + _add2(bob, carol); + assertEq(addresslist.addresslistLength(), 3); + } + + function test_addresslistLength_shrinksWithRemovals() public { + _add3(alice, bob, carol); + assertEq(addresslist.addresslistLength(), 3); + + _remove(alice); + assertEq(addresslist.addresslistLength(), 2); + + _remove2(bob, carol); + assertEq(addresslist.addresslistLength(), 0); + } + + // ------------------------------------------------------------------------- + // addresslistLengthAtBlock + // ------------------------------------------------------------------------- + + function test_addresslistLengthAtBlock_reflectsAddHistory() public { + // tx1 at block B1 adds alice → length 1 + uint256 b1 = block.number; + _add(alice); + vm.roll(block.number + 1); + + // tx2 at block B2 adds bob+carol → length 3 + uint256 b2 = block.number; + _add2(bob, carol); + vm.roll(block.number + 1); + + assertEq(addresslist.addresslistLengthAtBlock(b1 - 1), 0); + assertEq(addresslist.addresslistLengthAtBlock(b1), 1); + assertEq(addresslist.addresslistLengthAtBlock(b2), 3); + } + + function test_addresslistLengthAtBlock_reflectsRemovalHistory() public { + uint256 b1 = block.number; + _add3(alice, bob, carol); + vm.roll(block.number + 1); + + uint256 b2 = block.number; + _remove(alice); + vm.roll(block.number + 1); + + uint256 b3 = block.number; + _remove2(bob, carol); + vm.roll(block.number + 1); + + assertLt(b1, b2); + assertLt(b2, b3); + + assertEq(addresslist.addresslistLengthAtBlock(b1), 3); + assertEq(addresslist.addresslistLengthAtBlock(b2), 2); + assertEq(addresslist.addresslistLengthAtBlock(b3), 0); + } + + // ------------------------------------------------------------------------- + // isListed + // ------------------------------------------------------------------------- + + function test_isListed_returnsTrueIfListed() public { + _add(alice); + vm.roll(block.number + 1); + assertTrue(addresslist.isListed(alice)); + } + + function test_isListed_returnsFalseIfNotListed() public view { + assertFalse(addresslist.isListed(alice)); + } + + // ------------------------------------------------------------------------- + // isListedAtBlock + // ------------------------------------------------------------------------- + + function test_isListedAtBlock_returnsTrueAtSpecificBlock() public { + uint256 b1 = block.number; + _add(alice); + vm.roll(block.number + 1); + + uint256 b2 = block.number; + _remove(alice); + vm.roll(block.number + 1); + + assertLt(b1, b2); + assertTrue(addresslist.isListedAtBlock(alice, b1)); + assertFalse(addresslist.isListedAtBlock(alice, b2)); + } + + function test_isListedAtBlock_returnsFalseAtPriorBlock() public { + uint256 b1 = block.number; + _add(alice); + vm.roll(block.number + 1); + + // GAP: precision check — the very block before the add must report false. + assertFalse(addresslist.isListedAtBlock(alice, b1 - 1)); + assertTrue(addresslist.isListedAtBlock(alice, b1)); + } + + // ------------------------------------------------------------------------- + // addAddresses + // ------------------------------------------------------------------------- + + function test_addAddresses_addsAllProvidedEntries() public { + assertFalse(addresslist.isListed(alice)); + assertFalse(addresslist.isListed(bob)); + assertEq(addresslist.addresslistLength(), 0); + + _add2(alice, bob); + vm.roll(block.number + 1); + + assertTrue(addresslist.isListed(alice)); + assertTrue(addresslist.isListed(bob)); + assertEq(addresslist.addresslistLength(), 2); + } + + function test_addAddresses_revertsIfMemberAlreadyListed() public { + _add2(alice, carol); + vm.roll(block.number + 1); + assertEq(addresslist.addresslistLength(), 2); + + // Try to add bob and carol; carol is already listed so the call reverts + // on the second iteration. + address[] memory batch = new address[](2); + batch[0] = bob; + batch[1] = carol; + vm.expectRevert(abi.encodeWithSelector(Addresslist.InvalidAddresslistUpdate.selector, carol)); + addresslist.addAddresses(batch); + + vm.roll(block.number + 1); + assertTrue(addresslist.isListed(alice)); + assertTrue(addresslist.isListed(carol)); + assertFalse(addresslist.isListed(bob)); + assertEq(addresslist.addresslistLength(), 2); + } + + function test_addAddresses_revertsIfDuplicatesInBatch() public { + address[] memory batch = new address[](2); + batch[0] = alice; + batch[1] = alice; + vm.expectRevert(abi.encodeWithSelector(Addresslist.InvalidAddresslistUpdate.selector, alice)); + addresslist.addAddresses(batch); + } + + // ------------------------------------------------------------------------- + // removeAddresses + // ------------------------------------------------------------------------- + + function test_removeAddresses_removesAllProvidedEntries() public { + _add2(alice, bob); + vm.roll(block.number + 1); + + assertTrue(addresslist.isListed(alice)); + assertTrue(addresslist.isListed(bob)); + assertEq(addresslist.addresslistLength(), 2); + + _remove2(alice, bob); + vm.roll(block.number + 1); + + assertFalse(addresslist.isListed(alice)); + assertFalse(addresslist.isListed(bob)); + assertEq(addresslist.addresslistLength(), 0); + } + + function test_removeAddresses_revertsIfMemberNotListed() public { + _add2(alice, bob); + vm.roll(block.number + 1); + assertEq(addresslist.addresslistLength(), 2); + + // Try to remove bob and carol; carol is not listed. + address[] memory batch = new address[](2); + batch[0] = bob; + batch[1] = carol; + vm.expectRevert(abi.encodeWithSelector(Addresslist.InvalidAddresslistUpdate.selector, carol)); + addresslist.removeAddresses(batch); + + vm.roll(block.number + 1); + assertTrue(addresslist.isListed(alice)); + assertTrue(addresslist.isListed(bob)); + assertEq(addresslist.addresslistLength(), 2); + } + + function test_removeAddresses_revertsIfDuplicatesInBatch() public { + _add2(alice, bob); + vm.roll(block.number + 1); + + address[] memory batch = new address[](2); + batch[0] = alice; + batch[1] = alice; + vm.expectRevert(abi.encodeWithSelector(Addresslist.InvalidAddresslistUpdate.selector, alice)); + addresslist.removeAddresses(batch); + } + + // ------------------------------------------------------------------------- + // GAP — large-array behaviour (closes flaw log F18) + // ------------------------------------------------------------------------- + + /// 256 distinct addresses round-trip through add and remove cleanly. Locks + /// in the `_uncheckedAdd` / `_uncheckedSub` checkpoint writes against + /// silent overflow on large inputs. + function test_addAndRemove_largeArray() public { + uint256 N = 256; + address[] memory many = new address[](N); + for (uint256 i = 0; i < N; i++) { + many[i] = address(uint160(0x1000 + i)); + } + + addresslist.addAddresses(many); + vm.roll(block.number + 1); + assertEq(addresslist.addresslistLength(), N); + + for (uint256 i = 0; i < N; i++) { + assertTrue(addresslist.isListed(many[i])); + } + + addresslist.removeAddresses(many); + vm.roll(block.number + 1); + assertEq(addresslist.addresslistLength(), 0); + } + + // ------------------------------------------------------------------------- + // Internal helpers — terse wrappers over single/double/triple add+remove. + // ------------------------------------------------------------------------- + + function _add(address a) internal { + address[] memory arr = new address[](1); + arr[0] = a; + addresslist.addAddresses(arr); + } + + function _add2(address a, address b) internal { + address[] memory arr = new address[](2); + arr[0] = a; + arr[1] = b; + addresslist.addAddresses(arr); + } + + function _add3(address a, address b, address c) internal { + address[] memory arr = new address[](3); + arr[0] = a; + arr[1] = b; + arr[2] = c; + addresslist.addAddresses(arr); + } + + function _remove(address a) internal { + address[] memory arr = new address[](1); + arr[0] = a; + addresslist.removeAddresses(arr); + } + + function _remove2(address a, address b) internal { + address[] memory arr = new address[](2); + arr[0] = a; + arr[1] = b; + addresslist.removeAddresses(arr); + } +} diff --git a/test/common/plugin/extensions/proposal/Proposal.t.sol b/test/common/plugin/extensions/proposal/Proposal.t.sol new file mode 100644 index 000000000..53fae8655 --- /dev/null +++ b/test/common/plugin/extensions/proposal/Proposal.t.sol @@ -0,0 +1,154 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {Proposal} from "../../../../../src/common/plugin/extensions/proposal/Proposal.sol"; +import {ProposalUpgradeable} from "../../../../../src/common/plugin/extensions/proposal/ProposalUpgradeable.sol"; +import {IProposal} from "../../../../../src/common/plugin/extensions/proposal/IProposal.sol"; +import {ProposalMock} from "../../../../mocks/commons/plugin/extensions/proposal/ProposalMock.sol"; +import { + ProposalUpgradeableMock +} from "../../../../mocks/commons/plugin/extensions/proposal/ProposalUpgradeableMock.sol"; + +/// @dev Shared shape both Proposal-mock variants expose. Lets the abstract +/// base test call into either via one typed handle. +interface IProposalLike { + function supportsInterface(bytes4) external view returns (bool); + function proposalCount() external view returns (uint256); +} + +/// @notice Exposes `_createProposalId` for direct testing — needed because +/// the production `_createProposalId` is `internal`. Inherits from the +/// constructable mock to keep the abstract-function stubs intact. +contract ProposalIdHarness is ProposalMock { + function exposed_createProposalId(bytes32 _salt) external view returns (uint256) { + return _createProposalId(_salt); + } +} + +/// @notice Shared behaviour tests for `Proposal` and `ProposalUpgradeable` in +/// `src/common/plugin/extensions/proposal/`. +/// +/// Ports `osx-commons/contracts/test/plugin/extensions/proposal.ts` and closes +/// the gaps from `TESTS.md` §12: explicit `FunctionDeprecated` revert path, +/// legacy `IProposal` v1.0.0 frozen iface ID matches, `_createProposalId` +/// determinism + uniqueness under typical perturbations. +abstract contract ProposalSharedTest is Test { + /// Frozen v1.0.0 `IProposal` interface ID: at that time `IProposal` had + /// only one external function, `proposalCount()`. Single-function + /// interfaces have ID equal to that function's selector. + /// Computed via `cast sig "proposalCount()"`. + bytes4 internal constant IPROPOSAL_V1_0_0_INTERFACE_ID = 0xda35c664; + + IProposalLike internal target; + + function _deployTarget() internal virtual returns (IProposalLike); + + function setUp() public virtual { + target = _deployTarget(); + } + + // ------------------------------------------------------------------------- + // proposalCount — deprecated; reverts + // ------------------------------------------------------------------------- + + function test_proposalCount_revertsAsDeprecated() public { + // Match the selector defined on both `Proposal` and `ProposalUpgradeable`. + vm.expectRevert(Proposal.FunctionDeprecated.selector); + target.proposalCount(); + } + + // ------------------------------------------------------------------------- + // ERC-165 + // ------------------------------------------------------------------------- + + function test_supportsInterface_ERC165() public view { + assertTrue(target.supportsInterface(type(IERC165).interfaceId)); + } + + function test_supportsInterface_IProposalCurrent() public view { + assertTrue(target.supportsInterface(type(IProposal).interfaceId)); + } + + function test_supportsInterface_IProposalLegacyV1_0_0() public view { + // GAP/F12-ish: lock the literal frozen ID against the value computed + // from XOR'ing off the five functions added since v1.0.0 (which is what + // the source does inline). + assertTrue(target.supportsInterface(IPROPOSAL_V1_0_0_INTERFACE_ID)); + } + + function test_supportsInterface_returnsFalseForUnknownInterface() public view { + assertFalse(target.supportsInterface(0xdeadbeef)); + } + + /// Sanity: the legacy ID equals `type(IProposal).interfaceId` XOR'd with + /// the five v1.0.0-absent function selectors. Locks the source-side XOR + /// arithmetic to the literal we assert against above. + function test_supportsInterface_legacyXorMatchesSource() public pure { + bytes4 computed = type(IProposal).interfaceId ^ IProposal.createProposal.selector + ^ IProposal.hasSucceeded.selector ^ IProposal.execute.selector ^ IProposal.canExecute.selector + ^ IProposal.customProposalParamsABI.selector; + assertEq(computed, IPROPOSAL_V1_0_0_INTERFACE_ID); + } +} + +/// @notice Constructable variant. +contract ProposalTest is ProposalSharedTest { + function _deployTarget() internal override returns (IProposalLike) { + return IProposalLike(address(new ProposalMock())); + } +} + +/// @notice Upgradeable variant. +contract ProposalUpgradeableTest is ProposalSharedTest { + function _deployTarget() internal override returns (IProposalLike) { + return IProposalLike(address(new ProposalUpgradeableMock())); + } +} + +/// @notice GAP tests for `_createProposalId` determinism + collision behaviour. +/// Uses a dedicated harness to expose the internal function. Only the +/// constructable variant is tested because the implementation is identical to +/// `ProposalUpgradeable._createProposalId` (verified by inspection of source). +contract ProposalCreateProposalIdTest is Test { + ProposalIdHarness internal harness; + + function setUp() public { + harness = new ProposalIdHarness(); + } + + function test_createProposalId_isDeterministic() public view { + uint256 a = harness.exposed_createProposalId(bytes32("salt")); + uint256 b = harness.exposed_createProposalId(bytes32("salt")); + assertEq(a, b); + } + + function test_createProposalId_differsAcrossSalts() public view { + uint256 a = harness.exposed_createProposalId(bytes32("a")); + uint256 b = harness.exposed_createProposalId(bytes32("b")); + assertTrue(a != b); + } + + function test_createProposalId_differsAcrossBlockNumbers() public { + uint256 a = harness.exposed_createProposalId(bytes32("salt")); + vm.roll(block.number + 1); + uint256 b = harness.exposed_createProposalId(bytes32("salt")); + assertTrue(a != b); + } + + function test_createProposalId_differsAcrossChainIds() public { + uint256 a = harness.exposed_createProposalId(bytes32("salt")); + vm.chainId(block.chainid + 1); + uint256 b = harness.exposed_createProposalId(bytes32("salt")); + assertTrue(a != b); + } + + function test_createProposalId_differsAcrossContractInstances() public { + ProposalIdHarness other = new ProposalIdHarness(); + uint256 a = harness.exposed_createProposalId(bytes32("salt")); + uint256 b = other.exposed_createProposalId(bytes32("salt")); + assertTrue(a != b); + } +} From d6a24494522e1cee3d700312292f5385bada9def Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8r=E2=88=82=C2=A1?= Date: Tue, 19 May 2026 08:51:09 +0000 Subject: [PATCH 07/31] Test: src/common --- test/common/executors/Executor.t.sol | 298 ++++++++ .../permission/auth/DaoAuthorizable.t.sol | 31 +- .../condition/PermissionCondition.t.sol | 65 +- .../condition/extensions/RuledCondition.t.sol | 684 ++++++++++++++++++ test/common/plugin/Plugin.t.sol | 299 ++++++++ test/common/plugin/PluginCloneable.t.sol | 284 ++++++++ .../common/plugin/PluginUUPSUpgradeable.t.sol | 328 +++++++++ .../extensions/governance/Addresslist.t.sol | 32 +- .../plugin/extensions/proposal/Proposal.t.sol | 31 +- test/common/plugin/setup/PluginSetup.t.sol | 154 ++++ test/common/utils/deployment/ProxyLib.t.sol | 53 +- test/common/utils/math/BitMap.t.sol | 26 +- test/common/utils/math/Ratio.t.sol | 47 +- test/common/utils/math/UncheckedMath.t.sol | 23 +- .../utils/metadata/MetadataExtension.t.sol | 50 +- .../utils/versioning/ProtocolVersion.t.sol | 22 +- .../versioning/VersionComparisonLib.t.sol | 109 ++- 17 files changed, 2396 insertions(+), 140 deletions(-) create mode 100644 test/common/executors/Executor.t.sol create mode 100644 test/common/permission/condition/extensions/RuledCondition.t.sol create mode 100644 test/common/plugin/Plugin.t.sol create mode 100644 test/common/plugin/PluginCloneable.t.sol create mode 100644 test/common/plugin/PluginUUPSUpgradeable.t.sol create mode 100644 test/common/plugin/setup/PluginSetup.t.sol diff --git a/test/common/executors/Executor.t.sol b/test/common/executors/Executor.t.sol new file mode 100644 index 000000000..d83d63381 --- /dev/null +++ b/test/common/executors/Executor.t.sol @@ -0,0 +1,298 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test, Vm} from "forge-std/Test.sol"; +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {Executor} from "../../../src/common/executors/Executor.sol"; +import {IExecutor, Action} from "../../../src/common/executors/IExecutor.sol"; +import {ActionExecute} from "../../mocks/commons/executors/ActionExecute.sol"; +import {GasConsumer} from "../../mocks/commons/executors/GasConsumer.sol"; + +/// @notice Direct tests for `Executor` in `src/common/executors/Executor.sol`. +/// +/// Ports `osx-commons/contracts/test/executors/executor.ts` (284 lines, 11 +/// cases). Adds explicit `MAX_ACTIONS` boundary, exact `Executed` event +/// payload, `failureMap` construction, reentrancy guard reset, and storage- +/// slot probe for the custom reentrancy slot. +contract ExecutorTest is Test { + bytes32 internal constant ZERO_CALLID = bytes32(0); + uint256 internal constant MAX_ACTIONS = 256; + bytes4 internal constant ERROR_SELECTOR = 0x08c379a0; // Error(string) + /// keccak256("osx-commons.storage.Executor") — duplicated from source. + bytes32 internal constant REENTRANCY_GUARD_STORAGE_LOCATION = + 0x4d6542319dfb3f7c8adbb488d7b4d7cf849381f14faf4b64de3ac05d08c0bdec; + + Executor internal executor; + ActionExecute internal actionMock; + address internal alice; + + function setUp() public { + alice = makeAddr("alice"); + executor = new Executor(); + actionMock = new ActionExecute(); + } + + // ------------------------------------------------------------------------- + // Action factories + // ------------------------------------------------------------------------- + + function _failAction() internal view returns (Action memory) { + return Action({to: address(actionMock), value: 0, data: abi.encodeCall(ActionExecute.fail, ())}); + } + + function _succeedAction() internal view returns (Action memory) { + return Action({to: address(actionMock), value: 0, data: abi.encodeCall(ActionExecute.setTest, (20))}); + } + + function _reentrancyAction() internal view returns (Action memory) { + return Action({to: address(actionMock), value: 0, data: abi.encodeCall(ActionExecute.callBackCaller, ())}); + } + + function _gasConsumingAction(GasConsumer g, uint256 count) internal pure returns (Action memory) { + return Action({to: address(g), value: 0, data: abi.encodeCall(GasConsumer.consumeGas, (count))}); + } + + // ------------------------------------------------------------------------- + // ERC-165 + // ------------------------------------------------------------------------- + + function test_supportsInterface_ERC165() public view { + assertTrue(executor.supportsInterface(type(IERC165).interfaceId)); + } + + function test_supportsInterface_IExecutor() public view { + assertTrue(executor.supportsInterface(type(IExecutor).interfaceId)); + } + + function test_supportsInterface_returnsFalseForUnknownInterface() public view { + assertFalse(executor.supportsInterface(0xdeadbeef)); + } + + // ------------------------------------------------------------------------- + // Constructor sets reentrancy status to _NOT_ENTERED + // ------------------------------------------------------------------------- + + function test_constructor_setsReentrancyStatusNotEntered() public view { + // The contract uses a custom reentrancy slot; read it directly. + // `_NOT_ENTERED` is the magic value 1 per the source. + bytes32 raw = vm.load(address(executor), REENTRANCY_GUARD_STORAGE_LOCATION); + assertEq(uint256(raw), 1); + } + + // ------------------------------------------------------------------------- + // MAX_ACTIONS boundary + // ------------------------------------------------------------------------- + + function test_execute_revertsIfMoreThanMaxActions() public { + Action[] memory tooMany = new Action[](MAX_ACTIONS + 1); + for (uint256 i = 0; i < MAX_ACTIONS; i++) { + tooMany[i] = _succeedAction(); + } + tooMany[MAX_ACTIONS] = _failAction(); + + vm.expectRevert(Executor.TooManyActions.selector); + executor.execute(ZERO_CALLID, tooMany, 0); + } + + function test_execute_acceptsExactlyMaxActions() public { + Action[] memory exactly = new Action[](MAX_ACTIONS); + for (uint256 i = 0; i < MAX_ACTIONS; i++) { + exactly[i] = _succeedAction(); + } + executor.execute(ZERO_CALLID, exactly, 0); + } + + // ------------------------------------------------------------------------- + // Failure handling + // ------------------------------------------------------------------------- + + function test_execute_revertsIfActionFailsAndNotInAllowMap() public { + Action[] memory actions = new Action[](1); + actions[0] = _failAction(); + vm.expectRevert(abi.encodeWithSelector(Executor.ActionFailed.selector, 0)); + executor.execute(ZERO_CALLID, actions, 0); + } + + function test_execute_succeedsIfFailureAllowed() public { + Action[] memory actions = new Action[](1); + actions[0] = _failAction(); + (bytes[] memory results,) = executor.execute(ZERO_CALLID, actions, 1); + // The result includes the revert reason wrapped in `Error(string)`. + bytes4 sel = bytes4(results[0]); + assertEq(sel, ERROR_SELECTOR); + } + + function test_execute_returnsCorrectResultIfActionSucceeds() public { + Action[] memory actions = new Action[](1); + actions[0] = _succeedAction(); + (bytes[] memory results,) = executor.execute(ZERO_CALLID, actions, 0); + // `setTest(20)` returns 20. + assertEq(abi.decode(results[0], (uint256)), 20); + } + + function test_execute_constructsFailureMapCorrectly() public { + // 6 actions: first 3 fail, last 3 succeed. Allow bits 0,1,2 to fail. + uint256 allowMap = (1 << 0) | (1 << 1) | (1 << 2); + Action[] memory actions = new Action[](6); + for (uint256 i = 0; i < 3; i++) { + actions[i] = _failAction(); + } + for (uint256 i = 3; i < 6; i++) { + actions[i] = _succeedAction(); + } + (bytes[] memory results, uint256 failureMap) = executor.execute(ZERO_CALLID, actions, allowMap); + + // failureMap must have bits 0,1,2 set (those failed). + assertEq(failureMap, (1 << 0) | (1 << 1) | (1 << 2)); + // Failed action results include the Error(string) selector. + for (uint256 i = 0; i < 3; i++) { + assertEq(bytes4(results[i]), ERROR_SELECTOR); + } + // Succeeded action results encode the return value 20. + for (uint256 i = 3; i < 6; i++) { + assertEq(abi.decode(results[i], (uint256)), 20); + } + + // Now flip bit 2 off in the allow map → action 2 reverts. + uint256 allowMapMinus2 = allowMap ^ (1 << 2); + vm.expectRevert(abi.encodeWithSelector(Executor.ActionFailed.selector, 2)); + executor.execute(ZERO_CALLID, actions, allowMapMinus2); + } + + // ------------------------------------------------------------------------- + // Reentrancy + // ------------------------------------------------------------------------- + + function test_execute_revertsOnReentryWhenNotAllowed() public { + // The reentrancy action calls back into executor.execute. With + // allowMap = 0, the outer call reverts ActionFailed(0) because the + // inner reverts ReentrantCall (a sub-revert is wrapped). + Action[] memory actions = new Action[](1); + actions[0] = _reentrancyAction(); + vm.expectRevert(abi.encodeWithSelector(Executor.ActionFailed.selector, 0)); + executor.execute(ZERO_CALLID, actions, 0); + } + + function test_execute_capturesReentrancyErrorInResultsWhenAllowed() public { + // Allow the action to fail and capture the ReentrantCall selector + // inside the recorded execResult. + Action[] memory actions = new Action[](1); + actions[0] = _reentrancyAction(); + (bytes[] memory results,) = executor.execute(ZERO_CALLID, actions, 1); + assertEq(bytes4(results[0]), Executor.ReentrantCall.selector); + } + + function test_execute_reentrancyStatusResetsAfterCall() public { + // Verify the reentrancy guard resets to _NOT_ENTERED after a + // successful execute (per the `nonReentrant` modifier). + Action[] memory actions = new Action[](1); + actions[0] = _succeedAction(); + executor.execute(ZERO_CALLID, actions, 0); + + bytes32 raw = vm.load(address(executor), REENTRANCY_GUARD_STORAGE_LOCATION); + assertEq(uint256(raw), 1, "reentrancy guard not reset"); + } + + // ------------------------------------------------------------------------- + // Executed event + // ------------------------------------------------------------------------- + + bytes32 internal constant EXECUTED_TOPIC = + keccak256("Executed(address,bytes32,(address,uint256,bytes)[],uint256,uint256,bytes[])"); + + function test_execute_emitsExecutedWithCorrectFields() public { + Action[] memory actions = new Action[](1); + actions[0] = _succeedAction(); + + vm.recordLogs(); + executor.execute(ZERO_CALLID, actions, 0); + Vm.Log[] memory logs = vm.getRecordedLogs(); + + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].emitter == address(executor) && logs[i].topics[0] == EXECUTED_TOPIC) { + // topics[1] = indexed actor (== msg.sender of the execute call). + address actor = address(uint160(uint256(logs[i].topics[1]))); + assertEq(actor, address(this)); + + ( + bytes32 callId, + Action[] memory loggedActions, + uint256 allowFailureMap, + uint256 failureMap, + bytes[] memory execResults + ) = abi.decode(logs[i].data, (bytes32, Action[], uint256, uint256, bytes[])); + assertEq(callId, ZERO_CALLID); + assertEq(loggedActions.length, 1); + assertEq(loggedActions[0].to, address(actionMock)); + assertEq(loggedActions[0].value, 0); + assertEq(allowFailureMap, 0); + assertEq(failureMap, 0); + assertEq(execResults.length, 1); + assertEq(abi.decode(execResults[0], (uint256)), 20); + found = true; + break; + } + } + assertTrue(found, "Executed not emitted"); + } + + // ------------------------------------------------------------------------- + // InsufficientGas — see Executor.execute's 63/64 check + // ------------------------------------------------------------------------- + + function test_execute_revertsInsufficientGasManyActions() public { + GasConsumer g = new GasConsumer(); + Action[] memory actions = new Action[](1); + actions[0] = _gasConsumingAction(g, 20); + uint256 allowMap = 1; // allow action 0 to fail + + // The exact gas figure varies; run the call inside a try/catch loop + // until we find a limit that triggers `InsufficientGas`. The TS suite + // hard-codes `expectedGas - 32000`; here we just walk downward. + bool reverted; + for (uint256 trim = 8_000; trim <= 60_000; trim += 2_000) { + try this._callWithGasLimit(allowMap, actions, trim) returns (bool ok) { + ok; // ignore + } catch (bytes memory err) { + if (bytes4(err) == Executor.InsufficientGas.selector) { + reverted = true; + break; + } + } + } + assertTrue(reverted, "Could not trigger InsufficientGas; adjust trim range"); + } + + function test_execute_revertsInsufficientGasOneAction() public { + GasConsumer g = new GasConsumer(); + Action[] memory actions = new Action[](1); + actions[0] = _gasConsumingAction(g, 3); + uint256 allowMap = 1; + + bool reverted; + for (uint256 trim = 4_000; trim <= 20_000; trim += 1_000) { + try this._callWithGasLimit(allowMap, actions, trim) returns (bool ok) { + ok; + } catch (bytes memory err) { + if (bytes4(err) == Executor.InsufficientGas.selector) { + reverted = true; + break; + } + } + } + assertTrue(reverted, "Could not trigger InsufficientGas; adjust trim range"); + } + + /// External entrypoint to drive `executor.execute` with a constrained gas + /// budget. The `try/catch` in the tests above needs an external call to + /// catch the InsufficientGas selector cleanly. + function _callWithGasLimit(uint256 allowMap, Action[] calldata actions, uint256 trim) external returns (bool) { + // Budget headroom: enough so the outer test doesn't OOG, but tight + // enough to push the inner `.to.call` past the 63/64 boundary. + uint256 budget = 50_000 + trim; + executor.execute{gas: budget}(ZERO_CALLID, actions, allowMap); + return true; + } +} diff --git a/test/common/permission/auth/DaoAuthorizable.t.sol b/test/common/permission/auth/DaoAuthorizable.t.sol index 24dc62927..5eda06088 100644 --- a/test/common/permission/auth/DaoAuthorizable.t.sol +++ b/test/common/permission/auth/DaoAuthorizable.t.sol @@ -6,15 +6,14 @@ import {Test} from "forge-std/Test.sol"; import {IDAO} from "../../../../src/common/dao/IDAO.sol"; import {DaoUnauthorized} from "../../../../src/common/permission/auth/auth.sol"; import {DaoAuthorizableMock} from "../../../mocks/commons/permission/auth/DaoAuthorizableMock.sol"; -import { - DaoAuthorizableUpgradeableMock -} from "../../../mocks/commons/permission/auth/DaoAuthorizableUpgradeableMock.sol"; +import {DaoAuthorizableUpgradeableMock} from "../../../mocks/commons/permission/auth/DaoAuthorizableUpgradeableMock.sol"; import {DAOMock} from "../../../mocks/commons/dao/DAOMock.sol"; /// @dev Minimal shape both `DaoAuthorizableMock` and `DaoAuthorizableUpgradeableMock` expose. /// Lets the shared base test contract call into either variant through one typed reference. interface IDaoAuthorizableMock { function dao() external view returns (IDAO); + function authorizedFunc() external; } @@ -23,9 +22,8 @@ interface IDaoAuthorizableMock { /// Ports `osx-commons/contracts/test/permission/auth/dao-authorizable.ts`. The /// TS suite's `daoAuthorizableBaseTests(fixture)` DRY pattern is reproduced /// via this abstract base + two concrete derivations (one per source contract). -/// Gaps closed from `TESTS.md` §8: `DaoUnauthorized` error carries all four -/// fields, auth guard reverts (no silent fallthrough), and `dao()` returns the -/// exact stored address. +/// Adds: `DaoUnauthorized` error carries all four fields, auth guard reverts +/// (no silent fallthrough), and `dao()` returns the exact stored address. abstract contract DaoAuthorizableSharedTest is Test { bytes32 internal constant PERM_ID = keccak256("AUTHORIZED_FUNC_PERMISSION"); @@ -51,7 +49,13 @@ abstract contract DaoAuthorizableSharedTest is Test { assertFalse(daoMock.hasPermissionReturnValueMock()); vm.expectRevert( - abi.encodeWithSelector(DaoUnauthorized.selector, address(daoMock), address(target), bob, PERM_ID) + abi.encodeWithSelector( + DaoUnauthorized.selector, + address(daoMock), + address(target), + bob, + PERM_ID + ) ); vm.prank(bob); target.authorizedFunc(); @@ -67,7 +71,10 @@ abstract contract DaoAuthorizableSharedTest is Test { /// @notice Constructable variant: `DaoAuthorizable` is set via constructor. contract DaoAuthorizableTest is DaoAuthorizableSharedTest { function _deployTarget() internal override returns (IDaoAuthorizableMock) { - return IDaoAuthorizableMock(address(new DaoAuthorizableMock(IDAO(address(daoMock))))); + return + IDaoAuthorizableMock( + address(new DaoAuthorizableMock(IDAO(address(daoMock)))) + ); } } @@ -81,13 +88,17 @@ contract DaoAuthorizableUpgradeableTest is DaoAuthorizableSharedTest { } function test_initialize_revertsIfCalledTwice() public { - DaoAuthorizableUpgradeableMock m = DaoAuthorizableUpgradeableMock(address(target)); + DaoAuthorizableUpgradeableMock m = DaoAuthorizableUpgradeableMock( + address(target) + ); vm.expectRevert("Initializable: contract is already initialized"); m.initialize(IDAO(address(daoMock))); } function test_initInternal_revertsIfCalledOutsideInitializer() public { - DaoAuthorizableUpgradeableMock m = DaoAuthorizableUpgradeableMock(address(target)); + DaoAuthorizableUpgradeableMock m = DaoAuthorizableUpgradeableMock( + address(target) + ); vm.expectRevert("Initializable: contract is not initializing"); m.notAnInitializer(IDAO(address(daoMock))); } diff --git a/test/common/permission/condition/PermissionCondition.t.sol b/test/common/permission/condition/PermissionCondition.t.sol index 806d1cb5a..a34e8b447 100644 --- a/test/common/permission/condition/PermissionCondition.t.sol +++ b/test/common/permission/condition/PermissionCondition.t.sol @@ -7,15 +7,14 @@ import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import {IPermissionCondition} from "../../../../src/common/permission/condition/IPermissionCondition.sol"; import {IProtocolVersion} from "../../../../src/common/utils/versioning/IProtocolVersion.sol"; import {PermissionConditionMock} from "../../../mocks/commons/permission/condition/PermissionConditionMock.sol"; -import { - PermissionConditionUpgradeableMock -} from "../../../mocks/commons/permission/condition/PermissionConditionUpgradeableMock.sol"; +import {PermissionConditionUpgradeableMock} from "../../../mocks/commons/permission/condition/PermissionConditionUpgradeableMock.sol"; /// @dev Minimal shape both `PermissionConditionMock` and /// `PermissionConditionUpgradeableMock` expose. Lets the shared base call into /// either variant through one typed reference. interface IPermissionConditionMock { function supportsInterface(bytes4) external view returns (bool); + function protocolVersion() external view returns (uint8[3] memory); } @@ -24,18 +23,21 @@ interface IPermissionConditionMock { /// /// Ports `osx-commons/contracts/test/permission/condition/permission-condition.ts` /// (the TS describe was labelled `IProposal` due to a copy-paste error — fixed -/// here). Closes the gaps from `TESTS.md` §9: negative `supportsInterface` -/// case, exact `protocolVersion()` return value, frozen `IPermissionCondition` -/// iface ID matches v1.0.0. +/// here). Adds: negative `supportsInterface` case, exact `protocolVersion()` +/// return value, frozen `IPermissionCondition` iface ID matches v1.0.0. abstract contract PermissionConditionSharedTest is Test { /// Frozen `IPermissionCondition` interface ID introduced in v1.0.0. /// Single function `isGranted(address,address,bytes32,bytes)`. /// Computed via `cast sig "isGranted(address,address,bytes32,bytes)"`. - bytes4 internal constant IPERMISSION_CONDITION_V1_0_0_INTERFACE_ID = 0x2675fdd0; + bytes4 internal constant IPERMISSION_CONDITION_V1_0_0_INTERFACE_ID = + 0x2675fdd0; IPermissionConditionMock internal conditionMock; - function _deployConditionMock() internal virtual returns (IPermissionConditionMock); + function _deployConditionMock() + internal + virtual + returns (IPermissionConditionMock); function setUp() public virtual { conditionMock = _deployConditionMock(); @@ -45,15 +47,24 @@ abstract contract PermissionConditionSharedTest is Test { // IPermissionCondition iface ID (drift detector vs v1.0.0) // ------------------------------------------------------------------------- - function test_IPermissionCondition_hasSameInterfaceIdAsV1_0_0() public pure { - assertEq(type(IPermissionCondition).interfaceId, IPERMISSION_CONDITION_V1_0_0_INTERFACE_ID); + function test_IPermissionCondition_hasSameInterfaceIdAsV1_0_0() + public + pure + { + assertEq( + type(IPermissionCondition).interfaceId, + IPERMISSION_CONDITION_V1_0_0_INTERFACE_ID + ); } // ------------------------------------------------------------------------- // Protocol version // ------------------------------------------------------------------------- - function test_protocolVersion_returnsCurrentProductionVersion() public view { + function test_protocolVersion_returnsCurrentProductionVersion() + public + view + { uint8[3] memory v = conditionMock.protocolVersion(); assertEq(v[0], 1); assertEq(v[1], 4); @@ -69,28 +80,48 @@ abstract contract PermissionConditionSharedTest is Test { } function test_supportsInterface_IPermissionCondition() public view { - assertTrue(conditionMock.supportsInterface(type(IPermissionCondition).interfaceId)); + assertTrue( + conditionMock.supportsInterface( + type(IPermissionCondition).interfaceId + ) + ); } function test_supportsInterface_IProtocolVersion() public view { - assertTrue(conditionMock.supportsInterface(type(IProtocolVersion).interfaceId)); + assertTrue( + conditionMock.supportsInterface(type(IProtocolVersion).interfaceId) + ); } /// GAP: negative case — an unrelated random selector returns false. - function test_supportsInterface_returnsFalseForUnknownInterface() public view { + function test_supportsInterface_returnsFalseForUnknownInterface() + public + view + { assertFalse(conditionMock.supportsInterface(0xdeadbeef)); assertFalse(conditionMock.supportsInterface(0x00000000)); } } contract PermissionConditionTest is PermissionConditionSharedTest { - function _deployConditionMock() internal override returns (IPermissionConditionMock) { + function _deployConditionMock() + internal + override + returns (IPermissionConditionMock) + { return IPermissionConditionMock(address(new PermissionConditionMock())); } } contract PermissionConditionUpgradeableTest is PermissionConditionSharedTest { - function _deployConditionMock() internal override returns (IPermissionConditionMock) { - return IPermissionConditionMock(address(new PermissionConditionUpgradeableMock())); + function _deployConditionMock() + internal + override + returns (IPermissionConditionMock) + { + return + IPermissionConditionMock( + address(new PermissionConditionUpgradeableMock()) + ); } } diff --git a/test/common/permission/condition/extensions/RuledCondition.t.sol b/test/common/permission/condition/extensions/RuledCondition.t.sol new file mode 100644 index 000000000..7f0bbaac8 --- /dev/null +++ b/test/common/permission/condition/extensions/RuledCondition.t.sol @@ -0,0 +1,684 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test, Vm} from "forge-std/Test.sol"; +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {IPermissionCondition} from "../../../../../src/common/permission/condition/IPermissionCondition.sol"; +import {IProtocolVersion} from "../../../../../src/common/utils/versioning/IProtocolVersion.sol"; +import {RuledCondition} from "../../../../../src/common/permission/condition/extensions/RuledCondition.sol"; +import {RuledConditionMock} from "../../../../mocks/commons/permission/condition/extensions/RuledConditionMock.sol"; +import {PermissionConditionMock} from "../../../../mocks/commons/permission/condition/PermissionConditionMock.sol"; +import {DAOMock} from "../../../../mocks/commons/dao/DAOMock.sol"; + +/// @dev Always reverts. Used to verify `_checkCondition` swallows external +/// reverts and returns false (no propagation). +contract RevertingConditionMock is IPermissionCondition { + function isGranted( + address, + address, + bytes32, + bytes calldata + ) external pure override returns (bool) { + revert("nope"); + } +} + +/// @dev Returns a non-32-byte payload from `isGranted`. Used to verify +/// `_checkCondition` rejects malformed returndata as false. +contract WeirdReturndataConditionMock { + fallback() external { + // Return 64 bytes of garbage (not the expected 32-byte bool). + bytes memory data = abi.encode(uint256(1), uint256(1)); + assembly { + return(add(data, 0x20), mload(data)) + } + } +} + +/// @notice Direct tests for `RuledCondition` in +/// `src/common/permission/condition/extensions/RuledCondition.sol`. +/// +/// Ports `osx-commons/contracts/test/permission/condition/extensions/ruled-condition.ts` +/// (1,069 lines, 24 TS cases). The IF_ELSE argument-order regression is owned +/// by `test/other/RuledCondition.t.sol`; not duplicated here. Adds: +/// `_checkCondition` revert / malformed-returndata handling, encode/decode +/// round-trips, empty-array clear, large-array push. +contract RuledConditionTest is Test { + /// Rule-id constants — duplicated from the source (where they are `internal`). + uint8 internal constant BLOCK_NUMBER_RULE_ID = 200; + uint8 internal constant TIMESTAMP_RULE_ID = 201; + uint8 internal constant CONDITION_RULE_ID = 202; + uint8 internal constant LOGIC_OP_RULE_ID = 203; + uint8 internal constant VALUE_RULE_ID = 204; + + /// Op enum positions. + uint8 internal constant OP_NONE = 0; + uint8 internal constant OP_EQ = 1; + uint8 internal constant OP_NEQ = 2; + uint8 internal constant OP_GT = 3; + uint8 internal constant OP_LT = 4; + uint8 internal constant OP_GTE = 5; + uint8 internal constant OP_LTE = 6; + uint8 internal constant OP_RET = 7; + uint8 internal constant OP_NOT = 8; + uint8 internal constant OP_AND = 9; + uint8 internal constant OP_OR = 10; + uint8 internal constant OP_XOR = 11; + uint8 internal constant OP_IF_ELSE = 12; + + bytes32 internal constant DUMMY_PERMISSION = keccak256("DUMMY_PERMISSION"); + + RuledConditionMock internal ruled; + PermissionConditionMock internal subA; + PermissionConditionMock internal subB; + PermissionConditionMock internal subC; + DAOMock internal daoMock; + + address internal deployer; + + function setUp() public { + deployer = address(this); + daoMock = new DAOMock(); + ruled = new RuledConditionMock(); + subA = new PermissionConditionMock(); + subB = new PermissionConditionMock(); + subC = new PermissionConditionMock(); + } + + // ------------------------------------------------------------------------- + // Rule construction helpers — Solidity analogues of the TS factories. + // ------------------------------------------------------------------------- + + function _rule( + uint8 id, + uint8 op, + uint240 value + ) internal pure returns (RuledCondition.Rule memory) { + return + RuledCondition.Rule({ + id: id, + op: op, + value: value, + permissionId: DUMMY_PERMISSION + }); + } + + function _conditionAddr( + IPermissionCondition cond + ) internal pure returns (uint240) { + return uint240(uint160(address(cond))); + } + + function _isGranted(uint256[] memory list) internal view returns (bool) { + bytes memory data = list.length == 0 ? bytes("") : abi.encode(list); + return + ruled.isGranted(address(daoMock), deployer, DUMMY_PERMISSION, data); + } + + function _isGrantedEmpty() internal view returns (bool) { + return + ruled.isGranted(address(daoMock), deployer, DUMMY_PERMISSION, ""); + } + + // Rule-array factories that mirror the TS `*_rule` helpers. + + function _logicABRule( + uint8 op + ) internal view returns (RuledCondition.Rule[] memory rs) { + rs = new RuledCondition.Rule[](3); + rs[0] = _rule(LOGIC_OP_RULE_ID, op, ruled.encodeLogicalOperator(1, 2)); + rs[1] = _rule(CONDITION_RULE_ID, OP_EQ, _conditionAddr(subA)); + rs[2] = _rule(CONDITION_RULE_ID, OP_EQ, _conditionAddr(subB)); + } + + function _notRule( + uint240 value + ) internal view returns (RuledCondition.Rule[] memory rs) { + rs = new RuledCondition.Rule[](2); + rs[0] = _rule( + LOGIC_OP_RULE_ID, + OP_NOT, + ruled.encodeLogicalOperator(1, 2) + ); + rs[1] = _rule(VALUE_RULE_ID, OP_RET, value); + } + + function _comparisonRule( + uint8 op, + uint240 value + ) internal pure returns (RuledCondition.Rule[] memory rs) { + rs = new RuledCondition.Rule[](1); + // id = 0 → lookup _compareList[0] + rs[0] = _rule(0, op, value); + } + + /// `C || (A && B)` — TS `C_or_B_and_A_rule`. + function _COrAandBRule() + internal + view + returns (RuledCondition.Rule[] memory rs) + { + rs = new RuledCondition.Rule[](5); + rs[0] = _rule( + LOGIC_OP_RULE_ID, + OP_OR, + ruled.encodeLogicalOperator(1, 2) + ); + rs[1] = _rule(CONDITION_RULE_ID, OP_EQ, _conditionAddr(subC)); + rs[2] = _rule( + LOGIC_OP_RULE_ID, + OP_AND, + ruled.encodeLogicalOperator(3, 4) + ); + rs[3] = _rule(CONDITION_RULE_ID, OP_EQ, _conditionAddr(subA)); + rs[4] = _rule(CONDITION_RULE_ID, OP_EQ, _conditionAddr(subB)); + } + + /// IF subA THEN ret(1) ELSE subB — TS `if_A_else_B`. + function _ifAElseBRule() + internal + view + returns (RuledCondition.Rule[] memory rs) + { + rs = new RuledCondition.Rule[](4); + rs[0] = _rule( + LOGIC_OP_RULE_ID, + OP_IF_ELSE, + ruled.encodeIfElse(1, 2, 3) + ); + rs[1] = _rule(CONDITION_RULE_ID, OP_EQ, _conditionAddr(subA)); + rs[2] = _rule(VALUE_RULE_ID, OP_RET, 1); + rs[3] = _rule(CONDITION_RULE_ID, OP_EQ, _conditionAddr(subB)); + } + + /// `compareList[0] <= list[1] AND compareList[1] <= list[2]` — TS `three_elements_list_ordered_rule`. + function _threeElementsOrderedRule( + uint240 mid, + uint240 high + ) internal view returns (RuledCondition.Rule[] memory rs) { + rs = new RuledCondition.Rule[](3); + rs[0] = _rule( + LOGIC_OP_RULE_ID, + OP_AND, + ruled.encodeLogicalOperator(1, 2) + ); + rs[1] = _rule(0, OP_LTE, mid); + rs[2] = _rule(1, OP_LTE, high); + } + + // ------------------------------------------------------------------------- + // updateRules + emit + // ------------------------------------------------------------------------- + + function test_updateRules_storesAndEmitsRulesUpdated() public { + RuledCondition.Rule[] memory rs = new RuledCondition.Rule[](1); + rs[0] = _rule(CONDITION_RULE_ID, OP_EQ, 777); + + vm.recordLogs(); + ruled.updateRules(rs); + Vm.Log[] memory logs = vm.getRecordedLogs(); + assertEq( + logs[0].topics[0], + keccak256("RulesUpdated((uint8,uint8,uint240,bytes32)[])") + ); + + RuledCondition.Rule[] memory stored = ruled.getRules(); + assertEq(stored.length, 1); + assertEq(stored[0].id, CONDITION_RULE_ID); + assertEq(stored[0].op, OP_EQ); + assertEq(uint256(stored[0].value), 777); + assertEq(stored[0].permissionId, DUMMY_PERMISSION); + } + + // ------------------------------------------------------------------------- + // Simple condition rule (CONDITION_RULE_ID with sub-condition) + // ------------------------------------------------------------------------- + + function test_simpleRule_evaluatesTrue() public { + RuledCondition.Rule[] memory rs = new RuledCondition.Rule[](1); + rs[0] = _rule(CONDITION_RULE_ID, OP_EQ, _conditionAddr(subA)); + ruled.updateRules(rs); + + subA.setAnswer(true); + assertTrue(_isGrantedEmpty()); + } + + function test_simpleRule_evaluatesFalse() public { + RuledCondition.Rule[] memory rs = new RuledCondition.Rule[](1); + rs[0] = _rule(CONDITION_RULE_ID, OP_EQ, _conditionAddr(subA)); + ruled.updateRules(rs); + + // subA.answer defaults to false. + assertFalse(_isGrantedEmpty()); + } + + // ------------------------------------------------------------------------- + // Complex rule: C || (A && B) + // ------------------------------------------------------------------------- + + function test_complexRule_COrAandB_evaluatesTrue() public { + ruled.updateRules(_COrAandBRule()); + + subA.setAnswer(true); + subB.setAnswer(true); + // C(false) || (A(true) && B(true)) → true + assertTrue(_isGrantedEmpty()); + + subC.setAnswer(true); + subA.setAnswer(false); + subB.setAnswer(false); + // C(true) || ... → true + assertTrue(_isGrantedEmpty()); + } + + function test_complexRule_COrAandB_evaluatesFalse() public { + ruled.updateRules(_COrAandBRule()); + + subA.setAnswer(true); + // C(false) || (A(true) && B(false)) → false + assertFalse(_isGrantedEmpty()); + } + + // ------------------------------------------------------------------------- + // IF_ELSE — both-branch evaluation (param order is covered by test/other/RuledCondition.t.sol) + // ------------------------------------------------------------------------- + + function test_ifElse_evaluatesBothBranches() public { + ruled.updateRules(_ifAElseBRule()); + + // Both sub-conditions false → IF returns false, ELSE branch (subB) returns false. + assertFalse(_isGrantedEmpty()); + + // subA true → IF branch (VALUE_RULE RET 1) returns true. + subA.setAnswer(true); + assertTrue(_isGrantedEmpty()); + + // subA false, subB true → ELSE branch (subB) returns true. + subA.setAnswer(false); + subB.setAnswer(true); + assertTrue(_isGrantedEmpty()); + } + + // ------------------------------------------------------------------------- + // BLOCK_NUMBER / TIMESTAMP rules + // ------------------------------------------------------------------------- + + function test_blockNumberRule_evaluatesAgainstBlockNumber() public { + // block.number ≥ 1 → true + RuledCondition.Rule[] memory rs = new RuledCondition.Rule[](1); + rs[0] = _rule(BLOCK_NUMBER_RULE_ID, OP_GTE, 1); + ruled.updateRules(rs); + assertTrue(_isGrantedEmpty()); + + // block.number < 1 → false (test runs at block.number ≥ 1) + rs[0] = _rule(BLOCK_NUMBER_RULE_ID, OP_LT, 1); + ruled.updateRules(rs); + assertFalse(_isGrantedEmpty()); + } + + function test_timestampRule_evaluatesAgainstTimestamp() public { + RuledCondition.Rule[] memory rs = new RuledCondition.Rule[](1); + rs[0] = _rule(TIMESTAMP_RULE_ID, OP_GTE, 1); + ruled.updateRules(rs); + assertTrue(_isGrantedEmpty()); + + rs[0] = _rule(TIMESTAMP_RULE_ID, OP_LT, 1); + ruled.updateRules(rs); + assertFalse(_isGrantedEmpty()); + } + + // ------------------------------------------------------------------------- + // AND / OR / XOR / NOT + // ------------------------------------------------------------------------- + + function test_andOperation() public { + ruled.updateRules(_logicABRule(OP_AND)); + + subA.setAnswer(true); + subB.setAnswer(true); + assertTrue(_isGrantedEmpty()); + + subA.setAnswer(false); + subB.setAnswer(true); + assertFalse(_isGrantedEmpty()); + + subA.setAnswer(false); + subB.setAnswer(false); + assertFalse(_isGrantedEmpty()); + + subA.setAnswer(true); + subB.setAnswer(false); + assertFalse(_isGrantedEmpty()); + } + + function test_orOperation() public { + ruled.updateRules(_logicABRule(OP_OR)); + + subA.setAnswer(true); + subB.setAnswer(false); + assertTrue(_isGrantedEmpty()); + + subA.setAnswer(false); + subB.setAnswer(true); + assertTrue(_isGrantedEmpty()); + + subA.setAnswer(true); + subB.setAnswer(true); + assertTrue(_isGrantedEmpty()); + + subA.setAnswer(false); + subB.setAnswer(false); + assertFalse(_isGrantedEmpty()); + } + + function test_xorOperation() public { + ruled.updateRules(_logicABRule(OP_XOR)); + + subA.setAnswer(true); + subB.setAnswer(false); + assertTrue(_isGrantedEmpty()); + + subA.setAnswer(false); + subB.setAnswer(true); + assertTrue(_isGrantedEmpty()); + + subA.setAnswer(false); + subB.setAnswer(false); + assertFalse(_isGrantedEmpty()); + + subA.setAnswer(true); + subB.setAnswer(true); + assertFalse(_isGrantedEmpty()); + } + + function test_notOperation() public { + ruled.updateRules(_notRule(0)); + // NOT 0 → true + assertTrue(_isGrantedEmpty()); + + ruled.updateRules(_notRule(1)); + // NOT 1 → false + assertFalse(_isGrantedEmpty()); + } + + // ------------------------------------------------------------------------- + // Comparison operators against the compareList + // ------------------------------------------------------------------------- + + function test_eqOperation() public { + ruled.updateRules(_comparisonRule(OP_EQ, 1)); + + uint256[] memory list = new uint256[](3); + list[0] = 1; + list[1] = 2; + list[2] = 3; + assertTrue(_isGranted(list)); // 1 == 1 + + list[0] = 2; + assertFalse(_isGranted(list)); // 2 != 1 + } + + function test_neqOperation() public { + ruled.updateRules(_comparisonRule(OP_NEQ, 1)); + + uint256[] memory list = new uint256[](3); + list[0] = 2; + list[1] = 2; + list[2] = 3; + assertTrue(_isGranted(list)); // 2 != 1 + + list[0] = 1; + assertFalse(_isGranted(list)); // 1 == 1 + } + + function test_gtOperation() public { + ruled.updateRules(_comparisonRule(OP_GT, 5)); + + uint256[] memory list = new uint256[](3); + list[0] = 10; + list[1] = 20; + list[2] = 30; + assertTrue(_isGranted(list)); // 10 > 5 + + list[0] = 1; + assertFalse(_isGranted(list)); // 1 < 5 + } + + function test_gteOperation() public { + ruled.updateRules(_comparisonRule(OP_GTE, 10)); + + uint256[] memory list = new uint256[](3); + list[0] = 10; + list[1] = 20; + list[2] = 30; + assertTrue(_isGranted(list)); // 10 >= 10 + + list[0] = 1; + assertFalse(_isGranted(list)); // 1 < 10 + } + + function test_ltOperation() public { + ruled.updateRules(_comparisonRule(OP_LT, 10)); + + uint256[] memory list = new uint256[](3); + list[0] = 1; + list[1] = 2; + list[2] = 3; + assertTrue(_isGranted(list)); // 1 < 10 + + list[0] = 11; + assertFalse(_isGranted(list)); // 11 > 10 + } + + function test_lteOperation() public { + ruled.updateRules(_comparisonRule(OP_LTE, 10)); + + uint256[] memory list = new uint256[](3); + list[0] = 10; + list[1] = 20; + list[2] = 30; + assertTrue(_isGranted(list)); // 10 <= 10 + + list[0] = 11; + assertFalse(_isGranted(list)); // 11 > 10 + } + + function test_noneOperation_returnsFalse() public { + RuledCondition.Rule[] memory rs = new RuledCondition.Rule[](1); + // id=1, op=NONE → falls through `_compare`'s switch to `return false`. + rs[0] = _rule(1, OP_NONE, 2); + ruled.updateRules(rs); + + uint256[] memory list = new uint256[](3); + list[0] = 1; + list[1] = 2; + list[2] = 3; + assertFalse(_isGranted(list)); + } + + // ------------------------------------------------------------------------- + // Compare-list checks + // ------------------------------------------------------------------------- + + function test_compareList_orderedAscending() public { + // list=[1,2,3] with rule "list[0]≤list[1] AND list[1]≤list[2]" → true. + uint256[] memory list = new uint256[](3); + list[0] = 1; + list[1] = 2; + list[2] = 3; + ruled.updateRules( + _threeElementsOrderedRule(uint240(list[1]), uint240(list[2])) + ); + assertTrue(_isGranted(list)); + } + + function test_compareList_descendingFails() public { + // list=[3,2,1] — descending; the rule checks list[0]≤2 AND list[1]≤1 → first part false. + uint256[] memory list = new uint256[](3); + list[0] = 3; + list[1] = 2; + list[2] = 1; + ruled.updateRules( + _threeElementsOrderedRule(uint240(list[1]), uint240(list[2])) + ); + assertFalse(_isGranted(list)); + } + + function test_compareList_outOfOrderMidElement() public { + // list=[2,3,1] — rule "list[0]≤3 AND list[1]≤1": 2≤3 true, 3≤1 false → false. + uint256[] memory list = new uint256[](3); + list[0] = 2; + list[1] = 3; + list[2] = 1; + ruled.updateRules( + _threeElementsOrderedRule(uint240(list[1]), uint240(list[2])) + ); + assertFalse(_isGranted(list)); + } + + function test_idLargerThanCompareList_returnsFalse() public { + // id=5 with list of length 3 → source falls through to `return false`. + RuledCondition.Rule[] memory rs = new RuledCondition.Rule[](1); + rs[0] = _rule(5, OP_LTE, 2); + ruled.updateRules(rs); + + uint256[] memory list = new uint256[](3); + list[0] = 1; + list[1] = 2; + list[2] = 3; + assertFalse(_isGranted(list)); + } + + // ------------------------------------------------------------------------- + // ERC-165 + // ------------------------------------------------------------------------- + + function test_supportsInterface_ERC165() public view { + assertTrue(ruled.supportsInterface(type(IERC165).interfaceId)); + } + + function test_supportsInterface_IPermissionCondition() public view { + assertTrue( + ruled.supportsInterface(type(IPermissionCondition).interfaceId) + ); + } + + function test_supportsInterface_IProtocolVersion() public view { + assertTrue(ruled.supportsInterface(type(IProtocolVersion).interfaceId)); + } + + function test_supportsInterface_RuledCondition() public view { + assertTrue(ruled.supportsInterface(type(RuledCondition).interfaceId)); + } + + function test_supportsInterface_returnsFalseForUnknownInterface() + public + view + { + assertFalse(ruled.supportsInterface(0xdeadbeef)); + } + + // ------------------------------------------------------------------------- + // GAP: encode/decode round-trips + // ------------------------------------------------------------------------- + + function test_encodeIfElse_decodeRoundTrip() public view { + uint240 encoded = ruled.encodeIfElse(7, 13, 42); + (uint32 a, uint32 b, uint32 c) = ruled.decodeRuleValue( + uint256(encoded) + ); + assertEq(a, 7); + assertEq(b, 13); + assertEq(c, 42); + } + + function test_encodeLogicalOperator_decodeRoundTrip() public view { + uint240 encoded = ruled.encodeLogicalOperator(11, 99); + (uint32 a, uint32 b, ) = ruled.decodeRuleValue(uint256(encoded)); + assertEq(a, 11); + assertEq(b, 99); + } + + function testFuzz_encodeIfElse_decodeRoundTrip( + uint32 a, + uint32 b, + uint32 c + ) public view { + uint240 encoded = ruled.encodeIfElse( + uint256(a), + uint256(b), + uint256(c) + ); + (uint32 da, uint32 db, uint32 dc) = ruled.decodeRuleValue( + uint256(encoded) + ); + assertEq(da, a); + assertEq(db, b); + assertEq(dc, c); + } + + // ------------------------------------------------------------------------- + // GAP: empty / large rule arrays + // ------------------------------------------------------------------------- + + function test_updateRules_emptyArrayClears() public { + // Seed with one rule, then clear. + RuledCondition.Rule[] memory seed = new RuledCondition.Rule[](1); + seed[0] = _rule(VALUE_RULE_ID, OP_RET, 1); + ruled.updateRules(seed); + assertEq(ruled.getRules().length, 1); + + RuledCondition.Rule[] memory empty = new RuledCondition.Rule[](0); + ruled.updateRules(empty); + assertEq(ruled.getRules().length, 0); + } + + function test_updateRules_largeArrayPersists() public { + // 100 rules — well within the bound the contract can store. Confirms + // `delete rules; push * N` cycle works under sustained load. + uint256 N = 100; + RuledCondition.Rule[] memory rs = new RuledCondition.Rule[](N); + for (uint256 i = 0; i < N; i++) { + rs[i] = _rule(VALUE_RULE_ID, OP_RET, uint240(i + 1)); + } + ruled.updateRules(rs); + assertEq(ruled.getRules().length, N); + // Spot-check a few entries. + assertEq(uint256(ruled.getRules()[0].value), 1); + assertEq(uint256(ruled.getRules()[N - 1].value), N); + } + + // ------------------------------------------------------------------------- + // GAP: `_checkCondition` swallows reverts and rejects malformed returndata + // ------------------------------------------------------------------------- + + function test_checkCondition_swallowsRevertFromExternalCondition() public { + RevertingConditionMock rev = new RevertingConditionMock(); + RuledCondition.Rule[] memory rs = new RuledCondition.Rule[](1); + rs[0] = _rule( + CONDITION_RULE_ID, + OP_EQ, + _conditionAddr(IPermissionCondition(address(rev))) + ); + ruled.updateRules(rs); + + // Must not propagate the revert; instead `_checkCondition` returns + // false → value=0, comparedTo=1, EQ → false. + assertFalse(_isGrantedEmpty()); + } + + function test_checkCondition_rejectsNon32ByteReturndata() public { + WeirdReturndataConditionMock weird = new WeirdReturndataConditionMock(); + RuledCondition.Rule[] memory rs = new RuledCondition.Rule[](1); + rs[0] = _rule( + CONDITION_RULE_ID, + OP_EQ, + _conditionAddr(IPermissionCondition(address(weird))) + ); + ruled.updateRules(rs); + + // `_checkCondition` checks `returndatasize() == 32`; 64 bytes → return false. + assertFalse(_isGrantedEmpty()); + } +} diff --git a/test/common/plugin/Plugin.t.sol b/test/common/plugin/Plugin.t.sol new file mode 100644 index 000000000..060dee778 --- /dev/null +++ b/test/common/plugin/Plugin.t.sol @@ -0,0 +1,299 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test, Vm} from "forge-std/Test.sol"; +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {Plugin} from "../../../src/common/plugin/Plugin.sol"; +import {IPlugin} from "../../../src/common/plugin/IPlugin.sol"; +import {IDAO} from "../../../src/common/dao/IDAO.sol"; +import {IProtocolVersion} from "../../../src/common/utils/versioning/IProtocolVersion.sol"; +import {IExecutor, Action} from "../../../src/common/executors/IExecutor.sol"; +import {DaoUnauthorized} from "../../../src/common/permission/auth/auth.sol"; +import {PluginMockBuild1} from "../../mocks/commons/plugin/PluginMock.sol"; +import {CustomExecutorMock} from "../../mocks/commons/plugin/CustomExecutorMock.sol"; +import {DAOMock} from "../../mocks/commons/dao/DAOMock.sol"; + +/// @dev A contract that ERC-165-claims to be `IDAO`. Used to trigger the +/// `Plugin._setTargetConfig` defensive check that refuses +/// `(IDAO-like, DelegateCall)` configurations. `DAOMock` does not implement +/// `supportsInterface`, so it cannot stand in here. +contract IDAOLikeMock { + function supportsInterface(bytes4 _interfaceId) external pure returns (bool) { + return _interfaceId == type(IDAO).interfaceId || _interfaceId == type(IERC165).interfaceId; + } +} + +/// @notice Direct tests for the `Plugin` abstract contract in +/// `src/common/plugin/Plugin.sol`. +/// +/// Ports `osx-commons/contracts/test/plugin/plugin.ts` (370 lines, 21 cases) +/// and adds: `pluginType` enum value, `InvalidTargetConfig` revert, +/// `setTargetConfig` perm guard, `getTargetConfig` fallback semantics, +/// `TargetSet` event payload, and the `setTarget` XOR selector iface ID literal. +contract PluginTest is Test { + DAOMock internal daoMock; + PluginMockBuild1 internal plugin; + CustomExecutorMock internal executor; + address internal alice; + + bytes32 internal constant SET_TARGET_CONFIG_PERMISSION_ID = keccak256("SET_TARGET_CONFIG_PERMISSION"); + + function setUp() public { + alice = makeAddr("alice"); + daoMock = new DAOMock(); + // Default: hasPermission → true, so any caller can setTargetConfig. + daoMock.setHasPermissionReturnValueMock(true); + executor = new CustomExecutorMock(); + plugin = new PluginMockBuild1(IDAO(address(daoMock))); + } + + // ------------------------------------------------------------------------- + // pluginType + // ------------------------------------------------------------------------- + + function test_pluginType_returnsConstructable() public view { + assertEq(uint256(plugin.pluginType()), uint256(IPlugin.PluginType.Constructable)); + } + + // ------------------------------------------------------------------------- + // protocolVersion (inherited) + // ------------------------------------------------------------------------- + + function test_protocolVersion_returnsCurrent() public view { + uint8[3] memory v = plugin.protocolVersion(); + assertEq(v[0], 1); + assertEq(v[1], 4); + assertEq(v[2], 0); + } + + // ------------------------------------------------------------------------- + // ERC-165 + // ------------------------------------------------------------------------- + + function test_supportsInterface_ERC165() public view { + assertTrue(plugin.supportsInterface(type(IERC165).interfaceId)); + } + + function test_supportsInterface_IPlugin() public view { + assertTrue(plugin.supportsInterface(type(IPlugin).interfaceId)); + } + + function test_supportsInterface_IProtocolVersion() public view { + assertTrue(plugin.supportsInterface(type(IProtocolVersion).interfaceId)); + } + + function test_supportsInterface_setTargetXorSelectors() public view { + bytes4 xor = + plugin.setTargetConfig.selector ^ plugin.getTargetConfig.selector ^ plugin.getCurrentTargetConfig.selector; + assertTrue(plugin.supportsInterface(xor)); + } + + function test_supportsInterface_returnsFalseForUnknownInterface() public view { + assertFalse(plugin.supportsInterface(0xdeadbeef)); + } + + // ------------------------------------------------------------------------- + // setTargetConfig / getCurrentTargetConfig / getTargetConfig + // ------------------------------------------------------------------------- + + function test_setTargetConfig_revertsIfCallerLacksPermission() public { + daoMock.setHasPermissionReturnValueMock(false); + IPlugin.TargetConfig memory cfg = + IPlugin.TargetConfig({target: address(executor), operation: IPlugin.Operation.Call}); + + vm.expectPartialRevert(DaoUnauthorized.selector); + vm.prank(alice); + plugin.setTargetConfig(cfg); + } + + function test_setTargetConfig_updatesAndEmitsTargetSet() public { + IPlugin.TargetConfig memory cfg = + IPlugin.TargetConfig({target: address(executor), operation: IPlugin.Operation.Call}); + + vm.recordLogs(); + plugin.setTargetConfig(cfg); + Vm.Log[] memory logs = vm.getRecordedLogs(); + + // Event payload includes the struct, encoded as (address, uint8). + bytes32 topic = keccak256("TargetSet((address,uint8))"); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].topics[0] == topic && logs[i].emitter == address(plugin)) { + (address target, uint8 op) = abi.decode(logs[i].data, (address, uint8)); + assertEq(target, address(executor)); + assertEq(uint256(op), uint256(IPlugin.Operation.Call)); + found = true; + break; + } + } + assertTrue(found, "TargetSet not emitted"); + + IPlugin.TargetConfig memory stored = plugin.getCurrentTargetConfig(); + assertEq(stored.target, address(executor)); + assertEq(uint256(stored.operation), uint256(IPlugin.Operation.Call)); + } + + function test_setTargetConfig_revertsIfDAOTargetWithDelegateCall() public { + // GAP: F-class safety — refuses to set a DAO-typed target with + // DelegateCall, which would brick the plugin (delegatecall to a DAO + // contract overwrites plugin storage). The check uses ERC-165 + // `supportsInterface(type(IDAO).interfaceId)`, so a target that + // does NOT advertise IDAO via ERC-165 (e.g. `DAOMock`) bypasses the + // guard. Use a tiny ERC-165-claiming stub instead. + IDAOLikeMock daoLike = new IDAOLikeMock(); + IPlugin.TargetConfig memory cfg = + IPlugin.TargetConfig({target: address(daoLike), operation: IPlugin.Operation.DelegateCall}); + vm.expectPartialRevert(Plugin.InvalidTargetConfig.selector); + plugin.setTargetConfig(cfg); + } + + function test_setTargetConfig_allowsDAOLikeTargetWithCall() public { + // Inverse of the above: same ERC-165 claim, but Call is allowed. + IDAOLikeMock daoLike = new IDAOLikeMock(); + IPlugin.TargetConfig memory cfg = + IPlugin.TargetConfig({target: address(daoLike), operation: IPlugin.Operation.Call}); + plugin.setTargetConfig(cfg); + assertEq(plugin.getCurrentTargetConfig().target, address(daoLike)); + } + + function test_getCurrentTargetConfig_defaultsToZero() public view { + IPlugin.TargetConfig memory cfg = plugin.getCurrentTargetConfig(); + assertEq(cfg.target, address(0)); + assertEq(uint256(cfg.operation), uint256(IPlugin.Operation.Call)); + } + + function test_getTargetConfig_fallsBackToDAOWhenTargetUnset() public view { + // GAP: when currentTargetConfig.target == address(0), the convenience + // getter must return (dao(), Call) — not the raw stored zero. + IPlugin.TargetConfig memory cfg = plugin.getTargetConfig(); + assertEq(cfg.target, address(daoMock)); + assertEq(uint256(cfg.operation), uint256(IPlugin.Operation.Call)); + } + + function test_getTargetConfig_returnsStoredWhenSet() public { + IPlugin.TargetConfig memory toSet = + IPlugin.TargetConfig({target: address(executor), operation: IPlugin.Operation.DelegateCall}); + plugin.setTargetConfig(toSet); + + IPlugin.TargetConfig memory cfg = plugin.getTargetConfig(); + assertEq(cfg.target, address(executor)); + assertEq(uint256(cfg.operation), uint256(IPlugin.Operation.DelegateCall)); + } + + // ------------------------------------------------------------------------- + // execute(uint256, Action[], uint256) — uses the current target. + // ------------------------------------------------------------------------- + + function test_execute_routesToDAOIfTargetNotSet() public { + // No setTargetConfig → fallback (dao, Call) → DAOMock.execute is invoked. + // DAOMock's execute emits `Executed`; record + verify. + Action[] memory actions; + vm.recordLogs(); + plugin.execute(uint256(0xCAFE), actions, 0); + Vm.Log[] memory logs = vm.getRecordedLogs(); + // DAOMock emits IExecutor.Executed with the plugin as msg.sender. + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if ( + logs[i].emitter == address(daoMock) + && logs[i].topics[0] + == keccak256("Executed(address,bytes32,(address,uint256,bytes)[],uint256,uint256,bytes[])") + ) { + assertEq(address(uint160(uint256(logs[i].topics[1]))), address(plugin)); + found = true; + break; + } + } + assertTrue(found, "DAOMock.execute(Executed) not seen"); + } + + // ------------------------------------------------------------------------- + // execute(address, uint256, Action[], uint256, Operation) + // — explicit custom target, Call path. + // ------------------------------------------------------------------------- + + function test_execute_customTargetCall_forwardsAndEmitsFromTarget() public { + Action[] memory actions; + vm.recordLogs(); + // CustomExecutorMock emits `Executed` for any non-zero, non-123 callId. + plugin.execute(address(executor), uint256(1), actions, 0, IPlugin.Operation.Call); + Vm.Log[] memory logs = vm.getRecordedLogs(); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if ( + logs[i].emitter == address(executor) + && logs[i].topics[0] + == keccak256("Executed(address,bytes32,(address,uint256,bytes)[],uint256,uint256,bytes[])") + ) { + found = true; + break; + } + } + assertTrue(found, "CustomExecutorMock.Executed not emitted"); + } + + function test_execute_customTargetCall_revertsFromTarget() public { + // CustomExecutorMock reverts `Failed()` when callId == 0. + Action[] memory actions; + vm.expectRevert(CustomExecutorMock.Failed.selector); + plugin.execute(address(executor), uint256(0), actions, 0, IPlugin.Operation.Call); + } + + // ------------------------------------------------------------------------- + // execute(...) DelegateCall path + // ------------------------------------------------------------------------- + + function test_execute_customTargetDelegateCall_bubblesRevertMessage() public { + // callId == 0 → CustomExecutorMock.execute reverts `Failed()`. When + // invoked via delegatecall, the revert data is bubbled up through + // Plugin's assembly path. + Action[] memory actions; + vm.expectRevert(CustomExecutorMock.Failed.selector); + plugin.execute(address(executor), uint256(0), actions, 0, IPlugin.Operation.DelegateCall); + } + + function test_execute_customTargetDelegateCall_revertsDelegateCallFailedOnEmptyRevertData() public { + // callId == 123 → CustomExecutorMock.execute uses `revert()` with no + // data. Plugin's delegatecall path then reverts `DelegateCallFailed`. + Action[] memory actions; + vm.expectRevert(Plugin.DelegateCallFailed.selector); + plugin.execute(address(executor), uint256(123), actions, 0, IPlugin.Operation.DelegateCall); + } + + function test_execute_customTargetDelegateCall_emitsFromConsumerContext() public { + // For a successful callId, delegatecall runs `executor.execute` in the + // plugin's storage/event context: the `Executed` log is emitted by the + // plugin address, not by the executor. + Action[] memory actions; + vm.recordLogs(); + plugin.execute(address(executor), uint256(7), actions, 0, IPlugin.Operation.DelegateCall); + Vm.Log[] memory logs = vm.getRecordedLogs(); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if ( + logs[i].emitter == address(plugin) + && logs[i].topics[0] + == keccak256("Executed(address,bytes32,(address,uint256,bytes)[],uint256,uint256,bytes[])") + ) { + found = true; + break; + } + } + assertTrue(found, "Expected Executed emitted from plugin (delegatecall context)"); + } + + // ------------------------------------------------------------------------- + // address(0) target — undefined behaviour, must revert + // ------------------------------------------------------------------------- + + function test_execute_customTargetAddressZero_reverts() public { + // No explicit address(0) check in source; the call ends up trying to + // `abi.decode` empty returndata as `(bytes[], uint256)` and panics. The + // important property is just "this reverts" — the precise message is + // an implementation detail and may vary across Foundry versions. + Action[] memory actions; + vm.expectRevert(); + plugin.execute(address(0), uint256(1), actions, 0, IPlugin.Operation.Call); + } +} diff --git a/test/common/plugin/PluginCloneable.t.sol b/test/common/plugin/PluginCloneable.t.sol new file mode 100644 index 000000000..5d149878a --- /dev/null +++ b/test/common/plugin/PluginCloneable.t.sol @@ -0,0 +1,284 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test, Vm} from "forge-std/Test.sol"; +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; +import {PluginCloneable} from "../../../src/common/plugin/PluginCloneable.sol"; +import {IPlugin} from "../../../src/common/plugin/IPlugin.sol"; +import {IDAO} from "../../../src/common/dao/IDAO.sol"; +import {IProtocolVersion} from "../../../src/common/utils/versioning/IProtocolVersion.sol"; +import {DaoUnauthorized} from "../../../src/common/permission/auth/auth.sol"; +import {Action} from "../../../src/common/executors/IExecutor.sol"; +import {PluginCloneableMockBuild1, PluginCloneableMockBad} from "../../mocks/commons/plugin/PluginCloneableMock.sol"; +import {CustomExecutorMock} from "../../mocks/commons/plugin/CustomExecutorMock.sol"; +import {DAOMock} from "../../mocks/commons/dao/DAOMock.sol"; + +/// @dev A contract that ERC-165-claims to be `IDAO`. Used to trigger the +/// `PluginCloneable._setTargetConfig` defensive check. +contract IDAOLikeMock { + function supportsInterface(bytes4 _interfaceId) external pure returns (bool) { + return _interfaceId == type(IDAO).interfaceId || _interfaceId == type(IERC165).interfaceId; + } +} + +/// @notice Direct tests for the `PluginCloneable` abstract contract in +/// `src/common/plugin/PluginCloneable.sol`. +/// +/// Ports `osx-commons/contracts/test/plugin/plugin-clonable.ts` (453 lines, 24 +/// cases). Adds the `Initializable` surface (init OK, disabled-on-impl, +/// re-init via guard, `__PluginCloneable_init` outside `onlyInitializing`), +/// `pluginType == Cloneable`, the `InvalidTargetConfig` guard, and the full +/// execute/delegatecall matrix shared with `Plugin.t.sol`. +contract PluginCloneableTest is Test { + DAOMock internal daoMock; + PluginCloneableMockBuild1 internal impl; + PluginCloneableMockBuild1 internal plugin; + CustomExecutorMock internal executor; + address internal alice; + + function setUp() public { + alice = makeAddr("alice"); + daoMock = new DAOMock(); + daoMock.setHasPermissionReturnValueMock(true); + executor = new CustomExecutorMock(); + + // Deploy the implementation (constructor calls `_disableInitializers`) + // then clone it, initialize the clone, and use it as the plugin under + // test. Matches the production lifecycle. + impl = new PluginCloneableMockBuild1(); + plugin = PluginCloneableMockBuild1(Clones.clone(address(impl))); + plugin.initialize(IDAO(address(daoMock))); + } + + // ------------------------------------------------------------------------- + // Initializable + // ------------------------------------------------------------------------- + + function test_initialize_setsDaoAndState() public view { + assertEq(address(plugin.dao()), address(daoMock)); + assertEq(plugin.state1(), 1); + } + + function test_initialize_disabledOnImplementation() public { + // Constructor of the impl calls `_disableInitializers`, so calling + // `initialize` directly on the implementation reverts. + vm.expectRevert("Initializable: contract is already initialized"); + impl.initialize(IDAO(address(daoMock))); + } + + function test_initialize_revertsIfCalledTwice() public { + vm.expectRevert("Initializable: contract is already initialized"); + plugin.initialize(IDAO(address(daoMock))); + } + + function test_initInternal_revertsIfCalledOutsideInitializer() public { + // `PluginCloneableMockBad.notAnInitializer` calls + // `__PluginCloneable_init` without the `initializer` modifier. + PluginCloneableMockBad badImpl = new PluginCloneableMockBad(); + PluginCloneableMockBad bad = PluginCloneableMockBad(Clones.clone(address(badImpl))); + vm.expectRevert("Initializable: contract is not initializing"); + bad.notAnInitializer(IDAO(address(daoMock))); + } + + // ------------------------------------------------------------------------- + // pluginType + // ------------------------------------------------------------------------- + + function test_pluginType_returnsCloneable() public view { + assertEq(uint256(plugin.pluginType()), uint256(IPlugin.PluginType.Cloneable)); + } + + // ------------------------------------------------------------------------- + // protocolVersion (inherited) + // ------------------------------------------------------------------------- + + function test_protocolVersion_returnsCurrent() public view { + uint8[3] memory v = plugin.protocolVersion(); + assertEq(v[0], 1); + assertEq(v[1], 4); + assertEq(v[2], 0); + } + + // ------------------------------------------------------------------------- + // ERC-165 + // ------------------------------------------------------------------------- + + function test_supportsInterface_ERC165() public view { + assertTrue(plugin.supportsInterface(type(IERC165).interfaceId)); + } + + function test_supportsInterface_IPlugin() public view { + assertTrue(plugin.supportsInterface(type(IPlugin).interfaceId)); + } + + function test_supportsInterface_IProtocolVersion() public view { + assertTrue(plugin.supportsInterface(type(IProtocolVersion).interfaceId)); + } + + function test_supportsInterface_setTargetXorSelectors() public view { + bytes4 xor = + plugin.setTargetConfig.selector ^ plugin.getTargetConfig.selector ^ plugin.getCurrentTargetConfig.selector; + assertTrue(plugin.supportsInterface(xor)); + } + + function test_supportsInterface_returnsFalseForUnknownInterface() public view { + assertFalse(plugin.supportsInterface(0xdeadbeef)); + } + + // ------------------------------------------------------------------------- + // setTargetConfig / getCurrentTargetConfig / getTargetConfig + // ------------------------------------------------------------------------- + + function test_setTargetConfig_revertsIfCallerLacksPermission() public { + daoMock.setHasPermissionReturnValueMock(false); + IPlugin.TargetConfig memory cfg = + IPlugin.TargetConfig({target: address(executor), operation: IPlugin.Operation.Call}); + + vm.expectPartialRevert(DaoUnauthorized.selector); + vm.prank(alice); + plugin.setTargetConfig(cfg); + } + + function test_setTargetConfig_updatesAndEmitsTargetSet() public { + IPlugin.TargetConfig memory cfg = + IPlugin.TargetConfig({target: address(executor), operation: IPlugin.Operation.Call}); + + vm.recordLogs(); + plugin.setTargetConfig(cfg); + Vm.Log[] memory logs = vm.getRecordedLogs(); + + bytes32 topic = keccak256("TargetSet((address,uint8))"); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].topics[0] == topic && logs[i].emitter == address(plugin)) { + (address target, uint8 op) = abi.decode(logs[i].data, (address, uint8)); + assertEq(target, address(executor)); + assertEq(uint256(op), uint256(IPlugin.Operation.Call)); + found = true; + break; + } + } + assertTrue(found, "TargetSet not emitted"); + + IPlugin.TargetConfig memory stored = plugin.getCurrentTargetConfig(); + assertEq(stored.target, address(executor)); + assertEq(uint256(stored.operation), uint256(IPlugin.Operation.Call)); + } + + function test_setTargetConfig_revertsIfDAOTargetWithDelegateCall() public { + IDAOLikeMock daoLike = new IDAOLikeMock(); + IPlugin.TargetConfig memory cfg = + IPlugin.TargetConfig({target: address(daoLike), operation: IPlugin.Operation.DelegateCall}); + vm.expectPartialRevert(PluginCloneable.InvalidTargetConfig.selector); + plugin.setTargetConfig(cfg); + } + + function test_getCurrentTargetConfig_defaultsToZero() public view { + IPlugin.TargetConfig memory cfg = plugin.getCurrentTargetConfig(); + assertEq(cfg.target, address(0)); + assertEq(uint256(cfg.operation), uint256(IPlugin.Operation.Call)); + } + + function test_getTargetConfig_fallsBackToDAOWhenTargetUnset() public view { + IPlugin.TargetConfig memory cfg = plugin.getTargetConfig(); + assertEq(cfg.target, address(daoMock)); + assertEq(uint256(cfg.operation), uint256(IPlugin.Operation.Call)); + } + + function test_getTargetConfig_returnsStoredWhenSet() public { + IPlugin.TargetConfig memory toSet = + IPlugin.TargetConfig({target: address(executor), operation: IPlugin.Operation.DelegateCall}); + plugin.setTargetConfig(toSet); + + IPlugin.TargetConfig memory cfg = plugin.getTargetConfig(); + assertEq(cfg.target, address(executor)); + assertEq(uint256(cfg.operation), uint256(IPlugin.Operation.DelegateCall)); + } + + // ------------------------------------------------------------------------- + // execute(uint256, Action[], uint256) — uses the current target. + // ------------------------------------------------------------------------- + + function test_execute_routesToDAOIfTargetNotSet() public { + bytes32 executedTopic = keccak256("Executed(address,bytes32,(address,uint256,bytes)[],uint256,uint256,bytes[])"); + Action[] memory actions; + vm.recordLogs(); + plugin.execute(uint256(0xCAFE), actions, 0); + Vm.Log[] memory logs = vm.getRecordedLogs(); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].emitter == address(daoMock) && logs[i].topics[0] == executedTopic) { + assertEq(address(uint160(uint256(logs[i].topics[1]))), address(plugin)); + found = true; + break; + } + } + assertTrue(found, "DAOMock.execute(Executed) not seen"); + } + + // ------------------------------------------------------------------------- + // execute(address, ...) explicit target — Call path. + // ------------------------------------------------------------------------- + + function test_execute_customTargetCall_forwardsAndEmitsFromTarget() public { + bytes32 executedTopic = keccak256("Executed(address,bytes32,(address,uint256,bytes)[],uint256,uint256,bytes[])"); + Action[] memory actions; + vm.recordLogs(); + plugin.execute(address(executor), uint256(1), actions, 0, IPlugin.Operation.Call); + Vm.Log[] memory logs = vm.getRecordedLogs(); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].emitter == address(executor) && logs[i].topics[0] == executedTopic) { + found = true; + break; + } + } + assertTrue(found, "CustomExecutorMock.Executed not emitted"); + } + + function test_execute_customTargetCall_revertsFromTarget() public { + Action[] memory actions; + vm.expectRevert(CustomExecutorMock.Failed.selector); + plugin.execute(address(executor), uint256(0), actions, 0, IPlugin.Operation.Call); + } + + // ------------------------------------------------------------------------- + // execute(...) DelegateCall path + // ------------------------------------------------------------------------- + + function test_execute_customTargetDelegateCall_bubblesRevertMessage() public { + Action[] memory actions; + vm.expectRevert(CustomExecutorMock.Failed.selector); + plugin.execute(address(executor), uint256(0), actions, 0, IPlugin.Operation.DelegateCall); + } + + function test_execute_customTargetDelegateCall_revertsDelegateCallFailedOnEmptyRevertData() public { + Action[] memory actions; + vm.expectRevert(PluginCloneable.DelegateCallFailed.selector); + plugin.execute(address(executor), uint256(123), actions, 0, IPlugin.Operation.DelegateCall); + } + + function test_execute_customTargetDelegateCall_emitsFromConsumerContext() public { + bytes32 executedTopic = keccak256("Executed(address,bytes32,(address,uint256,bytes)[],uint256,uint256,bytes[])"); + Action[] memory actions; + vm.recordLogs(); + plugin.execute(address(executor), uint256(7), actions, 0, IPlugin.Operation.DelegateCall); + Vm.Log[] memory logs = vm.getRecordedLogs(); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].emitter == address(plugin) && logs[i].topics[0] == executedTopic) { + found = true; + break; + } + } + assertTrue(found, "Expected Executed emitted from plugin (delegatecall context)"); + } + + function test_execute_customTargetAddressZero_reverts() public { + Action[] memory actions; + vm.expectRevert(); + plugin.execute(address(0), uint256(1), actions, 0, IPlugin.Operation.Call); + } +} diff --git a/test/common/plugin/PluginUUPSUpgradeable.t.sol b/test/common/plugin/PluginUUPSUpgradeable.t.sol new file mode 100644 index 000000000..3cf87f4e3 --- /dev/null +++ b/test/common/plugin/PluginUUPSUpgradeable.t.sol @@ -0,0 +1,328 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test, Vm} from "forge-std/Test.sol"; +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import { + IERC1822ProxiableUpgradeable +} from "@openzeppelin/contracts-upgradeable/interfaces/draft-IERC1822Upgradeable.sol"; +import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import {PluginUUPSUpgradeable} from "../../../src/common/plugin/PluginUUPSUpgradeable.sol"; +import {IPlugin} from "../../../src/common/plugin/IPlugin.sol"; +import {IDAO} from "../../../src/common/dao/IDAO.sol"; +import {IProtocolVersion} from "../../../src/common/utils/versioning/IProtocolVersion.sol"; +import {Action} from "../../../src/common/executors/IExecutor.sol"; +import {DaoUnauthorized} from "../../../src/common/permission/auth/auth.sol"; +import { + PluginUUPSUpgradeableMockBuild1, + PluginUUPSUpgradeableMockBuild2, + PluginUUPSUpgradeableMockBad +} from "../../mocks/commons/plugin/PluginUUPSUpgradeableMock.sol"; +import {CustomExecutorMock} from "../../mocks/commons/plugin/CustomExecutorMock.sol"; +import {DAOMock} from "../../mocks/commons/dao/DAOMock.sol"; + +/// @dev A contract that ERC-165-claims to be `IDAO`. Used to trigger the +/// `PluginUUPSUpgradeable._setTargetConfig` defensive check. +contract IDAOLikeMock { + function supportsInterface(bytes4 _interfaceId) external pure returns (bool) { + return _interfaceId == type(IDAO).interfaceId || _interfaceId == type(IERC165).interfaceId; + } +} + +/// @notice Direct tests for the `PluginUUPSUpgradeable` abstract contract in +/// `src/common/plugin/PluginUUPSUpgradeable.sol`. +/// +/// Ports `osx-commons/contracts/test/plugin/plugin-uups-upgradeable.ts` (601 +/// lines, 28 cases). Covers the Initializable surface, `pluginType == UUPS`, +/// ERC-165 (incl. `IERC1822ProxiableUpgradeable`), the execute/delegatecall +/// matrix shared with `Plugin.t.sol`, and the upgradeability surface +/// (`implementation()`, `_authorizeUpgrade` permission gate, reinitialization +/// via `reinitializer(2)`). +contract PluginUUPSUpgradeableTest is Test { + DAOMock internal daoMock; + PluginUUPSUpgradeableMockBuild1 internal impl; + PluginUUPSUpgradeableMockBuild1 internal plugin; + CustomExecutorMock internal executor; + address internal alice; + + function setUp() public { + alice = makeAddr("alice"); + daoMock = new DAOMock(); + daoMock.setHasPermissionReturnValueMock(true); + executor = new CustomExecutorMock(); + + // Deploy the implementation, then wrap it in an ERC1967Proxy seeded + // with `initialize(daoMock)` calldata. Matches the production UUPS + // lifecycle. + impl = new PluginUUPSUpgradeableMockBuild1(); + bytes memory initCalldata = abi.encodeCall(impl.initialize, (IDAO(address(daoMock)))); + plugin = PluginUUPSUpgradeableMockBuild1(address(new ERC1967Proxy(address(impl), initCalldata))); + } + + // ------------------------------------------------------------------------- + // Initializable + // ------------------------------------------------------------------------- + + function test_initialize_setsDaoAndState() public view { + assertEq(address(plugin.dao()), address(daoMock)); + assertEq(plugin.state1(), 1); + } + + function test_initialize_disabledOnImplementation() public { + // Constructor of the impl calls `_disableInitializers`. + vm.expectRevert("Initializable: contract is already initialized"); + impl.initialize(IDAO(address(daoMock))); + } + + function test_initialize_revertsIfCalledTwice() public { + vm.expectRevert("Initializable: contract is already initialized"); + plugin.initialize(IDAO(address(daoMock))); + } + + function test_initInternal_revertsIfCalledOutsideInitializer() public { + // The `Bad` mock has `notAnInitializer` calling + // `__PluginUUPSUpgradeable_init` without the `initializer` modifier. + PluginUUPSUpgradeableMockBad badImpl = new PluginUUPSUpgradeableMockBad(); + PluginUUPSUpgradeableMockBad bad = + PluginUUPSUpgradeableMockBad(address(new ERC1967Proxy(address(badImpl), bytes("")))); + vm.expectRevert("Initializable: contract is not initializing"); + bad.notAnInitializer(IDAO(address(daoMock))); + } + + // ------------------------------------------------------------------------- + // pluginType + // ------------------------------------------------------------------------- + + function test_pluginType_returnsUUPS() public view { + assertEq(uint256(plugin.pluginType()), uint256(IPlugin.PluginType.UUPS)); + } + + // ------------------------------------------------------------------------- + // protocolVersion (inherited) + // ------------------------------------------------------------------------- + + function test_protocolVersion_returnsCurrent() public view { + uint8[3] memory v = plugin.protocolVersion(); + assertEq(v[0], 1); + assertEq(v[1], 4); + assertEq(v[2], 0); + } + + // ------------------------------------------------------------------------- + // ERC-165 + // ------------------------------------------------------------------------- + + function test_supportsInterface_ERC165() public view { + assertTrue(plugin.supportsInterface(type(IERC165).interfaceId)); + } + + function test_supportsInterface_IPlugin() public view { + assertTrue(plugin.supportsInterface(type(IPlugin).interfaceId)); + } + + function test_supportsInterface_IProtocolVersion() public view { + assertTrue(plugin.supportsInterface(type(IProtocolVersion).interfaceId)); + } + + function test_supportsInterface_IERC1822ProxiableUpgradeable() public view { + assertTrue(plugin.supportsInterface(type(IERC1822ProxiableUpgradeable).interfaceId)); + } + + function test_supportsInterface_setTargetXorSelectors() public view { + bytes4 xor = + plugin.setTargetConfig.selector ^ plugin.getTargetConfig.selector ^ plugin.getCurrentTargetConfig.selector; + assertTrue(plugin.supportsInterface(xor)); + } + + function test_supportsInterface_returnsFalseForUnknownInterface() public view { + assertFalse(plugin.supportsInterface(0xdeadbeef)); + } + + // ------------------------------------------------------------------------- + // setTargetConfig / getCurrentTargetConfig / getTargetConfig + // ------------------------------------------------------------------------- + + function test_setTargetConfig_revertsIfCallerLacksPermission() public { + daoMock.setHasPermissionReturnValueMock(false); + IPlugin.TargetConfig memory cfg = + IPlugin.TargetConfig({target: address(executor), operation: IPlugin.Operation.Call}); + + vm.expectPartialRevert(DaoUnauthorized.selector); + vm.prank(alice); + plugin.setTargetConfig(cfg); + } + + function test_setTargetConfig_updatesAndEmitsTargetSet() public { + IPlugin.TargetConfig memory cfg = + IPlugin.TargetConfig({target: address(executor), operation: IPlugin.Operation.Call}); + + vm.recordLogs(); + plugin.setTargetConfig(cfg); + Vm.Log[] memory logs = vm.getRecordedLogs(); + + bytes32 topic = keccak256("TargetSet((address,uint8))"); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].topics[0] == topic && logs[i].emitter == address(plugin)) { + (address target, uint8 op) = abi.decode(logs[i].data, (address, uint8)); + assertEq(target, address(executor)); + assertEq(uint256(op), uint256(IPlugin.Operation.Call)); + found = true; + break; + } + } + assertTrue(found, "TargetSet not emitted"); + + IPlugin.TargetConfig memory stored = plugin.getCurrentTargetConfig(); + assertEq(stored.target, address(executor)); + assertEq(uint256(stored.operation), uint256(IPlugin.Operation.Call)); + } + + function test_setTargetConfig_revertsIfDAOTargetWithDelegateCall() public { + IDAOLikeMock daoLike = new IDAOLikeMock(); + IPlugin.TargetConfig memory cfg = + IPlugin.TargetConfig({target: address(daoLike), operation: IPlugin.Operation.DelegateCall}); + vm.expectPartialRevert(PluginUUPSUpgradeable.InvalidTargetConfig.selector); + plugin.setTargetConfig(cfg); + } + + function test_getCurrentTargetConfig_defaultsToZero() public view { + IPlugin.TargetConfig memory cfg = plugin.getCurrentTargetConfig(); + assertEq(cfg.target, address(0)); + assertEq(uint256(cfg.operation), uint256(IPlugin.Operation.Call)); + } + + function test_getTargetConfig_fallsBackToDAOWhenTargetUnset() public view { + IPlugin.TargetConfig memory cfg = plugin.getTargetConfig(); + assertEq(cfg.target, address(daoMock)); + assertEq(uint256(cfg.operation), uint256(IPlugin.Operation.Call)); + } + + function test_getTargetConfig_returnsStoredWhenSet() public { + IPlugin.TargetConfig memory toSet = + IPlugin.TargetConfig({target: address(executor), operation: IPlugin.Operation.DelegateCall}); + plugin.setTargetConfig(toSet); + + IPlugin.TargetConfig memory cfg = plugin.getTargetConfig(); + assertEq(cfg.target, address(executor)); + assertEq(uint256(cfg.operation), uint256(IPlugin.Operation.DelegateCall)); + } + + // ------------------------------------------------------------------------- + // execute(uint256, Action[], uint256) — uses the current target. + // ------------------------------------------------------------------------- + + bytes32 internal constant EXECUTED_TOPIC = + keccak256("Executed(address,bytes32,(address,uint256,bytes)[],uint256,uint256,bytes[])"); + + function test_execute_routesToDAOIfTargetNotSet() public { + Action[] memory actions; + vm.recordLogs(); + plugin.execute(uint256(0xCAFE), actions, 0); + Vm.Log[] memory logs = vm.getRecordedLogs(); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].emitter == address(daoMock) && logs[i].topics[0] == EXECUTED_TOPIC) { + assertEq(address(uint160(uint256(logs[i].topics[1]))), address(plugin)); + found = true; + break; + } + } + assertTrue(found, "DAOMock.execute(Executed) not seen"); + } + + function test_execute_customTargetCall_forwardsAndEmitsFromTarget() public { + Action[] memory actions; + vm.recordLogs(); + plugin.execute(address(executor), uint256(1), actions, 0, IPlugin.Operation.Call); + Vm.Log[] memory logs = vm.getRecordedLogs(); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].emitter == address(executor) && logs[i].topics[0] == EXECUTED_TOPIC) { + found = true; + break; + } + } + assertTrue(found, "CustomExecutorMock.Executed not emitted"); + } + + function test_execute_customTargetCall_revertsFromTarget() public { + Action[] memory actions; + vm.expectRevert(CustomExecutorMock.Failed.selector); + plugin.execute(address(executor), uint256(0), actions, 0, IPlugin.Operation.Call); + } + + function test_execute_customTargetDelegateCall_bubblesRevertMessage() public { + Action[] memory actions; + vm.expectRevert(CustomExecutorMock.Failed.selector); + plugin.execute(address(executor), uint256(0), actions, 0, IPlugin.Operation.DelegateCall); + } + + function test_execute_customTargetDelegateCall_revertsDelegateCallFailedOnEmptyRevertData() public { + Action[] memory actions; + vm.expectRevert(PluginUUPSUpgradeable.DelegateCallFailed.selector); + plugin.execute(address(executor), uint256(123), actions, 0, IPlugin.Operation.DelegateCall); + } + + function test_execute_customTargetDelegateCall_emitsFromConsumerContext() public { + Action[] memory actions; + vm.recordLogs(); + plugin.execute(address(executor), uint256(7), actions, 0, IPlugin.Operation.DelegateCall); + Vm.Log[] memory logs = vm.getRecordedLogs(); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].emitter == address(plugin) && logs[i].topics[0] == EXECUTED_TOPIC) { + found = true; + break; + } + } + assertTrue(found, "Expected Executed emitted from plugin (delegatecall context)"); + } + + function test_execute_customTargetAddressZero_reverts() public { + Action[] memory actions; + vm.expectRevert(); + plugin.execute(address(0), uint256(1), actions, 0, IPlugin.Operation.Call); + } + + // ------------------------------------------------------------------------- + // Upgradeability + // ------------------------------------------------------------------------- + + function test_implementation_returnsImplementationSlot() public view { + // ERC-1967 implementation slot must point to the original Build1 impl. + assertEq(plugin.implementation(), address(impl)); + } + + function test_upgrade_revertsIfCallerLacksUpgradePermission() public { + // _authorizeUpgrade is gated by UPGRADE_PLUGIN_PERMISSION_ID. The DAOMock + // is currently set to allow all permissions; flip it to deny. + daoMock.setHasPermissionReturnValueMock(false); + + PluginUUPSUpgradeableMockBuild2 newImpl = new PluginUUPSUpgradeableMockBuild2(); + vm.expectPartialRevert(DaoUnauthorized.selector); + vm.prank(alice); + plugin.upgradeTo(address(newImpl)); + } + + function test_upgrade_succeedsWithPermission() public { + PluginUUPSUpgradeableMockBuild2 newImpl = new PluginUUPSUpgradeableMockBuild2(); + plugin.upgradeTo(address(newImpl)); + assertEq(plugin.implementation(), address(newImpl)); + } + + function test_upgrade_canBeReinitialized() public { + // Upgrade to Build2 with an `initializeFrom(1)` call to bump + // `_initialized` to 2 and set `state2`. Verifies the `reinitializer(2)` + // path works end-to-end. + PluginUUPSUpgradeableMockBuild2 newImpl = new PluginUUPSUpgradeableMockBuild2(); + bytes memory initFrom = abi.encodeCall(newImpl.initializeFrom, (uint16(1))); + plugin.upgradeToAndCall(address(newImpl), initFrom); + + assertEq(plugin.implementation(), address(newImpl)); + // Re-cast to Build2 to read `state2`. + PluginUUPSUpgradeableMockBuild2 asBuild2 = PluginUUPSUpgradeableMockBuild2(address(plugin)); + assertEq(asBuild2.state2(), 2); + } +} diff --git a/test/common/plugin/extensions/governance/Addresslist.t.sol b/test/common/plugin/extensions/governance/Addresslist.t.sol index 4b1f3f5c1..069c6e3df 100644 --- a/test/common/plugin/extensions/governance/Addresslist.t.sol +++ b/test/common/plugin/extensions/governance/Addresslist.t.sol @@ -12,8 +12,8 @@ import {AddresslistMock} from "../../../../mocks/commons/plugin/extensions/gover /// Ports `osx-commons/contracts/test/plugin/extensions/governance/addresslist.ts`. /// Each TS step that advanced the chain via `evm_mine` is reproduced with /// `vm.roll(block.number + 1)` so the checkpoint reads target distinct blocks. -/// Closes the gaps from `TESTS.md` §11: large-array behaviour, exact-block -/// precision, off-by-one window around the boundary block. +/// Adds large-array behaviour, exact-block precision, and off-by-one window +/// around the boundary block. contract AddresslistTest is Test { AddresslistMock internal addresslist; address internal alice; @@ -165,7 +165,12 @@ contract AddresslistTest is Test { address[] memory batch = new address[](2); batch[0] = bob; batch[1] = carol; - vm.expectRevert(abi.encodeWithSelector(Addresslist.InvalidAddresslistUpdate.selector, carol)); + vm.expectRevert( + abi.encodeWithSelector( + Addresslist.InvalidAddresslistUpdate.selector, + carol + ) + ); addresslist.addAddresses(batch); vm.roll(block.number + 1); @@ -179,7 +184,12 @@ contract AddresslistTest is Test { address[] memory batch = new address[](2); batch[0] = alice; batch[1] = alice; - vm.expectRevert(abi.encodeWithSelector(Addresslist.InvalidAddresslistUpdate.selector, alice)); + vm.expectRevert( + abi.encodeWithSelector( + Addresslist.InvalidAddresslistUpdate.selector, + alice + ) + ); addresslist.addAddresses(batch); } @@ -212,7 +222,12 @@ contract AddresslistTest is Test { address[] memory batch = new address[](2); batch[0] = bob; batch[1] = carol; - vm.expectRevert(abi.encodeWithSelector(Addresslist.InvalidAddresslistUpdate.selector, carol)); + vm.expectRevert( + abi.encodeWithSelector( + Addresslist.InvalidAddresslistUpdate.selector, + carol + ) + ); addresslist.removeAddresses(batch); vm.roll(block.number + 1); @@ -228,7 +243,12 @@ contract AddresslistTest is Test { address[] memory batch = new address[](2); batch[0] = alice; batch[1] = alice; - vm.expectRevert(abi.encodeWithSelector(Addresslist.InvalidAddresslistUpdate.selector, alice)); + vm.expectRevert( + abi.encodeWithSelector( + Addresslist.InvalidAddresslistUpdate.selector, + alice + ) + ); addresslist.removeAddresses(batch); } diff --git a/test/common/plugin/extensions/proposal/Proposal.t.sol b/test/common/plugin/extensions/proposal/Proposal.t.sol index 53fae8655..142d542a0 100644 --- a/test/common/plugin/extensions/proposal/Proposal.t.sol +++ b/test/common/plugin/extensions/proposal/Proposal.t.sol @@ -8,14 +8,13 @@ import {Proposal} from "../../../../../src/common/plugin/extensions/proposal/Pro import {ProposalUpgradeable} from "../../../../../src/common/plugin/extensions/proposal/ProposalUpgradeable.sol"; import {IProposal} from "../../../../../src/common/plugin/extensions/proposal/IProposal.sol"; import {ProposalMock} from "../../../../mocks/commons/plugin/extensions/proposal/ProposalMock.sol"; -import { - ProposalUpgradeableMock -} from "../../../../mocks/commons/plugin/extensions/proposal/ProposalUpgradeableMock.sol"; +import {ProposalUpgradeableMock} from "../../../../mocks/commons/plugin/extensions/proposal/ProposalUpgradeableMock.sol"; /// @dev Shared shape both Proposal-mock variants expose. Lets the abstract /// base test call into either via one typed handle. interface IProposalLike { function supportsInterface(bytes4) external view returns (bool); + function proposalCount() external view returns (uint256); } @@ -23,7 +22,9 @@ interface IProposalLike { /// the production `_createProposalId` is `internal`. Inherits from the /// constructable mock to keep the abstract-function stubs intact. contract ProposalIdHarness is ProposalMock { - function exposed_createProposalId(bytes32 _salt) external view returns (uint256) { + function exposed_createProposalId( + bytes32 _salt + ) external view returns (uint256) { return _createProposalId(_salt); } } @@ -31,10 +32,10 @@ contract ProposalIdHarness is ProposalMock { /// @notice Shared behaviour tests for `Proposal` and `ProposalUpgradeable` in /// `src/common/plugin/extensions/proposal/`. /// -/// Ports `osx-commons/contracts/test/plugin/extensions/proposal.ts` and closes -/// the gaps from `TESTS.md` §12: explicit `FunctionDeprecated` revert path, -/// legacy `IProposal` v1.0.0 frozen iface ID matches, `_createProposalId` -/// determinism + uniqueness under typical perturbations. +/// Ports `osx-commons/contracts/test/plugin/extensions/proposal.ts` and adds: +/// an explicit `FunctionDeprecated` revert path, a legacy `IProposal` v1.0.0 +/// frozen iface ID match, and `_createProposalId` determinism + uniqueness +/// under typical perturbations. abstract contract ProposalSharedTest is Test { /// Frozen v1.0.0 `IProposal` interface ID: at that time `IProposal` had /// only one external function, `proposalCount()`. Single-function @@ -79,7 +80,10 @@ abstract contract ProposalSharedTest is Test { assertTrue(target.supportsInterface(IPROPOSAL_V1_0_0_INTERFACE_ID)); } - function test_supportsInterface_returnsFalseForUnknownInterface() public view { + function test_supportsInterface_returnsFalseForUnknownInterface() + public + view + { assertFalse(target.supportsInterface(0xdeadbeef)); } @@ -87,9 +91,12 @@ abstract contract ProposalSharedTest is Test { /// the five v1.0.0-absent function selectors. Locks the source-side XOR /// arithmetic to the literal we assert against above. function test_supportsInterface_legacyXorMatchesSource() public pure { - bytes4 computed = type(IProposal).interfaceId ^ IProposal.createProposal.selector - ^ IProposal.hasSucceeded.selector ^ IProposal.execute.selector ^ IProposal.canExecute.selector - ^ IProposal.customProposalParamsABI.selector; + bytes4 computed = type(IProposal).interfaceId ^ + IProposal.createProposal.selector ^ + IProposal.hasSucceeded.selector ^ + IProposal.execute.selector ^ + IProposal.canExecute.selector ^ + IProposal.customProposalParamsABI.selector; assertEq(computed, IPROPOSAL_V1_0_0_INTERFACE_ID); } } diff --git a/test/common/plugin/setup/PluginSetup.t.sol b/test/common/plugin/setup/PluginSetup.t.sol new file mode 100644 index 000000000..1f439bede --- /dev/null +++ b/test/common/plugin/setup/PluginSetup.t.sol @@ -0,0 +1,154 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {PluginSetup} from "../../../../src/common/plugin/setup/PluginSetup.sol"; +import {PluginUpgradeableSetup} from "../../../../src/common/plugin/setup/PluginUpgradeableSetup.sol"; +import {IPluginSetup} from "../../../../src/common/plugin/setup/IPluginSetup.sol"; +import {IPlugin} from "../../../../src/common/plugin/IPlugin.sol"; +import {IProtocolVersion} from "../../../../src/common/utils/versioning/IProtocolVersion.sol"; +import {PluginCloneableSetupMockBuild1} from "../../../mocks/commons/plugin/PluginCloneableSetupMock.sol"; +import { + PluginUUPSUpgradeableSetupMockBuild1, + PluginUUPSUpgradeableSetupMockBuild2 +} from "../../../mocks/commons/plugin/PluginUUPSUpgradeableSetupMock.sol"; + +/// @dev Shared shape both setup-mock variants expose for the base tests. +interface IPluginSetupLike { + function supportsInterface(bytes4) external view returns (bool); + function protocolVersion() external view returns (uint8[3] memory); + function implementation() external view returns (address); +} + +/// @notice Direct tests for `PluginSetup`, `PluginUpgradeableSetup`, and the +/// `IPluginSetup` interface in `src/common/plugin/setup/`. +/// +/// Ports `osx-commons/contracts/test/plugin/setup/plugin-setup.ts` (192 lines, +/// 10 cases). Subsumes the OSx-side `framework/plugin/plugin-setup.ts` (the +/// same ERC-165 + protocol-version surface, exercised via the cloneable mock). +abstract contract PluginSetupSharedTest is Test { + IPluginSetupLike internal setupMock; + + function _deploySetupMock() internal virtual returns (IPluginSetupLike); + function _expectedImplementationInterface() internal virtual returns (bytes4); + + function setUp() public virtual { + setupMock = _deploySetupMock(); + } + + function test_implementation_returnsNonZeroAddress() public view { + address impl = setupMock.implementation(); + assertTrue(impl != address(0)); + } + + function test_implementation_supportsIPlugin() public view { + address impl = setupMock.implementation(); + assertTrue(IERC165(impl).supportsInterface(type(IPlugin).interfaceId)); + } + + function test_protocolVersion_returnsCurrent() public view { + uint8[3] memory v = setupMock.protocolVersion(); + assertEq(v[0], 1); + assertEq(v[1], 4); + assertEq(v[2], 0); + } + + function test_supportsInterface_ERC165() public view { + assertTrue(setupMock.supportsInterface(type(IERC165).interfaceId)); + } + + function test_supportsInterface_IPluginSetup() public view { + assertTrue(setupMock.supportsInterface(type(IPluginSetup).interfaceId)); + } + + function test_supportsInterface_IProtocolVersion() public view { + assertTrue(setupMock.supportsInterface(type(IProtocolVersion).interfaceId)); + } + + function test_supportsInterface_returnsFalseForUnknownInterface() public view { + assertFalse(setupMock.supportsInterface(0xdeadbeef)); + } +} + +/// @notice Non-upgradeable `PluginSetup` variant (via the cloneable mock). +contract PluginSetupTest is PluginSetupSharedTest { + function _deploySetupMock() internal override returns (IPluginSetupLike) { + return IPluginSetupLike(address(new PluginCloneableSetupMockBuild1())); + } + + function _expectedImplementationInterface() internal pure override returns (bytes4) { + return type(IPlugin).interfaceId; + } + + function test_prepareUpdate_revertsForNonUpgradeablePlugin() public { + PluginCloneableSetupMockBuild1 mock = PluginCloneableSetupMockBuild1(address(setupMock)); + IPluginSetup.SetupPayload memory payload = + IPluginSetup.SetupPayload({plugin: address(2), currentHelpers: new address[](0), data: bytes("")}); + vm.expectRevert(PluginSetup.NonUpgradeablePlugin.selector); + mock.prepareUpdate(address(1), uint16(123), payload); + } + + function test_prepareUpdate_revertsForAnyFromBuild() public { + // Lock the "always-reverts" contract semantics: every from-build input + // takes the revert path. + PluginCloneableSetupMockBuild1 mock = PluginCloneableSetupMockBuild1(address(setupMock)); + IPluginSetup.SetupPayload memory payload = + IPluginSetup.SetupPayload({plugin: address(2), currentHelpers: new address[](0), data: bytes("")}); + + uint16[3] memory froms = [uint16(0), uint16(1), uint16(type(uint16).max)]; + for (uint256 i = 0; i < froms.length; i++) { + vm.expectRevert(PluginSetup.NonUpgradeablePlugin.selector); + mock.prepareUpdate(address(1), froms[i], payload); + } + } +} + +/// @notice Upgradeable `PluginUpgradeableSetup` variant. +contract PluginUpgradeableSetupTest is PluginSetupSharedTest { + function _deploySetupMock() internal override returns (IPluginSetupLike) { + return IPluginSetupLike(address(new PluginUUPSUpgradeableSetupMockBuild1())); + } + + function _expectedImplementationInterface() internal pure override returns (bytes4) { + return type(IPlugin).interfaceId; + } + + function test_prepareUpdate_revertsOnInitialBuild() public { + PluginUUPSUpgradeableSetupMockBuild1 build1 = PluginUUPSUpgradeableSetupMockBuild1(address(setupMock)); + IPluginSetup.SetupPayload memory payload = + IPluginSetup.SetupPayload({plugin: address(2), currentHelpers: new address[](0), data: bytes("")}); + + // Build1's override reverts `InvalidUpdatePath(fromBuild: 0, thisBuild: 1)` + // regardless of the actual `_fromBuild` argument. + vm.expectRevert(abi.encodeWithSelector(PluginUpgradeableSetup.InvalidUpdatePath.selector, uint16(0), uint16(1))); + build1.prepareUpdate(address(1), uint16(0), payload); + } + + function test_prepareUpdate_succeedsOnNonInitialBuild() public { + // Build2's override allows updates from Build1. + PluginUUPSUpgradeableSetupMockBuild2 build2 = new PluginUUPSUpgradeableSetupMockBuild2(); + IPluginSetup.SetupPayload memory payload = + IPluginSetup.SetupPayload({plugin: address(2), currentHelpers: new address[](0), data: bytes("")}); + build2.prepareUpdate(address(1), uint16(123), payload); + } +} + +/// @notice Direct IPluginSetup interface-ID lock: the v1.0.0 frozen value +/// must still match the current `type(IPluginSetup).interfaceId`. +contract IPluginSetupInterfaceIdTest is Test { + /// Frozen v1.0.0 iface ID, computed by XOR'ing the four function + /// selectors of `IPluginSetup` (prepareInstallation, prepareUpdate, + /// prepareUninstallation, implementation). Verified inline below. + bytes4 internal constant IPLUGIN_SETUP_V1_0_0_INTERFACE_ID = + bytes4(0xf10832f1) ^ bytes4(0xa8a9c29e) ^ bytes4(0x9cb0a124) ^ bytes4(0x5c60da1b); + + function test_IPluginSetup_currentMatchesV1_0_0() public pure { + assertEq(type(IPluginSetup).interfaceId, IPLUGIN_SETUP_V1_0_0_INTERFACE_ID); + } + + function test_IPluginSetup_interfaceIdIsNotEmpty() public pure { + assertTrue(type(IPluginSetup).interfaceId != bytes4(0)); + } +} diff --git a/test/common/utils/deployment/ProxyLib.t.sol b/test/common/utils/deployment/ProxyLib.t.sol index ecd4f090f..8cdc1c8a9 100644 --- a/test/common/utils/deployment/ProxyLib.t.sol +++ b/test/common/utils/deployment/ProxyLib.t.sol @@ -11,16 +11,16 @@ import {PluginCloneableMockBuild1} from "../../../mocks/commons/plugin/PluginClo /// @notice Direct tests for `ProxyFactory` and the `ProxyLib` library it /// delegates to (`src/common/utils/deployment/`). /// -/// Ports `osx-commons/contracts/test/utils/deployment/proxy-lib.ts` and -/// closes the gaps from `TESTS.md` §7: explicit `ProxyCreated` event -/// emission, immutable `implementation()` getter, init-revert propagation -/// (closes central flaw log F15), and distinct addresses for consecutive +/// Ports `osx-commons/contracts/test/utils/deployment/proxy-lib.ts` and adds +/// explicit `ProxyCreated` event emission, immutable `implementation()` +/// getter, init-revert propagation, and distinct addresses for consecutive /// deploys. contract ProxyLibTest is Test { /// A non-zero address used as the `IDAO` argument to `initialize` to /// distinguish "uninitialized" (dao = address(0)) from "initialized" /// (dao = fakeDao). Value chosen for visual clarity in traces. - address internal constant FAKE_DAO = 0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF; + address internal constant FAKE_DAO = + 0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF; PluginUUPSUpgradeableMockBuild1 internal uupsImpl; PluginCloneableMockBuild1 internal cloneableImpl; @@ -36,7 +36,10 @@ contract ProxyLibTest is Test { // Both mocks expose `initialize(IDAO)` with identical signatures; the same // calldata works for either factory. - initCalldata = abi.encodeCall(PluginUUPSUpgradeableMockBuild1.initialize, (IDAO(FAKE_DAO))); + initCalldata = abi.encodeCall( + PluginUUPSUpgradeableMockBuild1.initialize, + (IDAO(FAKE_DAO)) + ); } // ------------------------------------------------------------------------- @@ -54,7 +57,9 @@ contract ProxyLibTest is Test { function test_deployUUPSProxy_initializedWhenCalldataProvided() public { address proxyAddr = uupsFactory.deployUUPSProxy(initCalldata); - PluginUUPSUpgradeableMockBuild1 proxy = PluginUUPSUpgradeableMockBuild1(proxyAddr); + PluginUUPSUpgradeableMockBuild1 proxy = PluginUUPSUpgradeableMockBuild1( + proxyAddr + ); assertEq(proxy.implementation(), uupsFactory.implementation()); assertEq(proxy.implementation(), address(uupsImpl)); @@ -64,14 +69,18 @@ contract ProxyLibTest is Test { function test_deployUUPSProxy_uninitializedWhenNoCalldata() public { address proxyAddr = uupsFactory.deployUUPSProxy(""); - PluginUUPSUpgradeableMockBuild1 proxy = PluginUUPSUpgradeableMockBuild1(proxyAddr); + PluginUUPSUpgradeableMockBuild1 proxy = PluginUUPSUpgradeableMockBuild1( + proxyAddr + ); assertEq(proxy.implementation(), address(uupsImpl)); assertEq(address(proxy.dao()), address(0)); assertEq(proxy.state1(), 0); } - function test_deployUUPSProxy_emitsProxyCreatedWithDeployedAddress() public { + function test_deployUUPSProxy_emitsProxyCreatedWithDeployedAddress() + public + { vm.recordLogs(); address proxy = uupsFactory.deployUUPSProxy(""); Vm.Log[] memory logs = vm.getRecordedLogs(); @@ -79,7 +88,10 @@ contract ProxyLibTest is Test { bytes32 expectedTopic = keccak256("ProxyCreated(address)"); bool found; for (uint256 i = 0; i < logs.length; i++) { - if (logs[i].topics[0] == expectedTopic && logs[i].emitter == address(uupsFactory)) { + if ( + logs[i].topics[0] == expectedTopic && + logs[i].emitter == address(uupsFactory) + ) { address emittedProxy = abi.decode(logs[i].data, (address)); assertEq(emittedProxy, proxy); found = true; @@ -93,12 +105,16 @@ contract ProxyLibTest is Test { // F15: an init that targets a non-existent selector reverts inside the // proxy's constructor delegatecall — the entire deployUUPSProxy call // must revert (no silent partial proxy creation). - bytes memory bogus = abi.encodeWithSelector(bytes4(keccak256("doesNotExist()"))); + bytes memory bogus = abi.encodeWithSelector( + bytes4(keccak256("doesNotExist()")) + ); vm.expectRevert(); uupsFactory.deployUUPSProxy(bogus); } - function test_deployUUPSProxy_consecutiveDeploysProduceDistinctAddresses() public { + function test_deployUUPSProxy_consecutiveDeploysProduceDistinctAddresses() + public + { address a = uupsFactory.deployUUPSProxy(""); address b = uupsFactory.deployUUPSProxy(""); address c = uupsFactory.deployUUPSProxy(initCalldata); @@ -127,7 +143,9 @@ contract ProxyLibTest is Test { assertEq(proxy.state1(), 0); } - function test_deployMinimalProxy_emitsProxyCreatedWithDeployedAddress() public { + function test_deployMinimalProxy_emitsProxyCreatedWithDeployedAddress() + public + { vm.recordLogs(); address proxy = cloneableFactory.deployMinimalProxy(""); Vm.Log[] memory logs = vm.getRecordedLogs(); @@ -135,7 +153,10 @@ contract ProxyLibTest is Test { bytes32 expectedTopic = keccak256("ProxyCreated(address)"); bool found; for (uint256 i = 0; i < logs.length; i++) { - if (logs[i].topics[0] == expectedTopic && logs[i].emitter == address(cloneableFactory)) { + if ( + logs[i].topics[0] == expectedTopic && + logs[i].emitter == address(cloneableFactory) + ) { address emittedProxy = abi.decode(logs[i].data, (address)); assertEq(emittedProxy, proxy); found = true; @@ -153,7 +174,9 @@ contract ProxyLibTest is Test { cloneableFactory.deployMinimalProxy(bogus); } - function test_deployMinimalProxy_consecutiveDeploysProduceDistinctAddresses() public { + function test_deployMinimalProxy_consecutiveDeploysProduceDistinctAddresses() + public + { address a = cloneableFactory.deployMinimalProxy(""); address b = cloneableFactory.deployMinimalProxy(""); address c = cloneableFactory.deployMinimalProxy(initCalldata); diff --git a/test/common/utils/math/BitMap.t.sol b/test/common/utils/math/BitMap.t.sol index 8d348039a..64d27da36 100644 --- a/test/common/utils/math/BitMap.t.sol +++ b/test/common/utils/math/BitMap.t.sol @@ -8,10 +8,9 @@ import {hasBit, flipBit} from "../../../../src/common/utils/math/BitMap.sol"; /// @notice Direct tests for the `hasBit` / `flipBit` file-level functions /// in `src/common/utils/math/BitMap.sol`. /// -/// Ports `osx-commons/contracts/test/utils/math/bitmap.ts` and adds the -/// gap-analysis items recorded in `TESTS.md` §1 (boundary at index 255, -/// bit-position isolation, and three cross-function invariants expressed -/// as fuzz tests). +/// Ports `osx-commons/contracts/test/utils/math/bitmap.ts` and adds boundary +/// coverage at index 255, bit-position isolation, and three cross-function +/// invariants expressed as fuzz tests. contract BitMapTest is Test { uint256 internal constant ZEROS = 0; uint256 internal constant ONES = type(uint256).max; @@ -116,15 +115,20 @@ contract BitMapTest is Test { // ------------------------------------------------------------------------- /// `flipBit` toggles the queried bit, every time, for any bitmap and index. - /// Closes gap #1 from `TESTS.md` §1: `hasBit(flipBit(bm, i), i) != hasBit(bm, i)`. - function testFuzz_flipBitTogglesQueriedBit(uint256 bm, uint8 idx) public pure { + /// Invariant: `hasBit(flipBit(bm, i), i) != hasBit(bm, i)`. + function testFuzz_flipBitTogglesQueriedBit( + uint256 bm, + uint8 idx + ) public pure { bool before = hasBit(bm, idx); assertEq(hasBit(flipBit(bm, idx), idx), !before); } /// `flipBit` is its own inverse: `flipBit(flipBit(bm, i), i) == bm`. - /// Closes gap #2 from `TESTS.md` §1. - function testFuzz_flipBitIsItsOwnInverse(uint256 bm, uint8 idx) public pure { + function testFuzz_flipBitIsItsOwnInverse( + uint256 bm, + uint8 idx + ) public pure { assertEq(flipBit(flipBit(bm, idx), idx), bm); } @@ -132,7 +136,11 @@ contract BitMapTest is Test { /// checking an independent index `j != i` against the original bitmap. /// This is the bit-position isolation invariant — a localized version of /// the gap test `test_hasBit_isolatesIndividualBits` above. - function testFuzz_flipBitOnlyAffectsQueriedBit(uint256 bm, uint8 idx, uint8 other) public pure { + function testFuzz_flipBitOnlyAffectsQueriedBit( + uint256 bm, + uint8 idx, + uint8 other + ) public pure { vm.assume(idx != other); assertEq(hasBit(flipBit(bm, idx), other), hasBit(bm, other)); } diff --git a/test/common/utils/math/Ratio.t.sol b/test/common/utils/math/Ratio.t.sol index 4cf302bd2..ef1118010 100644 --- a/test/common/utils/math/Ratio.t.sol +++ b/test/common/utils/math/Ratio.t.sol @@ -9,9 +9,9 @@ import {RATIO_BASE, _applyRatioCeiled, RatioOutOfBounds} from "../../../../src/c /// `src/common/utils/math/Ratio.sol`. /// /// Ports `osx-commons/contracts/test/utils/math/ratio.ts` and adds the -/// gap-analysis items from `TESTS.md` §2: identity case (ratio == base), -/// zero-value, zero-ratio, distinct revert paths for `RatioOutOfBounds` -/// vs arithmetic overflow, and the inclusive-boundary property. +/// identity case (ratio == base), zero-value, zero-ratio, distinct revert +/// paths for `RatioOutOfBounds` vs arithmetic overflow, and the inclusive- +/// boundary property. contract RatioTest is Test { /// 50% in `RATIO_BASE` units. Replaces the TS `pctToRatio(50)` SDK helper. uint256 internal constant HALF = RATIO_BASE / 2; @@ -20,7 +20,10 @@ contract RatioTest is Test { /// frame so `vm.expectRevert` can intercept them. File-level functions /// otherwise inline into the test contract and reverts collapse to the /// test's own frame. - function applyRatioCeiledExt(uint256 value, uint256 ratio) external pure returns (uint256) { + function applyRatioCeiledExt( + uint256 value, + uint256 ratio + ) external pure returns (uint256) { return _applyRatioCeiled(value, ratio); } @@ -47,12 +50,18 @@ contract RatioTest is Test { assertEq(_applyRatioCeiled(33, HALF), 17); } - function test_applyRatioCeiled_ratioEqualsBaseReturnsValueUnchanged() public pure { + function test_applyRatioCeiled_ratioEqualsBaseReturnsValueUnchanged() + public + pure + { // GAP: ratio == RATIO_BASE is the identity case. assertEq(_applyRatioCeiled(123, RATIO_BASE), 123); assertEq(_applyRatioCeiled(0, RATIO_BASE), 0); assertEq(_applyRatioCeiled(1, RATIO_BASE), 1); - assertEq(_applyRatioCeiled(type(uint128).max, RATIO_BASE), type(uint128).max); + assertEq( + _applyRatioCeiled(type(uint128).max, RATIO_BASE), + type(uint128).max + ); } function test_applyRatioCeiled_zeroValueReturnsZero() public pure { @@ -76,7 +85,13 @@ contract RatioTest is Test { function test_applyRatioCeiled_revertsIfRatioExceedsBase() public { uint256 tooBig = RATIO_BASE + 1; - vm.expectRevert(abi.encodeWithSelector(RatioOutOfBounds.selector, RATIO_BASE, tooBig)); + vm.expectRevert( + abi.encodeWithSelector( + RatioOutOfBounds.selector, + RATIO_BASE, + tooBig + ) + ); this.applyRatioCeiledExt(123, tooBig); } @@ -84,7 +99,9 @@ contract RatioTest is Test { // GAP: the custom error must carry the exact actual ratio in its second arg, // not just any too-large value. uint256 max = type(uint256).max; - vm.expectRevert(abi.encodeWithSelector(RatioOutOfBounds.selector, RATIO_BASE, max)); + vm.expectRevert( + abi.encodeWithSelector(RatioOutOfBounds.selector, RATIO_BASE, max) + ); this.applyRatioCeiledExt(0, max); } @@ -101,7 +118,10 @@ contract RatioTest is Test { this.applyRatioCeiledExt(overflowValue, RATIO_BASE); } - function test_applyRatioCeiled_justBelowOverflowDoesNotRevert() public pure { + function test_applyRatioCeiled_justBelowOverflowDoesNotRevert() + public + pure + { // The largest value that does NOT overflow `_value * RATIO_BASE`. uint256 borderValue = type(uint256).max / RATIO_BASE; _applyRatioCeiled(borderValue, RATIO_BASE); @@ -115,14 +135,19 @@ contract RatioTest is Test { /// ceiling, applying a ratio in `[0, 1]` to `value` cannot grow it. /// `value` bounded to `uint128` to stay inside the no-overflow envelope /// (`uint128 << uint256.max / RATIO_BASE`). - function testFuzz_applyRatioCeiled_resultNeverExceedsValue(uint128 value, uint256 ratio) public pure { + function testFuzz_applyRatioCeiled_resultNeverExceedsValue( + uint128 value, + uint256 ratio + ) public pure { ratio = bound(ratio, 0, RATIO_BASE); assertLe(_applyRatioCeiled(uint256(value), ratio), uint256(value)); } /// `ratio == RATIO_BASE` is the identity function on `value` for any value /// that does not overflow `value * RATIO_BASE`. - function testFuzz_applyRatioCeiled_baseRatioIsIdentity(uint256 value) public pure { + function testFuzz_applyRatioCeiled_baseRatioIsIdentity( + uint256 value + ) public pure { value = bound(value, 0, type(uint256).max / RATIO_BASE); assertEq(_applyRatioCeiled(value, RATIO_BASE), value); } diff --git a/test/common/utils/math/UncheckedMath.t.sol b/test/common/utils/math/UncheckedMath.t.sol index 954edbf4a..2414c8dc8 100644 --- a/test/common/utils/math/UncheckedMath.t.sol +++ b/test/common/utils/math/UncheckedMath.t.sol @@ -8,10 +8,10 @@ import {_uncheckedAdd, _uncheckedSub} from "../../../../src/common/utils/math/Un /// @notice Direct tests for `_uncheckedAdd` and `_uncheckedSub` in /// `src/common/utils/math/UncheckedMath.sol`. /// -/// No upstream TS coverage existed for these helpers — entire surface is -/// the gap recorded in `TESTS.md` §3. Boundary cases verify wrap-around -/// (no panic); fuzz tests verify each helper agrees with the same expression -/// inside an explicit `unchecked` block, pinning behavioural equivalence. +/// No upstream TS coverage existed for these helpers. Boundary cases verify +/// wrap-around (no panic); fuzz tests verify each helper agrees with the same +/// expression inside an explicit `unchecked` block, pinning behavioural +/// equivalence. contract UncheckedMathTest is Test { // ------------------------------------------------------------------------- // _uncheckedAdd @@ -30,7 +30,10 @@ contract UncheckedMathTest is Test { // `type(uint256).max + 2` wraps to 1. assertEq(_uncheckedAdd(type(uint256).max, 2), 1); // Two large operands wrap predictably. - assertEq(_uncheckedAdd(type(uint256).max, type(uint256).max), type(uint256).max - 1); + assertEq( + _uncheckedAdd(type(uint256).max, type(uint256).max), + type(uint256).max - 1 + ); } // ------------------------------------------------------------------------- @@ -60,7 +63,10 @@ contract UncheckedMathTest is Test { /// `_uncheckedAdd(a, b)` agrees with `unchecked { a + b }` for any inputs. /// Pins the helper to the canonical wrap-on-overflow semantics so a future /// refactor that accidentally re-introduces checked arithmetic fails here. - function testFuzz_uncheckedAdd_equalsInlineUnchecked(uint256 a, uint256 b) public pure { + function testFuzz_uncheckedAdd_equalsInlineUnchecked( + uint256 a, + uint256 b + ) public pure { uint256 expected; unchecked { expected = a + b; @@ -69,7 +75,10 @@ contract UncheckedMathTest is Test { } /// `_uncheckedSub(a, b)` agrees with `unchecked { a - b }` for any inputs. - function testFuzz_uncheckedSub_equalsInlineUnchecked(uint256 a, uint256 b) public pure { + function testFuzz_uncheckedSub_equalsInlineUnchecked( + uint256 a, + uint256 b + ) public pure { uint256 expected; unchecked { expected = a - b; diff --git a/test/common/utils/metadata/MetadataExtension.t.sol b/test/common/utils/metadata/MetadataExtension.t.sol index de97b647c..b277264ef 100644 --- a/test/common/utils/metadata/MetadataExtension.t.sol +++ b/test/common/utils/metadata/MetadataExtension.t.sol @@ -7,9 +7,7 @@ import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import {IDAO} from "../../../../src/common/dao/IDAO.sol"; import {DaoUnauthorized} from "../../../../src/common/permission/auth/auth.sol"; import {MetadataExtensionMock} from "../../../mocks/commons/utils/metadata/MetadataExtensionMock.sol"; -import { - MetadataExtensionUpgradeableMock -} from "../../../mocks/commons/utils/metadata/MetadataExtensionUpgradeableMock.sol"; +import {MetadataExtensionUpgradeableMock} from "../../../mocks/commons/utils/metadata/MetadataExtensionUpgradeableMock.sol"; import {DAOMock} from "../../../mocks/commons/dao/DAOMock.sol"; /// @dev Minimal shape that both `MetadataExtensionMock` and @@ -17,18 +15,21 @@ import {DAOMock} from "../../../mocks/commons/dao/DAOMock.sol"; /// source for the ERC-165 ID and as a typed handle in the shared tests. interface IMetadataExtension { function setMetadata(bytes calldata _metadata) external; + function getMetadata() external view returns (bytes memory); - function supportsInterface(bytes4 _interfaceId) external view returns (bool); + + function supportsInterface( + bytes4 _interfaceId + ) external view returns (bool); } /// @notice Shared behaviour tests for `MetadataExtension` and /// `MetadataExtensionUpgradeable` in `src/common/utils/metadata/`. /// -/// Ports `osx-commons/contracts/test/utils/metadata.ts` and closes the gaps -/// from `TESTS.md` §10: empty / large payload roundtrips, explicit XOR -/// selector check, auth guard verified via `DaoUnauthorized`, event-after- -/// state-change ordering, and (Upgradeable only) hard-coded storage slot -/// isolation via `vm.load`. +/// Ports `osx-commons/contracts/test/utils/metadata.ts` and adds empty / +/// large payload roundtrips, an explicit XOR selector check, the auth guard +/// verified via `DaoUnauthorized`, event-after-state-change ordering, and +/// (Upgradeable only) hard-coded storage slot isolation via `vm.load`. abstract contract MetadataExtensionSharedTest is Test { DAOMock internal daoMock; IMetadataExtension internal target; @@ -37,7 +38,8 @@ abstract contract MetadataExtensionSharedTest is Test { /// `MetadataExtension`'s `supportsInterface` returns true for this XOR /// (per source); pre-computed here so the assertion is double-anchored. bytes4 internal immutable METADATA_SELECTOR_INTERFACE_ID = - IMetadataExtension.setMetadata.selector ^ IMetadataExtension.getMetadata.selector; + IMetadataExtension.setMetadata.selector ^ + IMetadataExtension.getMetadata.selector; function _deployTarget() internal virtual returns (IMetadataExtension); @@ -64,7 +66,10 @@ abstract contract MetadataExtensionSharedTest is Test { assertEq(METADATA_SELECTOR_INTERFACE_ID, bytes4(0x940cac36)); } - function test_supportsInterface_returnsFalseForUnknownInterface() public view { + function test_supportsInterface_returnsFalseForUnknownInterface() + public + view + { assertFalse(target.supportsInterface(0xdeadbeef)); } @@ -90,7 +95,10 @@ abstract contract MetadataExtensionSharedTest is Test { bytes32 expectedTopic = keccak256("MetadataSet(bytes)"); bool found; for (uint256 i = 0; i < logs.length; i++) { - if (logs[i].topics[0] == expectedTopic && logs[i].emitter == address(target)) { + if ( + logs[i].topics[0] == expectedTopic && + logs[i].emitter == address(target) + ) { bytes memory decoded = abi.decode(logs[i].data, (bytes)); assertEq(decoded, payload); found = true; @@ -142,7 +150,10 @@ abstract contract MetadataExtensionSharedTest is Test { bytes32 expectedTopic = keccak256("MetadataSet(bytes)"); for (uint256 i = 0; i < logs.length; i++) { - if (logs[i].topics[0] == expectedTopic && logs[i].emitter == address(target)) { + if ( + logs[i].topics[0] == expectedTopic && + logs[i].emitter == address(target) + ) { assertEq(abi.decode(logs[i].data, (bytes)), payload); return; } @@ -154,12 +165,15 @@ abstract contract MetadataExtensionSharedTest is Test { /// @notice Constructable variant. contract MetadataExtensionTest is MetadataExtensionSharedTest { function _deployTarget() internal override returns (IMetadataExtension) { - return IMetadataExtension(address(new MetadataExtensionMock(IDAO(address(daoMock))))); + return + IMetadataExtension( + address(new MetadataExtensionMock(IDAO(address(daoMock)))) + ); } } /// @notice Upgradeable variant — adds the hard-coded storage-slot isolation -/// test (closes the `MetadataExtensionUpgradeable` storage-slot gap in §10). +/// test for `MetadataExtensionUpgradeable`. contract MetadataExtensionUpgradeableTest is MetadataExtensionSharedTest { /// `keccak256(abi.encode(uint256(keccak256("osx-commons.storage.MetadataExtension")) - 1)) & ~bytes32(uint256(0xff))` /// — duplicated verbatim from `MetadataExtensionUpgradeable.sol`. @@ -182,7 +196,11 @@ contract MetadataExtensionUpgradeableTest is MetadataExtensionSharedTest { bytes32 raw = vm.load(address(target), METADATA_STORAGE_SLOT); // The low byte encodes `length * 2` for short bytes. payload is 1 byte // long, so the low byte is 2. - assertEq(uint256(raw) & 0xff, 2, "short-bytes length encoding mismatch"); + assertEq( + uint256(raw) & 0xff, + 2, + "short-bytes length encoding mismatch" + ); // The high byte holds the payload itself. assertEq(uint256(raw) >> 248, 0x42, "short-bytes value mismatch"); } diff --git a/test/common/utils/versioning/ProtocolVersion.t.sol b/test/common/utils/versioning/ProtocolVersion.t.sol index f982f4c79..9676c0868 100644 --- a/test/common/utils/versioning/ProtocolVersion.t.sol +++ b/test/common/utils/versioning/ProtocolVersion.t.sol @@ -10,10 +10,10 @@ import {ProtocolVersionMock} from "../../../mocks/commons/utils/versioning/Proto /// @notice Direct tests for the abstract `ProtocolVersion` base contract and /// the `IProtocolVersion` interface in `src/common/utils/versioning/`. /// -/// Ports `osx-commons/contracts/test/utils/versioning/protocol-version.ts` -/// and closes the gaps from `TESTS.md` §5: exact return-value check against -/// the inline `[1, 4, 0]` constant (replacing the TS dependency on -/// `package.json`), stateless / deterministic across calls. +/// Ports `osx-commons/contracts/test/utils/versioning/protocol-version.ts`. +/// Asserts the exact return value against the inline `[1, 4, 0]` constant +/// (replacing the TS dependency on `package.json`), and that the function is +/// stateless / deterministic across calls. contract ProtocolVersionTest is Test { /// Frozen iface ID introduced in v1.3.0. `IProtocolVersion` has a single /// function `protocolVersion()`, so its ERC-165 ID equals that function's @@ -40,7 +40,10 @@ contract ProtocolVersionTest is Test { // ------------------------------------------------------------------------- function test_IProtocolVersion_hasSameInterfaceIdAsV1_3_0() public pure { - assertEq(type(IProtocolVersion).interfaceId, IPROTOCOL_VERSION_V1_3_0_INTERFACE_ID); + assertEq( + type(IProtocolVersion).interfaceId, + IPROTOCOL_VERSION_V1_3_0_INTERFACE_ID + ); } function test_IProtocolVersion_interfaceIdIsNotEmpty() public pure { @@ -52,14 +55,19 @@ contract ProtocolVersionTest is Test { function test_IProtocolVersion_interfaceIdIsNotIERC165() public pure { // Cross-check: `IProtocolVersion` is a distinct interface, not an alias // of `IERC165`. - assertTrue(type(IProtocolVersion).interfaceId != type(IERC165).interfaceId); + assertTrue( + type(IProtocolVersion).interfaceId != type(IERC165).interfaceId + ); } // ------------------------------------------------------------------------- // ProtocolVersion — concrete value // ------------------------------------------------------------------------- - function test_protocolVersion_returnsCurrentProductionVersion() public view { + function test_protocolVersion_returnsCurrentProductionVersion() + public + view + { uint8[3] memory v = mock.protocolVersion(); assertEq(v[0], MAJOR); assertEq(v[1], MINOR); diff --git a/test/common/utils/versioning/VersionComparisonLib.t.sol b/test/common/utils/versioning/VersionComparisonLib.t.sol index ccb4d0709..b04a7603c 100644 --- a/test/common/utils/versioning/VersionComparisonLib.t.sol +++ b/test/common/utils/versioning/VersionComparisonLib.t.sol @@ -9,10 +9,10 @@ import {VersionComparisonLib} from "../../../../src/common/utils/versioning/Vers /// `src/common/utils/versioning/VersionComparisonLib.sol`. /// /// Ports `osx-commons/contracts/test/utils/versioning/version-comparison-lib.ts` -/// and adds the gap items from `TESTS.md` §4: +/// and adds: /// - boundary versions [0,0,0] / [255,255,255] / asymmetric extremes, -/// - the logical-consistency invariant `!lt(a,b) && !eq(a,b) ⇔ gt(a,b)` -/// plus the lte/gte duals (closes central flaw log F16), +/// - the logical-consistency invariant `!lt(a,b) && !eq(a,b) <=> gt(a,b)` +/// plus the lte/gte duals, /// - transitivity of `lt`. /// /// The TS suite uses three matrix helpers (`eqChecks`, `ltChecks`, `gtChecks`) @@ -29,31 +29,53 @@ contract VersionComparisonLibTest is Test { // contract so we can pass it by reference into the matrix helpers below. // ------------------------------------------------------------------------- - function _opEq(uint8[3] memory l, uint8[3] memory r) internal pure returns (bool) { + function _opEq( + uint8[3] memory l, + uint8[3] memory r + ) internal pure returns (bool) { return l.eq(r); } - function _opNeq(uint8[3] memory l, uint8[3] memory r) internal pure returns (bool) { + function _opNeq( + uint8[3] memory l, + uint8[3] memory r + ) internal pure returns (bool) { return l.neq(r); } - function _opLt(uint8[3] memory l, uint8[3] memory r) internal pure returns (bool) { + function _opLt( + uint8[3] memory l, + uint8[3] memory r + ) internal pure returns (bool) { return l.lt(r); } - function _opLte(uint8[3] memory l, uint8[3] memory r) internal pure returns (bool) { + function _opLte( + uint8[3] memory l, + uint8[3] memory r + ) internal pure returns (bool) { return l.lte(r); } - function _opGt(uint8[3] memory l, uint8[3] memory r) internal pure returns (bool) { + function _opGt( + uint8[3] memory l, + uint8[3] memory r + ) internal pure returns (bool) { return l.gt(r); } - function _opGte(uint8[3] memory l, uint8[3] memory r) internal pure returns (bool) { + function _opGte( + uint8[3] memory l, + uint8[3] memory r + ) internal pure returns (bool) { return l.gte(r); } - function _v(uint8 a, uint8 b, uint8 c) internal pure returns (uint8[3] memory v) { + function _v( + uint8 a, + uint8 b, + uint8 c + ) internal pure returns (uint8[3] memory v) { v[0] = a; v[1] = b; v[2] = c; @@ -65,10 +87,13 @@ contract VersionComparisonLibTest is Test { /// 8 pairs where `lhs == rhs`. Covers every "all-zero subset" combination /// of the three semver components, including the all-zero and all-one vectors. - function _runEqualPairs(function(uint8[3] memory, uint8[3] memory) internal pure returns (bool) op, bool expected) - internal - pure - { + function _runEqualPairs( + function(uint8[3] memory, uint8[3] memory) + internal + pure + returns (bool) op, + bool expected + ) internal pure { assertEq(op(_v(1, 1, 1), _v(1, 1, 1)), expected); assertEq(op(_v(0, 1, 1), _v(0, 1, 1)), expected); assertEq(op(_v(1, 0, 1), _v(1, 0, 1)), expected); @@ -81,10 +106,13 @@ contract VersionComparisonLibTest is Test { /// 16 pairs where `lhs < rhs` — exercises every single-component step-up /// as well as multi-component variations. - function _runLtPairs(function(uint8[3] memory, uint8[3] memory) internal pure returns (bool) op, bool expected) - internal - pure - { + function _runLtPairs( + function(uint8[3] memory, uint8[3] memory) + internal + pure + returns (bool) op, + bool expected + ) internal pure { // Single-component bumps from [1,1,1]. assertEq(op(_v(1, 1, 1), _v(2, 1, 1)), expected); assertEq(op(_v(1, 1, 1), _v(1, 2, 1)), expected); @@ -110,10 +138,13 @@ contract VersionComparisonLibTest is Test { } /// Mirror of `_runLtPairs` with sides swapped. - function _runGtPairs(function(uint8[3] memory, uint8[3] memory) internal pure returns (bool) op, bool expected) - internal - pure - { + function _runGtPairs( + function(uint8[3] memory, uint8[3] memory) + internal + pure + returns (bool) op, + bool expected + ) internal pure { assertEq(op(_v(2, 1, 1), _v(1, 1, 1)), expected); assertEq(op(_v(1, 2, 1), _v(1, 1, 1)), expected); assertEq(op(_v(1, 1, 2), _v(1, 1, 1)), expected); @@ -245,7 +276,10 @@ contract VersionComparisonLibTest is Test { /// Exactly one of `lt`, `eq`, `gt` is true for any pair. The TS suite never /// asserted the full trichotomy across the 6 operators; locking it in here /// catches a class of subtle operator-drift bugs. - function testFuzz_trichotomy(uint8[3] memory a, uint8[3] memory b) public pure { + function testFuzz_trichotomy( + uint8[3] memory a, + uint8[3] memory b + ) public pure { bool isLt = a.lt(b); bool isEq = a.eq(b); bool isGt = a.gt(b); @@ -256,27 +290,42 @@ contract VersionComparisonLibTest is Test { } /// `neq` is the negation of `eq`. - function testFuzz_neqIsNegationOfEq(uint8[3] memory a, uint8[3] memory b) public pure { + function testFuzz_neqIsNegationOfEq( + uint8[3] memory a, + uint8[3] memory b + ) public pure { assertEq(a.neq(b), !a.eq(b)); } /// `lte` is `lt || eq`. Same shape for `gte`. - function testFuzz_lteEqualsLtOrEq(uint8[3] memory a, uint8[3] memory b) public pure { + function testFuzz_lteEqualsLtOrEq( + uint8[3] memory a, + uint8[3] memory b + ) public pure { assertEq(a.lte(b), a.lt(b) || a.eq(b)); } - function testFuzz_gteEqualsGtOrEq(uint8[3] memory a, uint8[3] memory b) public pure { + function testFuzz_gteEqualsGtOrEq( + uint8[3] memory a, + uint8[3] memory b + ) public pure { assertEq(a.gte(b), a.gt(b) || a.eq(b)); } - /// `!lt(a,b) && !eq(a,b) ⇔ gt(a,b)` — the consistency invariant called out - /// in `TESTS.md` F16. - function testFuzz_consistencyAcrossOperators(uint8[3] memory a, uint8[3] memory b) public pure { + /// `!lt(a,b) && !eq(a,b) <=> gt(a,b)` — trichotomy-consistency invariant. + function testFuzz_consistencyAcrossOperators( + uint8[3] memory a, + uint8[3] memory b + ) public pure { assertEq(!a.lt(b) && !a.eq(b), a.gt(b)); } /// `lt` is transitive: `lt(a, b) && lt(b, c) ⇒ lt(a, c)`. - function testFuzz_ltIsTransitive(uint8[3] memory a, uint8[3] memory b, uint8[3] memory c) public pure { + function testFuzz_ltIsTransitive( + uint8[3] memory a, + uint8[3] memory b, + uint8[3] memory c + ) public pure { vm.assume(a.lt(b) && b.lt(c)); assertTrue(a.lt(c)); } From fbae8e3cb23433d9f7b6ebd2de643ecd7957daf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8r=E2=88=82=C2=A1?= Date: Tue, 19 May 2026 08:57:22 +0000 Subject: [PATCH 08/31] Test: core utils --- .../test/core/dao/callback-handler.ts | 72 --------- .../test/framework/utils/registry-utils.ts | 66 -------- test/common/permission/PermissionLib.t.sol | 103 +++++++++++++ test/core/utils/CallbackHandler.t.sol | 131 ++++++++++++++++ test/framework/utils/RegistryUtils.t.sol | 143 ++++++++++++++++++ 5 files changed, 377 insertions(+), 138 deletions(-) delete mode 100644 packages/contracts/test/core/dao/callback-handler.ts delete mode 100644 packages/contracts/test/framework/utils/registry-utils.ts create mode 100644 test/common/permission/PermissionLib.t.sol create mode 100644 test/core/utils/CallbackHandler.t.sol create mode 100644 test/framework/utils/RegistryUtils.t.sol diff --git a/packages/contracts/test/core/dao/callback-handler.ts b/packages/contracts/test/core/dao/callback-handler.ts deleted file mode 100644 index 007b702a0..000000000 --- a/packages/contracts/test/core/dao/callback-handler.ts +++ /dev/null @@ -1,72 +0,0 @@ -import { - CallbackHandlerMockHelper, - CallbackHandlerMockHelper__factory, -} from '../../../typechain'; -import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; -import {expect} from 'chai'; -import {defaultAbiCoder, hexDataSlice, id} from 'ethers/lib/utils'; -import hre, {ethers} from 'hardhat'; - -const EVENTS = { - STANDARD_CALLBACK_REGISTERED: 'StandardCallbackRegistered', - CALLBACK_RECEIVED: 'CallbackReceived', -}; - -const callbackSelector = hexDataSlice(id('callbackFunc()'), 0, 4); // 0x1eb2075a -const magicNumber = `0x1${'0'.repeat(7)}`; -export const UNREGISTERED_INTERFACE_RETURN = `0x${'00'.repeat(4)}`; - -describe('CallbackHandler', function () { - let signers: SignerWithAddress[]; - let owner: string; - let callbackHandlerMockHelper: CallbackHandlerMockHelper; - - beforeEach(async () => { - signers = await ethers.getSigners(); - owner = await signers[0].getAddress(); - - callbackHandlerMockHelper = await hre.wrapper.deploy( - 'CallbackHandlerMockHelper' - ); - }); - - it('reverts for an unknown callback function signature', async () => { - await expect( - callbackHandlerMockHelper.handleCallback(callbackSelector, '0x') - ) - .to.be.revertedWithCustomError( - callbackHandlerMockHelper, - 'UnknownCallback' - ) - .withArgs(callbackSelector, UNREGISTERED_INTERFACE_RETURN); - }); - - it('returns the correct magic number from the `_handleCallback`', async () => { - await callbackHandlerMockHelper.registerCallback( - callbackSelector, - magicNumber - ); - - expect( - await callbackHandlerMockHelper.callStatic.handleCallback( - callbackSelector, - '0x' - ) - ).to.equal(magicNumber); - }); - - it('correctly emits the received callback event', async () => { - await callbackHandlerMockHelper.registerCallback( - callbackSelector, - magicNumber - ); - - const data = '0x1111'; - - await expect( - callbackHandlerMockHelper.handleCallback(callbackSelector, data) - ) - .to.emit(callbackHandlerMockHelper, EVENTS.CALLBACK_RECEIVED) - .withArgs(owner, callbackSelector, data); - }); -}); diff --git a/packages/contracts/test/framework/utils/registry-utils.ts b/packages/contracts/test/framework/utils/registry-utils.ts deleted file mode 100644 index 10b39c59d..000000000 --- a/packages/contracts/test/framework/utils/registry-utils.ts +++ /dev/null @@ -1,66 +0,0 @@ -import {RegistryUtils, RegistryUtils__factory} from '../../../typechain'; -import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; -import {expect} from 'chai'; -import hre, {ethers} from 'hardhat'; - -describe('RegistryUtils', () => { - let registryUtilsContract: RegistryUtils; - let signers: SignerWithAddress[]; - - before(async () => { - signers = await ethers.getSigners(); - }); - - beforeEach(async () => { - registryUtilsContract = await hre.wrapper.deploy('RegistryUtils'); - }); - - describe('isSubdomainValid', () => { - it('should validate the passed name correctly (< 32 bytes long name)', async () => { - const baseName = 'this-is-my-super-valid-name'; - - // loop through the ascii table - for (let i = 0; i < 127; i++) { - // replace the 10th char in the baseName - const subdomainName = - baseName.substring(0, 10) + - String.fromCharCode(i) + - baseName.substring(10 + 1); - - // test success if it is a valid char [0-9a-z\-] - if ((i > 47 && i < 58) || (i > 96 && i < 123) || i === 45) { - expect(await registryUtilsContract.isSubdomainValid(subdomainName)).to - .be.true; - continue; - } - - expect(await registryUtilsContract.isSubdomainValid(subdomainName)).to - .be.false; - } - }); - - it('should validate the passed name correctly (> 32 bytes long name)', async () => { - const baseName = - 'this-is-my-super-looooooooooooooooooooooooooong-valid-name'; - - // loop through the ascii table - for (let i = 0; i < 127; i++) { - // replace the 40th char in the baseName - const subdomainName = - baseName.substring(0, 40) + - String.fromCharCode(i) + - baseName.substring(40 + 1); - - // test success if it is a valid char [0-9a-z\-] - if ((i > 47 && i < 58) || (i > 96 && i < 123) || i === 45) { - expect(await registryUtilsContract.isSubdomainValid(subdomainName)).to - .be.true; - continue; - } - - expect(await registryUtilsContract.isSubdomainValid(subdomainName)).to - .be.false; - } - }); - }); -}); diff --git a/test/common/permission/PermissionLib.t.sol b/test/common/permission/PermissionLib.t.sol new file mode 100644 index 000000000..126165646 --- /dev/null +++ b/test/common/permission/PermissionLib.t.sol @@ -0,0 +1,103 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {PermissionLib} from "../../../src/common/permission/PermissionLib.sol"; + +/// @notice Direct tests for the `PermissionLib` data-only library in +/// `src/common/permission/PermissionLib.sol`. +/// +/// No upstream TS coverage existed for this library (it has no executable +/// code, only constants/enums/structs). Locks the constants and struct +/// layouts so a future rename or reorder fails loudly here. +contract PermissionLibTest is Test { + // ------------------------------------------------------------------------- + // NO_CONDITION constant + // ------------------------------------------------------------------------- + + function test_NO_CONDITION_isAddressZero() public pure { + assertEq(PermissionLib.NO_CONDITION, address(0)); + } + + // ------------------------------------------------------------------------- + // Operation enum values + // ------------------------------------------------------------------------- + + function test_Operation_enumOrdinals() public pure { + // The exact ordinal values are part of the on-wire encoding for + // `applyMultiTargetPermissions` and `applySingleTargetPermissions`. + // A future reorder of this enum would silently corrupt all callers. + assertEq(uint256(PermissionLib.Operation.Grant), 0); + assertEq(uint256(PermissionLib.Operation.Revoke), 1); + assertEq(uint256(PermissionLib.Operation.GrantWithCondition), 2); + } + + // ------------------------------------------------------------------------- + // Struct round-trips through abi.encode / abi.decode + // ------------------------------------------------------------------------- + + function test_SingleTargetPermission_abiRoundtrip() public pure { + PermissionLib.SingleTargetPermission memory original = PermissionLib.SingleTargetPermission({ + operation: PermissionLib.Operation.Grant, who: address(0xBEEF), permissionId: keccak256("DUMMY_PERMISSION") + }); + + bytes memory encoded = abi.encode(original); + PermissionLib.SingleTargetPermission memory decoded = + abi.decode(encoded, (PermissionLib.SingleTargetPermission)); + + assertEq(uint256(decoded.operation), uint256(original.operation)); + assertEq(decoded.who, original.who); + assertEq(decoded.permissionId, original.permissionId); + } + + function test_MultiTargetPermission_abiRoundtrip() public pure { + PermissionLib.MultiTargetPermission memory original = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.GrantWithCondition, + where: address(0xDEAD), + who: address(0xBEEF), + condition: address(0xCAFE), + permissionId: keccak256("DUMMY_PERMISSION") + }); + + bytes memory encoded = abi.encode(original); + PermissionLib.MultiTargetPermission memory decoded = abi.decode(encoded, (PermissionLib.MultiTargetPermission)); + + assertEq(uint256(decoded.operation), uint256(original.operation)); + assertEq(decoded.where, original.where); + assertEq(decoded.who, original.who); + assertEq(decoded.condition, original.condition); + assertEq(decoded.permissionId, original.permissionId); + } + + function test_MultiTargetPermission_arrayRoundtrip() public pure { + PermissionLib.MultiTargetPermission[] memory original = new PermissionLib.MultiTargetPermission[](2); + original[0] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Grant, + where: address(0x1), + who: address(0x2), + condition: PermissionLib.NO_CONDITION, + permissionId: keccak256("A") + }); + original[1] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Revoke, + where: address(0x3), + who: address(0x4), + condition: PermissionLib.NO_CONDITION, + permissionId: keccak256("B") + }); + + bytes memory encoded = abi.encode(original); + PermissionLib.MultiTargetPermission[] memory decoded = + abi.decode(encoded, (PermissionLib.MultiTargetPermission[])); + + assertEq(decoded.length, 2); + for (uint256 i = 0; i < 2; i++) { + assertEq(uint256(decoded[i].operation), uint256(original[i].operation)); + assertEq(decoded[i].where, original[i].where); + assertEq(decoded[i].who, original[i].who); + assertEq(decoded[i].condition, original[i].condition); + assertEq(decoded[i].permissionId, original[i].permissionId); + } + } +} diff --git a/test/core/utils/CallbackHandler.t.sol b/test/core/utils/CallbackHandler.t.sol new file mode 100644 index 000000000..a21cba19a --- /dev/null +++ b/test/core/utils/CallbackHandler.t.sol @@ -0,0 +1,131 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test, Vm} from "forge-std/Test.sol"; +import {CallbackHandler} from "../../../src/core/utils/CallbackHandler.sol"; +import {CallbackHandlerMockHelper} from "../../mocks/dao/CallbackHandlerHelperMock.sol"; + +/// @notice Direct tests for `CallbackHandler` in +/// `src/core/utils/CallbackHandler.sol`. +/// +/// Ports `packages/contracts/test/core/dao/callback-handler.ts` (72 lines, 3 +/// cases). Adds: idempotency under repeated `_registerCallback`, state +/// isolation across distinct selectors, exact `CallbackReceived` event field +/// values, and the `UNREGISTERED_CALLBACK == bytes4(0)` constant. +contract CallbackHandlerTest is Test { + bytes4 internal constant CALLBACK_SELECTOR = bytes4(keccak256("callbackFunc()")); + bytes4 internal constant MAGIC_NUMBER = bytes4(0x10000000); + bytes4 internal constant UNREGISTERED = bytes4(0); + + CallbackHandlerMockHelper internal handler; + address internal alice; + + function setUp() public { + alice = makeAddr("alice"); + handler = new CallbackHandlerMockHelper(); + } + + // ------------------------------------------------------------------------- + // _handleCallback + // ------------------------------------------------------------------------- + + function test_handleCallback_revertsIfNotRegistered() public { + vm.expectRevert( + abi.encodeWithSelector(CallbackHandler.UnknownCallback.selector, CALLBACK_SELECTOR, UNREGISTERED) + ); + handler.handleCallback(CALLBACK_SELECTOR, ""); + } + + function test_handleCallback_returnsMagicNumberWhenRegistered() public { + handler.registerCallback(CALLBACK_SELECTOR, MAGIC_NUMBER); + bytes4 result = handler.handleCallback(CALLBACK_SELECTOR, ""); + assertEq(result, MAGIC_NUMBER); + } + + function test_handleCallback_emitsCallbackReceivedWithExactFields() public { + handler.registerCallback(CALLBACK_SELECTOR, MAGIC_NUMBER); + + bytes memory payload = hex"1111"; + vm.recordLogs(); + vm.prank(alice); + handler.handleCallback(CALLBACK_SELECTOR, payload); + Vm.Log[] memory logs = vm.getRecordedLogs(); + + // event CallbackReceived(address sender, bytes4 indexed sig, bytes data); + // → topics: [keccak256(sig), sig], data: abi.encode(sender, data). + bytes32 expectedTopic = keccak256("CallbackReceived(address,bytes4,bytes)"); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].emitter == address(handler) && logs[i].topics[0] == expectedTopic) { + assertEq(bytes32(logs[i].topics[1]) >> 224, bytes32(CALLBACK_SELECTOR) >> 224); + (address sender, bytes memory data) = abi.decode(logs[i].data, (address, bytes)); + assertEq(sender, alice); + assertEq(data, payload); + found = true; + break; + } + } + assertTrue(found, "CallbackReceived not emitted"); + } + + // ------------------------------------------------------------------------- + // _registerCallback + // ------------------------------------------------------------------------- + + function test_registerCallback_overwriteReplacesMagicNumber() public { + // Register, then re-register with a different magic number. The second + // call must override the first; `_handleCallback` reflects the new value. + bytes4 newMagic = bytes4(0xAABBCCDD); + handler.registerCallback(CALLBACK_SELECTOR, MAGIC_NUMBER); + assertEq(handler.handleCallback(CALLBACK_SELECTOR, ""), MAGIC_NUMBER); + + handler.registerCallback(CALLBACK_SELECTOR, newMagic); + assertEq(handler.handleCallback(CALLBACK_SELECTOR, ""), newMagic); + } + + function test_registerCallback_unregistersWhenMagicIsZero() public { + // Setting back to `UNREGISTERED_CALLBACK = bytes4(0)` returns the + // selector to the "unknown" state. + handler.registerCallback(CALLBACK_SELECTOR, MAGIC_NUMBER); + assertEq(handler.handleCallback(CALLBACK_SELECTOR, ""), MAGIC_NUMBER); + + handler.registerCallback(CALLBACK_SELECTOR, UNREGISTERED); + vm.expectRevert( + abi.encodeWithSelector(CallbackHandler.UnknownCallback.selector, CALLBACK_SELECTOR, UNREGISTERED) + ); + handler.handleCallback(CALLBACK_SELECTOR, ""); + } + + function test_registerCallback_isolatesAcrossSelectors() public { + // Distinct selectors map to distinct magic numbers independently. + bytes4 selA = bytes4(keccak256("aFunc()")); + bytes4 selB = bytes4(keccak256("bFunc()")); + bytes4 magicA = bytes4(0x11111111); + bytes4 magicB = bytes4(0x22222222); + + handler.registerCallback(selA, magicA); + handler.registerCallback(selB, magicB); + + assertEq(handler.handleCallback(selA, ""), magicA); + assertEq(handler.handleCallback(selB, ""), magicB); + + // Registering a third selector does not perturb A or B. + bytes4 selC = bytes4(keccak256("cFunc()")); + handler.registerCallback(selC, bytes4(0x33333333)); + assertEq(handler.handleCallback(selA, ""), magicA); + assertEq(handler.handleCallback(selB, ""), magicB); + } + + // ------------------------------------------------------------------------- + // UNREGISTERED_CALLBACK constant + // ------------------------------------------------------------------------- + + function test_UNREGISTERED_CALLBACK_isZero() public { + // The source uses `UNREGISTERED_CALLBACK = bytes4(0)` to detect + // unregistered selectors. Lock that value via the revert payload that + // `_handleCallback` emits when the selector is not registered. + vm.expectRevert(abi.encodeWithSelector(CallbackHandler.UnknownCallback.selector, CALLBACK_SELECTOR, bytes4(0))); + handler.handleCallback(CALLBACK_SELECTOR, ""); + } +} diff --git a/test/framework/utils/RegistryUtils.t.sol b/test/framework/utils/RegistryUtils.t.sol new file mode 100644 index 000000000..df52f248e --- /dev/null +++ b/test/framework/utils/RegistryUtils.t.sol @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {RegistryUtils} from "../../mocks/utils/RegistryUtilsTest.sol"; + +/// @notice Direct tests for `isSubdomainValid` in +/// `src/framework/utils/RegistryUtils.sol`. +/// +/// Ports `packages/contracts/test/framework/utils/registry-utils.ts` (66 +/// lines, 2 cases) and adds: empty string, single character (valid and +/// invalid), leading/trailing hyphens, all valid character classes mixed. +/// Validates the canonical alphabet `[a-z0-9-]` exhaustively across the +/// printable ASCII range. +contract RegistryUtilsTest is Test { + RegistryUtils internal wrapper; + + function setUp() public { + wrapper = new RegistryUtils(); + } + + // ------------------------------------------------------------------------- + // Helpers — canonical character classification + // ------------------------------------------------------------------------- + + /// True iff the byte is in the valid subdomain alphabet `[a-z0-9-]`. + function _isValidByte(uint8 c) internal pure returns (bool) { + if (c >= 0x61 && c <= 0x7a) return true; // a-z + if (c >= 0x30 && c <= 0x39) return true; // 0-9 + if (c == 0x2d) return true; // - + return false; + } + + // ------------------------------------------------------------------------- + // Empty + minimal cases + // ------------------------------------------------------------------------- + + function test_isSubdomainValid_emptyIsAccepted() public view { + // Source allows empty by design (see the NatSpec). + assertTrue(wrapper.isSubdomainValid("")); + } + + function test_isSubdomainValid_singleLowercaseLetter() public view { + assertTrue(wrapper.isSubdomainValid("a")); + assertTrue(wrapper.isSubdomainValid("z")); + } + + function test_isSubdomainValid_singleDigit() public view { + assertTrue(wrapper.isSubdomainValid("0")); + assertTrue(wrapper.isSubdomainValid("9")); + } + + function test_isSubdomainValid_singleHyphen() public view { + assertTrue(wrapper.isSubdomainValid("-")); + } + + function test_isSubdomainValid_singleUppercaseLetter() public view { + assertFalse(wrapper.isSubdomainValid("A")); + assertFalse(wrapper.isSubdomainValid("Z")); + } + + function test_isSubdomainValid_singleUnderscore() public view { + assertFalse(wrapper.isSubdomainValid("_")); + } + + // ------------------------------------------------------------------------- + // Mixed valid alphabet + // ------------------------------------------------------------------------- + + function test_isSubdomainValid_allValidClassesMixed() public view { + // Letters + digits + hyphen, no invalid chars. + assertTrue(wrapper.isSubdomainValid("alice-123")); + assertTrue(wrapper.isSubdomainValid("a-b-c-1-2-3")); + assertTrue(wrapper.isSubdomainValid("0a")); + assertTrue(wrapper.isSubdomainValid("a0")); + } + + function test_isSubdomainValid_leadingAndTrailingHyphenAccepted() public view { + // The source allows hyphen at any position. ENS itself may disallow + // leading/trailing hyphens, but `isSubdomainValid` is purely character- + // class validation. Lock this in. + assertTrue(wrapper.isSubdomainValid("-alice")); + assertTrue(wrapper.isSubdomainValid("alice-")); + assertTrue(wrapper.isSubdomainValid("-alice-")); + assertTrue(wrapper.isSubdomainValid("--")); + } + + // ------------------------------------------------------------------------- + // Exhaustive ASCII scan, short name (< 32 bytes) + // ------------------------------------------------------------------------- + + function test_isSubdomainValid_asciiScanShortName() public view { + bytes memory base = bytes("this-is-my-super-valid-name"); + for (uint256 i = 0; i < 128; i++) { + uint8 c = uint8(i); + // Splice `c` into position 10 of the base name. + bytes memory mutated = bytes(base); + mutated[10] = bytes1(c); + bool expected = _isValidByte(c); + bool actual = wrapper.isSubdomainValid(string(mutated)); + assertEq(actual, expected); + } + } + + // ------------------------------------------------------------------------- + // Exhaustive ASCII scan, long name (> 32 bytes) + // ------------------------------------------------------------------------- + + function test_isSubdomainValid_asciiScanLongName() public view { + bytes memory base = bytes("this-is-my-super-looooooooooooooooooooooooooong-valid-name"); + for (uint256 i = 0; i < 128; i++) { + uint8 c = uint8(i); + // Splice `c` into position 40 of the base name (beyond 32 bytes). + bytes memory mutated = bytes(base); + mutated[40] = bytes1(c); + bool expected = _isValidByte(c); + bool actual = wrapper.isSubdomainValid(string(mutated)); + assertEq(actual, expected); + } + } + + // ------------------------------------------------------------------------- + // Boundary characters — the exact transitions around each allowed range + // ------------------------------------------------------------------------- + + function test_isSubdomainValid_boundaryCharactersAroundAlphabet() public view { + // 0x2c (",") just below "-" (0x2d) + assertFalse(wrapper.isSubdomainValid(",")); + // 0x2e (".") just above "-" + assertFalse(wrapper.isSubdomainValid(".")); + + // 0x2f ("/") just below "0" (0x30) + assertFalse(wrapper.isSubdomainValid("/")); + // 0x3a (":") just above "9" (0x39) + assertFalse(wrapper.isSubdomainValid(":")); + + // 0x60 ("`") just below "a" (0x61) + assertFalse(wrapper.isSubdomainValid("`")); + // 0x7b ("{") just above "z" (0x7a) + assertFalse(wrapper.isSubdomainValid("{")); + } +} From 1728f4547edab440386d51bb20bd8748af96f33a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8r=E2=88=82=C2=A1?= Date: Tue, 19 May 2026 09:08:48 +0000 Subject: [PATCH 09/31] Test: framework utils --- .../utils/ens/ens-subdomain-registry.ts | 564 ------------------ .../utils/interface-based-registry.ts | 131 ---- .../utils/InterfaceBasedRegistry.t.sol | 143 +++++ .../utils/ens/ENSSubdomainRegistrar.t.sol | 269 +++++++++ 4 files changed, 412 insertions(+), 695 deletions(-) delete mode 100644 packages/contracts/test/framework/utils/ens/ens-subdomain-registry.ts delete mode 100644 packages/contracts/test/framework/utils/interface-based-registry.ts create mode 100644 test/framework/utils/InterfaceBasedRegistry.t.sol create mode 100644 test/framework/utils/ens/ENSSubdomainRegistrar.t.sol diff --git a/packages/contracts/test/framework/utils/ens/ens-subdomain-registry.ts b/packages/contracts/test/framework/utils/ens/ens-subdomain-registry.ts deleted file mode 100644 index a82fddfbe..000000000 --- a/packages/contracts/test/framework/utils/ens/ens-subdomain-registry.ts +++ /dev/null @@ -1,564 +0,0 @@ -import { - ENSSubdomainRegistrar, - DAO, - PublicResolver, - ENSRegistry, - ENSRegistry__factory, - PublicResolver__factory, - ENSSubdomainRegistrar__factory, -} from '../../../../typechain'; -import {ENSSubdomainRegistrar__factory as ENSSubdomainRegistrar_V1_0_0__factory} from '../../../../typechain/@aragon/osx-v1.0.1/framework/utils/ens/ENSSubdomainRegistrar.sol'; -import {ENSSubdomainRegistrar__factory as ENSSubdomainRegistrar_V1_3_0__factory} from '../../../../typechain/@aragon/osx-v1.3.0/framework/utils/ens/ENSSubdomainRegistrar.sol'; -import {ensDomainHash, ensLabelHash} from '../../../../utils/ens'; -import {deployNewDAO} from '../../../test-utils/dao'; -import {setupResolver} from '../../../test-utils/ens'; -import {osxContractsVersion} from '../../../test-utils/protocol-version'; -import { - deployAndUpgradeFromToCheck, - deployAndUpgradeSelfCheck, -} from '../../../test-utils/uups-upgradeable'; -import {ARTIFACT_SOURCES} from '../../../test-utils/wrapper'; -import { - ENS_REGISTRAR_PERMISSIONS, - getProtocolVersion, -} from '@aragon/osx-commons-sdk'; -import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; -import {expect} from 'chai'; -import {ContractFactory} from 'ethers'; -import hre, {ethers} from 'hardhat'; - -// Setup ENS with signers[0] owning the ENS root node (''), the resolver node ('resolver'), the managing DAO, and the subdomain registrar -async function setupENS( - owner: SignerWithAddress -): Promise<[ENSRegistry, PublicResolver, DAO, ENSSubdomainRegistrar]> { - // Deploy the ENSRegistry - const ens = await hre.wrapper.deploy('ENSRegistry'); - - // Deploy the Resolver - const resolver = await hre.wrapper.deploy('PublicResolver', { - args: [ens.address, ethers.constants.AddressZero], - }); - - await setupResolver(ens, resolver, owner); - - // Deploy the managing DAO - const dao = await deployNewDAO(owner); - - // Deploy the registrar - const registrar = await hre.wrapper.deploy( - ARTIFACT_SOURCES.ENS_SUBDOMAIN_REGISTRAR, - { - withProxy: true, - } - ); - - return [ens, resolver, dao, registrar]; -} - -describe('ENSSubdomainRegistrar', function () { - let signers: SignerWithAddress[]; - let managingDao: DAO; - let ens: ENSRegistry; - let resolver: PublicResolver; - let registrar: ENSSubdomainRegistrar; - - // A Helper function to register a subdomain under parent domain - async function registerSubdomainHelper( - subdomain: string, - domain: string, - domainOwner: SignerWithAddress, - subdomainOwnerAddress: string - ) { - let fullDomain: string; - if (domain === '') { - fullDomain = subdomain; - } else { - fullDomain = subdomain + '.' + domain; - } - - let tx = await ens - .connect(domainOwner) - .setSubnodeRecord( - ensDomainHash(domain), - ensLabelHash(subdomain), - subdomainOwnerAddress, - resolver.address, - 0 - ); - await tx.wait(); - - // Verify that the subdomain is owned by the correct address - expect(await ens.owner(ensDomainHash(fullDomain))).to.equal( - subdomainOwnerAddress - ); - // Verify that that the subdomain's resolver address is set correctly - expect(await ens.resolver(ensDomainHash(fullDomain))).to.equal( - resolver.address - ); - } - - before(async function () { - signers = await ethers.getSigners(); - }); - - beforeEach(async function () { - [ens, resolver, managingDao, registrar] = await setupENS(signers[0]); - }); - - describe('Check the initial ENS state', async () => { - it('unregistered domains are owned by the zero address on ENS', async () => { - expect(await ens.owner(ensDomainHash('test'))).to.equal( - ethers.constants.AddressZero - ); - }); - - it('unregistered domains resolve to the zero address on ENS', async () => { - expect(await resolver['addr(bytes32)'](ensDomainHash('test'))).to.equal( - ethers.constants.AddressZero - ); - }); - }); - - describe('Registrar is the domain owner but not approved', () => { - beforeEach(async () => { - // Register the parent domain 'test' through signers[0] who owns the ENS root node ('') and make the subdomain registrar the owner - await registerSubdomainHelper('test', '', signers[0], registrar.address); - }); - - it('initializes correctly', async () => { - expect( - await registrar - .connect(signers[0]) - .initialize(managingDao.address, ens.address, ensDomainHash('test')) - ).to.not.be.revertedWithCustomError(registrar, 'InvalidResolver'); - }); - - postInitializationTests(); - - it('reverts if the registrar do not have the ownership of the domain node', async () => { - // Register the parent domain 'test2' through signers[0] who owns the ENS root node ('') and make the subdomain registrar the owner - await registerSubdomainHelper( - 'test2', - '', - signers[0], - signers[0].address - ); - - // Initialize the registrar with the 'test' domain - await registrar.initialize( - managingDao.address, - ens.address, - ensDomainHash('test2') - ); - - // Grant signers[1] the `REGISTER_ENS_SUBDOMAIN_PERMISSION_ID` permission - await managingDao.grant( - registrar.address, - await signers[1].getAddress(), - ENS_REGISTRAR_PERMISSIONS.REGISTER_ENS_SUBDOMAIN_PERMISSION_ID - ); - - // signers[0] can't register subdomains - await expect( - registrar - .connect(signers[1]) - .registerSubnode(ensLabelHash('my1'), await signers[1].getAddress()) - ).to.be.reverted; - }); - - it('reverts if the ownership of the domain node is removed from the registrar', async () => { - // Initialize the registrar with the 'test' domain - await registrar.initialize( - managingDao.address, - ens.address, - ensDomainHash('test') - ); - - // Grant signers[1] the `REGISTER_ENS_SUBDOMAIN_PERMISSION_ID` permission - await managingDao.grant( - registrar.address, - await signers[1].getAddress(), - ENS_REGISTRAR_PERMISSIONS.REGISTER_ENS_SUBDOMAIN_PERMISSION_ID - ); - - // signers[1] can register subdomain - expect( - await registrar - .connect(signers[1]) - .registerSubnode(ensLabelHash('my1'), await signers[1].getAddress()) - ); - - // Remove ownership of 'test' from the registrar contract address through the parent domain node owner - await ens - .connect(signers[0]) - .setSubnodeOwner( - ensDomainHash(''), - ensLabelHash('test'), - await signers[0].getAddress() - ); - - // signers[1] can't register subdomains anymore - await expect( - registrar - .connect(signers[1]) - .registerSubnode(ensLabelHash('my2'), await signers[1].getAddress()) - ).to.be.reverted; - }); - }); - - describe('Registrar is not the domain owner but it is approved', () => { - beforeEach(async () => { - // Register the parent domain 'test' through signers[0] who owns the ENS root node ('') and make the signers[0] the owner - await registerSubdomainHelper('test', '', signers[0], signers[0].address); - - // Approve the subdomain registrar contract address to operate for signers[0] (who owns 'test') - await ens.connect(signers[0]).setApprovalForAll(registrar.address, true); - }); - - it('initializes correctly', async () => { - expect( - await registrar - .connect(signers[0]) - .initialize(managingDao.address, ens.address, ensDomainHash('test')) - ); - - // the default resolver is the resolver of the parent domain node - expect(await registrar.resolver()).to.equal(resolver.address); - }); - - postInitializationTests(); - - it('reverts if the approval of the registrar is removed', async () => { - // Initialize the registrar with the 'test' domain - await registrar.initialize( - managingDao.address, - ens.address, - ensDomainHash('test') - ); - - // Grant signers[1] the `REGISTER_ENS_SUBDOMAIN_PERMISSION_ID` permission - await managingDao.grant( - registrar.address, - await signers[1].getAddress(), - ENS_REGISTRAR_PERMISSIONS.REGISTER_ENS_SUBDOMAIN_PERMISSION_ID - ); - - // signers[1] can register subdomain - expect( - await registrar - .connect(signers[1]) - .registerSubnode(ensLabelHash('my1'), await signers[1].getAddress()) - ); - - // Remove approval of the registrar to manage all domains owned by signers[0] including 'test' - await ens.connect(signers[0]).setApprovalForAll(registrar.address, false); - - // signers[1] can't register subdomains anymore - await expect( - registrar - .connect(signers[1]) - .registerSubnode(ensLabelHash('my2'), await signers[1].getAddress()) - ).to.be.reverted; - }); - }); - - describe('Registrar is not the domain owner and is not approved but has permission', () => { - beforeEach(async () => { - // Grant signers[1] the `REGISTER_ENS_SUBDOMAIN_PERMISSION_ID` permission - await managingDao.grant( - registrar.address, - await signers[1].getAddress(), - ENS_REGISTRAR_PERMISSIONS.REGISTER_ENS_SUBDOMAIN_PERMISSION_ID - ); - }); - - expectedReverts(); - }); - - describe('Random signer with no permissions at all', () => { - expectedReverts(); - }); - - describe('Upgrades', () => { - let legacyContractFactory: ContractFactory; - let currentContractFactory: ContractFactory; - let initArgs: any; - - before(() => { - currentContractFactory = new ENSSubdomainRegistrar__factory(signers[0]); - }); - - beforeEach(async () => { - await registerSubdomainHelper('test', '', signers[0], registrar.address); - - initArgs = { - managingDao: managingDao.address, - ens: ens.address, - parentDomain: ensDomainHash('test'), - }; - }); - - it('upgrades to a new implementation', async () => { - await deployAndUpgradeSelfCheck( - 0, - 1, - { - initArgs: initArgs, - initializer: 'initialize', - }, - ARTIFACT_SOURCES.ENS_SUBDOMAIN_REGISTRAR, - ARTIFACT_SOURCES.ENS_SUBDOMAIN_REGISTRAR, - ENS_REGISTRAR_PERMISSIONS.UPGRADE_REGISTRAR_PERMISSION_ID, - managingDao - ); - }); - - it('upgrades from v1.0.0', async () => { - legacyContractFactory = new ENSSubdomainRegistrar_V1_0_0__factory( - signers[0] - ); - - const {fromImplementation, toImplementation} = - await deployAndUpgradeFromToCheck( - 0, - 1, - { - initArgs: initArgs, - initializer: 'initialize', - }, - ARTIFACT_SOURCES.ENS_SUBDOMAIN_REGISTRAR_V1_0_0, - ARTIFACT_SOURCES.ENS_SUBDOMAIN_REGISTRAR, - ENS_REGISTRAR_PERMISSIONS.UPGRADE_REGISTRAR_PERMISSION_ID, - managingDao - ); - expect(toImplementation).to.not.equal(fromImplementation); - - const fromProtocolVersion = await getProtocolVersion( - legacyContractFactory.attach(fromImplementation) - ); - const toProtocolVersion = await getProtocolVersion( - currentContractFactory.attach(toImplementation) - ); - - expect(fromProtocolVersion).to.not.deep.equal(toProtocolVersion); - expect(fromProtocolVersion).to.deep.equal([1, 0, 0]); - expect(toProtocolVersion).to.deep.equal(osxContractsVersion()); - }); - - it('from v1.3.0', async () => { - legacyContractFactory = new ENSSubdomainRegistrar_V1_3_0__factory( - signers[0] - ); - - const {fromImplementation, toImplementation} = - await deployAndUpgradeFromToCheck( - 0, - 1, - { - initArgs: initArgs, - initializer: 'initialize', - }, - ARTIFACT_SOURCES.ENS_SUBDOMAIN_REGISTRAR_V1_3_0, - ARTIFACT_SOURCES.ENS_SUBDOMAIN_REGISTRAR, - ENS_REGISTRAR_PERMISSIONS.UPGRADE_REGISTRAR_PERMISSION_ID, - managingDao - ); - expect(toImplementation).to.not.equal(fromImplementation); - - const fromProtocolVersion = await getProtocolVersion( - legacyContractFactory.attach(fromImplementation) - ); - const toProtocolVersion = await getProtocolVersion( - currentContractFactory.attach(toImplementation) - ); - - expect(fromProtocolVersion).to.not.deep.equal(toProtocolVersion); - expect(fromProtocolVersion).to.deep.equal([1, 0, 0]); - expect(toProtocolVersion).to.deep.equal(osxContractsVersion()); - }); - }); - - function expectedReverts() { - it('reverts during initialization if node does not have a valid resolver', async () => { - await expect( - registrar - .connect(signers[1]) - .initialize(managingDao.address, ens.address, ensDomainHash('test2')) - ) - .to.be.revertedWithCustomError(registrar, 'InvalidResolver') - .withArgs(ensDomainHash('test2'), ethers.constants.AddressZero); - }); - - it('reverts on attempted subnode registration', async () => { - // signers[1] can register subdomain - await expect( - registrar - .connect(signers[1]) - .registerSubnode(ensLabelHash('my'), await signers[1].getAddress()) - ).to.be.reverted; - }); - - it('reverts on attempted default resolver setting', async () => { - const newResolverAddr = ethers.constants.AddressZero; - - // signers[1] can register subdomain - await expect( - registrar.connect(signers[1]).setDefaultResolver(newResolverAddr) - ).to.be.reverted; - }); - } - - function postInitializationTests() { - describe('After registrar initialization', () => { - beforeEach(async () => { - // Initialize the registrar with the 'test' domain - await registrar.initialize( - managingDao.address, - ens.address, - ensDomainHash('test') - ); - }); - - it('reverts if initialized a second time', async () => { - await expect( - registrar.initialize( - managingDao.address, - ens.address, - ensDomainHash('foo') - ) - ).to.be.revertedWith('Initializable: contract is already initialized'); - }); - - it('reverts subnode registration if the calling address lacks permission of the managing DAO', async () => { - const targetAddress = managingDao.address; - - // Register the subdomain 'my.test' as signers[1] who does not have the `REGISTER_ENS_SUBDOMAIN_PERMISSION_ID` granted - await expect( - registrar - .connect(signers[1]) - .registerSubnode(ensLabelHash('my'), targetAddress) - ) - .to.be.revertedWithCustomError(registrar, 'DaoUnauthorized') - .withArgs( - managingDao.address, - registrar.address, - signers[1].address, - ENS_REGISTRAR_PERMISSIONS.REGISTER_ENS_SUBDOMAIN_PERMISSION_ID - ); - }); - - it('reverts setting the resolver if the calling address lacks permission of the managing DAO', async () => { - // Set a new resolver as signers[1] who does not have the `REGISTER_ENS_SUBDOMAIN_PERMISSION_ID` granted - await expect( - registrar - .connect(signers[1]) - .setDefaultResolver(ethers.constants.AddressZero) - ) - .to.be.revertedWithCustomError(registrar, 'DaoUnauthorized') - .withArgs( - managingDao.address, - registrar.address, - signers[1].address, - ENS_REGISTRAR_PERMISSIONS.REGISTER_ENS_SUBDOMAIN_PERMISSION_ID - ); - }); - - describe('After granting permission to the calling address via the managing DAO', () => { - beforeEach(async () => { - // Grant signers[1] and signers[2] the `REGISTER_ENS_SUBDOMAIN_PERMISSION_ID` permission - await managingDao.grant( - registrar.address, - await signers[1].getAddress(), - ENS_REGISTRAR_PERMISSIONS.REGISTER_ENS_SUBDOMAIN_PERMISSION_ID - ); - await managingDao.grant( - registrar.address, - await signers[2].getAddress(), - ENS_REGISTRAR_PERMISSIONS.REGISTER_ENS_SUBDOMAIN_PERMISSION_ID - ); - }); - - it('registers the subdomain and resolves to the target address', async () => { - // register 'my.test' as signers[1] and set it to resovle to the target address - const targetAddress = managingDao.address; - let tx = await registrar - .connect(signers[1]) - .registerSubnode(ensLabelHash('my'), targetAddress); - await tx.wait(); - - // Check that the subdomain is still owned by the subdomain registrar - expect(await ens.owner(ensDomainHash('my.test'))).to.equal( - registrar.address - ); - - // Check that the subdomain resolves to the target address - expect( - await resolver['addr(bytes32)'](ensDomainHash('my.test')) - ).to.equal(targetAddress); - }); - - it('reverts subnode registration if the subdomain was already registered before', async () => { - // register 'my.test' as signers[1] and set it to resovle to the target address - const targetAddress = managingDao.address; - let tx = await registrar - .connect(signers[1]) - .registerSubnode(ensLabelHash('my'), targetAddress); - await tx.wait(); - - // try to register the same subnode again as signers[2] - await expect( - registrar - .connect(signers[2]) - .registerSubnode( - ensLabelHash('my'), - await signers[2].getAddress() - ) - ) - .to.be.revertedWithCustomError(registrar, 'AlreadyRegistered') - .withArgs(ensDomainHash('my.test'), registrar.address); - }); - - it('reverts subnode registration if the subdomain was already registered before, also for the same caller', async () => { - // register 'my.test' as signers[1] and set it to resovle to the target address - const targetAddress = managingDao.address; - let tx = await registrar - .connect(signers[1]) - .registerSubnode(ensLabelHash('my'), targetAddress); - await tx.wait(); - - // try to register the same subnode again as signers[1] - await expect( - registrar - .connect(signers[1]) - .registerSubnode( - ensLabelHash('my'), - await signers[1].getAddress() - ) - ) - .to.be.revertedWithCustomError(registrar, 'AlreadyRegistered') - .withArgs(ensDomainHash('my.test'), registrar.address); - }); - - it('revert if invalid resolver is set', async () => { - const newResolverAddr = ethers.constants.AddressZero; - - await expect( - registrar.connect(signers[1]).setDefaultResolver(newResolverAddr) - ) - .to.be.revertedWithCustomError(registrar, 'InvalidResolver') - .withArgs(ensDomainHash('test'), newResolverAddr); - }); - - it('sets the resolver correctly', async () => { - const newResolverAddr = await signers[8].getAddress(); - let tx = await registrar - .connect(signers[1]) - .setDefaultResolver(newResolverAddr); - await tx.wait(); - - expect(await registrar.resolver()).to.equal(newResolverAddr); - }); - }); - }); - } -}); diff --git a/packages/contracts/test/framework/utils/interface-based-registry.ts b/packages/contracts/test/framework/utils/interface-based-registry.ts deleted file mode 100644 index 25cbb5950..000000000 --- a/packages/contracts/test/framework/utils/interface-based-registry.ts +++ /dev/null @@ -1,131 +0,0 @@ -import { - DAO, - IDAO__factory, - InterfaceBasedRegistryMock, - InterfaceBasedRegistryMock__factory, - PluginRepo__factory, -} from '../../../typechain'; -import {deployNewDAO} from '../../test-utils/dao'; -import {ARTIFACT_SOURCES} from '../../test-utils/wrapper'; -import {getInterfaceId} from '@aragon/osx-commons-sdk'; -import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; -import {expect} from 'chai'; -import hre, {ethers} from 'hardhat'; - -const REGISTER_PERMISSION_ID = ethers.utils.id('REGISTER_PERMISSION'); - -const EVENTS = { - Registered: 'Registered', -}; - -describe('InterfaceBasedRegistry', function () { - let signers: SignerWithAddress[]; - let interfaceBasedRegistryMock: InterfaceBasedRegistryMock; - let dao: DAO; - let ownerAddress: string; - - before(async () => { - signers = await ethers.getSigners(); - ownerAddress = await signers[0].getAddress(); - - // DAO - dao = await deployNewDAO(signers[0]); - }); - - beforeEach(async () => { - interfaceBasedRegistryMock = await hre.wrapper.deploy( - 'InterfaceBasedRegistryMock', - {withProxy: true} - ); - - // Let the interface registry register `DAO` contracts for testing purposes - await interfaceBasedRegistryMock.initialize( - dao.address, - getInterfaceId(IDAO__factory.createInterface()) - ); - - // grant REGISTER_PERMISSION_ID to registrer - await dao.grant( - interfaceBasedRegistryMock.address, - ownerAddress, - REGISTER_PERMISSION_ID - ); - }); - - describe('Register', async () => { - it('fail if registrant address is not a contract', async function () { - const randomAddress = await signers[8].getAddress(); - - await expect(interfaceBasedRegistryMock.register(randomAddress)) - .to.be.revertedWithCustomError( - interfaceBasedRegistryMock, - 'ContractInterfaceInvalid' - ) - .withArgs(randomAddress); - }); - - it('fail to register if the interface is not supported', async () => { - // Use the `PluginRepo` contract for testing purposes here, because the interface differs from the `DAO` interface - let contractNotBeingADao = await hre.wrapper.deploy( - ARTIFACT_SOURCES.PLUGIN_REPO - ); - - await expect( - interfaceBasedRegistryMock.register(contractNotBeingADao.address) - ) - .to.be.revertedWithCustomError( - interfaceBasedRegistryMock, - 'ContractInterfaceInvalid' - ) - .withArgs(contractNotBeingADao.address); - }); - - it('fail to register if the sender lacks the required permissionId', async () => { - await dao.revoke( - interfaceBasedRegistryMock.address, - ownerAddress, - REGISTER_PERMISSION_ID - ); - - await expect(interfaceBasedRegistryMock.register(dao.address)) - .to.be.revertedWithCustomError( - interfaceBasedRegistryMock, - 'DaoUnauthorized' - ) - .withArgs( - dao.address, - interfaceBasedRegistryMock.address, - ownerAddress, - REGISTER_PERMISSION_ID - ); - }); - - it('fail to register if the contract is already registered', async () => { - // contract is now registered - await interfaceBasedRegistryMock.register(dao.address); - - // try to register the same contract again - await expect(interfaceBasedRegistryMock.register(dao.address)) - .to.be.revertedWithCustomError( - interfaceBasedRegistryMock, - 'ContractAlreadyRegistered' - ) - .withArgs(dao.address); - }); - - it('register a contract with known interface', async () => { - // make sure the address is not already registered - expect(await interfaceBasedRegistryMock.entries(dao.address)).to.equal( - false - ); - - await expect(interfaceBasedRegistryMock.register(dao.address)) - .to.emit(interfaceBasedRegistryMock, EVENTS.Registered) - .withArgs(dao.address); - - expect(await interfaceBasedRegistryMock.entries(dao.address)).to.equal( - true - ); - }); - }); -}); diff --git a/test/framework/utils/InterfaceBasedRegistry.t.sol b/test/framework/utils/InterfaceBasedRegistry.t.sol new file mode 100644 index 000000000..cee4ec063 --- /dev/null +++ b/test/framework/utils/InterfaceBasedRegistry.t.sol @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test, Vm} from "forge-std/Test.sol"; +import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import {InterfaceBasedRegistry} from "../../../src/framework/utils/InterfaceBasedRegistry.sol"; +import {IDAO} from "../../../src/common/dao/IDAO.sol"; +import {DaoUnauthorized} from "../../../src/common/permission/auth/auth.sol"; +import {InterfaceBasedRegistryMock} from "../../mocks/utils/InterfaceBasedRegistryMock.sol"; +import {DAOMock} from "../../mocks/commons/dao/DAOMock.sol"; + +/// @dev A contract that ERC-165-claims to be `IDAO`. Stand-in for the real +/// `DAO` contract used by the upstream TS test as a "valid registrant". +contract IDAOClaimer { + function supportsInterface(bytes4 _interfaceId) external pure returns (bool) { + return _interfaceId == type(IDAO).interfaceId || _interfaceId == type(IERC165).interfaceId; + } +} + +/// @dev A contract that ERC-165 supports `IERC165` but not `IDAO`. Stand-in +/// for `PluginRepo` in the upstream test — passes the contract-existence +/// check but fails the target-interface check. +contract WrongInterfaceClaimer { + function supportsInterface(bytes4 _interfaceId) external pure returns (bool) { + return _interfaceId == type(IERC165).interfaceId; + } +} + +/// @notice Direct tests for the `InterfaceBasedRegistry` abstract contract in +/// `src/framework/utils/InterfaceBasedRegistry.sol`. +/// +/// Ports `packages/contracts/test/framework/utils/interface-based-registry.ts` +/// (131 lines, 5 cases). Adds: `targetInterfaceId` getter state, explicit +/// `entries(addr)` state mutation check, distinct revert path coverage +/// (`ContractInterfaceInvalid` for both EOA and wrong-interface registrants). +contract InterfaceBasedRegistryTest is Test { + bytes32 internal constant REGISTER_PERMISSION_ID = keccak256("REGISTER_PERMISSION"); + + DAOMock internal daoMock; + InterfaceBasedRegistryMock internal registry; + address internal alice; + + function setUp() public { + alice = makeAddr("alice"); + daoMock = new DAOMock(); + // Default-allow so the caller of `register` clears the auth gate; tests + // that exercise the auth path flip this off explicitly. + daoMock.setHasPermissionReturnValueMock(true); + + // Deploy the registry behind a UUPS proxy (required by the + // `Initializable` base) and initialize with IDAO as the target iface. + InterfaceBasedRegistryMock impl = new InterfaceBasedRegistryMock(); + bytes memory initCalldata = abi.encodeCall(impl.initialize, (IDAO(address(daoMock)), type(IDAO).interfaceId)); + registry = InterfaceBasedRegistryMock(address(new ERC1967Proxy(address(impl), initCalldata))); + } + + // ------------------------------------------------------------------------- + // Init / view state + // ------------------------------------------------------------------------- + + function test_init_storesTargetInterfaceId() public view { + assertEq(registry.targetInterfaceId(), type(IDAO).interfaceId); + } + + function test_init_storesDaoAddress() public view { + assertEq(address(registry.dao()), address(daoMock)); + } + + function test_entries_defaultsToFalse() public view { + assertFalse(registry.entries(address(0xBEEF))); + } + + // ------------------------------------------------------------------------- + // Register — revert paths + // ------------------------------------------------------------------------- + + function test_register_revertsIfRegistrantIsEOA() public { + // The OZ ERC165Checker safe-staticcalls into the registrant; a call + // against an EOA fails its contract-code check and returns false. + address eoa = makeAddr("eoa"); + vm.expectRevert(abi.encodeWithSelector(InterfaceBasedRegistry.ContractInterfaceInvalid.selector, eoa)); + registry.register(eoa); + } + + function test_register_revertsIfTargetInterfaceUnsupported() public { + WrongInterfaceClaimer wrong = new WrongInterfaceClaimer(); + vm.expectRevert( + abi.encodeWithSelector(InterfaceBasedRegistry.ContractInterfaceInvalid.selector, address(wrong)) + ); + registry.register(address(wrong)); + } + + function test_register_revertsIfCallerLacksRegisterPermission() public { + daoMock.setHasPermissionReturnValueMock(false); + IDAOClaimer claimer = new IDAOClaimer(); + + vm.expectRevert( + abi.encodeWithSelector( + DaoUnauthorized.selector, address(daoMock), address(registry), alice, REGISTER_PERMISSION_ID + ) + ); + vm.prank(alice); + registry.register(address(claimer)); + } + + function test_register_revertsIfAlreadyRegistered() public { + IDAOClaimer claimer = new IDAOClaimer(); + registry.register(address(claimer)); + vm.expectRevert( + abi.encodeWithSelector(InterfaceBasedRegistry.ContractAlreadyRegistered.selector, address(claimer)) + ); + registry.register(address(claimer)); + } + + // ------------------------------------------------------------------------- + // Register — happy path + // ------------------------------------------------------------------------- + + function test_register_storesEntryAndEmitsEvent() public { + IDAOClaimer claimer = new IDAOClaimer(); + assertFalse(registry.entries(address(claimer))); + + vm.recordLogs(); + registry.register(address(claimer)); + Vm.Log[] memory logs = vm.getRecordedLogs(); + + bytes32 expectedTopic = keccak256("Registered(address)"); + bool found; + for (uint256 i = 0; i < logs.length; i++) { + if (logs[i].emitter == address(registry) && logs[i].topics[0] == expectedTopic) { + address emitted = abi.decode(logs[i].data, (address)); + assertEq(emitted, address(claimer)); + found = true; + break; + } + } + assertTrue(found, "Registered event not emitted"); + + assertTrue(registry.entries(address(claimer))); + } +} diff --git a/test/framework/utils/ens/ENSSubdomainRegistrar.t.sol b/test/framework/utils/ens/ENSSubdomainRegistrar.t.sol new file mode 100644 index 000000000..d1ac9d3ac --- /dev/null +++ b/test/framework/utils/ens/ENSSubdomainRegistrar.t.sol @@ -0,0 +1,269 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.17; + +import {Test} from "forge-std/Test.sol"; +import {ENS} from "@ensdomains/ens-contracts/contracts/registry/ENS.sol"; +import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import {ENSSubdomainRegistrar} from "../../../../src/framework/utils/ens/ENSSubdomainRegistrar.sol"; +import {IDAO} from "../../../../src/common/dao/IDAO.sol"; +import {DaoUnauthorized} from "../../../../src/common/permission/auth/auth.sol"; +import {DAOMock} from "../../../mocks/commons/dao/DAOMock.sol"; +import {MockENS} from "../../member/mocks/MockENS.sol"; +import {MockResolver} from "../../member/mocks/MockResolver.sol"; + +/// @notice Direct tests for `ENSSubdomainRegistrar` in +/// `src/framework/utils/ens/ENSSubdomainRegistrar.sol`. +/// +/// Ports `packages/contracts/test/framework/utils/ens/ens-subdomain-registry.ts` +/// (564 lines, 28 cases). Re-uses the local `MockENS` + `MockResolver` from the +/// MemberRegistry test scaffolding so no fork is required. Adds: explicit +/// initial-state assertions, multiple-subnode registration in sequence, +/// `node()` / `ens()` / `resolver()` getter snapshots, and the empty-string +/// edge for `setDefaultResolver`. The Upgrade sub-block is owned by the +/// component 31 fork test (needs `lib/osx-v1.0.0` + `lib/osx-v1.3.0`). +contract ENSSubdomainRegistrarTest is Test { + bytes32 internal constant REGISTER_PERMISSION_ID = keccak256("REGISTER_ENS_SUBDOMAIN_PERMISSION"); + + // namehash("test") — verified via `cast namehash test`. + bytes32 internal constant TEST_NODE = 0x04f740db81dc36c853ab4205bddd785f46e79ccedca351fc6dfcbd8cc9a33dd6; + // namehash("test2") + bytes32 internal constant TEST2_NODE = 0x4e40f6e0b682912885261b48c6a9ba4f76aac8f74cb47354d0508b49a6c988d8; + // namehash("my.test") + bytes32 internal constant MY_TEST_NODE = 0x8834dc600444c280d7c51f15bc14777069771166fd9427bb40f11ab21bc00bbc; + + bytes32 internal constant TEST_LABEL = keccak256("test"); + bytes32 internal constant TEST2_LABEL = keccak256("test2"); + bytes32 internal constant MY_LABEL = keccak256("my"); + bytes32 internal constant MY2_LABEL = keccak256("my2"); + + DAOMock internal managingDao; + MockENS internal ens; + MockResolver internal resolver; + ENSSubdomainRegistrar internal registrar; + ENSSubdomainRegistrar internal impl; + + address internal alice = makeAddr("alice"); + address internal bob = makeAddr("bob"); + address internal carol = makeAddr("carol"); + address internal target = makeAddr("target"); + + function setUp() public { + managingDao = new DAOMock(); + managingDao.setHasPermissionReturnValueMock(true); + + ens = new MockENS(); + ens.setOwner(bytes32(0), address(this)); + resolver = new MockResolver(ENS(address(ens))); + + impl = new ENSSubdomainRegistrar(); + registrar = ENSSubdomainRegistrar(address(new ERC1967Proxy(address(impl), ""))); + } + + /// Register the parent domain `