[Feature] Authorizer_v2 #760

FHieser · 2025-06-10T11:21:34Z

This is it:
The final PR to get the Authorizer fully rolling.
The PR itself is the followup PR to the Authorizer Audit that adjusts the Versioning of all Contracts that were affected by the Authorizer update itself and some minor Documentation changes.

This PR is enormous. In size as well as depth. Thats why I want as many eyes on this as possible:
@0xNuggan and @marvinkruse are the main reviewers on all the version changes that are happening.
There are a lot of them and I personally think that the implications of these changes is where the heart of this PR lies.
Take a look at the changes that tackle the explicit Interface Checks.
Looking up -> ).supports <- and -> ERC165Checker <- references should work best on this
@0xNuggan I had some talks with @marvinkruse about the implications on the Factory/Governor/existing Workflow side already, but I think it is healthy for you to also look into this, so ask as many questions as you can, so we can build up some understandings on the topic itself. @fabianschu if you have the time this would be something where I would appreciate your input.

@fabianschu and @Zitzak it would be very cool if you could take a look at how the new Authorization system will work.
The RoleAuthroizer E2E tests should be the easiest to get a grip on this in my opinion, including the RoleAuthorizer itself of course.

* Feature: Roleauthorizer_v1.1 upgrade * Test Folder restructured * Rename folder mistake * adapt rest of references * Implement Function Lock * Delete TestFolderRefactoring.md * Implement Function Lock * Adapt functionality * Create Tests * Include the rest of the new functions * Change public Role to Id 1 * Deprecate Functions * Rename to key to Permission * Clean up last functions that were still from old implementation * Refactor function postions * Test Permissioned Modifier * Adapt onlyRole to permissioned in Authorizer * Cleanup and missed tests * Adapt modules to use permissioned modifier * Cleanup comments and todos * Update Staking Module * rename to t.sol * Update Recurring Payments * Update Paymentrouter * Comment Cleanup * Add Event references * Update KPIRewarder * Update Optimistic Oracle * Update Bounties * Update PP_Streaming * Update TokenVault * Update BondingCurveBase * Adapt comments * Update RedeemingBondingCurveBase * Update comments * Adapt Event references in tests * Update Bancor Redeeming * Update BondingSurface Redeeming * Update comments * Update Bonding Surface Redeeming Restricted * Fix Typo * Update RoleAuthorizerE2E.t.sol * Update MetaTxAndMulticallE2E.t.sol * Adapt comments * Make the test of the upgraded Authorizer the new standard * Update VotingRoleManagerE2E.t.sol * Update BountyManagerE2E.t.sol * Update BondingCurveFundingManagerE2E.t.sol * Update BondingCurveTokenRescueE2E.t.sol * Update BondingSurfaceFundingManagerE2E.t.sol * Update StakingManagerLifecycle.t.sol * Update KPIRewarderLifecycle.t.sol * Remove Module__CallerNotAuthorized from VotingRoles * Remove Module__CallerNotAuthorized from IModule * Update comments * Remove Authorization from ModuleManagerBase * Adapt Orchestrator to use permissioned functions * Update KPIRewarderLifecycle.t.sol * Make supportsInterface check mandatory for ModuleTest * Update Template * Adapt E2E tests to take regular Authorizer * Remove PIM_Workflowfactory * Remove Restricted Bancor Redeeming * Remove TokenGatedRoleAuthorizer * Adapt comments * Fix Merge with dev * Make FM_Oracle and PP_Queue buildable * Adapt correct file ending * Update OracleFundingManagerAndManualQueueBasedPaymentProcessorE2E.t.sol * Test modifier in places * Do leftover todos - Removing leftover Role declarations - moving Authorization comment to downstream implementation - cleanup comments * Adapt based on Pablos comments * Remove deprecated functions * Fix test * Revert "Remove TokenGatedRoleAuthorizer" This reverts commit b5fa277. * Adapt comments * Adapt Token Gated Roles * Update TokenGatedRoleAuthorizerE2E.t.sol * Update AUT_TokenGated_Roles_v1.sol * Adapt tests * Turn down the rate of assume problems * Addressed Pablos comments * Fix test It was the || * Adapt the function positioning * Adapt natspec * Fix typo Damn copy paste * Fix test for real * Update comments * Adapt natspec * Update Natspec for Token Gated Roles * Apply suggestions from code review Co-authored-by: Marvin Kruse <[email protected]> * Apply suggestions from code review Co-authored-by: Marvin Kruse <[email protected]> * Rename RoleIdCounter to LastAssignedRoleId * Address Marvins comments * Update Voting Roles variable naming * Adapt to inverter standard * Adapt OnlyCallableByMotion modifier and error * Apply suggestions from code review Co-authored-by: Marvin Kruse <[email protected]> * Adapt comments * Remove Pim Workflow as it is outdated * Adapt comments * Apply suggestions from code review Co-authored-by: Marvin Kruse <[email protected]> * Update Authorizer Comments.md * Delete Authorizer Comments.md * Adapt documentation of TokenGated_Roles --------- Co-authored-by: Marvin Kruse <[email protected]> * Move Interface to correct position * Adapt threshold in motion * remove redundant section * Fix merge * Add _validateThreshold to addVoter * Revert "Adapt threshold in motion" This reverts commit 7217a67. And extends it by a natspec section * Move _addVoter to its own subfunction --------- Co-authored-by: Marvin Kruse <[email protected]>

