Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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) #5071base: master
Are you sure you want to change the base?
Add
ERC7674
(draft) #5071Changes from 9 commits
969dacf
ac39524
b69b988
162d942
1b66ff4
2527ef6
40765c0
1d285c7
a6b8cb7
281510a
036ed54
8f15f1f
b2d4966
17833c7
0490902
ddd7576
cf3c26d
f8c2e7d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
The changes here don't introduce a revert, and don't remove any event.
The logic here is the following:
ERC20TemporaryApproval._spendAllowance
only do the super call if value > 0_spendAllowance
ERC20
change the semantics of_spendAllowance
to mean "if there is nothing to spend, we are good anyway".If we look at
ERC20._spendAllowance
, this has the following impactcurrentAllowance
cannot be smaller than value.ERC20InsufficientAllowance
is never triggered, so the if doesn't change anything regarding the revert.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.
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.
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.
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
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.
This raises an interesting discussion about using events for signaling transient storage updates.
I first thought it shouldn't be needed, but it may disrupt indexers. The case I'm considering is when a company uses an indexer to report financial operations to authorities, for such cases there may be some transfer events they can't track because they were transient.
Whether it's relevant or not for most people is another discussion, but I see one of the main points of the ERC is to provide cheaper allowances, in that case, it'd be a matter of time before getting it widely adopted where possible.
What do you think?
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.
IMO not having events would make it harder to look for historical activity/usage -- especially when transient operations will occur inside other contracts, so it's not as easy as just looking for the
temporaryApproval
selector in transactions' calldata.i think that optimizations related to removing events is a separate discussion with regards to shadow logs.
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.
We already not have event to notify of allowance changes in transferFrom. Honestly, if we don't have those, I'm not sure why we would need one here.
On thing event are usefull for it tracking state changes ... but here, the (allowance) state doesn't really change. At the end of the tx its reset anyway. Its not like you are setting an allowance that will stay forever if you forger about it (that IMO needs to be notified)
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've thought about this in the past for ERC-6909X (a proposed extension I designed) and ended up deciding to make events a "SHOULD":
It's not a "MUST" because I understand that for max savings tokens would want to omit them.
But that was in the context of ERC-6909 compliance... which by default has transfer events that include the operator.
This is a valid point.
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.
Right. Based on other's comments I realize it's not a big deal since users may decide whether or not to add an event. That's the decision we took by removing the event from ERC20's
_spendAllowance
.Yes, I agree with this. The difference is that the event is not defined in this case whereas regular ERC20 users would identify the lack of the
Approval
event during testing and before deploying/upgrading.Seems like this is a recommendation in the ERC already. It should be specific: https://github.com/ethereum/ERCs/pull/358/files#r1633892313
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.