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

[BUG] AbstractPortal implementation is *required* for portals, while docs specify it as merely *highly encouraged* #875

Open
MOZGIII opened this issue Jan 23, 2025 · 0 comments

Comments

@MOZGIII
Copy link

MOZGIII commented Jan 23, 2025

Describe the bug Docs say that deriving from AbstractPortal is "highly encouraged", which is != "required". PortalRegistry actually requires the portal to implement AbstractPortal interface to register.

To Reproduce Steps to reproduce the behavior:

  1. Create a contract that doesn't inherit from AbstractPortal and provides a trivial IERC165 implementation:
    function supportsInterface(
        bytes4 interfaceID
    ) public pure virtual override returns (bool) {
        return
            interfaceID == type(IPortal).interfaceId ||
            interfaceID == type(IERC165).interfaceId;
    }
  2. Try registering a contract that contract with PortalRegistry.
  3. Get the PortalInvalid(); custom error revert.

Expected behavior Registration should work. AbstractPortal should not actually be required. We don't need or want 90% of it. The IPortal should express the interfaces we actually have to implement. There should be no assumption that all portals are derived from AbstractPortal, but requiring an interface implemetnation is fine.
If we are forced to use AbstractPortal we will stub all the functions that we don't want with reverts - and nobody wants that.

Also, we'd like to have a library (instead of an abstract contract) that would provide the functionality we like, without forcing a specific interface.

For now, we'll lie about implementing AbstractPortal in our IERC165's supportsInterface implementation.

Screenshots Nope.

Additional context We are building a portal that would be easy to use - without the need for passing any attestation payloads as input (we have the msg.sender already and that's all we need to generate everything on the fly).

The code in question:

// Check if portal has implemented AbstractPortal
if (!ERC165CheckerUpgradeable.supportsInterface(id, type(IPortal).interfaceId)) revert PortalInvalid();

@MOZGIII MOZGIII changed the title [BUG] AbstractPortal implementation is *required* for portals, while docs specify it *highly encouraged* [BUG] AbstractPortal implementation is *required* for portals, while docs specify it as merely *highly encouraged* Jan 23, 2025
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

No branches or pull requests

1 participant