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: Remote procedure call specification #1474

Merged
merged 11 commits into from Mar 7, 2019
Merged

Conversation

@bitpshr
Copy link
Contributor

@bitpshr bitpshr commented Oct 5, 2018

Much of Ethereum's effectiveness as an enterprise-grade application platform depends on its ability to provide a reliable and predictable developer experience. Nodes created by the current generation of Ethereum clients can expose RPC endpoints with differing method signatures; this forces applications to work around method inconsistencies to maintain compatibility with various Ethereum RPC implementations.

Both Ethereum client developers and downstream dapp developers lack a formal Ethereum RPC specification. This proposal attempts to standardize such a specification in a way that's versionable and modifiable through the traditional EIP process.

This initial draft was based on the current Ethereum RPC wiki documentation with necessary fixes and additions made along the way (such as documenting eth_signTransaction).

Note: There doesn't appear to be any RPC-friendly API documentation standard, otherwise I'd be open to using that over markdown. Still, this EIP is meant to provide a human-readable RPC specification and isn't necessarily meant to be program-readable or to drive other documentation tooling (though the markdown itself could drive tooling if desired).

Resolves #1442 (cc @Arachnid)
Resolves #136

@bitpshr bitpshr force-pushed the eip-rpc branch 2 times, most recently from 909e273 to 522bbeb Oct 5, 2018
@perpetualescap3
Copy link

@perpetualescap3 perpetualescap3 commented Oct 5, 2018

I went through each method and it looks like every data structure define correctly (return types and parameters and the required encoding) based on Geth and the current wiki. I also noticed you force-pushed an amended commit that added a section about new RPC methods in the future. Great to see! (but please try to avoid force-pushing so we maintain holistic history at least on the pull request, which will ultimately be squashed...I think.)

+1 for landing this as a draft as soon as @Arachnid or @Souptacular or any other maintainer thinks it is both worthwhile and formatted correctly (looks to be...)

Loading

@gogratta
Copy link

@gogratta gogratta commented Oct 7, 2018

I think this should be landed as it doesn't look like it changes the current wiki methods, just fixes it with better formatting and error codes.

Doing constant transaction research against different clients has been difficult for our developers - they follow the wiki "spec", it changes without any process, and not all clients even return the same values. A true RPC standard / EIP is a cornerstone for real ethereum products.

Loading

Copy link

@chaftacode chaftacode left a comment

eth_call had an incorrect return value. I see that's been fixed. LGTM. Nice formatting too 🧐

Loading

@ryanschneider
Copy link
Contributor

@ryanschneider ryanschneider commented Oct 7, 2018

I think one missing code I'd like to see is something to tell the requestor that the server recognizes the RPC in question but has specifically chosen not to support it for some reason.

This concept was first mentioned here:
#136 (comment)

I would like to add "Unsupported" error code which would be a good way for Metamask style sub-providers to tell the client to use a different provider. Not sure if NOT FOUND can be used in this instance...

And we would be happy to use something similar for Infura to let end-users know that a particular method is not supported on our platform. It would be ideal if service providers could return an standard JSONRPC error response w/ a specific code like -32xxx method unsupported on this platform so that client libraries can respond accordingly. Basically this would be an error that can tells the client "yes the node recognized your request as a valid Ethereum RPC listed in this EIP, but we are rejecting it for the reason stated in the error message".

Loading

@bitpshr
Copy link
Contributor Author

@bitpshr bitpshr commented Oct 7, 2018

Hi @ryanschneider, thanks very much for the feedback. Considering the different sets of methods exposed by different Ethereum clients, I think the suggested error code is valid and I agree that it should be different than the standard JSON-RPC error code -32601 for Method not found.

Proposal updated accordingly.

Loading

@evalcode
Copy link

@evalcode evalcode commented Oct 8, 2018

Joined to express my frustration along with my colleagues: ethereum lacking an RPC spec that follows a W3C / EIP-style standards process almost renders it useless for real applications. At its core ethereum is RPC. Without it...applications just couldn't exist. Major companies are totally relying on a freely-editable wiki for API info to build high-value applications, or trying to, without huge success. User adoption is part of ethereum, but business adoption is also part of it, and business adoption requires stable, standards-based specs like what's presented here.

