Porting the TypeScript tests into solidity

[brickpop]

Description

• Porting the testing suite from TypeScript /HardHat) into native solidity (Foundry)
• Consolidating OSx + the former osx-commons
• Extending the testing coverage (unit, integration, smoke)

Also:

• Syncing the docs (to pass the checks)
• Syncing the ABI (to pass the checks)
• Syncing just-foundry

[xavikh]

Solid port overall: builds clean, 896 tests pass. Detailed findings are inline. One thing up front that needs a call before merge:

Managing-DAO post-deployment checks were dropped with no replacement. The old deploy/managing-dao.ts (7 cases) verified the live managing DAO holds ROOT on itself and upgrade permission over the registries/registrars. A fork test against mainnet is the natural home (the pattern already exists in test/framework/member/fork/). Land it here or as a follow-up?

[xavikh]

P0 — registries have zero upgrade-path coverage. DAORegistry and PluginRepoRegistry are live, managing-DAO-controlled UUPS proxies (both inherit InterfaceBasedRegistry, gated by UPGRADE_REGISTRY_PERMISSION_ID, _authorizeUpgrade at InterfaceBasedRegistry.sol:55-57) — but neither test file has any upgrade test. The old suite ran a shared UUPS helper against every framework proxy; only the DAO kept it (test-upgrade/DAOUpgrade.t.sol).

(a) Auth-gate tests — add to these files (both already use a DAOMock with setHasPermissionReturnValueMock), mirroring ENSSubdomainRegistrar.t.sol:341-362:

• test_authorizeUpgrade_revertsWithoutPermission: mock false, prank alice, upgradeTo(newImpl) → DaoUnauthorized(...UPGRADE_REGISTRY_PERMISSION_ID).
• test_authorizeUpgrade_succeedsWithPermission: mock true, upgradeTo(newImpl), assert the ERC1967 impl slot == newImpl.

(b) Legacy v1.0.0 / v1.3.0 → current regression — new test-upgrade/DAORegistryUpgrade.t.sol and PluginRepoRegistryUpgrade.t.sol copying DAOUpgrade.t.sol. Rotate v1.0.0→v1.3.0→current, asserting per hop that entries / subdomainRegistrar / targetInterfaceId are preserved and protocolVersion() goes reverting→tuple. These registries define no initializeFrom, so every hop is a bare upgradeTo. Needs just test-upgrade-setup + FOUNDRY_PROFILE=upgrade.

Same legacy-regression gap (lower priority) for PluginRepo and ENSSubdomainRegistrar — their auth gate is tested (PluginRepo.t.sol:827-883, ENSSubdomainRegistrar.t.sol:336-362) but the v1.0.0/v1.3.0 upgrade regression (2 cases each) was dropped. (Note: PluginRepo.initializeFrom is a placeholder that reverts, so its final hop must be a bare upgradeTo.)

[xavikh]

Missing revert & edge-case coverage — five cases the TS suite had and the port dropped:

1. DAO initializeFrom major-version revert — untested. DAO.sol:200-203 reverts ProtocolVersionUpgradeNotSupported when _previousProtocolVersion[0] != 1; no test hits it. Add to DAO.t.sol: deploy a DAO impl behind an ERC1967Proxy without initialize (so _initialized==0, satisfying reinitializer(3)), then

   vm.expectRevert(abi.encodeWithSelector(DAO.ProtocolVersionUpgradeNotSupported.selector, [uint8(2),0,0]));
   dao.initializeFrom([uint8(2),0,0], "");

   plus a [1,3,0] positive control.
2. _reentrancyStatus after pre-1.3.0 upgrade — not asserted. DAO.sol:206-211 sets it to _NOT_ENTERED for pre-1.3.0 sources. In test-upgrade/DAOUpgrade.t.sol, after _upgradeTo_v1_3_0():

   assertEq(uint256(vm.load(proxy, bytes32(uint256(304)))), 1, "reentrancy guard == _NOT_ENTERED");

