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

Remove the draft prefix to ERC20Permit #3793

Merged
merged 5 commits into from Nov 4, 2022

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Nov 2, 2022

Fixes #3790

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx Amxx marked this pull request as ready for review November 2, 2022 11:28
@pcaversaccio
Copy link
Contributor

pcaversaccio commented Nov 2, 2022

I actually started working on a PR without realising that @Amxx has already done it :-D. In any case, I wanted to make you aware of that in docs/modules/ROOT/pages/governance.adoc & scripts/migrate-imports.js we probably also need to make the adjustments. See my commit here:

I'm not yet sure about the usage of scripts/migrate-imports.js but we probably would need as well need the following lines:

'cryptography/EIP712.sol': 'utils/cryptography/EIP712.sol', // You forgot to update this in the EIP-712 update PR
'token/ERC20/ERC20Permit.sol': 'token/ERC20/extensions/ERC20Permit.sol',
'token/ERC20/IERC20Permit.sol': 'token/ERC20/extensions/IERC20Permit.sol',

@Amxx
Copy link
Collaborator Author

Amxx commented Nov 2, 2022

We are keeping the old files (as redirect to the new) for backward compatibility ... so forgetting that would not have been a major issue ... but you are right, fixing that is better !

Co-authored-by: Francisco <frangio.1@gmail.com>
@Amxx Amxx requested a review from frangio November 4, 2022 09:33
@frangio frangio merged commit 0b6becd into OpenZeppelin:master Nov 4, 2022
@Amxx Amxx deleted the erc/2612final branch November 4, 2022 21:22
@cpecorari
Copy link

Hi, just seen that 4.8.0 version does not include final ERC20Permit, is that a mistake because draft-ERC20Permit.sol file states :

// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v4.8.0) (token/ERC20/extensions/draft-ERC20Permit.sol)

pragma solidity ^0.8.0;

// EIP-2612 is Final as of 2022-11-01. This file is deprecated.

import "./ERC20Permit.sol";

Thanks !

@pcaversaccio
Copy link
Contributor

@cpecorari no that's correct - you have to check the released version here: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.8.0/contracts/token/ERC20/extensions/draft-ERC20Permit.sol. I guess the release 4.9.0 will include this change - otherwise use OpenZeppelin via submodules to use the latest master version.

@frangio
Copy link
Contributor

frangio commented Dec 20, 2022

Indeed this didn't make it to 4.8. But please do not use the master branch, this is highly discouraged because the release process contributes to the security of the code.

There is nothing different in the code, so you can use the "draft" version from 4.8.

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

Successfully merging this pull request may close these issues.

Remove the draft prefix for ERC20Permit and IERC20Permit as it is now final
4 participants