add deprecated comment to DOMAIN_SEPARATOR function#6366
add deprecated comment to DOMAIN_SEPARATOR function#6366nugi32 wants to merge 3 commits intoOpenZeppelin:masterfrom
Conversation
🦋 Changeset detectedLatest commit: 1a52df2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughDocumentation was added to the public Possibly related issues
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/token/ERC20/extensions/ERC20Permit.sol`:
- Around line 72-74: The NatSpec for DOMAIN_SEPARATOR() in ERC20Permit.sol is
broken by an inline solhint comment and uses an invalid `@deprecated` tag; update
the contiguous triple-slash docblock so it includes both the inheritdoc and
deprecation tag (replace `@deprecated` with `@custom`:deprecated) and then place the
// solhint-disable-next-line func-name-mixedcase comment after the closing
docblock so it does not split the NatSpec; apply the same tag fix in
IERC20Permit.sol (replace `@deprecated` with `@custom`:deprecated) or move the
deprecation text into an `@dev` description.
In `@contracts/token/ERC20/extensions/IERC20Permit.sol`:
- Around line 87-88: In the IERC20Permit.sol doc comment that mentions ERC-5267
(via {eip712Domain}), replace the invalid NatSpec tag "@deprecated" with the
supported custom tag "@custom:deprecated" and remove the trailing whitespace on
the blank documentation line so Prettier and the formal verification pipeline
accept the file; update the comment text near the ERC-5267 reference accordingly
(look for the comment block in IERC20Permit.sol that contains "This function
will be removed in v6.0." and edit the tag and whitespace there).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/token/ERC20/extensions/ERC20Permit.solcontracts/token/ERC20/extensions/IERC20Permit.sol
| /// @inheritdoc IERC20Permit | ||
| // solhint-disable-next-line func-name-mixedcase | ||
| /// @deprecated Use ERC-5267 (via {eip712Domain}) instead. This function will be removed in v6.0. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the ERC20Permit.sol file around lines 72-74
find . -name "ERC20Permit.sol" -type f | head -5Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 129
🏁 Script executed:
# Once we have the file, read lines 70-80 to see the full context
sed -n '70,80p' contracts/token/ERC20/extensions/ERC20Permit.solRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 384
🏁 Script executed:
# Also check the interface IERC20Permit.sol to see if deprecation is defined there
find . -name "IERC20Permit.sol" -type fRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 130
🏁 Script executed:
# Search for `@deprecated` usage in the codebase to understand conventions
rg "@deprecated" --type sol -B 2 -A 2 | head -50Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 106
🌐 Web query:
Solidity NatSpec @deprecated tag valid documentation
💡 Result:
@deprecated is not a Solidity NatSpec tag that the Solidity compiler recognizes. Solidity only supports the tags listed in the NatSpec docs (@title, @author, @notice, @dev, @param, @return, @inheritdoc, and @custom:...)—anything else (including @deprecated) is not part of the compiler-interpreted NatSpec set. [1]
What to use instead (valid NatSpec):
- Put the deprecation note in
@dev(and/or@noticefor end users). [1] - Or use a custom tag such as
@custom:deprecated(this is explicitly supported; semantics are tool-defined). [1]
Example:
/// `@notice` Deprecated: use fooV2() instead.
/// `@dev` Deprecated since v1.3.0; will be removed after 2026-06-01.
/// `@custom`:deprecated use fooV2()
function foo() external { }Sources:
[1] Solidity docs: NatSpec format and supported tags list. (docs.soliditylang.org)
Citations:
🏁 Script executed:
# Check the IERC20Permit.sol file for DOMAIN_SEPARATOR definition and deprecation note
sed -n '1,100p' contracts/token/ERC20/extensions/IERC20Permit.sol | cat -nRepository: OpenZeppelin/openzeppelin-contracts
Length of output: 4641
🏁 Script executed:
# Search for DOMAIN_SEPARATOR in IERC20Permit to see its NatSpec
rg "DOMAIN_SEPARATOR" contracts/token/ERC20/extensions/IERC20Permit.sol -B 5 -A 2Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 432
Two issues: @deprecated is an invalid NatSpec tag (pipeline blocker), and the solhint comment breaks the NatSpec block structure in the implementation.
Issue 1 — Invalid tag: @deprecated is not a recognized Solidity NatSpec tag. Valid tags are @title, @author, @notice, @dev, @param, @return, @inheritdoc, and @custom:*. This will cause documentation parsing errors. The same invalid tag also appears in IERC20Permit.sol (line 88), so both files must be corrected.
Issue 2 — Broken NatSpec block in implementation: The plain // solhint-disable-next-line comment at line 73 terminates the contiguous /// NatSpec block that started with /// @inheritdoc at line 72. The parser associates only the immediately preceding contiguous `///` block with a function, so the `/// `@deprecated at line 74 becomes the sole docstring for DOMAIN_SEPARATOR(), orphaning @inheritdoc. (This issue does not affect the interface, which uses block comment format.)
Fix: Use @custom:deprecated and keep the NatSpec block contiguous. The solhint directive must come after the NatSpec block ends:
Recommended fix
/// `@inheritdoc` IERC20Permit
+ /// `@custom`:deprecated Use ERC-5267 (via {eip712Domain}) instead. This function will be removed in v6.0.
// solhint-disable-next-line func-name-mixedcase
- /// `@deprecated` Use ERC-5267 (via {eip712Domain}) instead. This function will be removed in v6.0.
function DOMAIN_SEPARATOR() external view returns (bytes32) {Also correct the identical invalid tag in IERC20Permit.sol line 88 from @deprecated to @custom:deprecated or move it into the @dev description.
🧰 Tools
🪛 GitHub Actions: checks
[error] 74-74: DocstringParsingError: Documentation tag @deprecated not valid for functions.
🪛 GitHub Actions: formal verification
[error] 74-74: Documentation tag @deprecated not valid for functions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/token/ERC20/extensions/ERC20Permit.sol` around lines 72 - 74, The
NatSpec for DOMAIN_SEPARATOR() in ERC20Permit.sol is broken by an inline solhint
comment and uses an invalid `@deprecated` tag; update the contiguous triple-slash
docblock so it includes both the inheritdoc and deprecation tag (replace
`@deprecated` with `@custom`:deprecated) and then place the //
solhint-disable-next-line func-name-mixedcase comment after the closing docblock
so it does not split the NatSpec; apply the same tag fix in IERC20Permit.sol
(replace `@deprecated` with `@custom`:deprecated) or move the deprecation text into
an `@dev` description.
| * | ||
| * @deprecated Use ERC-5267 (via {eip712Domain}) instead. This function will be removed in v6.0. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n contracts/token/ERC20/extensions/IERC20Permit.sol | sed -n '80,95p'Repository: OpenZeppelin/openzeppelin-contracts
Length of output: 750
🌐 Web query:
Solidity NatSpec documentation valid tags @deprecated custom tags specification
💡 Result:
Solidity NatSpec: valid tags (compiler-supported)
Solidity’s official NatSpec parser recognizes these tags: @title, @author, @notice, @dev, @param, @return, @inheritdoc, and @custom:... (application-defined). [1]
Is @deprecated a valid NatSpec tag?
No—@deprecated is not one of Solidity’s supported NatSpec tags (so Solidity won’t treat it specially when producing userdoc / devdoc). [1]
Custom tags (@custom:...) specification
- Must start with
@custom: - The name after
:must be one or more lowercase letters or hyphens - It cannot start with a hyphen
- Can be used everywhere
- Included in the developer documentation output (
devdoc) [1]
Practical pattern for deprecation: use something like @custom:deprecated (and/or note deprecation in @notice / @dev). [1]
Source
[1] Solidity docs — “NatSpec Format” (supported tags + @custom: rules): https://docs.soliditylang.org/en/latest/natspec-format.html
@deprecated is not a valid NatSpec tag — use @custom:deprecated instead and fix trailing whitespace.
Solidity's NatSpec specification only supports: @title, @author, @notice, @dev, @param, @return, @inheritdoc, and custom tags via @custom: prefix (followed by one or more lowercase letters or hyphens, not starting with a hyphen). The bare @deprecated tag is not recognized and will cause the formal verification pipeline to fail.
Additionally, line 87 has trailing whitespace on the blank documentation line, which Prettier will flag.
Replace @deprecated with @custom:deprecated and remove the trailing space:
📝 Proposed fix
/**
* `@dev` Returns the domain separator used in the encoding of the signature for {permit}, as defined by {EIP712}.
- *
- * `@deprecated` Use ERC-5267 (via {eip712Domain}) instead. This function will be removed in v6.0.
+ *
+ * `@custom`:deprecated Use ERC-5267 (via {eip712Domain}) instead. This function will be removed in v6.0.
*/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/token/ERC20/extensions/IERC20Permit.sol` around lines 87 - 88, In
the IERC20Permit.sol doc comment that mentions ERC-5267 (via {eip712Domain}),
replace the invalid NatSpec tag "@deprecated" with the supported custom tag
"@custom:deprecated" and remove the trailing whitespace on the blank
documentation line so Prettier and the formal verification pipeline accept the
file; update the comment text near the ERC-5267 reference accordingly (look for
the comment block in IERC20Permit.sol that contains "This function will be
removed in v6.0." and edit the tag and whitespace there).
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Caution Review the following alerts detected in dependencies. According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.
|
|
hi? |
This comment was marked as spam.
This comment was marked as spam.
There was a problem hiding this comment.
There was a problem hiding this comment.
The DOMAIN_SEPARATOR function is part of ERC-2612. We cannot just remove it from here
3 participants
Title:
docs: deprecate DOMAIN_SEPARATOR in ERC20Permit
Description:
This PR addresses issue #6363.
The DOMAIN_SEPARATOR() function in ERC20Permit is marked as deprecated in preparation for its removal in the next major release (v6.0). The deprecation aligns with the transition toward ERC-5267 domain introspection (eip712Domain) and avoids maintaining redundant interfaces going forward.
Changes:
Added a @deprecated NatSpec annotation to DOMAIN_SEPARATOR().
The deprecation notice explicitly recommends using ERC-5267 (eip712Domain) instead.
No behavioral changes were introduced.
No function removal was performed to avoid breaking changes in the current release line.
Rationale:
The DOMAIN_SEPARATOR() getter exposes the EIP-712 domain separator directly, but with ERC-5267 support available, domain information can be accessed through standardized mechanisms. Marking this function as deprecated signals its planned removal while maintaining backward compatibility until the next major version.
Implementation
/// @inheritdoc IERC20Permit
// solhint-disable-next-line func-name-mixedcase
/// @deprecated Use ERC-5267 (via {eip712Domain}) instead. This function will be removed in v6.0.
function DOMAIN_SEPARATOR() external view returns (bytes32) {
return _domainSeparatorV4();
}
Notes:
If additional steps are required (e.g., changelog entry or documentation updates), I am happy to include them.