3. PSP applyUpdate stale-preparation — dropped. The uninstall analog was ported (PSP.Uninstallation.t.sol:245 test_applyUninstallation_otherPendingPrepsBecomeInapplicable); the update equivalent is missing. Add test_applyUpdate_otherPendingPrepsBecomeInapplicable: prepare update A (setupV2.mockPermissionIndexes(1,2)) and B ((3,4)) for the same plugin, apply A, then assert applying B reverts SetupNotApplicable (B's preparedBlock ≤ the bumped pluginState.blockNumber).
4. prepareInstallation PluginAlreadyInstalled branch — untested. PSP.sol:323 (preparing an install for an already-installed plugin) has no test. The TS suite asserted PluginAlreadyInstalled on both the prepare and apply paths.
5. PSP protocolVersion() never asserted (trivial — returns [1,4,0]):

   function test_protocolVersion_returnsCurrent() public view {
       uint8[3] memory v = psp.protocolVersion();
       assertEq(v[0], 1); assertEq(v[1], 4); assertEq(v[2], 0);
   }

[xavikh]

Weak assertions — _assertState checks too little. It verifies only the implementation slot and the appliedSetupId hash. It never observes that the plugin's re-init actually ran or that the update's permissions were actually granted on the DAO — both are only hashed into the setup id. A regression that skips the upgradeToAndCall init data, or skips the permission application, would still pass. Both are directly observable (real DAO under test; plugin exposes public state2()/state3()):

if (expectedBuild >= 2) {
    assertEq(PluginUUPSUpgradeableV3Mock(plugin).state2(), 2, "V2 re-init ran");
    assertTrue(dao.hasPermission(address(1), address(1), keccak256("MOCK_PERMISSION"), ""), "V2 perm granted");
}
if (expectedBuild >= 3) {
    assertEq(PluginUUPSUpgradeableV3Mock(plugin).state3(), 3, "V3 re-init ran");
    assertTrue(dao.hasPermission(address(2), address(2), keccak256("MOCK_PERMISSION"), ""), "V3 perm granted");
}

Apply to test_v1ThenV2_endsAtV2, test_v1ThenV2ThenV3_endsAtV3, test_v1ThenV3_skipsBuild2, test_v2ThenV3_endsAtV3. For test_v3ThenV4_*, V4 grants (address(3), address(3)) with no init data → assert that permission and that state3 is unchanged.

[xavikh]

Tighten bare reverts to typed selectors. vm.expectRevert() with no argument passes on any revert — where a custom error exists, please pin it. Two of these also assert the wrong error:

⚠️ Correctness (not just looseness):

• This line (PSP.Installation.t.sol:241) test_applyInstallation_revertsOnSecondApply — the comment says SetupNotApplicable, but PluginAlreadyInstalled is checked first (PluginSetupProcessor.sol:368, before validatePreparedSetupId at :372). On the second apply, currentAppliedSetupId != 0, so PluginAlreadyInstalled fires and SetupNotApplicable is unreachable. → vm.expectRevert(PluginSetupProcessor.PluginAlreadyInstalled.selector); and fix the comment.
• PSP.Update.t.sol:366 test_prepareUpdate_revertsIfPluginIsCloneable — comment says "PluginNonupgradeable or IPluginNotSupported", but a cloneable plugin does support IPlugin; only PluginNonupgradeable is reachable (PSP.sol:468). → vm.expectRevert(PluginSetupProcessor.PluginNonupgradeable.selector);

PSP.Installation.t.sol

Line	Selector
52	PluginRepo.VersionHashDoesNotExist.selector
134	PluginSetupProcessor.SetupAlreadyPrepared.selector
229	PermissionManager.Unauthorized.selector (reverts in DAO.applyMultiTargetPermissions)
251	PluginSetupProcessor.SetupNotApplicable.selector
296	PluginSetupProcessor.SetupNotApplicable.selector
307	PermissionManager.Unauthorized.selector

PSP.Uninstallation.t.sol

Line	Selector
71, 80, 87	PluginSetupProcessor.InvalidAppliedSetupId.selector
136	PluginSetupProcessor.SetupAlreadyPrepared.selector
192, 269	PluginSetupProcessor.SetupNotApplicable.selector
324	PermissionManager.Unauthorized.selector (needs PermissionManager import — not yet in this file)

PSP.Update.t.sol

Line	Selector
73, 79, 87	PluginSetupProcessor.InvalidUpdateVersion.selector (args are literal Tags if you want full abi.encodeWithSelector)
96, 104, 111	PluginSetupProcessor.InvalidAppliedSetupId.selector
141	PluginSetupProcessor.SetupAlreadyPrepared.selector
207	PluginSetupProcessor.SetupNotApplicable.selector

PermissionManager.t.sol

Line	Selector
850	PermissionManager.PermissionAlreadyGrantedForDifferentCondition.selector
860	PermissionManager.ConditionNotAContract.selector
952	PermissionManager.GrantWithConditionNotSupported.selector

Factories / registry (args are constructor-derived → use vm.expectPartialRevert):

Line	Selector
DAOFactory.t.sol:493	vm.expectPartialRevert(ENSSubdomainRegistrar.AlreadyRegistered.selector)
DAOFactory.t.sol:508	vm.expectPartialRevert(PluginRepo.VersionHashDoesNotExist.selector)
PluginRepoFactory.t.sol:303	vm.expectPartialRevert(ENSSubdomainRegistrar.AlreadyRegistered.selector)
InterfaceBasedRegistry.t.sol:176	vm.expectPartialRevert(DaoUnauthorized.selector) (already imported)
ENSSubdomainRegistrar.t.sol:346	vm.expectRevert(abi.encodeWithSelector(DaoUnauthorized.selector, address(managingDao), address(registrar), alice, registrar.UPGRADE_REGISTRAR_PERMISSION_ID())) (declare/read the perm id)

This also closes the audit note that InvalidUpdateVersion and PluginAlreadyInstalled are never asserted by name anywhere.

One to confirm: DAO.t.sol:1184 test_deposit_daoAsToken_reverts — if the DAO fallback's UnknownCallback(bytes4,bytes4) fires before SafeERC20's decode, it's tightenable too; needs a quick trace.

[xavikh]

Tighten bare reverts to reason strings (OZ 4.9.6). No custom error exists for these (OZ v4.9.6 uses reason strings), but the string can still be pinned so the test fails if the wrong guard fires.

DAO deposit — the audit's "allowance vs balance collapsed" finding. The two test functions already exist with the right distinct setups; only the assertion is bare:

• This line (DAO.t.sol:458) test_deposit_revertsIfSenderLacksERC20Balance → vm.expectRevert("ERC20: transfer amount exceeds balance");
• DAO.t.sol:466 test_deposit_revertsIfNoERC20Approval → vm.expectRevert("ERC20: insufficient allowance");

Initializable ("contract is already initialized") — same string everywhere:

Line	Test
DAO.t.sol:826	test_initialize_revertsOnImplDirectly
DAOFactory.t.sol:376	test_daoBase_cannotBeInitializedDirectly
InterfaceBasedRegistry.t.sol:150	test_initialize_revertsIfCalledTwice
PluginRepoRegistry.t.sol:238	test_impl_cannotBeInitializedDirectly
PluginRepoRegistry.t.sol:244	test_initialize_revertsIfCalledTwice
PluginRepoFactory.t.sol:267	test_pluginRepoBase_cannotBeInitializedDirectly
DAORegistry.t.sol:232	test_impl_cannotBeInitializedDirectly
DAORegistry.t.sol:238	test_initialize_revertsIfCalledTwice

→ vm.expectRevert("Initializable: contract is already initialized"); for all eight.