-
Notifications
You must be signed in to change notification settings - Fork 61
Feat: Add ERC-7802 Facet for Cross-Chain Token Interoperability #158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: Add ERC-7802 Facet for Cross-Chain Token Interoperability #158
Conversation
Rushikesh0125
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @beebozy, please make the following changes to match the convention and standards followed in the compose repo
- maintain consistency by appending an underscore to all parameter names
- state variable and iternal names should not start with an underscore
src/ERC7802/ERC7802Facet.sol
Outdated
| /// @dev Increases totalSupply and recipient balance and emits CrosschainMint. | ||
| /// @param _account The account to mint tokens to. | ||
| /// @param _value The amount to mint. | ||
| function crooschainMint(address _account, uint256 _value) external { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor the function name here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is misspelled.
|
@beebozy, your implementation is not exposing other functions that an ERC20 exposes. The ERC7802 draft inherits ERC20, which basically exposes all functions from ERC20 to ERC7802. Also I think the facet should be named as ERC20BridgeableFacet |
|
Alright, I will make the necessary corrections and revert, thank you |
mudgen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this pull request! This is a great start on this functionality.
The facets designed in Compose need to be composable between each other.
This facet needs to interact and compose with the existing ERC20Facet and LibERC20 in Compose. I made some comments of some things that need to change. More things need to change so that this facet composes with the other existing functionality in Compose. I'll let you discover these things.
Also, keep in mind, according to the documentation, each facet needs to be self contained, so do not import anything from any other files.
src/ERC7802/libraries/LibERC7802.sol
Outdated
| function supportInterface(bytes4 interfaceId) internal { | ||
| ERC7802Storage storage s = getStorage(); | ||
| if (interfaceId != 0x33331994 || interfaceId == 0x01ffc9a7) revert ERC7802InvalidInterfaceId(interfaceId); | ||
|
|
||
| s._supportedInterfaces[interfaceId] = true; | ||
|
|
||
| emit Interface(interfaceId); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this function because we already have it implemented here: https://github.com/Perfect-Abstractions/Compose/tree/main/src/interfaceDetection/ERC165
src/ERC7802/libraries/LibERC7802.sol
Outdated
| bytes32 constant STORAGE_POSITION = keccak256("compose.erc7802"); | ||
|
|
||
| struct ERC7802Storage { | ||
| address owner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To access the owner you will need to use and access the already existing struct for owner and its location in storage. See here: https://github.com/Perfect-Abstractions/Compose/blob/main/src/access/Owner/LibOwner.sol
src/ERC7802/libraries/LibERC7802.sol
Outdated
| uint256 totalSupply; | ||
| mapping(address owner => uint256 balance) balanceOf; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These storage variables are already defined. Please use and access these from the structs and their locations defined in LibERC20.sol: https://github.com/Perfect-Abstractions/Compose/blob/main/src/token/ERC20/ERC20/LibERC20.sol
|
@Rushikesh0125 Thanks for reviewing this pull request, and reviewing pull requests is something we need more help with, so I'd really appreciate it if you continue doing this. Instead of adding the ERC20 functions to this facet, we need it to compose with the existing ERC20Facet. We are building a number of contracts/facets, that when deployed, can be composed together. |
yeah sure. Oh yeah, I realized that now. Got it pierced right through my skull. |
|
@beebozy let me know if you need help Happy to assist. |
|
@beebozy What is the status of this work? |
|
@mudgen |
src/ERC7802/ERC7802Facet.sol
Outdated
| /// @notice Query whether an interface id is supported (ERC-165 style). | ||
| /// @param interfaceId The interface id to query. | ||
|
|
||
| function supportInterface(bytes4 interfaceId) external { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, please remove this function because this functionality is already supplied by the ERC165Facet.
src/ERC7802/libraries/LibERC7802.sol
Outdated
| function addTrustedBridges(address _bridge) internal { | ||
| ERC7802Storage storage s = getStorage(); | ||
|
|
||
| if (msg.sender != s.owner) revert ERC7802InvalidOwner(msg.sender); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the owner variable from use in this library. Authorization should not happen in libraries. See here: https://compose.diamonds/docs/design/design-for-composition#facets--libraries
src/ERC7802/libraries/LibERC7802.sol
Outdated
| address owner; | ||
| uint256 totalSupply; | ||
| mapping(address owner => uint256 balance) balanceOf; | ||
| mapping(address => bool) _trustedBridges; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the underscore from variables defined in storage.
Thank you for asking about this. If your facet is going to compose (be used with) other facets and access the storage that is defined in those other facets, then you need to use the same structs as those other facets and store them in storage in the same place as those other facets -- otherwise your facet will not be accessing the same storage locations as those other facets. So you need to copy the struct from ERC20Facet and put it in your facet and store it at For your new storage variable We recently wrote new documentation for how we are building Compose. Your question should be answered by the "Extending Facets" section of the documentation here: https://compose.diamonds/docs/design/design-for-composition#extending-facets If it still is not answered, please let me know. Please review the new documentation for Compose here: |
👷 Deploy request for compose-diamonds pending review.Visit the deploys page to approve it
|
|
@mudgen @Rushikesh0125 could you please review it? |
|
|
||
| /// @notice Storage slot for ERC-20 Bridgeable using ERC8042 for storage location standardization | ||
| /// @dev Storage position determined by the keccak256 hash of the diamond storage identifier. | ||
| bytes32 constant STORAGE_POSITION = keccak256("compose.erc20"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming these variables for better readability [including other two storage position variables]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean I should name it like something of these sort
bytes32 constant ERC20_STORAGE_SLOT = keccak256("compose.erc20");
bytes32 constant OWNER_STORAGE_SLOT = keccak256("compose.owner");
bytes32 constant BRIDGEABLE_STORAGE_SLOT = keccak256("compose.erc20.bridgeable");
?
Does it look more readable? I used the former storage variable definition for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, please also change the name getStorage1()
mudgen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beebozy Thanks for your pull request. We are definitely getting there!
| ERC20Storage storage s = getStorage(); | ||
| ERC20BridgeableStorage storage s2 = getStorage2(); | ||
|
|
||
| if (s2.trustedBridges[msg.sender] == false) revert ERC20InvalidBridgeAccount(msg.sender); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single statement if conditions are not allowed. Please see the Solidity ban: https://compose.diamonds/docs/design/banned-solidity-features
Please update the code to fix every single if statement.
|
|
||
| /// @notice Storage slot for ERC-20 Bridgeable using ERC8042 for storage location standardization | ||
| /// @dev Storage position determined by the keccak256 hash of the diamond storage identifier. | ||
| bytes32 constant STORAGE_POSITION = keccak256("compose.erc20"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is better.
|
|
||
| /// @notice Storage slot for ERC-20 Bridgeable using ERC8042 for storage location standardization | ||
| /// @dev Storage position determined by the keccak256 hash of the diamond storage identifier. | ||
| bytes32 constant STORAGE_POSITION = keccak256("compose.erc20"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, please also change the name getStorage1()
| mapping(address => bool) trustedBridges; | ||
| } | ||
|
|
||
| function getStorage2() internal pure returns (ERC20BridgeableStorage storage s2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need here to use the name s2. Just use the name s here.
|
|
||
| // @notice Add a trusted bridge address. Owner-only. | ||
| /// @param _bridge The bridge address to add. | ||
| function addTrustedBridges(address _bridge) external { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and some of the other external functions are not part of ERC-7802 standard so please remove them.
| # Build the project | ||
| forge build | ||
|
|
||
| # Run tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this file from your pull request.
63bbea6 to
57d8cd9
Compare
|
@Rushikesh0125 @mudgen kindly review the latest push. I removed the readme file, and rename the variable and remove some of the functions that are not needed.. |
|
Hey @beebozy, This implementation looks good to me. One question - It is not necessary, but as the OZ draft mentions, using access control for whitelisting trusted bridges. Can we not leave that to developers to override and create their own access control logic? It also avoids this facet from using its own storage slot. In my opinion, a developer can use the access control and bridgable facets together, which demonstrates a higher degree of composability and feels quite plug-and-play. cc @mudgen |
|
It does make sense for developer to use acess control for bridgeable facets. But, the reason in my implementation is that bridgeable contract should have some kind of trusted gating as it should not be called by any arbitrary contract. If access control is left outside the facet, "I think" it may accidentally expose the burn or mint function(left to the developer) Just for safety. The developer can also layer their access control facet on top of it IMO..
It does make sense for developer to use acess control for bridgeable facets. But, the reason in my implementation is that bridgeable contract should have some kind of trusted gating as it should not be called by any arbitrary contract. If access control is left outside the facet, "I think" it may accidentally expose the burn or mint function(left to the developer) Just for safety. The developer can also layer their access control facet on top of it IMO.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beebozy Thanks for these changes. Very much appreciated.
Please review the https://eips.ethereum.org/EIPS/eip-7802 standard to ensure nothing is missing. And please review the OpenZeppelin implementation to ensure this implementation is compatible with it -- externally behaves the same way.
Merging this pull request would delete the REAME.md file from the repo, which we don't want to do that. I think the way to handle this is this:
- In your local repository undo your deletion of README.md.
- Sync your fork with https://github.com/Perfect-Abstractions/Compose. The Github user interface provides a way to do this.
- Run
git pullon your local repository to update your local repository. This should give you the current README.md file. - Commit your changes and push them.
| erc20.totalSupply -= _value; | ||
| erc20.balanceOf[_from] -= _value; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be compatible with the OpenZeppelin implementation we should also emit the Transfer event when burning the token.
|
On the matter concerning permissions for this facet:
@Rushikesh0125
I agree, and that's my opinion too. We are all about plug-and-play on-chain. I looked at this and thought this over and I think that we should use the access control we have with this bridgable facet.
AccessControlStorage storage acs = getAccessControlStorage();
if (!acs.hasRole[msg.sender]["trusted-bridge"]) {
revert AccessControlUnauthorizedAccount(_account, _role);
}When initializing the diamond during deployment the user can set the appropriate addresses for "trusted-bridge" and choose to add the By the way the documentation for composing functionality is here: https://compose.diamonds/docs/design/design-for-composition |
| // SPDX-License-Identifier: MIT | ||
| pragma solidity >=0.8.30; | ||
|
|
||
| /// @title LibERC20Bridgeable — ERC-7802 like Library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an ERC-7802 library, not like one.
| // SPDX-License-Identifier: MIT | ||
| pragma solidity >=0.8.30; | ||
|
|
||
| /// @title ERC20Bridgeable — ERC-7802-like Implementation Facet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove "-like"
57d8cd9 to
0eb553c
Compare
|
@mudgen made all the changes and PR is now updated. Kindly look into it sir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @beebozy for the changes. Please see my comments. Getting there.
Please run forge fmt on your code after you make all changes. This will ensure that the code is formatted correctly.
| } | ||
| /// @notice role identifier for trusted bridge actors | ||
|
|
||
| bytes32 internal constant TRUSTED_BRIDGE_ROLE = keccak256("trusted-bridge"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The permission is not a keccak256 hash of "trusted-bridge". It is just the string "trusted-bridge", with no hashing.
| AccessControlStorage storage acs = getAccessControlStorage(); | ||
|
|
||
| // authorize: caller must have the trusted-bridge role | ||
| if (!acs.hasRole[msg.sender][TRUSTED_BRIDGE_ROLE]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use a constant for this. Please use the string literal "trusted-bridge".
So it looks like this:
if (!acs.hasRole[msg.sender]["trusted-bridge"]) {Please update every place the constant is used.
| /// @dev This function checks if the diamond supports the given interface ID | ||
| /// @return `true` if the contract implements `_interfaceId` and | ||
| /// `_interfaceId` is not 0xffffffff, `false` otherwise | ||
| function supportsInterface(bytes4 _interfaceId) external view returns (bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this function. This function is already provided by the ERC165Facet
Coverage Report
Last updated: Tue, 25 Nov 2025 17:01:42 GMT for commit |
Gas ReportNo gas usage changes detected between All functions maintain the same gas costs. ✅ Last updated: Tue, 25 Nov 2025 17:02:17 GMT for commit |
|
@mudgen please check again. I have update the code |
|
Hi @mudgen could you check again I have done all update? |
|
@beebozy Good job and much appreciated! |
I impemented the ERC-7802 for cross-chain Token interoperability.. Using the Openzeppelin code as a template: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/draft-ERC20Bridgeable.sol