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

Use Solady's ERC6551 implementation #83

Closed
Ramarti opened this issue Apr 12, 2024 · 1 comment · Fixed by #133
Closed

Use Solady's ERC6551 implementation #83

Ramarti opened this issue Apr 12, 2024 · 1 comment · Fixed by #133
Assignees

Comments

@Ramarti
Copy link

Ramarti commented Apr 12, 2024

  • We have to inherit from ERC6551 and override _isValidSigner() to use our AccessController
  • If we use the upgradeable version, it will also use isValidSigner(). We have to be SUPER careful with AccessController in this context, we may have to parse if the call is part of an upgrade from msg.data
@kingster-will
Copy link

To migrate to Solady's ERC6551, might need to add two parameters, target and data in _isValidSigner() of Solady's ERC6551.

Currently, _isValidSigner() in Solady's ERC6551 only accepts one parameter, signer.

    function _isValidSigner(address signer) internal view virtual returns (bool) {
        return signer == owner();
    }

However, _isValidSigner() in IPAccountImpl needs two more parameters, to(i.e., target) and data, in order to check permission with AccessContoller.

    /// @dev Checks if the signer is valid for the given data and recipient via the AccessController permission system.
    /// @param signer The signer to check
    /// @param to The recipient of the transaction
    /// @param data The calldata to check against
    /// @return bool is true if the signer is valid, false otherwise
    function _isValidSigner(address signer, address to, bytes calldata data) internal view returns (bool) {...}

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 a pull request may close this issue.

4 participants