It's getting and harder to take ethereum seriously as a solution for big business at major companies, so thank you @bitpshr for this.

+1000 to land as draft so geth, partity, and others can help fine-tune over time.

Loading

@gogratta
Copy link

@gogratta gogratta commented Oct 8, 2018

@bitpshr is this ready for final review from @Arachnid or @Souptacular and friends for formatting? The community really needs this ASAP, I don't think anyone wanted to take the time to do it. Let us know if it's ready.

Loading

@bitpshr
Copy link
Contributor Author

@bitpshr bitpshr commented Oct 9, 2018

Hi @gogratta, thanks for your support of the proposal. Yes, everything should be formatted correctly and adherent to the currently-defined Ethereum JSON-RPC wiki, along with a few fixes. It should be ready for review whenever @Arachnid, @Souptacular, or any other maintainer has time.

Loading

@ryanschneider
Copy link
Contributor

@ryanschneider ryanschneider commented Oct 9, 2018

I thought of one more error case that would be useful to have an explicit code for: the concept of exceeding quotas. For example, Geth has a configurable RPC time-out, but currently just unceremoniously closes the TCP stream, would be preferable for it to return a JSONRPC error when the processing deadline is exceeded. Likewise currently there’s nothing preventing a requestor from asking for every event ever published via a poorly crafted eth_getLogs request, would be nice if the clients could prevent that and return an appropriate error. Same for max batch size and maximum payload size for requests. I think all of these cases could be covered by a single quota-related error code with proper message strings. Thoughts?

Loading

@bitpshr
Copy link
Contributor Author

@bitpshr bitpshr commented Oct 9, 2018

I think all of these cases could be covered by a single quota-related error code with proper message strings

Seems reasonable; shooting for maximum agnosticism in terms of the messaging itself, what about something like this (which can always be further-clarified by specific clients):

Code Message Meaning Category
-32005 Limit exceeded Request exceeds defined limit non-standard

@ryanschneider

Loading

EIPS/eip-rpc.md Outdated

## Simple Summary

This proposal defines a standard set of remote procedure call methods that an Ethereum node must implement.
Copy link
Member

@pipermerriam pipermerriam Oct 9, 2018

Choose a reason for hiding this comment

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

The must in this sentence my need to be changed or wording added to cover RPC implementations which don't implement all methods.

  • infura doesn't support stateful methods like eth_sign or filters.
  • trinity doesn't plan to implement any account support so eth_sendTransaction and eth_sign won't be supported.

There is discussion to add an "Unsupported" status code which I think is a good way for RPC servers to signal that they do not support an RPC method, however this sentence suggests that not implementing a method would make a JSON-RPC server implementation non-compliant which seems wrong.

  • This could be updated to just use should instead of must
  • We could define a subset of the endpoints which are required and the remaining ones are recommended (aka optional)

Open to other suggestions.

Loading

Copy link
Contributor Author

@bitpshr bitpshr Oct 16, 2018

Choose a reason for hiding this comment

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

Good catch. I changed must to should and also added an "Unsupported" status code as per @ryanschneider's suggestion above.

Loading

Copy link
Member

@pipermerriam pipermerriam left a comment

I think this needs to be expanded to cover recommended behavior and error codes for the common error cases. This is often where the ambiguity and different behavior across different implementations shows up.

  • eth_getTransactionByHash: what should the response be if no transaction is found for that hash (if error, what error code is recommended).
  • eth_getTransactionReceipt:
    • what should the behavior be if no receipt is found for the provided transaction hash (if error, what error code is recommended)
    • if the txn hash is known, but unmined should this behave differently than when the txn is just not yet mined.

Same type of questions for all of the other lookups for things like eth_getBlockByHash, etc.

Loading

EIPS/eip-rpc.md Outdated

#### Returns

