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

EIP-831: Standard URL Format #831

Merged
merged 7 commits into from
Jan 25, 2018
Merged

EIP-831: Standard URL Format #831

merged 7 commits into from
Jan 25, 2018

Conversation

ligi
Copy link
Member

@ligi ligi commented Jan 15, 2018

as discussed with @nagydani - extracting the container format from #681 for a cleaner future

cc @rmeissner

@ligi ligi force-pushed the url_format branch 2 times, most recently from efa7473 to 7f9ab5f Compare January 15, 2018 15:05
@ligi ligi changed the title WIP: URL Format EIP-831: Standard URL Format Jan 15, 2018
EIPS/eip-831.md Outdated
Type: Standard Track
Category: ERC
Status: Draft
Replaces: 67
Copy link
Member

Choose a reason for hiding this comment

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

@nicksavers is this how to refer to the original GitHub issue? Or should the issue be mentioned in Motivation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@nicksavers nicksavers Jan 19, 2018

Choose a reason for hiding this comment

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

Does this refer to issue #67?
It doesn't really replace a previously adopted EIP, so I don't see the need for adding it to the EIP. Besides, that issue is still open. If this PR is supposed to supersede it, shouldn't it be closed? Yes, some EIPs contain previous discussions in the references, which is where you could add the link to GitHub for historic purposes.

Copy link
Member Author

Choose a reason for hiding this comment

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

was inspired by how it was done in 681 - will remove it then


request = "ethereum" ":" [ prefix "-" ] payload
prefix = STRING
payload = STRING
Copy link
Member

Choose a reason for hiding this comment

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

Nitpicking:

How to distinguish

payload = "with-hyphen"
request = "ethereum:with-hyphen"

against

prefix = "with"
payload = "hyphen"
request = "ethereum:with-hyphen"

?

Perhaps, if prefix is omitted, payload can contain no "-"s?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. We can use the property of 681 that if the prefix is omitted the payload must start with 0x (be an address) - will add this constraint - thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

Dam - doing this I noticed that it is not working as the address can now also be a ens-domain. And AFAIK the ens-domain can contain a "-" - so this is really a problem - think then we really need to prefix ens - think this is a good idea anyway for future updates - @nagydani - what do you think - or do you have a better idea?

EIPS/eip-831.md Outdated

### Semantics

`prefix` is optional and defines the use-case for this URL. If no prefix is given "pay-" is assumed to be concise and ensure backward compatibility to ERC-67
Copy link
Member

Choose a reason for hiding this comment

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

A period is missing.

ERC-67 was used but never merged - so there can be no replacemenmt
Using Rationale as it is a better fit and leaving out optional Motivation
Copy link
Contributor

@nagydani nagydani left a comment

Choose a reason for hiding this comment

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

I agree with both changes.

@ligi
Copy link
Member Author

ligi commented Jan 23, 2018

@pirapira your change requests should be fulfilled now - thanks for them & let me know if there is anything else I can do to get this one merged

EIPS/eip-831.md Outdated
@@ -28,14 +28,14 @@ Ethereum URLs contain "ethereum" in their schema (protocol) part and are constru

### Semantics

`prefix` is optional and defines the use-case for this URL. If no prefix is given "pay-" is assumed to be concise and ensure backward compatibility to ERC-67.
`prefix` is optional and defines the use-case for this URL. If no prefix is given: "pay-" is assumed to be concise and ensure backward compatibility to ERC-67. When the prefix is omitted - the payload must start with `0x`. Also prefixes must not start with `0x`. So starting with `0x` can be used as a clear signal that there is no prefix.
Copy link
Member

Choose a reason for hiding this comment

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

Is the first sentence intended as:

When the prefix is omitted, ...

(notice , instead of -).

EIPS/eip-831.md Outdated

`payload` is mandatory and the content depends on the prefix. Structuring of the content is defined in the ERC for the specific use-case and not in the scope of this document. One example is ERC-681 for the pay- prefix.


## Rationale

The need for this ERC emerged when refining ERC-681. We need a container that does not carry the weight of the use-cases. ERC-67 was the first standard defining Ethereum-URLs - this ERC tries to keep backward compatibibility and not break existing flows. This means ERC-67 URLs should still be valid and readable. But if the prefix feature is used - ERC-67 parsers might break.
The need for this ERC emerged when refining ERC-681. We need a container that does not carry the weight of the use-cases. ERC-67 was the first attempt on defining Ethereum-URLs - this ERC tries to keep backward compatibibility and not break existing flows. This means ERC-67 URLs should still be valid and readable. But if the prefix feature is used - ERC-67 parsers might break.
Copy link
Member

Choose a reason for hiding this comment

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

In this paragraph, the first - might be a semicolon ; and the second - might be a comma ,.

@pirapira
Copy link
Member

Final editor-like nitpicks.

@ligi
Copy link
Member Author

ligi commented Jan 24, 2018

@pirapira thanks - addressed the editor-like nitpicks - text not that dashy anymore now ;-)

@pirapira
Copy link
Member

Now I found a solution to how to add a link to ERC-67. There is one good way to add links for EIPs that only exist as issues, like this:

https://github.com/ethereum/EIPs/pull/824/files

@ligi
Copy link
Member Author

ligi commented Jan 25, 2018

@pirapira good idea - added 76 and 681 as references

@pirapira pirapira merged commit 71d6f0b into ethereum:master Jan 25, 2018
nagydani added a commit to nagydani/EIPs that referenced this pull request Feb 19, 2018
ligi added a commit to ligi/EIPs that referenced this pull request May 18, 2018
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.

4 participants