Zitzak

Did a review into the main role AUT and module, and adjacent contracts I've touched in the past so I know the change. When I started the review we didn't know the strategic change was coming.

Zitzak · 2025-07-07T08:07:19Z

src/modules/authorizer/role/AUT_Roles_v2.sol

+        if (hasRole(DEFAULT_ADMIN_ROLE, caller_)) {
+            return true;
+        }


Is there no way to restrict the default admin to calling a function? I could see usecases whereby a function should (from a legal perspective) be restricted to only a specific address which should for example could be held by an external party for regulation needs

You are correct that there is no obvious/default way to do this. There is a workaround though:
You can give a role the rights to change the Function Permissions and then renounce the default admin.
THe new role then cannot call all functions automatically, but has the rights to adapt that (kinda simulating the rights of the default admin)

Zitzak · 2025-07-07T08:16:03Z

src/modules/authorizer/role/AUT_Roles_v2.sol

+    }
+
+    /// @inheritdoc IAuthorizer_v2
+    function transferAdminRole(bytes32 roleId_, bytes32 newAdminRoleId_)


This function, and multiple below, are not virtual

0xNuggan · 2025-07-21T18:30:14Z

docs/src/modules/authorizer/role/AUT_Roles_v2.md

+- The role id of the role that will become the admin of the new role.
+- The addresses of the initial members of the new role.
+
+The rolename in this context refers to the label of the role and is therefor not referenceable onchain. The function can only be called by a permissioned address (See [permissioned](#permissioned-modifier)).


0xNuggan · 2025-07-21T18:44:11Z

script/deploymentSuite/MetadataCollection_v1.s.sol


    // RoleAuthorizer
-    IModule_v1.Metadata public roleAuthorizerMetadata = IModule_v1.Metadata(
-        1, 0, 0, "https://github.com/InverterNetwork/contracts", "AUT_Roles_v1"


Should be 2

0xNuggan · 2025-07-21T18:55:21Z

src/modules/fundingManager/depositVault/FM_DepositVault_v1.sol

    "@fm/depositVault/interfaces/IFM_DepositVault_v1.sol";

 // Internal Dependencies
-import {Module_v1, IModule_v1} from "src/modules/base/Module_v1.sol";


Curious about your choice to not make deposit permissioned here. Not saying I disagree, I think it makes sense to have a "super basic" FM, but was there any particular reason apart from ressource allocation?

Nope, it was just to enable everybody to do deposits

0xNuggan · 2025-07-21T19:00:30Z

script/deploymentSuite/SingletonDeployer_v1.s.sol

        console2.log("  -- Logic Modules");

        impl_mod_LM_PC_Oracle_Permissioned_v1 = deployAndLogWithCreate2(
-            "LM_Oracle_Permissioned_v1",


var name should be renamed :|

0xNuggan · 2025-07-21T19:20:37Z

src/modules/base/IModule_v1.sol

+    /// @notice Returns the module's {Orchestrator_v2} interface, {IOrchestrator_v2}.
    /// @return The module's {Orchestrator_1}.
-    function orchestrator() external view returns (IOrchestrator_v1);
+    function orchestrator() external view returns (IOrchestrator_v2); //@todo what to do with this? Should this be version 1?


Why are we keeping the V1s at all?

because workflows and modules still exist that reference them, use them, etc. - so I think we have a few cases where we can't just remove this.
we could look into this though with our new strategy potentially, to just cut old modules out kinda.

0xNuggan · 2025-07-21T20:21:56Z

src/modules/logicModule/interfaces/IERC20PaymentClientBase_v3.sol

+pragma solidity ^0.8.0;
+
+// Internal Interfaces
+import {IPaymentProcessor_v3} from


Import can be removed, also in v2 and v1

marvinkruse · 2025-08-05T01:53:52Z

README.md


 # Inverter Network Smart Contracts
-*Inverter is the pioneering web3 protocol for token economies, enabling conditional token issuance, dynamic utility management, and token distribution. Build, customize, and innovate with Inverter's modular logic and extensive web3 interoperability.*
+


Why is this change being done? Or rather: was this an accident or on purpose?

Looks like it may just be your formatter or something? Same as later in the same file?

Yeah seems to be formatter issue
Doesnt seem to change the form of the output file though
So I would keep it

docs/src/modules/authorizer/role/AUT_Roles_v2.md

marvinkruse · 2025-08-05T01:57:59Z

docs/src/modules/authorizer/role/AUT_Roles_v2.md

+- The role id of the role to revoke
+- The address to revoke the role from
+
+The revokeRole function can only be called by according admin of the role.


There is a general inconsistency in formatting function names as normal text vs codeText. I think it makes more sense to be strict and just have it all as codeText, but at least it should be consistent throughout.

I tried my best 😅

marvinkruse · 2025-08-05T02:00:28Z

src/external/governance/Governor_v1.sol

The metadata didn't change, so it makes no difference alltogether I'd say. I think the Metadata shouldn't have been part of the Module in the first place probably, but we can leave it like it as and just use v2. Unless I'm missing something?

marvinkruse · 2025-08-05T02:05:52Z

src/modules/authorizer/role/AUT_Roles_v2.sol

+        if (permissionLength == 0) {
+            return false;
+        }


What does this do in the end? The code behaves the same way if this didn't exist, as the for loop below just doesn't do anything and the return false is reached below.

I removed it

marvinkruse · 2025-08-05T02:08:04Z

src/modules/authorizer/role/AUT_TokenGated_Roles_v2.sol

+    // Storage
+
+    /// @dev	Stores if a role is token gated.
+    mapping(bytes32 => bool) internal _isTokenGated;


I think we wanted to have named parameters here for mapping as well.

src/modules/authorizer/role/AUT_TokenGated_Roles_v2.sol

marvinkruse · 2025-08-05T02:10:58Z

src/modules/base/IModule_v1.sol

+    /// @notice Returns the module's {Orchestrator_v2} interface, {IOrchestrator_v2}.
    /// @return The module's {Orchestrator_1}.
-    function orchestrator() external view returns (IOrchestrator_v1);
+    function orchestrator() external view returns (IOrchestrator_v2); //@todo what to do with this? Should this be version 1?


because workflows and modules still exist that reference them, use them, etc. - so I think we have a few cases where we can't just remove this.
we could look into this though with our new strategy potentially, to just cut old modules out kinda.

Co-authored-by: Marvin Kruse <[email protected]>

…twork/contracts into feature/Authorizer_v2

0xNuggan · 2025-08-12T10:29:47Z

src/modules/authorizer/role/AUT_Roles_v2.sol

+    // Mutating - Mixed Utility
+
+    /// @inheritdoc IAuthorizer_v2
+    function createRoleAndAddAccessPermissions(


Just a suggestion: I could imagine this running into "out of gas" issues if the role setup becomes too complex. Maybe add a upper cap in the amount of targets/selectors? To force the developer to "split the setup" in advance

In theory yes, this could happen, but splitting up the process might not be ideal too, as the process of determining the Id of the role win addition to then adding the access permissions separately would be a very manual process.
If gas is a general problem one could instead use the createRole and addAccessPermission functions independently anyway

So I would keep it as is

I had in mind sth more like "giving the user a clear error msg instead of the tx just failing". But I agree it's secondary. Let's just keep it.

FHieser self-assigned this Jun 10, 2025

FHieser marked this pull request as draft June 10, 2025 11:21

FHieser changed the base branch from main to dev June 10, 2025 11:21

FHieser added 26 commits June 10, 2025 13:36

Create AUT_Roles_v1.md

8ee380f

Adapt the natspec header sections

bbe3bf5

Fix documentation

4167b48

Update AUT_Roles_v1.md

b584027

Create AUT_TokenGated_Roles_v1.md

7776e79

Remove according natspec from contract headers

7c908c9

Create 2025-06-05-team-omega-authorizer-update.pdf

6ca5225

Rename Authorizer Interface

904f84d

Rename IAUT_EXT_VotingRoles_v1

490ed94

Adapt versioning of md files

78d229b

Rename AUT_EXT_VotingRoles_v1

098d087

Adapt natspec

213ab0b

Rename AUT_Roles_v1

1bf4ef1

Rename AUT_TokenGated_Roles_v1

395c52b

Rename AuthorizerV1Mock

4cf9fe5

Rename ModuleV1Mock

586720b

Rename Module_v1

d7c268a

Rename FM_BC_Bancor_Redeeming_VirtualSupply_v1

2d33fe7

Rename FM_BC_BondingSurface_Redeeming_Restricted_Repayer_Seizable_v1

b8f334e

Rename FM_BC_BondingSurface_Redeeming_v1

35425ef

Rename IBondingCurveBase_v1

cbe9396

Rename RedeemingBondingCurveBase_v1

0736c04

Rename Tests

dc50efd

Rename BondingCurveBase_v1

a1140b1

Remove falsely placed comment

d0dcfc4

Rename Module test

e94ca86

FHieser added 2 commits June 19, 2025 12:31

Fix Merge

f7aa43f

Rename Crosschain Contracts

b85558f

FHieser requested review from Zitzak, fabianschu and marvinkruse June 19, 2025 12:38

FHieser marked this pull request as ready for review June 19, 2025 12:38

Merge branch 'dev' into feature/Authorizer_v2

68892f7

Zitzak reviewed Jul 7, 2025

View reviewed changes

0xNuggan reviewed Jul 21, 2025

View reviewed changes

marvinkruse requested changes Aug 5, 2025

View reviewed changes

FHieser and others added 13 commits August 6, 2025 12:47

Make functions virtual

9efa4ff

Update MetadataCollection_v1.s.sol

832ec68

Fixing comments

73c30fa

adapt variable naming

0f19bf8

Adapt references

48bb4b8

Apply suggestions from code review

98952a8

Co-authored-by: Marvin Kruse <[email protected]>

Apply suggestions from code review

3061d2d

Co-authored-by: Marvin Kruse <[email protected]>

Merge branch 'feature/Authorizer_v2' of https://github.com/InverterNe…

ee6bc33

…twork/contracts into feature/Authorizer_v2

Update md to use correct function format

a614527

Adapt documentation link

6f65c74

Remove unnecessary code

55779c0

Update Mapping natspec

31ee35d

Merge branch 'dev' into feature/Authorizer_v2

237311c

0xNuggan reviewed Aug 12, 2025

View reviewed changes

0xNuggan approved these changes Aug 12, 2025

View reviewed changes


		# Inverter Network Smart Contracts
		Inverter is the pioneering web3 protocol for token economies, enabling conditional token issuance, dynamic utility management, and token distribution. Build, customize, and innovate with Inverter's modular logic and extensive web3 interoperability.

[Feature] Authorizer_v2 #760

Are you sure you want to change the base?

[Feature] Authorizer_v2 #760

Uh oh!

Conversation

FHieser commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zitzak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xNuggan Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

FHieser commented Jun 10, 2025 •

edited

Loading

0xNuggan Jul 21, 2025 •

edited

Loading