Remove unecessary natspec comments from the upgradeable patch#6219
Remove unecessary natspec comments from the upgradeable patch#6219ernestognw merged 4 commits intoOpenZeppelin:masterfrom
Conversation
|
WalkthroughThis pull request refactors OpenZeppelin's upgradeable contracts patch. Changes include removing cached domain-separator logic from EIP712Upgradeable in favor of on-demand recomputation, introducing a ReentrancyGuardStorage struct for the upgradeable pattern, and updating package metadata to reflect the upgradeable variant naming. Remappings are adjusted, and documentation and tests are updated to align with upgradeable initialization patterns. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
scripts/upgradeable/upgradeable.patch (2)
240-242: Misleading comment: immutables are not in storage.The comment states "Reset prior values in storage if upgrading" but
_hashedNameand_hashedVersionare declared asimmutable. Immutable values are embedded in the contract bytecode, not stored in storage slots. Consider revising the comment to accurately reflect that these are being set to sentinel values (0) to signal the upgradeable pattern.- // Reset prior values in storage if upgrading + // Set to 0 to indicate upgradeable pattern (name/version are in storage, not immutable) _hashedName = 0; _hashedVersion = 0;
267-269: Consider a more descriptive error message.The error message
"EIP712: Uninitialized"may be confusing. When_hashedName != 0 || _hashedVersion != 0, it actually indicates the contract has non-zero immutable hashes from an old (non-upgradeable) deployment, not that initialization is missing. A message like"EIP712: Invalid upgrade state"or"EIP712: Not configured for upgradeable"might be clearer.- require(_hashedName == 0 && _hashedVersion == 0, "EIP712: Uninitialized"); + require(_hashedName == 0 && _hashedVersion == 0, "EIP712: Not upgradeable");
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/upgradeable/upgradeable.patch(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-15T02:52:05.027Z
Learnt from: ernestognw
Repo: OpenZeppelin/openzeppelin-contracts PR: 5891
File: test/account/modules/ERC7579Module.behavior.js:56-61
Timestamp: 2025-10-15T02:52:05.027Z
Learning: In ERC7579 validator tests for `isValidSignatureWithSender`, using `this.mock` (not bound to a specific account) is valid when testing signature validation with any arbitrary sender, while `this.mockFromAccount` is used when testing account-specific validation scenarios.
Applied to files:
scripts/upgradeable/upgradeable.patch
📚 Learning: 2025-08-29T13:16:08.640Z
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5904
File: contracts/mocks/crosschain/ERC7786RecipientMock.sol:12-14
Timestamp: 2025-08-29T13:16:08.640Z
Learning: In OpenZeppelin contracts, mock contracts (like ERC7786RecipientMock) don't require input validation such as zero-address checks in constructors, as they are only used for testing purposes in controlled environments.
Applied to files:
scripts/upgradeable/upgradeable.patch
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: Pages changed - solidity-contracts
- GitHub Check: halmos
- GitHub Check: coverage
- GitHub Check: tests-upgradeable
- GitHub Check: slither
- GitHub Check: tests
- GitHub Check: tests-foundry
🔇 Additional comments (8)
scripts/upgradeable/upgradeable.patch (8)
61-117: LGTM!The README updates correctly reflect the upgradeable variant usage patterns, including the proper
initializefunction withinitializermodifier and__ERC721_initcall.
144-146: Verify the peerDependencies version placeholder.The
"<package-version>"literal looks like a placeholder that should be substituted during the build/release process. Ensure this is intentional and that tooling replaces it with the actual version constraint (e.g.,"5.5.0"or"^5.5.0").
148-174: LGTM!The addition of
ReentrancyGuardStoragestruct with the ERC-7201 storage location annotation and removal of the constructor aligns with the upgradeable pattern. Storage initialization will be handled by the initializer.
283-302: LGTM!Making
_EIP712Name()and_EIP712Version()virtualallows derived contracts to override them with constant values for gas optimization, which is a good pattern for upgradeable contracts where storage reads are more expensive.
304-345: LGTM!The
_EIP712NameHash()and_EIP712VersionHash()functions correctly implement the fallback logic: prioritize computing the hash from the storage string, fall back to stored hash if available (for backwards compatibility), or default to an empty string hash. This provides a safe upgrade path.
360-367: LGTM!The remappings correctly set up the upgradeable variant to map
@openzeppelin/contracts-upgradeable/to local contracts and@openzeppelin/contracts/to the base contracts library.
368-397: LGTM!The test updates correctly reflect that in the EIP7702 signer context (before the mock is deployed/initialized), the domain name and version are empty strings. The comments clarify this is expected behavior.
398-427: LGTM!Removing the "adjusts when behind proxy" test is appropriate for the upgradeable variant, as this behavior is handled differently through the initializer pattern rather than relying on immutable caching.
Fix grammar 712 upgradeable
|
For the record, this is what you get with this PR after applying the patch + transpile: // SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v5.5.0) (utils/cryptography/EIP712.sol)
pragma solidity ^0.8.24;
import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol";
import {IERC5267} from "@openzeppelin/contracts/interfaces/IERC5267.sol";
import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.sol";
/**
* @dev https://eips.ethereum.org/EIPS/eip-712[EIP-712] is a standard for hashing and signing of typed structured data.
*
* The encoding scheme specified in the EIP requires a domain separator and a hash of the typed structured data, whose
* encoding is very generic and therefore its implementation in Solidity is not feasible, thus this contract
* does not implement the encoding itself. Protocols need to implement the type-specific encoding they need in order to
* produce the hash of their typed data using a combination of `abi.encode` and `keccak256`.
*
* This contract implements the EIP-712 domain separator ({_domainSeparatorV4}) that is used as part of the encoding
* scheme, and the final step of the encoding to obtain the message digest that is then signed via ECDSA
* ({_hashTypedDataV4}).
*
* The implementation of the domain separator was designed to be as efficient as possible while still properly updating
* the chain id to protect against replay attacks on an eventual fork of the chain.
*
* 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].
*
* NOTE: The upgradeable version of this contract does not use an immutable cache and recomputes the domain separator
* each time {_domainSeparatorV4} is called. This is cheaper than accessing a cached version in cold storage.
*/
abstract contract EIP712Upgradeable is Initializable, IERC5267 {
bytes32 private constant TYPE_HASH =
keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");
/// @custom:storage-location erc7201:openzeppelin.storage.EIP712
struct EIP712Storage {
bytes32 _hashedName;
bytes32 _hashedVersion;
string _name;
string _version;
}
// keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.EIP712")) - 1)) & ~bytes32(uint256(0xff))
bytes32 private constant EIP712StorageLocation = 0xa16a46d94261c7517cc8ff89f61c0ce93598e3c849801011dee649a6a557d100;
function _getEIP712Storage() private pure returns (EIP712Storage storage $) {
assembly {
$.slot := EIP712StorageLocation
}
}
/**
* @dev Initializes the domain separator and parameter caches.
*
* The meaning of `name` and `version` is specified in
* https://eips.ethereum.org/EIPS/eip-712#definition-of-domainseparator[EIP-712]:
*
* - `name`: the user readable name of the signing domain, i.e. the name of the DApp or the protocol.
* - `version`: the current major version of the signing domain.
*
* NOTE: These parameters cannot be changed except through a xref:learn::upgrading-smart-contracts.adoc[smart
* contract upgrade].
*/
function __EIP712_init(string memory name, string memory version) internal onlyInitializing {
__EIP712_init_unchained(name, version);
}
function __EIP712_init_unchained(string memory name, string memory version) internal onlyInitializing {
EIP712Storage storage $ = _getEIP712Storage();
$._name = name;
$._version = version;
}
/**
* @dev Returns the domain separator for the current chain.
*/
function _domainSeparatorV4() internal view returns (bytes32) {
return _buildDomainSeparator();
}
function _buildDomainSeparator() private view returns (bytes32) {
return keccak256(abi.encode(TYPE_HASH, _EIP712NameHash(), _EIP712VersionHash(), block.chainid, address(this)));
}
/**
* @dev Given an already https://eips.ethereum.org/EIPS/eip-712#definition-of-hashstruct[hashed struct], this
* function returns the hash of the fully encoded EIP712 message for this domain.
*
* This hash can be used together with {ECDSA-recover} to obtain the signer of a message. For example:
*
* ```solidity
* bytes32 digest = _hashTypedDataV4(keccak256(abi.encode(
* keccak256("Mail(address to,string contents)"),
* mailTo,
* keccak256(bytes(mailContents))
* )));
* address signer = ECDSA.recover(digest, signature);
* ```
*/
function _hashTypedDataV4(bytes32 structHash) internal view virtual returns (bytes32) {
return MessageHashUtils.toTypedDataHash(_domainSeparatorV4(), structHash);
}
/// @inheritdoc IERC5267
function eip712Domain()
public
view
virtual
returns (
bytes1 fields,
string memory name,
string memory version,
uint256 chainId,
address verifyingContract,
bytes32 salt,
uint256[] memory extensions
)
{
return (
hex"0f", // 01111
_EIP712Name(),
_EIP712Version(),
block.chainid,
address(this),
bytes32(0),
new uint256[](0)
);
}
/**
* @dev The name parameter for the EIP712 domain.
*
* NOTE: This function reads from storage by default, but can be redefined to return a constant value if gas costs
* are a concern.
*/
function _EIP712Name() internal view virtual returns (string memory) {
EIP712Storage storage $ = _getEIP712Storage();
return $._name;
}
/**
* @dev The version parameter for the EIP712 domain.
*
* NOTE: This function reads from storage by default, but can be redefined to return a constant value if gas costs
* are a concern.
*/
function _EIP712Version() internal view virtual returns (string memory) {
EIP712Storage storage $ = _getEIP712Storage();
return $._version;
}
/**
* @dev The hash of the name parameter for the EIP712 domain.
*
* NOTE: In previous versions this function was virtual. In this version you should override `_EIP712Name` instead.
*/
function _EIP712NameHash() internal view returns (bytes32) {
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.
*/
function _EIP712VersionHash() internal view returns (bytes32) {
return keccak256(bytes(_EIP712Version()));
}
} |
|
We agreed this change doesn't require a changelog entry |
3 participants
In the transition from 4.8 to 4.9, we had to add a lot of logic to support the storage transition from
_hashedNameto_name(and_hashedVersiontoversion). This logic was necessary to support cases where the old values had to be taken into consideration because the new one were not set during re-initialization.Since 5.0, the
_hashedNameand_hashedVersionwere kept in storage (oversight), and we still check their value. We shouldn't! Since no version (since the transition to namespace storage) ever wrote to these slot, we know for a fact that they are empty.This PR simplifies the logic by removing the (now superfluous) hangling of these values.