[Feature]: AccessControl Library Enhancements - Composable Extensions

[adamgall]

This issue is an extension of #149

"Phase 2" has been extracted from the original issue and included here.

Phase 2 (Composable Facets - Future):

• Design and implement AccessControlPausableFacet
• Design and implement AccessControlTemporalFacet
• Ensure shared storage slot compatibility

@isihin-3 do you want to work on this one too?

[isihin-3]

@adamgall Yes I want to work on this
can you assign me

[isihin-3]

Storage Architecture Question for AccessControl Extensions

I've implemented Phase 2 (AccessControlPausableFacet + AccessControlTemporalFacet) and need guidance on storage architecture.

Question: Shared Storage vs Isolated Storage?

I currently have two valid approaches. Which should we use?

---

Option 1: Shared Storage (Current Implementation)

All facets share the same storage slot with identical struct layout:

Storage Slot:

bytes32 constant STORAGE_POSITION = keccak256("compose.accesscontrol");

Struct (in ALL facets):

struct AccessControlStorage {
    mapping(address account => mapping(bytes32 role => bool hasRole)) hasRole;
    mapping(bytes32 role => bytes32 adminRole) adminRole;
    mapping(bytes32 role => bool paused) pausedRoles;        // ← Pausable
    mapping(address account => mapping(bytes32 role => uint256 expiryTimestamp)) roleExpiry;  // ← Temporal
}

Pros:

• Single source of truth for all role data
• Cross-feature checks are simpler (can check role + pause + expiry in one struct)
• Fewer SLOAD operations when combining features
• Safer invariants (all role state in one place)
• Matches existing pattern (LibERC721 shares storage with ERC721Facet)

Cons:

• Tighter coupling between features
• If struct changes, all facets must update

Gas Impact: Better - Unused mappings don't cost gas, and combined checks use fewer SLOADs

---

Option 2: Isolated Storage (Alternative)

Each feature uses separate storage slots:

Pausable Storage:

bytes32 constant STORAGE_POSITION = keccak256("compose.accesscontrol.pausable");

struct AccessControlPausableStorage {
    mapping(bytes32 role => bool paused) pausedRoles;
}

Temporal Storage:

bytes32 constant STORAGE_POSITION = keccak256("compose.accesscontrol.temporal");

struct AccessControlTemporalStorage {
    mapping(address account => mapping(bytes32 role => uint256 expiryTimestamp)) roleExpiry;
}

Base AccessControl stays unchanged:

bytes32 constant STORAGE_POSITION = keccak256("compose.accesscontrol");

struct AccessControlStorage {
    mapping(address account => mapping(bytes32 role => bool hasRole)) hasRole;
    mapping(bytes32 role => bytes32 adminRole) adminRole;
    // No pausable/temporal fields
}

** Pros:**

• True modularity - features can be added/removed independently
• Looser coupling
• Clear separation of concerns

** Cons:**

• Cross-feature checks require multiple storage reads (more SLOADs)
• More complex code paths
• Higher risk of inconsistent state
• More gas when combining checks (e.g., hasRole + isPaused + isExpired)

Gas Impact: Worse - Multiple storage slot reads increase gas costs

---

Comparison Example

Shared Storage (Option 1):

// Single storage read
AccessControlStorage storage s = getStorage();
if (s.hasRole[account][role] && !s.pausedRoles[role] && s.roleExpiry[account][role] > block.timestamp) {
    // Valid role
}

Isolated Storage (Option 2):

// Multiple storage reads
AccessControlStorage storage s = getStorage();
AccessControlPausableStorage storage ps = getPausableStorage();
AccessControlTemporalStorage storage ts = getTemporalStorage();
if (s.hasRole[account][role] && !ps.pausedRoles[role] && ts.roleExpiry[account][role] > block.timestamp) {
    // Valid role - 3 separate storage reads
}

---

My Recommendation

I recommend Option 1 (Shared Storage) because:

1. Pausable and Temporal are extensions of AccessControl, not separate systems
2. Lower gas costs for combined checks
3. Matches Compose's pattern of libraries duplicating storage from facets
4. Simpler mental model for users
5. Better for composability (all role state in one place)

---

@adamgall @mudgen Which approach aligns with Compose's design philosophy? Should composable features share storage slots or remain isolated?

Current implementation uses Option 1 and all tests pass. I can refactor to Option 2 if preferred. Or should I open a PR with changes so that it can be reviewed.

[mudgen]

FYI: @adamgall @maxnorm @farismln

@isihin-3 Do Option 1.

I am actually wondering if we can just add both extensions to the main AccessControlFacet facet. Because we want to add all useful functionality to a facet as long as the optional/extension functionality doesn't interfere with standard functionality and doesn't add gas costs to the standard/existing functionality.

Because Compose isn't just creating composability at the facet/contract level, it is also doing so at the function level. People can add or not add functions from each facet.

For example if someone using the ERC721Facet doesn't want to use the "metadata extension" then they simply don't add the "name", "symbol", tokenURI" functions from the ERC721Facet to their diamond.

If the pausable and temporal functions can be added to the existing AccessControlFacet facet without adding or modifying the existing standard functions, that would be best. But if that is not possible then we will need separate facets. If we have separate facets, then I agree with you about the option 1 approach.

[isihin-3]

