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

Data structure for the REVERT instruction #207

Closed
wants to merge 4 commits into from

Conversation

axic
Copy link
Member

@axic axic commented Feb 6, 2017

Depends on #206.

@vbuterin
Copy link
Contributor

vbuterin commented Feb 9, 2017

No proliferation of many different encoding standards please. We have RLP and ABI, those two are enough. And given the use of ABI everywhere else in-EVM, I am strongly inclined to favor ABI.

@miohtama
Copy link

miohtama commented Feb 9, 2017

Does the architecture allow more than two error arguments?

If possible I'd suggest a third return argument

  • The first argument defines the type of the error and that may map to different Exception classes in high level languages

  • The second argument is human readable message, akin Exception.message in Python

  • The third argument would give a machine readable data that gives more run-time error details in the form of root cause of the revert. This could be e.g. a nested error in the case the error happened in an inter contract calls. This way one would be able chain different errors together. This is similar to how Java and Python do nested exception handling. The ability to trace errors to their root cause becomes very important in the complex systems with interdependencies, especially if some of those dependencies are smart contract that the developers themselves cannot control.

@axic
Copy link
Member Author

axic commented Feb 9, 2017

No proliferation of many different encoding standards please.

I agree that we should try to limit the number of competing standards being used. I wonder what was the need to invent RLP over competing formats (including CBOR, which was accepted as an RFC in 2013).

Note that CBOR already infiltrated Ethereum, Swarm is (going to?) use it for the manifests as well as Solidity uses it for the metadata hash.

My main reason for CBOR is it's flexibility to represent multiple return types:

  • simple string
  • array with two members (as in the EIP now, number + strings)
  • array with three members (as proposed by @miohtama)

RLP can be used to determine the length of arrays, but it won't support different data types within. The ABI encoding is very rigid, we'd need to define a fixed field for type or length.

@Arachnid
Copy link
Contributor

Arachnid commented Feb 9, 2017

I really think that it makes the most sense for REVERT data to take the same conventional form as return data from a CALL: ABI-encoded, and defined as part of the signature of the function.

@axic
Copy link
Member Author

axic commented Feb 9, 2017

Does the architecture allow more than two error arguments?

This is really above the level of the EVM; the EVM only cares about returning a blob of memory. Compilers can decide on whatever standard and ABI specification they all agree on.

That is the scope of this proposal.

@Arachnid
Copy link
Contributor

Arachnid commented Feb 9, 2017

That is the scope of this proposal.

Yes, I thought I was commenting on another issue. I deleted my comment when I realised I was mistaken.

@Arachnid
Copy link
Contributor

Do you still want to pursue this? AFAIK we had decided on leaving REVERT opcode to the HLL, and defaulting to the same format as regular return data?

@axic axic changed the title Draft of data encoding of the REVERT opcode Data structure for the REVERT instruction Dec 5, 2017
@Arachnid
Copy link
Contributor

This is a courtesy notice to let you know that the format for EIPs has been modified slightly. If you want your draft merged, you will need to make some small changes to how your EIP is formatted:

  • Frontmatter is now contained between lines with only a triple dash ('---')
  • Headers in the frontmatter are now lowercase.

If your PR is editing an existing EIP rather than creating a new one, this has already been done for you, and you need only rebase your PR.

In addition, a continuous build has been setup, which will check your PR against the rules for EIP formatting automatically once you update your PR. This build ensures all required headers are present, as well as performing a number of other checks.

Please rebase your PR against the latest master, and edit your PR to use the above format for frontmatter. For convenience, here's a sample header you can copy and adapt:

---
eip: <num>
title: <title>
author: <author>
type: [Standards Track|Informational|Meta]
category: [Core|Networking|Interface|ERC] (for type: Standards Track only)
status: Draft
created: <date>
---

@MicahZoltu MicahZoltu mentioned this pull request Jun 6, 2018
@nicksavers
Copy link
Contributor

@axic I get the idea that this proposal fall in the category: Interface

Includes improvements around client API/RPC specifications and standards, and also certain language-level standards like method names and contract ABIs.

What is your current position on this now that #206 has been merged?

@github-actions
Copy link

There has been no activity on this pull request for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the stale label Sep 22, 2020
@github-actions
Copy link

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants