Skip to content
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

implement eip-165 for dapps #159

Merged
merged 1 commit into from
Dec 7, 2023
Merged

implement eip-165 for dapps #159

merged 1 commit into from
Dec 7, 2023

Conversation

ZzzzHui
Copy link
Contributor

@ZzzzHui ZzzzHui commented Nov 30, 2023

No description provided.

@ZzzzHui ZzzzHui self-assigned this Nov 30, 2023
@ZzzzHui ZzzzHui linked an issue Nov 30, 2023 that may be closed by this pull request
@@ -17,7 +20,7 @@ struct Proof {
}

/// @title Cartesi DApp interface
interface ICartesiDApp {
interface ICartesiDApp is IERC721Receiver, IERC1155Receiver {
Copy link
Contributor Author

@ZzzzHui ZzzzHui Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It occurred to me that we should inherit from IERC721Receiver and IERC1155Receiver as well. So users can more easily detect that the dapp implemented receivers for ERC721 and ERC1155.
And IERC1155Receiver inherits from IERC165 so we don't need to.

@@ -11,9 +11,11 @@ import {Bitmask} from "@cartesi/util/contracts/Bitmask.sol";

import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
import {ERC721Holder} from "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
import {ERC1155Holder} from "@openzeppelin/contracts/token/ERC1155/utils/ERC1155Holder.sol";
import {ERC1155Holder, ERC1155Receiver} from "@openzeppelin/contracts/token/ERC1155/utils/ERC1155Holder.sol";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the newer versions of openzeppelin, ERC1155Receiver is merged into ERC1155Holder, and thus ERC1155Receiver is deprecated.

@@ -114,6 +116,15 @@ contract CartesiDApp is
_consensus.join();
}

function supportsInterface(
bytes4 interfaceId
) public view virtual override(ERC1155Receiver, IERC165) returns (bool) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we upgrade openzeppelin, this function would be overriding ERC1155Holder rather than ERC1155Receiver

@guidanoli
Copy link
Collaborator

You don't need to make CartesiDApp inherit from ERC1155Receiver, because it already inherits from ERC1155Holder, which in turn inherits from ERC1155Receiver.

@ZzzzHui
Copy link
Contributor Author

ZzzzHui commented Dec 5, 2023

You don't need to make CartesiDApp inherit from ERC1155Receiver, because it already inherits from ERC1155Holder, which in turn inherits from ERC1155Receiver.

But CartesiDApp didn't inherit from ERC1155Receiver directly

@ZzzzHui ZzzzHui force-pushed the feature/eip165-for-dapp branch from bc88fa1 to 421085e Compare December 5, 2023 13:24
Comment on lines 5 to 7
Implemented EIP-165 for CartesiDApp contract.
Also updated ICartesiDApp to include IERC721Receiver, IERC1155Receiver (which inherits from IERC165).
We implemented EIP-165 so that it would be more convenient to detect a potential CartesDApp instance.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please format contract names as code.

Suggested change
Implemented EIP-165 for CartesiDApp contract.
Also updated ICartesiDApp to include IERC721Receiver, IERC1155Receiver (which inherits from IERC165).
We implemented EIP-165 so that it would be more convenient to detect a potential CartesDApp instance.
Implemented EIP-165 for CartesiDApp contract.
Also updated `ICartesiDApp` to include `IERC721Receiver`, `IERC1155Receiver` (which inherits from `IERC165`).
We made the `ICartesiDApp` interface inherit from `ERC165` so that it would be possible to detect contracts that do not support such interface.

@ZzzzHui ZzzzHui force-pushed the feature/eip165-for-dapp branch from 421085e to 9e0111e Compare December 7, 2023 03:45
@ZzzzHui ZzzzHui merged commit 28777dc into main Dec 7, 2023
3 checks passed
@ZzzzHui ZzzzHui deleted the feature/eip165-for-dapp branch December 7, 2023 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 Under review
Development

Successfully merging this pull request may close these issues.

Make CartesiDApp implement EIP-165
2 participants