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 private variable instead of immutable to allow calls to _msgSender in constructor. #2754

Merged
merged 2 commits into from Jul 6, 2021

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Jul 6, 2021

Immutable _trustedForwarder causes issues when _msgSender() is called in constructors

Fixes #2739

PR Checklist

  • Tests
  • Changelog entry

@Amxx Amxx requested a review from frangio July 6, 2021 15:37
@frangio frangio merged commit 23b3807 into OpenZeppelin:master Jul 6, 2021
@Amxx Amxx deleted the fix/erc2771context/immutable branch July 6, 2021 21:18
@drortirosh
Copy link

Sorry, but I think this solution is sub-optimal, compared to the solution in https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2739/files

  • the _trustedForwarder is a member modified only by the constructor. there is no need to make it private (which also costs more.)
  • the issue is not with the ERC2771Context at all (which indeed, works with any other use-case, e.g. with ERC20, AccessControl ) but with the Ownable contract: _msgSender() is completely meaningless in the constructor code, as it can't be called directly via a meta-transaction call.

@frangio
Copy link
Contributor

frangio commented Jul 6, 2021

@drortirosh Thanks for the feedback. The reasons why we went with this change instead are:

  1. The problem extends beyond Ownable. Contracts like AccessControl are also affected.
  2. In some of those cases, _msgSender is not used directly in the constructor but indirectly through an internal function, and this becomes a lot harder to work around.
  3. Other variants of Context._msgSender could technically make sense in the constructor, so we prefer to continue using it consistently.

Additionally, the Solidity team has hinted that this limitation could soon be lifted at the compiler level, and we plan to add immutable back then. ethereum/solidity#10463 (comment)

@Amxx Amxx mentioned this pull request Aug 5, 2021
2 tasks
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.

None yet

3 participants