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

Adding EIP for described data #3224

Merged
merged 29 commits into from
Feb 17, 2021
Merged

Conversation

ricmoo
Copy link
Contributor

@ricmoo ricmoo commented Jan 27, 2021

Adding initial draft for described data to allocate an EIP so a discussion-to URL can be added in the issues of the EIP repo.

Abstract

Human-readable descriptions for machine executable operations,
described in higher level machine readable data, so that wallets
can provide meaningful feedback to the user describing the
action the user is about to perform.

@ricmoo ricmoo mentioned this pull request Jan 27, 2021
EIPS/eip-3224.md Outdated Show resolved Hide resolved
EIPS/eip-3224.md Outdated Show resolved Hide resolved
Copy link
Contributor

@MicahZoltu MicahZoltu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only real blocker is the footer, which should be deleted. If you aren't ready to merge this as a draft you can mark the PR as a draft PR and then editors won't look at it.

The rest of the feedback I encourage reviewing. We also discussed in Discord a lot of it and I think we generally are in agreement for most things, but that is largely unrelated to my editorial duties.

EIPS/eip-3224.md Outdated Show resolved Hide resolved
EIPS/eip-3224.md Show resolved Hide resolved
EIPS/eip-3224.md Show resolved Hide resolved
EIPS/eip-3224.md Outdated Show resolved Hide resolved
EIPS/eip-3224.md Outdated Show resolved Hide resolved
EIPS/eip-3224.md Outdated Show resolved Hide resolved
@ricmoo
Copy link
Contributor Author

ricmoo commented Jan 29, 2021

Thanks @MicahZoltu! I’ll get these updates made today. :)

@ricmoo
Copy link
Contributor Author

ricmoo commented Feb 16, 2021

It took longer to get to than expected. But I've addressed the reviews above.

I'll also start two discussions on the discussion-to link; localization and splitting the EIP up. @Arachnid and I have been having some conversations regarding this, so it would be good to get them out for others to comment on.

@ricmoo
Copy link
Contributor Author

ricmoo commented Feb 17, 2021

I've resolved all the conversations. Let me know if there is something else that is needed.

Thanks! :)

Comment on lines +405 to +410
- HTML could be enbedded to attempt to trick web-based wallets into [executing code](https://en.wikipedia.org/wiki/Code_injection) using the script tag (possibly uploading any private keys to a server)
- In general, extreme care must be used when rendering HTML; consider the ENS names `<span style="display:none">not-</span>ricmoo.eth` or `&thinsp;ricmoo.eth`, which if rendered without care would appear as `ricmoo.eth`, which it is not
- Other marks which require escaping could be included, such as quotes (`"`), formatting (`\n` (new line), `\f` (form feed), `\t` (tab), any of many [non-standard whitespaces](https://en.wikipedia.org/wiki/Whitespace_character#Spaces_in_Unicode)), back-slassh (`\`)
- UTF-8 has had bugs in the past which could allow arbitrary code execution and [crashing renderers](https://osxdaily.com/2015/05/27/bug-crashes-messages-app-ios-workaround/); consider using the UTF-8 replacement character (or *something*) for code-points outside common planes or common sub-sets within planes
- [Homoglyphs attacks](https://en.wikipedia.org/wiki/IDN_homograph_attack)
- [Right-to-left](https://en.wikipedia.org/wiki/Right-to-left_mark) mark may affect rendering
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

External links should be avoided whenever possible, as they tend to 404 over time and we don't have the manpower to maintain dead links. I think most of these are easily searchable, so no need to link to them (naming them is fine).

@MicahZoltu MicahZoltu merged commit 90aa954 into ethereum:master Feb 17, 2021
@ethereum ethereum deleted a comment Feb 18, 2021
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
Human-readable descriptions for machine executable operations, described in higher level machine readable data, so that wallets can provide meaningful feedback to the user describing the action the user is about to perform.
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.

3 participants