Implement ERC-165 facet and library for interface detection#137
Conversation
Coverage Report
Last updated: Mon, 27 Oct 2025 23:36:13 GMT for commit |
|
@mudgen please kindly review |
|
@ogazboiz We have two pull requests for this. Should we have two for some reason? Or can you close one of them? |
|
@mudgen it was a mistake |
|
@mudgen i have close one |
There was a problem hiding this comment.
@ogazboiz Thanks for submitting this.
Please review my comments. The README and CONTRIBUTING documentation has been updated. Please read them, so you can better understand how we are designing Compose.
Please create this new directory: src/interfaceDetection. And create this directory in that: ERC165. Put all your source code in that directory, including your IERC165.sol. No libraries directory.
Thanks for your work on this and I look forward to merging the pull request when the changes are done.
| import {IERC165} from "../interfaces/IERC165.sol"; | ||
| import {LibERC165} from "../libraries/LibERC165.sol"; |
There was a problem hiding this comment.
Please remove these imports. Each facet is a standard alone source code file.
| /// @title ERC165Facet — ERC-165 Standard Interface Detection Facet | ||
| /// @notice Facet implementation of ERC-165 for diamond proxy pattern | ||
| /// @dev Allows querying which interfaces are implemented by the diamond | ||
| contract ERC165Facet is IERC165 { |
There was a problem hiding this comment.
uh-oh, submitting a pull request with a facet that uses inheritance is a finable offense. Well, since I really appreciate you working on this and it is valuable, we'll let you pass this time.
Please remove the inheritance.
| // SPDX-License-Identifier: MIT | ||
| pragma solidity >=0.8.30; | ||
|
|
||
| import {IERC165} from "../interfaces/IERC165.sol"; |
There was a problem hiding this comment.
Please remove this. We currently do not use interfaces.
|
thanks for the feedback because i want to be good in this open source contribution |
Excellent. I look forward to your changes. |
should i still create a standalone interfaces for the 165 too in the interface folder? |
|
Created directory src/interfaceDetection/ERC165. Moved the ERC165Facet into that directory. Removed passage and imports. The facet is standalone with the interface defined inline. Removed the library file and separate interface file. The facet follows SCOP principles. No inheritance. No imports. Uses ERC-8042 diamond storage. Files formatted and building. Ready for your review. |
|
@mudgen did i get it right |
|
Hi @ogazboiz your willingness to learn and adapt to our project's architecture is commendable. Once these changes are made, this will be a valuable addition to the Compose library. Please let us know if you need clarification on any of these requirements! |
I have made the changes and am now waiting for feedback. I follow you on Twitter and have been following your journey since you applied for jobs. |
There was a problem hiding this comment.
@ogazboiz Did you read the README and the CONTRIBUTING files so you can understand how we are writing these files? Do those make sense to you? Any questions about them?
ERC165Facet.sol is in the right directory now, thanks.
ERC165Facet.sol looks good now, but please remove the registerInterface internal function, since it is unused. The interface defined inline like that is fine.
Please undelete the LibERC165.sol file. It belongs in the same directory as ERC165Facet.sol. But please delete the supportsInterface function from LibERC165.sol because I don't think it is needed in the library file.
Thanks.
@mudgen Thank you for your patience and the detailed feedback. Yes, I read both files. I now understand the project's approach. Facets are standalone with all code in one file. Libraries help developers integrate with those facets. Here’s what I did:
Changes are committed. Ready for review. |
|
@ogazboiz Very good. Good job. Merged. |
…-facet Implement ERC-165 facet and library for interface detection
Added ERC-165 support to the diamond pattern.
Created:
Behavior:
Files compile without errors and follow the project’s ERC-8042 storage layout.
Addresses #123.