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
Described transactions #4430
Described transactions #4430
Changes from 6 commits
0eee426
89d7bda
b85b59c
6225b92
5b5f9b9
347f3bb
fa4b3cb
165dde6
91797c7
a75d55e
f2f7dc2
2981252
90659e1
d34724f
0e7bac5
c77bde7
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.
the usual recommendation is to link to a thread in the Ethereum Magicians Discourse
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.
Should I change it? There is already some (but not much discussion) on the current link.
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.
Yes, all new EIPs should be using Ethereum Magicians. Since this is a new EIP, it should as well. We currently only make exceptions for existing EIPs (merged as draft before we moved to Ethereum Magicians) that are receiving updates or moving through status changes.
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.
Also, I don't fully understand the spec. Is there a function actually named
eip4430Describe
that needs to be present in the contract in order to leverage this EIP? I suspect not since there could only be one per contract (or per argument set?) whereas the contract may contain many, if so, what is the specification for the name (and arguments) of a EIP-4430 compliant function?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.
For now I want to avoid giving it a real name, since people might start using it before its ready and I want to avoid a
signTypeData_v4
situation.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.
Reference Implementation is optional, so this could be merged without it, but I do feel like it would be very useful for this EIP, and also figure you didn't want to leave this section quite like this - if you'd like you can still leave the latter line, but the
@TODO
should be removed imhoThere 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'd recommend reiterating the concern the point made above that contracts can implement this maliciously (purposely giving an inaccurate description)