refactor: separate the logic of burn and permit from the ERC20Facet

[akronim26]

Summary

Fixes #207
Extracted the burn and permit logic from the ERC20Facet into their separate facets. The storage position for both the facets are same as ERC20Facet as they are extending the ERC20Facet functionality.

Changes Made

Extracted the burn and the permit logic into separate facets and tested the facets in separate files with the help of harnesses.

Checklist

Before submitting this PR, please ensure:

• Code follows the Solidity feature ban - No inheritance, constructors, modifiers, public/private variables, external library functions, using for directives, or selfdestruct

• Code follows Design Principles - Readable, uses diamond storage, favors composition over inheritance

• Code matches the codebase style - Consistent formatting, documentation, and patterns (e.g. ERC20Facet.sol)

• Code is formatted with forge fmt

• Existing tests pass - Run tests to be sure existing tests pass.

• New tests are optional - If you don't provide tests for new functionality or changes then please create a new issue so this can be assigned to someone.

• All tests pass - Run forge test and ensure everything works

• Documentation updated - If applicable, update relevant documentation

Make sure to follow the contributing guidelines.

Additional Notes

[netlify]

👷 Deploy request for compose-diamonds pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	7bc31e5

[github-actions]

Coverage Report

Metric	Coverage	Details
Lines	66%	1173/1777 lines
Functions	73%	233/319 functions
Branches	45%	134/295 branches

Last updated: Sat, 22 Nov 2025 17:11:50 GMT for commit 7bc31e5

[github-actions]

Gas Report

Comparing gas usage between main and refactor/ERC20

Summary

• Optimized: 28 functions (🟢 -5,189,031 gas)
• Increased: 14 functions (🔴 +459 gas)
• Net Change: 🟢 -5,188,572 gas

Details

Contract	Function	Before	After	Change
Test	test_DiamondCut_initializeCall()	N/A	N/A	🟢 -518,933
Test	test_DiamondCut_replaceAction()	N/A	N/A	🟢 -518,915
Test	test_DiamondCut_initalizeCallWithWrongCalldataReturningErrorMessage()	N/A	N/A	🟢 -518,863
Test	test_diamondCut_InitializeCallWithWrongCalldataReturningErrorMessage()	N/A	N/A	🟢 -518,862
Test	test_DiamondCut_initializeCallWithWrongCalldata()	N/A	N/A	🟢 -518,862
Test	test_diamondCut_InitializeCallWithWrongCalldata()	N/A	N/A	🟢 -518,861
Test	test_replaceFunctions_ReplaceFunctionThatDoesNotExists()	N/A	N/A	🟢 -518,837
Test	test_replaceFunctions()	N/A	N/A	🟢 -518,837
Test	test_diamondCut_MultipleActions()	N/A	N/A	🟢 -518,836
Test	test_DiamondCut_multipleActions()	N/A	N/A	🟢 -518,721
Test	test_TransferFrom_UnlimitedAllowance()	N/A	N/A	🔴 +104
Test	test_RevertWhen_TransferOverflowsRecipient()	N/A	N/A	🟢 -84
Test	test_Name()	N/A	N/A	🟢 -73
Test	test_TransferFrom_PartialAllowance()	N/A	N/A	🔴 +69
Test	test_TotalSupply()	N/A	N/A	🟢 -67
Test	test_Approve_ZeroAmount()	N/A	N/A	🟢 -53
Test	test_RevertWhen_TransferFromZeroAddressReceiver()	N/A	N/A	🔴 +41
Test	test_RevertWhen_TransferFromInsufficientBalance()	N/A	N/A	🔴 +37
Test	test_Symbol()	N/A	N/A	🟢 -37
Test	test_TransferFrom_UnlimitedAllowance_MultipleTransfers()	N/A	N/A	🔴 +36

Showing top 20 changes out of 42 total.

View all 42 changes

