ERC20Permit: deprecate DOMAIN_SEPARATOR in favor of ERC-5267#6382
ERC20Permit: deprecate DOMAIN_SEPARATOR in favor of ERC-5267#6382Atul-Koundal wants to merge 3 commits intoOpenZeppelin:masterfrom
Conversation
|
WalkthroughThe DOMAIN_SEPARATOR() function in ERC20Permit is marked as deprecated via updated documentation and made virtual to permit overrides. Changes include: adding Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 6: Update the placeholder PR reference in the CHANGELOG entry that
mentions "`ERC20Permit`: `DOMAIN_SEPARATOR()` is deprecated..." by replacing
`#XXXX` with the actual pull request number or link to the merged PR so the
citation is valid; locate the changelog line containing `ERC20Permit` and
`#XXXX` and substitute the placeholder with the real PR identifier or URL before
merging.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
CHANGELOG.mdcontracts/token/ERC20/extensions/ERC20Permit.solcontracts/token/ERC20/extensions/IERC20Permit.soltest/token/ERC20/extensions/ERC20Permit.test.js
| ## Unreleased | ||
|
|
||
| ### Deprecations | ||
| - `ERC20Permit`: `DOMAIN_SEPARATOR()` is deprecated and will be removed in v6.0. Use `eip712Domain()` as defined by {ERC-5267} instead, which provides full domain information with native EIP-712 tooling support. ([#XXXX](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/XXXX)) |
There was a problem hiding this comment.
Replace placeholder PR link before merge.
Line 6 still references #XXXX, which leaves a broken changelog citation.
Suggested fix
-- `ERC20Permit`: `DOMAIN_SEPARATOR()` is deprecated and will be removed in v6.0. Use `eip712Domain()` as defined by {ERC-5267} instead, which provides full domain information with native EIP-712 tooling support. ([`#XXXX`](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/XXXX))
+- `ERC20Permit`: `DOMAIN_SEPARATOR()` is deprecated and will be removed in v6.0. Use `eip712Domain()` as defined by {ERC-5267} instead, which provides full domain information with native EIP-712 tooling support. ([`#6382`](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/6382))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `ERC20Permit`: `DOMAIN_SEPARATOR()` is deprecated and will be removed in v6.0. Use `eip712Domain()` as defined by {ERC-5267} instead, which provides full domain information with native EIP-712 tooling support. ([#XXXX](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/XXXX)) | |
| - `ERC20Permit`: `DOMAIN_SEPARATOR()` is deprecated and will be removed in v6.0. Use `eip712Domain()` as defined by {ERC-5267} instead, which provides full domain information with native EIP-712 tooling support. ([`#6382`](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/6382)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 6, Update the placeholder PR reference in the CHANGELOG
entry that mentions "`ERC20Permit`: `DOMAIN_SEPARATOR()` is deprecated..." by
replacing `#XXXX` with the actual pull request number or link to the merged PR
so the citation is valid; locate the changelog line containing `ERC20Permit` and
`#XXXX` and substitute the placeholder with the real PR identifier or URL before
merging.
509d8e3 to
548ab42
Compare
|
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.
|
Description
Adds a deprecation notice to
DOMAIN_SEPARATOR()inERC20Permitand
IERC20Permit, scheduling it for removal in v6.0.Motivation
ERC-5267 provides native support for domain information via
eip712Domain(),making the standalone
DOMAIN_SEPARATOR()function redundant.EIP712 has native support for ERC-5267 and the
DOMAIN_SEPARATOR()function is not recommended going forward.
Changes
ERC20Permit.sol: Added@custom:deprecatednotice, addedvirtual, replaced@inheritdocwith full NatSpecIERC20Permit.sol: Added@custom:deprecatednotice to the function commentCHANGELOG.md: Documented deprecation under## UnreleasedERC20Permit.test.js: Added deprecation comment above thedomain separatortestMigration
Users should replace:
With:
References
DOMAIN_SEPARATORfunction from ERC20Permit #6363