Skip to content

Remove unecessary natspec comments from the upgradeable patch #6219

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.

Sign up for GitHub

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

Merged
ernestognw merged 4 commits into from
Dec 19, 2025
Merged

Remove unecessary natspec comments from the upgradeable patch #6219

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa5db58
Remove unecessary natspec comments from the upgradeable patch
Amxx Dec 17, 2025
693a22e
removed unecessary logic
Amxx Dec 17, 2025
4c3053c
Fix grammar 712 upgradeable
arr00 Dec 18, 2025
cd71493
Merge pull request #16 from arr00/nit/spelling-eip712-upgradeable
Amxx Dec 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 16 additions & 57 deletions scripts/upgradeable/upgradeable.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ index ff596b0c3..000000000
-<!-- Make sure that you have reviewed the OpenZeppelin Contracts Contributor Guidelines. -->
-<!-- https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/CONTRIBUTING.md -->
diff --git a/README.md b/README.md
index 6a01f5616..168b74aa7 100644
index 6a01f5616..b01c6e88f 100644
--- a/README.md
+++ b/README.md
@@ -18,6 +18,9 @@
Expand Down Expand Up @@ -173,7 +173,7 @@ index c156fa1cc..895e39342 100644
* @dev Prevents a contract from calling itself, directly or indirectly.
* Calling a `nonReentrant` function from another `nonReentrant`
diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol
index 2bc45a4b2..09e84815e 100644
index 2bc45a4b2..a5aa41d21 100644
--- a/contracts/utils/cryptography/EIP712.sol
+++ b/contracts/utils/cryptography/EIP712.sol
@@ -4,7 +4,6 @@
Expand All @@ -184,7 +184,7 @@ index 2bc45a4b2..09e84815e 100644
import {IERC5267} from "../../interfaces/IERC5267.sol";

