OpenZeppelin / openzeppelin-contracts-upgradeable

Upgradeable variant of OpenZeppelin Contracts, meant for use in upgradeable contracts.
https://openzeppelin.com/contracts
MIT License
996 stars 432 forks source link

ERC2771ContextUpgradeable.sol issues #175

Closed altyni86 closed 2 years ago

altyni86 commented 2 years ago

1) when running npm, not upgradable version of this file is downloaded.

💻 Environment

📝 Details

🔢 Code to reproduce bug

Amxx commented 2 years ago

When I run npm i @openzeppelin/contracts-upgradeable

I get it in

node_modules/@openzeppelin/contracts-upgradeable/metatx/ERC2771ContextUpgradeable.sol

What version of the package are you using ?

altyni86 commented 2 years ago

So my last install was 4.5.1.

Yes there is a file, but it is not the right one. It comes with constructor, but we need with init function.

altyni86 commented 2 years ago

Just checked 4.7.3, the same problem. Or I am not getting something right...

Amxx commented 2 years ago

So my last install was 4.5.1.

Yes there is a file, but it is not the right one. It comes with constructor, but we need with init function.

Its a choice we made that, even in the upgradeable version, the relayer should be in immutable storage. This saves a lot of gas. It means that all contract sharing the implementation will have the same relayer, and that if you want to change the relayer you need to upgrade the implementation.

This is similar to our logic to not make it mutable in the non-upgradeable version.

If you want to read that from storage, you can very easily work this contract, but keep in mind it will increase the gas cost of each transaction by 2000 + 100 per _msgSender() call

altyni86 commented 2 years ago

Then how are you passing the forwarder from the initializer function of the upgradable contract? Without Forwarder ERC2771ContextUpgradeable will not construct right?

Amxx commented 2 years ago

You don't pass it with the initializers function of the upgradeable proxy, you pass it with the constructor arguments of the implementation contract.

Amxx commented 2 years ago

If you are using the OpenZeppelin upgrades plugin, here is an example of how your can do that:

https://github.com/forta-network/forta-contracts/blob/master/scripts/deploy-platform.js#L157-L161

upgrades.deployProxy(contract, params, { kind, constructorArgs: [forwarder.address] })
altyni86 commented 2 years ago

In my case, I am deploying beacon proxies through factory contract. There is no way I could call constructor with args from there right?

Amxx commented 2 years ago

The beacon proxy will point to an implementation contract. This contract has to be deployed. The forwarder must be set during this deployment

altyni86 commented 2 years ago

Got it! Thanks for guiding through. You have another version of this contract within your repositories. Combination of these factors confused me a bit. Thanks again!

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

/*
 * @dev Context variant with ERC2771 support.
 */
abstract contract ERC2771ContextUpgradeable is Initializable, ContextUpgradeable {
    address _trustedForwarder;

    function __ERC2771Context_init(address trustedForwarder) internal initializer {
        __Context_init_unchained();
        __ERC2771Context_init_unchained(trustedForwarder);
    }

    function __ERC2771Context_init_unchained(address trustedForwarder) internal initializer {
        _trustedForwarder = trustedForwarder;
    }

    function isTrustedForwarder(address forwarder) public view virtual returns (bool) {
        return forwarder == _trustedForwarder;
    }

    function _msgSender() internal view virtual override returns (address sender) {
        if (isTrustedForwarder(msg.sender)) {
            // The assembly code is more direct than the Solidity version using `abi.decode`.
            assembly {
                sender := shr(96, calldataload(sub(calldatasize(), 20)))
            }
        } else {
            return super._msgSender();
        }
    }

    function _msgData() internal view virtual override returns (bytes calldata) {
        if (isTrustedForwarder(msg.sender)) {
            return msg.data[:msg.data.length - 20];
        } else {
            return super._msgData();
        }
    }
    uint256[50] private __gap;
}