Flatten if structure in checkPermissions

[Ramarti]

Maintaining the same logic, we can reorder the code to:

• Revert/return early
• Reduce if nesting

[kingster-will]

I haven’t reviewed all the logic and cases yet. However, it's generally best to avoid modifying sensitive logic just before a code freeze, unless it's necessary to fix a bug.

[LeoHChen]

Any more test coverage needed ?

[LeoHChen]

I like the flatten code, easier to read and understand.

We have to make sure we have 100% branch coverage of this function. And the new code shouldn't break it.

[Ramarti]

@kingster-will already achieved 100% coverage, these changes don't seem to affect coverage

We could add extra checks to make sure an upgrade doesn't mistakenly add an additional state that is not UNSET, bypassing the check in setPermissions. This would actually lower coverage, since we can't hit those checks.

function checkPermission(address ipAccount, address signer, address to, bytes4 func) external view {
        AccessControllerStorage storage $ = _getAccessControllerStorage();
        // Must be a valid IPAccount
        if (!IIPAccountRegistry($.ipAccountRegistry).isIpAccount(ipAccount)) {
            revert Errors.AccessController__IPAccountIsNotValid(ipAccount);
        }
        // Owner can call any contracts either registered module or unregistered/external contracts
        if (IIPAccount(payable(ipAccount)).owner() == signer) {
            return;
        }

        // If the caller (signer) is not the Owner, IPAccount is limited to interactions with only registered modules.
        // These interactions can be either initiating calls to these modules or receiving calls from them.
        // The IP account can also modify its own Permissions settings.
        if (
            to != address(this) &&
            !IModuleRegistry($.moduleRegistry).isRegistered(to) &&
            !IModuleRegistry($.moduleRegistry).isRegistered(signer)
        ) {
            revert Errors.AccessController__BothCallerAndRecipientAreNotRegisteredModule(signer, to);
        }
        uint functionPermission = getPermission(ipAccount, signer, to, func);
        // This is unreachable, but just in case
        if (functionPermission > AccessPermission.DENY) {
            revert Errors.AccessController__PermissionIsNotValid();  // ----> Unreachable
        }
        // Specific function permission overrides wildcard/general permission
        if (functionPermission == AccessPermission.ALLOW) {
            return;
        }
        // Specific function permission is DENY, revert
        if (functionPermission == AccessPermission.DENY) {
            revert Errors.AccessController__PermissionDenied(ipAccount, signer, to, func);
        }
        // Function permission is UNSET, check module level permission
        uint8 modulePermission = getPermission(ipAccount, signer, to, bytes4(0));
        // This is unreachable, but just in case
        if (modulePermission > AccessPermission.DENY) {
            revert Errors.AccessController__PermissionIsNotValid(); // ----> Unreachable
        }
        // Return true if allow to call all functions of the module
        if (modulePermission == AccessPermission.ALLOW) {
            return;
        }
        // Module level permission is DENY, revert
        if (modulePermission == AccessPermission.DENY) {
            revert Errors.AccessController__PermissionDenied(ipAccount, signer, to, func);
        }
        // Module level permission is UNSET, check transaction signer level permission
        // Pass if the ipAccount allow the signer to call all functions of all modules
        // Otherwise, revert
        if (getPermission(ipAccount, signer, address(0), bytes4(0)) != AccessPermission.ALLOW) {
            revert Errors.AccessController__PermissionDenied(ipAccount, signer, to, func);
        }
        // If the transaction signer is allowed to call all functions of all modules, return
    }

[jdubpark]

Flattening looks good, I think this PR can be merged since it's just shuffling around the branches.

[kingster-will]

General looks good!
Please handle the naming change and merge.