@mudgen
Ok I will Implement like this only

[mudgen]

@isihin-3 I am sorry, I have reversed course about what I said. We are changing direction on this design:

I am actually wondering if we can just add both extensions to the main AccessControlFacet facet. Because we want to add all useful functionality to a facet as long as the optional/extension functionality doesn't interfere with standard functionality and doesn't add gas costs to the standard/existing functionality.

Because Compose isn't just creating composability at the facet/contract level, it is also doing so at the function level. People can add or not add functions from each facet.

We are not doing that anymore. This quote is from a comment I made on issue #163 (comment)

Design Philosophy: Add useful functionality to the main facet unless it significantly increases gas costs on transfers/mints/burns (similar to how ERC721Enumerable is separate).

Correct. This is absolutely what I said. Fortunately we are still early in the composition of Compose, and I can still change its basic design principles (though I think that will be rare). I am changing how we are doing this.

We are no longer trying to pack as much useful functionality as we can in a single facet.

Prior design philosophy

The prior idea was that we could make Compose composable at the facet/contract level and at the function level. Function level composability means that a person can choose which functions from each facet to add to their diamond. Just because a facet has a number of functions doesn't mean a person needs to add all of them to their diamond -- they can instead add the ones they want to based on the functionality they want to have. That was the prior idea.

There are a few problems with this approach:

1. I think it will be harder for people to conceptualize, or understand their system at a function level. It is too fine-grained.
2. It will be harder to understand the source code of a deployed system since each facet contains all the function source code, not just the source code of the functions that they use. So the facets will contain a lot of source code not actually used by diamonds, since only some of the functions were added to the diamonds.
3. Larger facets are be harder to read and understand than smaller ones.
4. Organizationally it is harder to see different functionality if they are packed in facets. For example if a file directory (or any list of facets) shows a list of different facets by different extensions and functionality, people can easily see what functionality exists or is supported.

Our new and current design philosophy

We are no longer composing at the function level and facet level. We are composing at the facet level only.

1, Each facet is a self-contained, conceptual unit.
2, A facet is designed for all of its functions to be added, not just some of them.
3. The facet is our smallest building block.
4. Every extension of a standard should be implemented as a separate facet that composes with the standard functionality it works with.
5. A set of functions that represent a single unit of self-contained functionality should only be extended with a new facet that composes with that functionality.
6. The source code of a facet should only contain the code (including storage variables, if possible) that it actually uses.
7. A facet should reuse the structs and their diamond storage locations which are originally defined in other facets and facet libraries. But, if possible, it should only include the storage variables that it actually uses. Of course a reused struct must maintain the same storage layout as an originally defined struct. Reusing structs is done by copying structs, and modifying them as needed to remove unused variables.
8. Facets are self-contained code units. They do not import anything.
9. Facet libraries are self-contained code units. They do not import anything.
10. Facet libraries are used to initialize facets upon deployment, and in deploying upgrades. Facet libraries are also used by users of Compose to integrate custom facets with Compose facets.
11. Facet libraries can have an initialize function which is used to initialize storage variables defined and used by the corresponding facet during deployment.
12. Facets and facet libraries should not contain owner or admin authorization checks, unless absolutely required or a core part of the functionality being implemented.

I think this is the right level of onchain composability for creating the most organized, modular, and easy to understand onchain smart contract systems.

If you think there is some kind of exception to these design principles/rules/choices in some case, for some reason, then let me know so I can look at it and we can discuss.

@isihin-3 It is a tough decision but am now in favor of Option 2 concerning storage variables.

I see almost no gas cost difference between Option 1 and Option 2. Reading from mappings costs the same whether its from the same struct or different structs.

Calling each one of these functions costs about 8 gas:

AccessControlStorage storage s = getStorage();
AccessControlPausableStorage storage ps = getPausableStorage();
AccessControlTemporalStorage storage ts = getTemporalStorage();

It adds code clutter to have to call three different storage functions, however I think we can let users of Compose solve this if they want to, for example by wrapping them in an internal function.

[isihin-3]

Thanks for the clarification! @mudgen I understand the new design direction.

I'll implement:

1. Separate facets:

   • AccessControlPausableFacet + LibAccessControlPausable (separate files)
   • AccessControlTemporalFacet + LibAccessControlTemporal (separate files)
   • Remove Pausable/Temporal functions from AccessControlFacet and LibAccessControl

2. Storage approach (Option 2):

   • Separate storage structs with different storage slots
   • AccessControlPausableStorage at keccak256("compose.accesscontrol.pausable")
   • AccessControlTemporalStorage at keccak256("compose.accesscontrol.temporal")
   • Self-contained facets with no imports

3. Restore core AccessControl:

   • Keep only standard functionality (including Phase 1: setRoleAdmin, grantRoleBatch, revokeRoleBatch)

4. Separate test files for each facet/library

This aligns with the new design: facets as self-contained units, composing at the facet level only. Should I proceed?

[mudgen]

@isihin-3 That's perfect! Proceed!

[isihin-3]

@mudgen @adamgall @maxnorm

Hey guys I am thinking to open 2 pull requests 1st for the AccessControlPausableFacet + LibAccessControlPausable and then for AccessControlTemporalFacet + LibAccessControlTemporal this one

[mudgen]

Sounds good 👍