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

Publish EIP-67 as withdrawn #5338

Merged
merged 2 commits into from
Jul 26, 2022
Merged

Conversation

SamWilsn
Copy link
Contributor

@ligi wants to refer to this EIP in EIP-831.

@alexvandesande this'll need your approval. Two important things to note:

  • This text will be made available under CC0 (basically public domain.)
  • Withdrawn EIPs are immutable: be completely sure you want this withdrawn.

@eth-bot
Copy link
Collaborator

eth-bot commented Jul 25, 2022

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):


(fail) eip-67.md

classification
updateEIP

@@ -0,0 +1,80 @@
---
eip: 67
Copy link
Contributor Author

Choose a reason for hiding this comment

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

title: URI Scheme with Metadata, Value and Bytecode
description: Format for encoding transactions into a URI
author: Alex Van de Sande (@alexvandesande)
discussions-to: https://github.com/ethereum/EIPs/issues/67
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, this should be grandfathered in.

@github-actions
Copy link

The commit 88253b2 (as a parent of 9a1ae5b) contains errors. Please inspect the Run Summary for details.

Copy link
Member

@ligi ligi left a comment

Choose a reason for hiding this comment

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

thanks!

Comment on lines +70 to +76
## Rationale

TODO

## Security Considerations

TODO
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
## Rationale
TODO
## Security Considerations
TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aren't these both required sections, that we normally allow with only TODO in the draft state?

Copy link
Member

Choose a reason for hiding this comment

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

I guess. It seems weird to be terminally putting them in as a "TODO" that can never be done.

EIPS/eip-67.md Outdated
type: Standards Track
category: ERC
created: 2016-02-17
withdrawal-reason: Superseded by EIP-831

This comment was marked as outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, is markdown supported in the preamble? I've always assumed it wasn't!

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it isn't. This is fine.

This is the same function as above, to send 5 unicorns from he sender to _deadbeef_, but now with a more readable function, which the client converts to bytecode.

```
ethereum:0x89205A3A3b2A69De6Dbf7f01ED13B2108B2c43e7?gas=100000&function=transfer(address 0xdeadbeef, uint 5)
Copy link
Member

Choose a reason for hiding this comment

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

function isn't included in the spec?!

@alexvansande
Copy link

I approve this. However notice that I don't have access to the old account @alexvandesande but instead this is the new one @alexvansande. You can check that the history matches. Not sure if the bot will allow it.

EIPS/eip-67.md Outdated Show resolved Hide resolved
@Pandapip1
Copy link
Member

Huh, this isn't the expected output. There should have to be at least one approving editor.
image

@MicahZoltu
Copy link
Contributor

An editor submitted the PR, and the system counts editor author as an "editor approval" at the moment. Same reason why the bot will auto-merge PRs by editors in some cases I believe.

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
@github-actions
Copy link

The commit 9c6ac2e (as a parent of ff00f96) contains errors. Please inspect the Run Summary for details.

@Pandapip1
Copy link
Member

An editor submitted the PR, and the system counts editor author as an "editor approval" at the moment. Same reason why the bot will auto-merge PRs by editors in some cases I believe.

This isn't the case though. I have two open PRs to create ERCs that need editor approval (despite the fact that I am an ERC editor)

#5289
#5219

@SamWilsn SamWilsn marked this pull request as ready for review July 25, 2022 20:15
@eth-bot eth-bot enabled auto-merge (squash) July 25, 2022 20:15
@SamWilsn
Copy link
Contributor Author

You'll need to approve this whenever you're ready @alexvansande.

@MicahZoltu
Copy link
Contributor

This isn't the case though. I have two open PRs to create ERCs that need editor approval (despite the fact that I am an ERC editor)

I think the way it works is that you need 1 editor approval and 1 author approval. The person who submits the PR can fulfill either the editor approval requirement or the author approval requirement but not both. In this case, Sam has fulfilled the editor approval requirement but we still need author approval requirement fulfilled. In your example cases, you have fulfilled the author approval requirement, but we still need the editor approval requirement fulfilled.

@Pandapip1
Copy link
Member

Pandapip1 commented Jul 26, 2022

@alexvansande this is ready for approval.

@lightclient lightclient disabled auto-merge July 26, 2022 15:13
@lightclient lightclient merged commit 6765f08 into ethereum:master Jul 26, 2022
@SamWilsn SamWilsn deleted the withdraw-eip-67 branch July 26, 2022 15:14
nachomazzara pushed a commit to nachomazzara/EIPs that referenced this pull request Jan 13, 2023
* Publish EIP-67 as withdrawn

* Change author to new username

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>

Co-authored-by: Pandapip1 <45835846+Pandapip1@users.noreply.github.com>
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.

7 participants