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

Resurrect EIP-831 and add "eth" shorhand #5335

Merged
merged 2 commits into from
Aug 5, 2022

Conversation

ligi
Copy link
Member

@ligi ligi commented Jul 25, 2022

This EIP was stagnant but used - was contacted by @3esmit to resurrect it and add a eth shorthand as alternative to the verbose "ethereum" scheme.
Resurrecting is a no-brainer - the shortcut could use some discussion as it might be problematic e.g. when registering the scheme - so this might need a check.
Let's discuss it here when there is time: https://github.com/ethereum-cat-herders/EIPIP/issues/159

@eth-bot eth-bot enabled auto-merge (squash) July 25, 2022 10:59
@eth-bot
Copy link
Collaborator

eth-bot commented Jul 25, 2022

All tests passed; auto-merging...

(pass) eip-831.md

classification
updateEIP
  • passed!

@github-actions
Copy link

The commit de43874 (as a parent of 2f6a18e) contains errors. Please inspect the Run Summary for details.

EIPS/eip-831.md Outdated Show resolved Hide resolved
auto-merge was automatically disabled July 25, 2022 12:07

Head branch was pushed to by a user without write access

@github-actions
Copy link

The commit 1940ca7 (as a parent of e72e161) contains errors. Please inspect the Run Summary for details.

@github-actions
Copy link

The commit cfe895c (as a parent of 523b3dc) contains errors. Please inspect the Run Summary for details.

@github-actions
Copy link

The commit 71faf45 (as a parent of 7f67a4f) contains errors. Please inspect the Run Summary for details.

@github-actions
Copy link

The commit 7b7bd5d (as a parent of 8860895) contains errors. Please inspect the Run Summary for details.

@github-actions
Copy link

The commit 6afacde (as a parent of 21493e9) contains errors. Please inspect the Run Summary for details.

@github-actions
Copy link

The commit 48bcf7d (as a parent of fda1851) contains errors. Please inspect the Run Summary for details.

EIPS/eip-831.md Outdated
`prefix` is optional and defines the use-case for this URI. 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.

`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.
`prefix` is optional and defines the use-case for this URI. If no prefix is given: "pay-" is assumed to be concise and ensure backward compatibility to [EIP-67](https://github.com/ethereum/EIPs/issues/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
Contributor

Choose a reason for hiding this comment

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

There is no EIP-67, so you cannot link to EIP-67. Recommend just removing this comment about compatibility. You can't be backward incompatible with a thing that doesn't exist. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I really want to leave this in as it gives important context.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`prefix` is optional and defines the use-case for this URI. If no prefix is given: "pay-" is assumed to be concise and ensure backward compatibility to [EIP-67](https://github.com/ethereum/EIPs/issues/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.
`prefix` is optional and defines the use-case for this URI. If no prefix is given: "pay-" is assumed to be concise and ensure backward compatibility with other standards. 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.

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

What other standards are being referred to by "with other standards"? If there are no standards then this feels like a vacuous statement.

Perhaps instead, "backward compatibility with existing contracts in the ecosystem" to make it clear that there are no existing standards but there does exist contracts in the wild that there is a desire to be compatible with?

Copy link
Member

Choose a reason for hiding this comment

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

What other standards are being referred to by "with other standards"? If there are no standards then this feels like a vacuous statement.

There are other standards out there that aren't EIPs, or even official standards. "EIP-223" (not an EIP) is a good example of this.

Copy link
Contributor

Choose a reason for hiding this comment

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

At best, that is a defacto standard which I personally wouldn't call a standard.

@github-actions
Copy link

The commit aca98a5 (as a parent of 991305f) contains errors. Please inspect the Run Summary for details.

@github-actions
Copy link

The commit c81e2fa (as a parent of 5503035) contains errors. Please inspect the Run Summary for details.

EIPS/eip-831.md Outdated
`prefix` is optional and defines the use-case for this URI. 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.

`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.
`prefix` is optional and defines the use-case for this URI. If no prefix is given: "pay-" is assumed to be concise and ensure backward compatibility to a former URL format by @alexvandesande that was commonly used but never formalized and so cannot be mentioned by it's name without making the EIPW bot angry. 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
Contributor

Choose a reason for hiding this comment

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

Come on, the rules aren't that bad 🤣

You could try:

