Skip to content

Conversation

@dadadave80
Copy link

@dadadave80 dadadave80 commented Nov 25, 2025

Summary

Unify and enhance the Diamond proxy implementation with improved import management and initialization logic.

Changes Made

  • Removed specific EVM version to default to the latest mainnet version.

  • Added remappings in foundry.toml for better import path management.

  • Introduced an abstract Diamond contract implementing the ERC-2535 Diamond proxy pattern.

  • Refactored MinimalDiamond to inherit from the abstract Diamond contract, simplifying its deployment.

  • Updated imports and initialization logic in BaseBenchmark to use the new structure.

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

No additional notes at this time.

Introduced remappings for compose-related directories in foundry.toml to improve import path management and organization.
Introduces the Diamond contract, an abstract implementation of the ERC-2535 Diamond proxy pattern. Provides mechanisms for dynamic addition, replacement, and removal of facets, including storage management, error handling, and fallback delegation logic.
MinimalDiamond now inherits from the Diamond contract and is initialized via its constructor, simplifying deployment and initialization. Imports have been updated to use the @compose package.
Updated imports to use @compose package for Diamond and DiamondLoupeFacet. Refactored diamond initialization to pass facet cuts and initialization arguments directly to MinimalDiamond constructor, simplifying setup in BaseBenchmark.
Introduces initializable storage and logic to the Diamond contract, including new errors and events for initialization. Replaces the constructor with an external initialize function, updates function parameter types to calldata for efficiency, and adds a receive function for handling plain ether transfers.
Updated import paths in Base.t.sol to use @compose-benchmark namespace. Refactored MinimalDiamond to remove constructor and use an initialize method instead, simplifying contract deployment in tests.
@netlify
Copy link

netlify bot commented Nov 25, 2025

👷 Deploy request for compose-diamonds pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit cda7cf7

@github-actions
Copy link

Coverage Report

Coverage

Metric Coverage Details
Lines 83% 1680/2035 lines
Functions 86% 319/369 functions
Branches 67% 234/351 branches

Last updated: Tue, 25 Nov 2025 14:07:13 GMT for commit 530eec6

@github-actions
Copy link

Gas Report

Comparing gas usage between main and main

Summary

  • Optimized: 0 functions (🟢 -0 gas)
  • Increased: 9 functions (🔴 +209 gas)
  • Net Change: 🔴 +209 gas

Details

Contract Function Before After Change
Test testGas_Loupe_Facets() N/A N/A 🔴 +26
Test testGas_Loupe_Facets() N/A N/A 🔴 +26
Test testGas_Loupe_FacetAddresses() N/A N/A 🔴 +26
Test testGas_Loupe_FacetAddresses() N/A N/A 🔴 +26
Test testGas_Loupe_FacetFunctionSelectors() N/A N/A 🔴 +26
Test testGas_Loupe_FacetFunctionSelectors() N/A N/A 🔴 +26
Test testGas_Loupe_FacetAddress() N/A N/A 🔴 +26
Test testGas_Loupe_FacetAddress() N/A N/A 🔴 +26
Test test_BurnFuzz() N/A N/A 🔴 +1
ℹ️ 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: Tue, 25 Nov 2025 14:07:54 GMT for commit 530eec6

@mudgen
Copy link
Contributor

mudgen commented Nov 26, 2025

@dadadave80 I will review this. Thanks for this!

@mudgen
Copy link
Contributor

mudgen commented Nov 28, 2025

@dadadave80 I like this approach, but have you explored a diamond contract implementation where the facet functions are added to the diamond in the constructor function of the diamond contract? And storage variable and any other initialization is done in the constructor function of the diamond contract? I am currently exploring that approach, which you can see here: https://github.com/Perfect-Abstractions/Compose/blob/main/src/diamond/deployment/ExampleDiamond.sol

@dadadave80
Copy link
Author

@mudgen I used a constructor in this commit before replacing it with initializers

@mudgen
Copy link
Contributor

mudgen commented Nov 30, 2025

@mudgen I used a constructor in this commit before replacing it with initializers

ah, okay, thanks.

@mudgen
Copy link
Contributor

mudgen commented Nov 30, 2025

@dadadave80 Do you prefer the initializer approach over the constructor approach? If so, why?

@dadadave80
Copy link
Author

@dadadave80 Do you prefer the initializer approach over the constructor approach? If so, why?

Initializing the diamond with a constructor loads FacetCuts[] into memory, while using an initializer accesses FacetCuts[] from the calldata, enabling the use of LibDiamond's diamondCut for initialization.

Loading large arrays of structs into memory tends to explode gas usage.

Additionally, the removal of diamondCut init, replace, and remove functions here restricts custom/complex initialization flows

@mudgen
Copy link
Contributor

mudgen commented Nov 30, 2025

Initializing the diamond with a constructor loads FacetCuts[] into memory, while using an initializer accesses FacetCuts[] from the calldata, enabling the use of LibDiamond's diamondCut for initialization.

Loading large arrays of structs into memory tends to explode gas usage.

Yes, this is true. It would be very interested and very useful to see some gas benchmark comparisons between using a constructor version that uses FacetCuts[] memory and an external call version that uses FacetCuts[] calldata. For example with 10 facets, 12 functions each, and 50 facets, 12 functions each. There is a 21,000 gas overhead for making a contract call, so the calldata version would need to be at least that much cheaper, to be cheaper.

Additionally, the removal of diamondCut init, replace, and remove functions here restricts custom/complex initialization flows

The idea with ComposeDiamond is that people can inherit it in their own contract and create a constructor function with whatever parameters that they want. ExampleDiamond is an example.

Also, they can import the LibDiamondCut library and use diamondCut in their own contract that inherits ComposeDiamond, if they want to. They can import and use what they want.

I don't know why people would need replace/remove functions when constructing a diamond. If you have ideas about that, I am interested to know.

@dadadave80
Copy link
Author

Alright, use what you feel is best for the library

@mudgen
Copy link
Contributor

mudgen commented Nov 30, 2025

Alright, use what you feel is best for the library

Yes, of course. This discussion with you is very good for me and what you have submitted is helpful in this phase of development. I really appreciate it and hope to keep working and discussing things with you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants