-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Add ERC7674
(draft)
#5071
Add ERC7674
(draft)
#5071
Conversation
🦋 Changeset detectedLatest commit: 887121c 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 |
contracts/token/ERC20/ERC20.sol
Outdated
if (value > 0) { | ||
uint256 currentAllowance = allowance(owner, spender); | ||
if (currentAllowance != type(uint256).max) { | ||
if (currentAllowance < value) { | ||
revert ERC20InsufficientAllowance(spender, currentAllowance, value); | ||
} | ||
unchecked { | ||
_approve(owner, spender, currentAllowance - value, false); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we decide against address poisoning for similar reasons we would decide against filtering value == 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About address poisoning we decided to:
- not revert if value is 0 (for many reason)
- keep emitting the transfer event.
The changes here don't introduce a revert, and don't remove any event.
The logic here is the following:
- if the temporary allowance is enough, we should not sload/sstore the persistent value (otherwize it break the point of gas savings). So we can do it in two ways:
- in
ERC20TemporaryApproval._spendAllowance
only do the super call if value > 0- this is bad practice if someone else overrides
_spendAllowance
- this is bad practice if someone else overrides
- in
ERC20
change the semantics of_spendAllowance
to mean "if there is nothing to spend, we are good anyway".
- in
If we look at ERC20._spendAllowance
, this has the following impact
- if value = 0,
currentAllowance
cannot be smaller than value.ERC20InsufficientAllowance
is never triggered, so the if doesn't change anything regarding the revert. - if value is 0, 5.0 code does:
- load the allowance (from a potentially overridable function)
- substract zero from it
- set it as the new allowance, without emitting an event.
So there is a change, we are no longer calling _approve
with the current value. It may be possible to create edge cases where the missed call to an overrident _approve
has an effect. IMO its a non issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About address poisoning, it doesn't change the possibility of doing it (or not doing it). It makes it cheaper though, because the poisonning call would not read/write the zero allowance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frangio curious to having your opinion on this if in the core ERC20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be possible to create edge cases where the missed call to an overrident
_approve
has an effect.
Hm, I have a vague memory that this actually was a concern for a project once... It does seem quite risky to change this in a minor version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we implement this by overriding only allowance and _approve instead of _spendAllowance?
I have no idea how. In particular I'm not sure how to make transferFrom spend only temporary allowance, without touching the normal allowance, when the temporary allowance is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible, but not without the same problem of not invoking super._approve
. There seems to be no way around that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in
ERC20TemporaryApproval._spendAllowance
only do the super call if value > 0
- this is bad practice if someone else overrides
_spendAllowance
This is true, but it may be preferable than changing the behavior of ERC20
in a minor version. I think it's worth considering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its what I used to do, before realizing there was a way to have the super call always happen.
I'm leanning toward the current version, but I'm open to re-using the old one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, I re-implemented the old one.
It negativelly affect the tests because the error emitted in some edge conditions depends on _approve being called (with spender = 0 and value = 0).
See 0490902
contracts/token/ERC20/extensions/draft-ERC20TemporaryApproval.sol
Outdated
Show resolved
Hide resolved
contracts/token/ERC20/extensions/draft-ERC20TemporaryApproval.sol
Outdated
Show resolved
Hide resolved
80261e2
to
036ed54
Compare
contracts/token/ERC20/extensions/draft-ERC20TemporaryApproval.sol
Outdated
Show resolved
Hide resolved
test/token/ERC20/extensions/draft-ERC20TemporaryApproval.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implementation lgtm. codecov is failing, rebasing should fix.
* @dev Set the temporary allowance, allowing allows `spender` to withdraw (within the same transaction) assets | ||
* held by the caller. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5071/files#diff-7b0f1795668241cd100ce0a5728fc598e10533cf7bd2846a8b5c86b8a7ac6fa5R60-R71 is much clearer imo. for consistency (and to not duplicate), shouldn't we have these comments in the interface, and point to the interface in the implementation function comments? as in here https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L118
Co-authored-by: Ernesto García <ernestognw@gmail.com>
19c414a
to
a18f963
Compare
Co-authored-by: cairo <cairoeth@protonmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good for a draft. Thanks all!
Fixes #4959
ERC-7674 is still WIP, but having it implemented as a draft contract would be valuable for users
Pending discussions:
if (value > 0) {...}
inERC20._spendAllowance
vsif (value > 0) super._spendAllowance(...)
inERC20TemporaryApproval
_approve
call when value is 0PR Checklist
npx changeset add
)