/**
@@ -25,33 +24,20 @@ import {IERC5267} from "../../interfaces/IERC5267.sol";
@@ -25,33 +24,18 @@ import {IERC5267} from "../../interfaces/IERC5267.sol";
* NOTE: This contract implements the version of the encoding known as "v4", as implemented by the JSON RPC method
* https://docs.metamask.io/guide/signing-data.html[`eth_signTypedDataV4` in MetaMask].
*
Expand All @@ -194,7 +194,7 @@ index 2bc45a4b2..09e84815e 100644
- *
- * @custom:oz-upgrades-unsafe-allow state-variable-immutable
+ * NOTE: The upgradeable version of this contract does not use an immutable cache and recomputes the domain separator
+ * each time {_domainSeparatorV4} is called. That is cheaper than accessing a cached version in cold storage.
+ * each time {_domainSeparatorV4} is called. This is cheaper than accessing a cached version in cold storage.
*/
abstract contract EIP712 is IERC5267 {
- using ShortStrings for *;
Expand All @@ -208,9 +208,7 @@ index 2bc45a4b2..09e84815e 100644
- uint256 private immutable _cachedChainId;
- address private immutable _cachedThis;
-
+ /// @custom:oz-renamed-from _HASHED_NAME
bytes32 private immutable _hashedName;
+ /// @custom:oz-renamed-from _HASHED_VERSION
bytes32 private immutable _hashedVersion;

- ShortString private immutable _name;
Expand All @@ -224,7 +222,7 @@ index 2bc45a4b2..09e84815e 100644

/**
* @dev Initializes the domain separator and parameter caches.
@@ -66,29 +52,23 @@ abstract contract EIP712 is IERC5267 {
@@ -66,29 +50,19 @@ abstract contract EIP712 is IERC5267 {
* contract upgrade].
*/
constructor(string memory name, string memory version) {
Expand All @@ -238,10 +236,6 @@ index 2bc45a4b2..09e84815e 100644
- _cachedThis = address(this);
+ _name = name;
+ _version = version;
+
+ // Reset prior values in storage if upgrading
+ _hashedName = 0;
+ _hashedVersion = 0;
}

/**
Expand All @@ -262,18 +256,7 @@ index 2bc45a4b2..09e84815e 100644
}

/**
@@ -125,6 +105,10 @@ abstract contract EIP712 is IERC5267 {
uint256[] memory extensions
)
{
+ // If the hashed name and version in storage are non-zero, the contract hasn't been properly initialized
+ // and the EIP712 domain is not reliable, as it will be missing name and version.
+ require(_hashedName == 0 && _hashedVersion == 0, "EIP712: Uninitialized");
+
return (
hex"0f", // 01111
_EIP712Name(),
@@ -139,22 +123,62 @@ abstract contract EIP712 is IERC5267 {
@@ -139,22 +113,38 @@ abstract contract EIP712 is IERC5267 {
/**
* @dev The name parameter for the EIP712 domain.
*
Expand All @@ -296,10 +279,7 @@ index 2bc45a4b2..09e84815e 100644
- * It only reads from storage if necessary (in case the value is too large to fit in a ShortString).
+ * NOTE: This function reads from storage by default, but can be redefined to return a constant value if gas costs
+ * are a concern.
*/
- // solhint-disable-next-line func-name-mixedcase
- function _EIP712Version() internal view returns (string memory) {
- return _version.toStringWithFallback(_versionFallback);
+ */
+ function _EIP712Version() internal view virtual returns (string memory) {
+ return _version;
+ }
Expand All @@ -310,44 +290,23 @@ index 2bc45a4b2..09e84815e 100644
+ * NOTE: In previous versions this function was virtual. In this version you should override `_EIP712Name` instead.
+ */
+ function _EIP712NameHash() internal view returns (bytes32) {
+ string memory name = _EIP712Name();
+ if (bytes(name).length > 0) {
+ return keccak256(bytes(name));
+ } else {
+ // If the name is empty, the contract may have been upgraded without initializing the new storage.
+ // We return the name hash in storage if non-zero, otherwise we assume the name is empty by design.
+ bytes32 hashedName = _hashedName;
+ if (hashedName != 0) {
+ return hashedName;
+ } else {
+ return keccak256("");
+ }
+ }
+ return keccak256(bytes(_EIP712Name()));
+ }
+
+ /**
+ * @dev The hash of the version parameter for the EIP712 domain.
+ *
+ * NOTE: In previous versions this function was virtual. In this version you should override `_EIP712Version` instead.
+ */
*/
- // solhint-disable-next-line func-name-mixedcase
- function _EIP712Version() internal view returns (string memory) {
- return _version.toStringWithFallback(_versionFallback);
+ function _EIP712VersionHash() internal view returns (bytes32) {
+ string memory version = _EIP712Version();
+ if (bytes(version).length > 0) {
+ return keccak256(bytes(version));
+ } else {
+ // If the version is empty, the contract may have been upgraded without initializing the new storage.
+ // We return the version hash in storage if non-zero, otherwise we assume the version is empty by design.
+ bytes32 hashedVersion = _hashedVersion;
+ if (hashedVersion != 0) {
+ return hashedVersion;
+ } else {
+ return keccak256("");
+ }
+ }
+ return keccak256(bytes(_EIP712Version()));
}
}
diff --git a/package.json b/package.json
index 0e387a8e7..4f2a6bea6 100644
index 6f2d411dd..956933f33 100644
--- a/package.json
+++ b/package.json
@@ -35,7 +35,7 @@
Expand All @@ -368,7 +327,7 @@ index 304d1386a..a1cd63bee 100644
+@openzeppelin/contracts-upgradeable/=contracts/
+@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/
diff --git a/test/account/AccountEIP7702.test.js b/test/account/AccountEIP7702.test.js
index f442d49af..8f22dc926 100644
index d832e6877..6a0ab4a53 100644
--- a/test/account/AccountEIP7702.test.js
+++ b/test/account/AccountEIP7702.test.js
@@ -26,8 +26,8 @@ async function fixture() {
Expand All @@ -383,7 +342,7 @@ index f442d49af..8f22dc926 100644
verifyingContract: mock.address,
};
diff --git a/test/account/examples/AccountEIP7702WithModulesMock.test.js b/test/account/examples/AccountEIP7702WithModulesMock.test.js
index 8ceab19d1..c3f4194a6 100644
index 86816e55e..de6adc2c5 100644
--- a/test/account/examples/AccountEIP7702WithModulesMock.test.js
+++ b/test/account/examples/AccountEIP7702WithModulesMock.test.js
@@ -36,8 +36,8 @@ async function fixture() {
Expand Down