Contract	Function	Before	After	Change
Test	test_DiamondCut_initializeCall()	N/A	N/A	🟢 -518,933
Test	test_DiamondCut_replaceAction()	N/A	N/A	🟢 -518,915
Test	test_DiamondCut_initalizeCallWithWrongCalldataReturningErrorMessage()	N/A	N/A	🟢 -518,863
Test	test_diamondCut_InitializeCallWithWrongCalldataReturningErrorMessage()	N/A	N/A	🟢 -518,862
Test	test_DiamondCut_initializeCallWithWrongCalldata()	N/A	N/A	🟢 -518,862
Test	test_diamondCut_InitializeCallWithWrongCalldata()	N/A	N/A	🟢 -518,861
Test	test_replaceFunctions_ReplaceFunctionThatDoesNotExists()	N/A	N/A	🟢 -518,837
Test	test_replaceFunctions()	N/A	N/A	🟢 -518,837
Test	test_diamondCut_MultipleActions()	N/A	N/A	🟢 -518,836
Test	test_DiamondCut_multipleActions()	N/A	N/A	🟢 -518,721
Test	test_TransferFrom_UnlimitedAllowance()	N/A	N/A	🔴 +104
Test	test_RevertWhen_TransferOverflowsRecipient()	N/A	N/A	🟢 -84
Test	test_Name()	N/A	N/A	🟢 -73
Test	test_TransferFrom_PartialAllowance()	N/A	N/A	🔴 +69
Test	test_TotalSupply()	N/A	N/A	🟢 -67
Test	test_Approve_ZeroAmount()	N/A	N/A	🟢 -53
Test	test_RevertWhen_TransferFromZeroAddressReceiver()	N/A	N/A	🔴 +41
Test	test_RevertWhen_TransferFromInsufficientBalance()	N/A	N/A	🔴 +37
Test	test_Symbol()	N/A	N/A	🟢 -37
Test	test_TransferFrom_UnlimitedAllowance_MultipleTransfers()	N/A	N/A	🔴 +36
Test	test_Decimals()	N/A	N/A	🟢 -35
Test	test_BalanceOf()	N/A	N/A	🔴 +32
Test	test_RevertWhen_TransferFromZeroBalance()	N/A	N/A	🟢 -28
Test	test_Transfer()	N/A	N/A	🔴 +26
Test	testFuzz_TransferFrom()	N/A	N/A	🔴 +25
Test	test_Approve_UpdateExisting()	N/A	N/A	🟢 -23
Test	test_Approve()	N/A	N/A	🟢 -23
Test	testFuzz_Approve()	N/A	N/A	🟢 -23
Test	test_Approve_ReturnsTrue()	N/A	N/A	🔴 +21
Test	test_Transfer_ZeroAmount()	N/A	N/A	🟢 -20
Test	test_RevertWhen_TransferFromNoAllowance()	N/A	N/A	🔴 +19
Test	test_RevertWhen_TransferToZeroAddress()	N/A	N/A	🔴 +19
Test	testFuzz_Transfer()	N/A	N/A	🔴 +14
Test	test_TransferFrom_ReturnsTrue()	N/A	N/A	🔴 +12
Test	test_Transfer_ToSelf()	N/A	N/A	🟢 -12
Test	test_Transfer_ReturnsTrue()	N/A	N/A	🟢 -7
Test	test_Transfer_EntireBalance()	N/A	N/A	🟢 -7
Test	test_RevertWhen_TransferInsufficientBalance()	N/A	N/A	🟢 -5
Test	test_TransferFrom()	N/A	N/A	🔴 +4
Test	test_RevertWhen_TransferFromZeroAddressSender()	N/A	N/A	🟢 -3
Test	test_RevertWhen_TransferFromInsufficientAllowance()	N/A	N/A	🟢 -2
Test	test_RevertWhen_ApproveZeroAddressSpender()	N/A	N/A	🟢 -2

ℹ️ About this report

This report compares gas usage between the base branch and this PR using forge snapshot.

• 🟢 indicates a gas improvement (reduction)
• 🔴 indicates a gas regression (increase)
• Functions not shown have unchanged gas costs

To run this locally:

# Generate snapshot for current branch
forge snapshot

# Compare with another branch
git checkout main
forge snapshot --diff .gas-snapshot

Last updated: Sat, 22 Nov 2025 17:12:16 GMT for commit 7bc31e5

[mudgen]

@akronim26 Thanks for this work! This work is really needed.

Our smallest building block for Compose is the facet. All external functions in a facet are expected to be added to a diamond. Right now it is not possible to add all the external functions in ERC20BurnFacet and all the external functions in ERC20Facet to the same diamond because they have some of the same external functions, such as balanceOf etc.

Please review the documentation for composition.

Please remove duplicate external functions among ERC20BurnFacet, ERC20Facet and ERC20PermitFacet.

The nonces variable needs to be defined in its own separate diamond storage in the ERC20PermitFacet.

I changed my mind about the order of variables for the ERC20Storage struct. Please change it to this order:

