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 EIP-5748: Approval Expiration for EIP-20 Tokens #5748
Add EIP-5748: Approval Expiration for EIP-20 Tokens #5748
Changes from 20 commits
7781c2c
83890f9
fd971da
9127219
3ef9e71
0556b62
6b9c8f5
314bccc
7b6cb76
aed99f0
c5817f1
51d77b1
a7d068a
f756e39
b74515a
506bdff
93fa230
65d3750
e6dd69c
b615e30
470912b
b0183f0
d9ffc78
96dc083
8db8319
df609c6
9a368a2
77bc9b3
a11982f
86cc023
836dc7e
6fde6e8
1846db3
abfe631
84631f1
5459e2f
d098211
a1e8bb8
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.
This link doesn't seem to exist.
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.
👀🐞🧑🔧✅
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 proposal doesn't add an expiration time, but rather a validity period after the transaction is included on-chain. I know it's a pedantic difference, but it's important from a security point of view.
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.
Where is this
_approve
coming from?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 suspect
_approve
comes from a well known EIP-721 implementation? The standard itself doesn't define it.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 motivation is good background (though it might be polite to avoid mentioning a specific company/product), but it doesn't actually convince me that an expiring approval solves these problems. Maybe add a sentence along the lines of:
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.
What does "mapped on top of" mean here?
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.
EIP-20 does not define an
_approve()
method. If you are referring to a particular implementation of EIP-20, don't. Instead, limit your specification to the visible interface of the contract and not any implementation details.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.
seen, edited
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.
@Mr-Lucky
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.
Again, the tone is off. You should state explicitly when compliant contracts emit the
AllowanceExpireTimeUpdated
event.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.
Move to Reference Implementation if you didn't intend to specify a specific behavior but only the interface.
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.
Instead of using a contract, please only include an interface. EIPs should only specify the visible portion of a contract, and leave the implementation up to implementers.
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.
@Mr-Lucky
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.
Please move this paragraph to the Backwards Compatibility section.
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.
committed