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

Allow Ownable to work with meta-trasnactions (ERC2771Context) #2739

Closed
wants to merge 1 commit into from

Conversation

drortirosh
Copy link

@drortirosh drortirosh commented Jun 24, 2021

The following code fails to compile:

  import "@openzeppelin/contracts/metatx/ERC2771Context.sol";
  import "@openzeppelin/contracts/access/Ownable.sol";

  contract MyContract is ERC2771Context, Ownable {
  
    constructor(address forwarder_) 
      ERC2771Context(forwarder_) {
    }

The error:

TypeError: Immutable variables cannot be read during contract creation time, which means they cannot be read in the constructor or any function or modifier called from it.
  --> @openzeppelin/contracts/metatx/ERC2771Context.sol:18:29:
   |
18 |         return forwarder == _trustedForwarder;
   |                             ^^^^^^^^^^^^^^^^^

The reason for this problem, is that Ownable attempt to set the owner, and uses _msgSender() in its constructor.
However, this fails to compile, since it is forbidden to access members from a constructor code (the trustedForwarder address)
Luckily, it is OK to use the normal msg.sender in this case, since a constructor call can't be called directly through meta-tx anyway..

Fixes #????

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

Ownable attempt to send the owner, and uses _msgSender().
however, this fails to compile, since it is forbidden to acccess members
from a constructor code (the `trustedForwarder` address)
Luckily, it is OK to use the normal `msg.sender` in this case, since a
constructor call can't be called directly through meta-tx anyway..
@Amxx
Copy link
Collaborator

Amxx commented Jun 24, 2021

WOW, thank you for this finding @drortirosh.

This is a serious issue that we will try to address quickly!

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 this pull request may close these issues.

3 participants