struct ERC20Storage {
    mapping(address owner => uint256 balance) balanceOf;
    uint256 totalSupply; 
    mapping(address owner => mapping(address spender => uint256 allowance)) allowances;
    uint8 decimals;
    string name;
    string symbol;

[akronim26]

Thanks for reviewing @mudgen. I have a few doubts:

• Should each facet define its own storage struct and storage location, or is the intention that related facets share a single storage layout?
• Functions like balanceOf, approve and others are required for testing the ERC20BurnFacet and ERC20PermitFacet, so what is the recommended approach to test them?.
  Could you please clarify these doubts?

[mudgen]

@akronim26 I am glad you are asking these questions to clarify these things and I appreciate it.

Should each facet define its own storage struct and storage location, or is the intention that related facets share a single storage layout?

One of my major goals for this project is to write documentation that makes it clear to contributors how the project is designed and how to do things and how things should be. I need to keep improving the documentation until it can achieve that goal. Can you check to see if the documentation that I wrote for composing facets, the document Design for Composition, answers your above question? If there is anything unclear in the documentation, please let me know. Please also read the example given about extending facets and the summary below it.

Facets are designed to be composed together, so a facet that needs to access storage that is defined in another facet (such as ERC20Facet) needs to access that same storage.

The nonces variable needs to be defined in its own separate diamond storage in the ERC20PermitFacet. The nonces variable does not belong in the ERC20Storage struct because the ERC20Facet doesn't use it (because we are taking the permit function out of ERC20Facet).

Functions like balanceOf, approve and others are required for testing the ERC20BurnFacet and ERC20PermitFacet, so what is the recommended approach to test them?.

I am a lot less familiar with writing tests for Compose. Some documentation for writing tests for Compose is here: https://compose.diamonds/docs/contribution/testing. I also suggest looked at existing tests for other functionality to get ideas.

I don't have a solution to the problem of lacking functions like balanceOf for testing ERC20BurnFacet and ERC20PermitFacet. How can it be done?

[akronim26]

Thanks for the clarification @mudgen.
I re-read the documentation and the part that made me uncertain was the following:

“The nonces variable needs to be defined in its own separate diamond storage in the ERC20PermitFacet. The nonces variable does not belong in the ERC20Storage struct because the ERC20Facet doesn't use it.”

Since the ERC20PermitFacet needs access to nonces, it makes sense for nonces to live in a separate ERC20PermitStorage struct. But at the same time, the facet will still interact with the standard ERC20Storage for balances, allowances, etc.

So in this facet we will have two storage structs.
My question is: how should the STORAGE_POSITION be decided?
Because we will have to remove unused fields of ERC20Storage struct and if we put the new permit struct after the ERC20Storage struct, then the storage layout will be different from the ERC20Facet and hence the STORAGE_POSITION should be changed.

Regarding testing:
I checked the tests for other facets, and most ERC20-related tests rely on functions like approve, balanceOf, etc. Since these are fundamental for verifying ERC20 behavior, my understanding is that they need to be accessible during testing even if the facet under test does not define them. One solution for this is to create separate harnesses for the permit and burn functionality which inherits the ERC20Facet along with their individual facets so that the required functions could be used from the ERC20Facet.

[mudgen]

Hi @akronim26

Since the ERC20PermitFacet needs access to nonces, it makes sense for nonces to live in a separate ERC20PermitStorage struct. But at the same time, the facet will still interact with the standard ERC20Storage for balances, allowances, etc.

So in this facet we will have two storage structs.

Yes, correct!

Because we will have to remove unused fields of ERC20Storage struct and if we put the new permit struct after the ERC20Storage struct, then the storage layout will be different from the ERC20Facet and hence the STORAGE_POSITION should be changed.

So in this facet we will have two storage structs.
My question is: how should the STORAGE_POSITION be decided?

Inside ERC20PermitFacet the ERC20Storage should be defined like this:

bytes32 constant ERC20_STORAGE_POSITION = keccak256("compose.erc20");

/// The `ERC20PermitFacet` only uses the `allowances` and `name` variables inside
/// the `ERC20Storage` struct from `ERC20Facet`. 
/// We cannot remove the `balanceOf`, `totalSupply`, and `decimals` variables from 
/// the struct even though they aren't used. This is because we must maintain the
/// order of variables defined in structs. Only variables at the end of structs can be
/// removed. In this case there is only one variable at the end that isn't used and that
/// is the `symbol` variable so that is removed from the struct below.
/// @custom:storage-location erc8042:compose.erc20
struct ERC20Storage {
    mapping(address owner => uint256 balance) balanceOf;
    uint256 totalSupply; 
    mapping(address owner => mapping(address spender => uint256 allowance)) allowances;
    uint8 decimals;
    string name; 
}

function getERC20Storage() internal pure returns (ERC20Storage storage s) {
    bytes32 position = ERC20_STORAGE_POSITION;
    assembly {
        s.slot := position
    }
}

bytes32 constant STORAGE_POSITION = keccak256("compose.erc20.permit");

/// @custom:storage-location erc8042:compose.erc20.permit
struct ERC20PermitStorage {
    mapping(address owner => uint256) nonces;
}

function getStorage() internal pure returns (ERC20PermitStorage storage s) {
    bytes32 position = STORAGE_POSITION;
    assembly {
        s.slot := position
    }
}

The above code for ERC20PermitFacet defines two structs at two different storage locations.

The ERC20Storage struct is stored at keccak256("compose.erc20") which is the same place that ERC20Facet stores the struct. This enables ERC20PermitFacet to access the data created by ERC20Facet.

The ERC20PermitStorage struct is stored in its own separate space at keccak256("compose.erc20.permit") where no other data currently exists.

Does this make sense?

[akronim26]

Hi @akronim26

Since the ERC20PermitFacet needs access to nonces, it makes sense for nonces to live in a separate ERC20PermitStorage struct. But at the same time, the facet will still interact with the standard ERC20Storage for balances, allowances, etc.

So in this facet we will have two storage structs.

Yes, correct!

Because we will have to remove unused fields of ERC20Storage struct and if we put the new permit struct after the ERC20Storage struct, then the storage layout will be different from the ERC20Facet and hence the STORAGE_POSITION should be changed.

So in this facet we will have two storage structs.
My question is: how should the STORAGE_POSITION be decided?

Inside ERC20PermitFacet the ERC20Storage should be defined like this:

bytes32 constant ERC20_STORAGE_POSITION = keccak256("compose.erc20");

/// The `ERC20PermitFacet` only uses the `allowances` and `name` variables inside
/// the `ERC20Storage` struct from `ERC20Facet`. 
/// We cannot remove the `balanceOf`, `totalSupply`, and `decimals` variables from 
/// the struct even though they aren't used. This is because we must maintain the
/// order of variables defined in structs. Only variables at the end of structs can be
/// removed. In this case there is only one variable at the end that isn't used and that
/// is the `symbol` variable so that is removed from the struct below.
/// @custom:storage-location erc8042:compose.erc20
struct ERC20Storage {
    mapping(address owner => uint256 balance) balanceOf;
    uint256 totalSupply; 
    mapping(address owner => mapping(address spender => uint256 allowance)) allowances;
    uint8 decimals;
    string name; 
}

function getERC20Storage() internal pure returns (ERC20Storage storage s) {
    bytes32 position = ERC20_STORAGE_POSITION;
    assembly {
        s.slot := position
    }
}

bytes32 constant STORAGE_POSITION = keccak256("compose.erc20.permit");

/// @custom:storage-location erc8042:compose.erc20.permit
struct ERC20PermitStorage {
    mapping(address owner => uint256) nonces;
}

function getStorage() internal pure returns (ERC20PermitStorage storage s) {
    bytes32 position = STORAGE_POSITION;
    assembly {
        s.slot := position
    }
}

The above code for ERC20PermitFacet defines two structs at two different storage locations.

The ERC20Storage struct is stored at keccak256("compose.erc20") which is the same place that ERC20Facet stores the struct. This enables ERC20PermitFacet to access the data created by ERC20Facet.

The ERC20PermitStorage struct is stored in its own separate space at keccak256("compose.erc20.permit") where no other data currently exists.

Does this make sense?

Yes @mudgen. I'll resolve the requested changes in a few hours.

[mudgen]

@akronim26

I checked the tests for other facets, and most ERC20-related tests rely on functions like approve, balanceOf, etc. Since these are fundamental for verifying ERC20 behavior, my understanding is that they need to be accessible during testing even if the facet under test does not define them. One solution for this is to create separate harnesses for the permit and burn functionality which inherits the ERC20Facet along with their individual facets so that the required functions could be used from the ERC20Facet.

Sounds good!

[akronim26]

@akronim26

I checked the tests for other facets, and most ERC20-related tests rely on functions like approve, balanceOf, etc. Since these are fundamental for verifying ERC20 behavior, my understanding is that they need to be accessible during testing even if the facet under test does not define them. One solution for this is to create separate harnesses for the permit and burn functionality which inherits the ERC20Facet along with their individual facets so that the required functions could be used from the ERC20Facet.

Sounds good!

This solution was not possible due to some functions and custom errors giving the same function selector error. Hence, a workaround, I defined only the required functions and errors in the harnesses instead of the facets.

[mudgen]

@akronim26 Good job!