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 immutable for OpenZeppelin AccessControl's Roles Declarations #89

Closed
pcaversaccio opened this issue Feb 16, 2022 · 3 comments
Closed

Comments

@pcaversaccio
Copy link
Contributor

Access roles marked as constant results in computing the keccak256 operation each time the variable is used because assigned operations for constant variables are re-evaluated every time. Changing the variables to immutable results in computing the hash only once on deployment, leading to gas savings. Some background information can be found here and here.

@frangio
Copy link
Contributor

frangio commented Feb 16, 2022

Your observation is correct about constant variables in general, but keccak256 of string literals is treated specially and the hash is evaluated at compile time. It's not even necessary to have optimizations enabled. You can see this in the changelog items for 0.6.12, and I've just confirmed that this is the way it works currently.

immutable may result in cheaper deployment costs because the hash doesn't have to be included in the bytecode... But this is something we'd have to confirm through benchmarks.

@frangio frangio closed this as completed Feb 16, 2022
@pcaversaccio
Copy link
Contributor Author

Yeah, you are right - thanks for the clarification. For the sake of completeness, there is however a general issue that is still open: ethereum/solidity#3157.

@pcaversaccio
Copy link
Contributor Author

@frangio your _PERMIT_TYPEHASH variable is however declared as immutable here. Is there a specific reason? Otherwise, I would propose to make this constant as well due to the sake of consistency (happy to make a PR).

Amxx added a commit to OpenZeppelin/openzeppelin-contracts that referenced this issue Mar 9, 2022
* replace `immutable` with `constant` for _PERMIT_TYPEHASH

This commit is related to the following issue discussion: OpenZeppelin/contracts-wizard#89 (comment)

Since Solidity version `0.6.12` the `keccak256` of string literals is treated specially and the hash is evaluated at compile time. Since the OpenZeppelin Wizard also uses `constant` for OpenZeppelin's AccessControl's roles declarations, it's good practice to make this consistent.

* Update CHANGELOG

* fix: ensure transpiler compatibility

* fix: fixing var-name-mixedcase

* prettier & lint check

Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
SuperGNUS pushed a commit to GeniusVentures/openzeppelin-contracts-diamond that referenced this issue Jan 11, 2023
* replace `immutable` with `constant` for _PERMIT_TYPEHASH

This commit is related to the following issue discussion: OpenZeppelin/contracts-wizard#89 (comment)

Since Solidity version `0.6.12` the `keccak256` of string literals is treated specially and the hash is evaluated at compile time. Since the OpenZeppelin Wizard also uses `constant` for OpenZeppelin's AccessControl's roles declarations, it's good practice to make this consistent.

* Update CHANGELOG

* fix: ensure transpiler compatibility

* fix: fixing var-name-mixedcase

* prettier & lint check

Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch>

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
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
@frangio @pcaversaccio and others