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

Grant roles or owner using constructor arguments #205

Open
ericglau opened this issue Dec 6, 2022 · 4 comments
Open

Grant roles or owner using constructor arguments #205

ericglau opened this issue Dec 6, 2022 · 4 comments

Comments

@ericglau
Copy link
Member

ericglau commented Dec 6, 2022

When access control is used, the constructor currently grants roles such as:

        _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
        _grantRole(PAUSER_ROLE, msg.sender);

It would be better practice to set roles explicitly using constructor arguments instead.

@frangio
Copy link
Contributor

frangio commented Dec 6, 2022

A question to ask is whether each role should have its own constructor argument, or if we add just one argument and reuse it for all roles. A third option could be to add one argument for the admin and a separate argument for all other roles.

If the address passed in is zero, the role shouldn't be granted.

@ericglau ericglau changed the title Grant roles for access control using constructor arguments Grant roles or owner using constructor arguments Mar 8, 2023
@ericglau
Copy link
Member Author

ericglau commented Mar 8, 2023

This should also apply for owner if using Ownable.

@SKYBITDev3
Copy link

This issue is important because if the contract is deployed using CREATE2 or via the "CREATE3" method, msg.sender's value is the address of the factory contract or a temporary proxy contract, instead of the expected account initiating the deployment. Then the factory contract ends up owning the deployed contract and receiving initially minted tokens (that then may be stuck forever).

@SKYBITDev3
Copy link

tx.origin gives the expected value but Vitalik said it shouldn't be relied upon.

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

3 participants