{[`Data`](#data)} - signature hash of the transaction object
Copy link
Member

@pipermerriam pipermerriam Oct 9, 2018

Choose a reason for hiding this comment

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

This descriptions seems confusing. Maybe better as something like:

The RLP encoded signed transaction.

Loading

EIPS/eip-rpc.md Outdated

#### Description

Calculates an Ethereum-specific signature in the form of `keccak256("\x19Ethereum Signed Message:\n" + len(message) + message))`
Copy link
Member

@pipermerriam pipermerriam Oct 9, 2018

Choose a reason for hiding this comment

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

This description looks incorrect (probably copy/pasted from eth_sign). This should be updated to reference EIP-712

Loading

Copy link
Contributor Author

@bitpshr bitpshr Oct 16, 2018

Choose a reason for hiding this comment

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

Based on the RPC method defined in EIP-712, eth_signTypedData still calculates a hash using keccak256("\x19Ethereum Signed Message:\n" + len(message) + message)). As I understand it, the only difference between this method and eth_sign is the message itself.

Loading

Copy link

@kclowes kclowes Mar 1, 2019

Choose a reason for hiding this comment

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

@pipermerriam @bitpshr I know this conversation happened a while ago, but I came across this while pulling EIP-712 in to web3.py and wanted some feedback. The way I am reading EIP-712 is that eth_signTypedData does not necessarily append \x19Ethereum Signed Message:\n, whereas eth_sign does. I'm looking at this part of the specification. I wonder if the link to the eth_signTypedData spec above is in fact a copy paste error, because it says the same thing verbatim in the eth_sign RPC docs.

Also, let me know if there is a better place for this discussion.

Loading

Copy link
Member

@pipermerriam pipermerriam Mar 1, 2019

Choose a reason for hiding this comment

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

@kclowes based on my recolection of the spec, I believe you are correct. the \x19Ethereum Signed Message is indeed not part of either of the new signing EIPs.

Loading

EIPS/eip-rpc.md Outdated

Destroys a filter based on filter ID

**Note:** This should only be called if a filter and its notifications are no longer needed. This will also be called automatically on a filter if its notifications are not retrieved using `eth_getFilterChanges` for a period of time.
Copy link
Member

@pipermerriam pipermerriam Oct 9, 2018

Choose a reason for hiding this comment

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

It seems beneficial to provide a recommended timeout length here so that different clients are more likely to use similar timeouts.

Loading

Copy link
Contributor Author

@bitpshr bitpshr Oct 16, 2018

Choose a reason for hiding this comment

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

Great catch. The geth source code uses 5 minutes, so I will go with that for now.

Loading

EIPS/eip-rpc.md Outdated
- {[`Data[]`](#data)} `topics` - list of order-dependent topics
- {`boolean`} `removed` - `true` if this filter has been destroyed and is invalid

**Note:** The return value of `eth_getFilterChanges` when retrieving logs from `eth_newBlockFilter` and `eth_newPendingTransactionFilter` filters will be an array of hashes, not an array of Log objects.
Copy link
Member

@pipermerriam pipermerriam Oct 9, 2018

Choose a reason for hiding this comment

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

Since filters can timeout if not queried regularly, it seems appropriate to provide a recommended error code for clients to use when the provided filder_id is not found.

Loading

Copy link
Contributor Author

@bitpshr bitpshr Oct 16, 2018

Choose a reason for hiding this comment

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

Another good catch. I believe geth currently returns -32000 - Invalid input for this, but I feel that -32001 - Resource not found is more appropriate.

Loading

@ryanschneider
Copy link
Contributor

@ryanschneider ryanschneider commented Oct 9, 2018

RE:

Code Message Meaning Category
-32005 Limit exceeded Request exceeds defined limit non-standard

Looks perfect to me @bitpshr, thanks!

Loading

@dekz
Copy link

@dekz dekz commented Oct 17, 2018

@ryanschneider would love to discuss eth_call and some other query functions and their behaviour. They often differ in Geth and Parity and as a library writer it's better to work towards standardised behaviour than work arounds and detecting client behaviours.

I've started with eth_call in the ethereum magicians forum. Would love to hear your thoughts.

Loading

@cdetrio
Copy link
Member

@cdetrio cdetrio commented Oct 17, 2018

In the long run, I think a machine-readable spec (like the one I proposed here #217) will be less burdensome to maintain. I left a comment on the EthMagi forum with more explanation: https://ethereum-magicians.org/t/eip-remote-procedure-call-specification/1537/15?u=cdetrio

Loading

@pipermerriam
Copy link
Member

@pipermerriam pipermerriam commented Oct 17, 2018

@cdetrio

I'm not sure if you were proposing having the machine readable spec be the authoritative spec, but if you are, my vote would be 👎

  1. There are normally rules that cannot be expressed using JSON-schema. This likely applies to most alternatives to JSON-schema.
  2. It's much harder to review a machine readable spec
  3. It requires more expertise to contribute to a machine readable spec.
  4. It's harder to express "why" or provide context in machine readable specs.

I'd propose deriving a machine readable from a natural language spec (like it is currently written) once we've finalized this ERC. JSON-schema seems like a good candidate, upon which someone could build a Swagger spec if they were feeling especially ambitious.

Loading

@cdetrio
Copy link
Member

@cdetrio cdetrio commented Oct 17, 2018

  1. There are normally rules that cannot be expressed using JSON-schema. This likely applies to most alternatives to JSON-schema.

I use the term "spec" loosely. The JSON schemas I proposed are basically a set of example inputs and outputs, not unlike the examples in the wiki and this PR.

  1. It's much harder to review a machine readable spec

Having more people to review the RPC spec doesn't directly help solve the problem stated in the PR:

Nodes created by the current generation of Ethereum clients can expose RPC endpoints with differing method signatures; this forces applications to work around method inconsistencies to maintain compatibility with various Ethereum RPC implementations.

We can directly address this problem with a machine-readable spec, because that enables us to automatically check that clients are compatible and consistent.

  1. It's harder to express "why" or provide context in machine readable specs.

That's what comments are for.

I'd propose deriving a machine readable from a natural language spec (like it is currently written) once we've finalized this ERC. JSON-schema seems like a good candidate, upon which someone could build a Swagger spec if they were feeling especially ambitious.

What's written is based on the wiki, and so are the JSON schemas (and comments within) that I proposed. It isn't so much my schema format that I'm trying to advocate, but that the "specification" come with machine-readable test cases. Then we only have to maintain a test runner to ensure that clients are compatible with the spec or at least know where they deviate, which will be a lot easier than relying on humans to sort it out manually from something human-readable (that's what led us to the current situation where the wiki and clients are all inconsistent with each other).

What can we do to make it easier to derive machine readable test cases from this natural language documentation? My goal with #217 was to do it the other way around (generate wiki docs from comments in the schema). But I'll be happy with either, so long as we have test cases!

Loading

@pipermerriam
Copy link
Member

@pipermerriam pipermerriam commented Oct 17, 2018

What can we do to make it easier to derive machine readable test cases from this natural language documentation?

Here's my best stab at a starting point (I'm willing to own both coordination as well as basic execution of this plan)

  1. let this ERC and any others which get defined be the authoritative source of truth for the spec.
  2. in a separate repository which I'll refer to as ethereum/json-rpc-tests
  • define the JSON-schema documents for each endpoint. Bonus points if we can also include a list of requirements for each endpoint which are not tested via the JSON-schema and which must be validated via some secondary mechanism.
  • a list of test cases in a generic format (probably JSON) that chain setup, RPC method & payload and expected response. Exact format to be determined. High likelyhood we'll borrow heavily from the existing ethereum/tests format.
  1. basic documentation which explains the layout and intended way these schemas and tests should be applied

We should also be able to write a generic tool which is easy enough to install in a CI environment which can be configured with a JSON-RPC server's connection info which will run these tests. Bonus points for an option to fuzz the endpoints.

Loading


Example error response:

```sh
Copy link
Collaborator

@MicahZoltu MicahZoltu Oct 25, 2018

Choose a reason for hiding this comment

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

Suggested change
```sh
```json

Loading

Copy link
Collaborator

@MicahZoltu MicahZoltu Oct 25, 2018

Choose a reason for hiding this comment

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

Same for all other code blocks. In some cases, there is a shell script and JSON in a single code block, recommend splitting into two code blocks.

Loading

@bmann
Copy link
Contributor

@bmann bmann commented Oct 27, 2018

Talking to @egalano that Infura has a JSON-RPC test suite that might be able to be deployed for public community testing. Essentially, a validator for clients' JSON-RPC interfaces.

Loading

@bitpshr bitpshr closed this Nov 5, 2018
@bitpshr bitpshr deleted the eip-rpc branch Nov 5, 2018
@ryanschneider
Copy link
Contributor

@ryanschneider ryanschneider commented Nov 5, 2018

Why was this closed, has this EIP been superseded by another?

Loading

@bitpshr bitpshr restored the eip-rpc branch Nov 5, 2018
@bitpshr bitpshr reopened this Nov 5, 2018
- {[`Quantity`](#quantity)} `logIndex` - index of this log within its block or `null` if pending
- {[`Quantity`](#quantity)} `transactionIndex` - index of the transaction that created this log or `null` if pending
- {[`Data[]`](#data)} `topics` - list of order-dependent topics
- {`boolean`} `removed` - `true` if this filter has been destroyed and is invalid
Copy link

@lithp lithp Nov 8, 2018

Choose a reason for hiding this comment

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

This seems to be a copy-paste error:

Suggested change
- {`boolean`} `removed` - `true` if this filter has been destroyed and is invalid
- {`boolean`} `removed` - `true` when the log was removed, due to a chain reorganization. `false` if it is a valid log.

Loading

- {[`Quantity`](#quantity)} `size` - size of this block in bytes
- {[`Quantity`](#quantity)} `timestamp` - unix timestamp of when this block was collated
- {[`Quantity`](#quantity)} `totalDifficulty` - total difficulty of the chain until this block
- {`Array<Transaction>`} `transactions` - list of transaction objects or hashes
Copy link

@lithp lithp Nov 8, 2018

Choose a reason for hiding this comment

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

It's not clear what a Transaction object is, that's defined implicitly by the eth_getTransactionBy* sections. This EIP would be easier to read if the definition of Transaction was pulled out and explicitly named somewhere.

Loading

- {[`Quantity`](#quantity)} `timestamp` - unix timestamp of when this block was collated
- {[`Quantity`](#quantity)} `totalDifficulty` - total difficulty of the chain until this block
- {`Array<Transaction>`} `transactions` - list of transaction objects or hashes
- {`Array<Transaction>`} `uncles` - list of uncle hashes
Copy link

@lithp lithp Nov 8, 2018

Choose a reason for hiding this comment

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

I'm pretty sure Transactions don't double as uncle hashes :)

Loading

Copy link

@lithp lithp Nov 8, 2018

Choose a reason for hiding this comment

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

(There are three copy-paste's of this mistake later in the file too, grepping for <Transaction> will find them)

Loading

@bmann
Copy link
Contributor

@bmann bmann commented Mar 2, 2019

I think it closed then re-opened. Likely we need to bug the editors to merge as a Draft at least.

@Arachnid can this be merged as Draft? May still be worked in the external repo for a while.

Loading

|-32002|Resource unavailable|Requested resource not available|non-standard|
|-32003|Transaction rejected|Transaction creation failed|non-standard|
|-32004|Method not supported|Method is not implemented|non-standard|

Copy link
Member

@MrChico MrChico Dec 22, 2019

Choose a reason for hiding this comment

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

Parity uses a whole suite of error codes that aren't really covered here, but they don't seem to be uniformly implemented. Some references: https://github.com/paritytech/parity-ethereum/blob/80a83c95d3168c8af18e492f79657fd69558821a/rpc/src/v1/helpers/errors.rs#L37
ethereum/go-ethereum#19027

Loading

Choose a reason for hiding this comment

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

@MrChico
Haven't "account is locked" error already implemented with code -32000 here -
https://ethereum.stackexchange.com/questions/8478/account-is-locked-how-to-unlock-it-using-json-rpc
But in the docs we have -32000 as "Invalid Input" error, here -
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1474.md#specification
I am confused.

Loading

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

Successfully merging this pull request may close these issues.