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-1344: Move to Final after Last Call Review #1994

Merged
merged 4 commits into from
Jul 29, 2019

Conversation

fubuloubu
Copy link
Contributor

@fubuloubu fubuloubu commented May 3, 2019

  • One additional reviewer added comment
  • No technical updates suggested

Moving to Final, with inclusion into Istanbul determined by EIP-1679, with discussion here.

- One additional reviewer added comment
- No technical updates suggested
EIPS/eip-1344.md Outdated Show resolved Hide resolved
EIPS/eip-1344.md Outdated Show resolved Hide resolved
@eip-automerger
Copy link

eip-automerger commented May 3, 2019

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

  • Trying to change EIP 1344 state from Last Call to Final

@axic
Copy link
Member

axic commented May 24, 2019

Oh btw, once this is merged, you won't be able to automerge changes to it.

@fubuloubu
Copy link
Contributor Author

fubuloubu commented May 24, 2019

Gotcha. Well, no one has asked for any changes to it in over a month. People just question whether they want it at all, so it's basically all or nothing at this point.

The most I'll change is to link to implementations and testing.

@axic
Copy link
Member

axic commented May 24, 2019

I think some of the rationale you have written on #2014 and on FEM are not represented in the EIP yet.

@fubuloubu
Copy link
Contributor Author

fubuloubu commented May 24, 2019

Three things I see missing:

  • This proposal is the most flexible of the 3 (4? 5?) proposed, allowing developers to make their own decision for how a change in chain ID should be handled.
  • Link to my trustless oracle contract implementation.
  • Discussion of the Plasma use case, where it makes more sense for the user to handle it themselves to synchronize with the Plasma block submission process instead of using a global oracle.

Will add

@fubuloubu
Copy link
Contributor Author

Added: #2079

@axic
Copy link
Member

axic commented Jul 17, 2019

@nicksavers @Arachnid ready to merge (given #2196 documents the state transitions we have)?

@Arachnid
Copy link
Contributor

We've been trying to remove any connection between EIP status and presence in a hard fork. After last call, EIPs move to Final, not to Accepted. To get to Final, the author needs to list any objections that were raised during last call and how they were addressed.

If the core devs decide to implement it, they add it to the relevant fork meta EIP.

@fubuloubu
Copy link
Contributor Author

@Arachnid I don't see this reflected in EIP-1 yet, is there a change coming that explains the update?

Also, what section would "Last Call Updates" be listed under?

@Arachnid
Copy link
Contributor

What do you mean by "what section would last call updates be listed under"?

@fubuloubu
Copy link
Contributor Author

To get to Final, the author needs to list any objections that were raised during last call and how they were addressed.

This sentence.

@Arachnid
Copy link
Contributor

That's the requirement. I don't understand what you're asking, though. Section of what?

@ethereum ethereum deleted a comment from Gezegorz1 Jul 23, 2019
@fubuloubu
Copy link
Contributor Author

Where does the "list of any objections that were raised" get recorded?

@Arachnid
Copy link
Contributor

It doesn't get recorded, except insofar as Github PR comments are persistent. But the author needs to show justification that the requirements for moving to last call were met, and the PR requesting the change is a sensible place to do that.

@fubuloubu
Copy link
Contributor Author

fubuloubu commented Jul 23, 2019

Outside of the comments on this PR, there was nothing actionable besides adding some additional detail to the "Rationale" section to make key considerations for users of this opcode more apparent (which was resolved in #2079).

Discussions from the latest ACD call and the gitter chat mostly resolved around implementation decisions for this particular opcode, which I believe are out of scope of this proposal?


Edit: I provided an implementation in the proposal, but the consensus seems to be to try and handle it in a different way, which will be reflected in the client code if accepted for Istanbul.

@fubuloubu
Copy link
Contributor Author

Full change summary of this proposal after Move to Last Call:

@Arachnid
Copy link
Contributor

As I said earlier - we're no longer using 'accepted' to track hard fork state, and that would in any case require all core devs approval. The next state after 'last call' is 'final' - feel free to edit this PR to that instead.

@fulldecent
Copy link
Contributor

I raised numerous arguments during last call and I believe [the summary above] #1994 (comment) accurately addresses every technical issue I have seen and they provide clarifications to the specification without doing anything that counts as a material change.

Good to merge, after switch from ACCEPTED to FINAL.

@fubuloubu
Copy link
Contributor Author

@Arachnid this was discussed/approved on ACD for implementation into Istanbul, so should this standard wait to push to Final until after implementation into Istanbul? Or is that status now decoupled from client implementation in a process simplification?

@Arachnid
Copy link
Contributor

@fubuloubu In theory, nothing can be in a fork unless it's final. In practice, we're not handling things nearly that cleanly.

@fubuloubu fubuloubu changed the title EIP-1344: Move to Accepted after Last Call Review EIP-1344: Move to Final after Last Call Review Jul 29, 2019
@Arachnid Arachnid merged commit fbb6f37 into ethereum:master Jul 29, 2019
@fubuloubu fubuloubu deleted the patch-2 branch July 30, 2019 03:55
ilanolkies pushed a commit to ilanolkies/EIPs that referenced this pull request Nov 12, 2019
* EIP-1344: Move to Accepted after Last Call Review

- One additional reviewer added comment
- No technical updates suggested

* EIP-1344: Remove review period

* EIP-1344: Update status to Final
MadeofTin pushed a commit to MadeofTin/EIPs that referenced this pull request Nov 13, 2019
* EIP-1344: Move to Accepted after Last Call Review

- One additional reviewer added comment
- No technical updates suggested

* EIP-1344: Remove review period

* EIP-1344: Update status to Final
BelfordZ pushed a commit to BelfordZ/EIPs that referenced this pull request Dec 13, 2019
* EIP-1344: Move to Accepted after Last Call Review

- One additional reviewer added comment
- No technical updates suggested

* EIP-1344: Remove review period

* EIP-1344: Update status to Final
tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
* EIP-1344: Move to Accepted after Last Call Review

- One additional reviewer added comment
- No technical updates suggested

* EIP-1344: Remove review period

* EIP-1344: Update status to Final
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