Suggested change
`prefix` is optional and defines the use-case for this URI. If no prefix is given: "pay-" is assumed to be concise and ensure backward compatibility to a former URL format by @alexvandesande that was commonly used but never formalized and so cannot be mentioned by it's name without making the EIPW bot angry. 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.
`prefix` is optional and defines the use-case for this URI. If no prefix is given: "pay-" is assumed to be concise and ensure backward compatibility with preexisting implementations (including a draft EIP by @alexvandesande). 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.

If you want to keep this text literally, I suggest a small grammar edit:

Suggested change
`prefix` is optional and defines the use-case for this URI. If no prefix is given: "pay-" is assumed to be concise and ensure backward compatibility to a former URL format by @alexvandesande that was commonly used but never formalized and so cannot be mentioned by it's name without making the EIPW bot angry. 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.
`prefix` is optional and defines the use-case for this URI. If no prefix is given: "pay-" is assumed to be concise and ensure backward compatibility to a former URL format by @alexvandesande that was commonly used but never formalized and so cannot be mentioned by its name without making the EIPW bot angry. 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
Contributor

Choose a reason for hiding this comment

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

I'm not personally a fan of name dropping like that. It doesn't add value to some future implementer who wants to implement this specification.

EIPS/eip-831.md Outdated
`prefix` is optional and defines the use-case for this URI. 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.

`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.
`prefix` is optional and defines the use-case for this URI. If no prefix is given: "pay-" is assumed to be concise and ensure backward compatibility to a former URL format by @alexvandesande (#67) that was commonly used but never formalized and so cannot be mentioned by it's name without making the EIPW bot angry. 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
Contributor

@MicahZoltu MicahZoltu Jul 25, 2022

Choose a reason for hiding this comment

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

Suggested change
`prefix` is optional and defines the use-case for this URI. If no prefix is given: "pay-" is assumed to be concise and ensure backward compatibility to a former URL format by @alexvandesande (#67) that was commonly used but never formalized and so cannot be mentioned by it's name without making the EIPW bot angry. 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.
`prefix` is optional and defines the use-case for this URI. If no prefix is given: "pay-" is assumed to be concise and ensure backward compatibility with a common pattern used on Ethereum mainnet prior to the introduction of this standard. 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 Author

Choose a reason for hiding this comment

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

Thanks - but it was never used on "Ethereum mainnet" as this is used on another layer and not really on a network - and seems I can reference EIP-67 again soon as for: #5338
This kinda solves it.

Copy link
Contributor

Choose a reason for hiding this comment

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

IF that gets approved by the author you can reference EIP-67. Note that this PR will be blocked from merging until then though, and be prepared for that to never happen as many EIP authors never respond. 😢

@ligi ligi force-pushed the resurrect_eip831 branch from 6d5a2f4 to 6afacde Compare July 26, 2022 11:52
@ligi
Copy link
Member Author

ligi commented Jul 26, 2022

I removed the commits that tried to trick EIPW by not mentioning EIP-67 as @SamWilsn now submitted a PR toadd EIP-67 so we can reference it

@github-actions
Copy link

The commit 6afacde (as a parent of 0f01818) contains errors. Please inspect the Run Summary for details.

@ligi ligi force-pushed the resurrect_eip831 branch from 6afacde to 4424ba5 Compare July 27, 2022 17:20
@ligi
Copy link
Member Author

ligi commented Jul 27, 2022

Rebased to master so it picks up EIP-67 now - so from my side it would be good to go

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.

On the requires line, there is active discussion among the editors to remove this requirement, but if you want to get this merged sooner rather than later you'll need to require any referenced EIPs.

EIPS/eip-831.md Show resolved Hide resolved
Co-authored-by: Micah Zoltu <micah@zoltu.net>
@SamWilsn
Copy link
Contributor

SamWilsn commented Aug 5, 2022

@ligi can you close / re-open this to get the renamed (:rage:) eipw check to pass?

@MicahZoltu MicahZoltu closed this Aug 5, 2022
@MicahZoltu MicahZoltu reopened this Aug 5, 2022
@eth-bot eth-bot enabled auto-merge (squash) August 5, 2022 19:41
@eth-bot eth-bot merged commit 642b991 into ethereum:master Aug 5, 2022
nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
* Resurrect EIP-831 and add "eth" shorhand

* Update EIPS/eip-831.md

Co-authored-by: Micah Zoltu <micah@zoltu.net>

Co-authored-by: Micah Zoltu <micah@zoltu.net>
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.

5 participants