Skip to content

Conversation

alita-moore
Copy link

@alita-moore alita-moore commented Jun 20, 2021

This specification uses EIP 1474, splits up the spec into a more manageable multi-file format, and I've begun incorporating the yellow paper where applicable.

This is the initial commit, and changes are expected. Particularly as they pertain to including edgecases as an md in each endpoints description. This will be accomplished by storing each edgecase in the same format as the methods themselves but in an "edgecases" folder.

the current structure allows for more easy reuse of components / schemas with method specific descriptions and requirements and it more clearly separates schemas from components from methods which are all different levels. This separation is necessary due to the way that OpenRPC parses the json.

You may notice that I use external references to internal entries in the json (i.e. ./File.json#/item instead of #/item). This is just a work around for a weird bug I ran into where internally referenced items would try to resolve relative to the externally imported component (i.e. file2 imports File.json#/item and then #/item is not in file2 so it errors).

you can see the built specification here

The goal here was maximum maintainability by keeping relevant info where it's most useful and never repeating things. I'd like to get rid of the components in the future fit this reason but they already to be a key part of the openrpc. Nevertheless I've done my best here to keep things as clean as possible. There are some ways to clean it up still. But I think it's fine for now. But what do you think?

@lightclient lightclient marked this pull request as draft June 20, 2021 19:38
@alita-moore alita-moore force-pushed the main branch 2 times, most recently from e441180 to 16d7c0e Compare July 2, 2021 00:06
@alita-moore alita-moore marked this pull request as ready for review July 6, 2021 07:05
Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Left a couple comments, but mainly trying to understand the need for the aliases? I loaded the built spec into the openrpc playground, and it didn't seem like there were any additional docs. Additionally, it seems like a lot of these things aren't used (like addressOrNull).

@@ -0,0 +1,2613 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

We should remove files that can be auto generated. We can add a CI task to build the spec into a non-main branch.

Copy link
Author

Choose a reason for hiding this comment

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

You need this to be pre generated for the open RPC playground.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I understand. It's generally standard though to do this as a CI task rather than require developers to do this on push. You can see how we do it for the eth1 spec here: https://github.com/quilt/eth1.0-specs/blob/pyspec/.github/workflows/gh-pages.yaml

@shanejonas
Copy link
Contributor

shanejonas commented Jul 6, 2021

as as stop-gap for the external md files, you could use the method.externalDocumentation object to link back to the repos markdown files.

or just compile them to a string in the build step, and add them as the description:

$ cat foo.md
# Hello World

my markdown file


## Second thing


- a list
- of things


$ node
> const fs = require('fs');
undefined
> const mdFile = fs.readFileSync("foo.md");
undefined
> mdFile.toString();
'# Hello World\n\nmy markdown file\n\n\n## Second thing\n\n\n- a list\n- of things\n'

Co-authored-by: lightclient <[email protected]>
@alita-moore
Copy link
Author

@lightclient i don't know what you mean by additional docs. The aliases are useful because they abstract components as to be easier to maintain. There's some clutter right now that I can clean up, though.

@alita-moore
Copy link
Author

Thanks @shanejonas !

@alita-moore
Copy link
Author

@lightclient this is ready for review again with the exception of the build on push (which is still in progress)

@alita-moore alita-moore requested a review from lightclient July 14, 2021 17:50
@shanejonas
Copy link
Contributor

I like that the content descriptors are inline now, easier to follow.

the schemas left need to be cleaned up to only have title and description, not name and summary

@alita-moore
Copy link
Author

got it @shanejonas will make that change, it's especially nice that the schemas are separated because it can be fixed easily using regex. Thanks!

@@ -1,6 +1,6 @@
# Ethereum JSON-RPC Specification

[View the spec](https://playground.open-rpc.org/?uiSchema%5BappBar%5D%5Bui:splitView%5D=false&schemaUrl=https://raw.githubusercontent.com/lightclient/eth1.0-apis/main/openrpc.json&uiSchema%5BappBar%5D%5Bui:input%5D=false)
[View the spec](https://playground.open-rpc.org/?uiSchema[appBar][ui:splitView]=false&schemaUrl=https://github.com/alita-moore/eth1.0-apis/blob/main/build/openrpc.json&uiSchema[appBar][ui:input]=false)
Copy link
Contributor

Choose a reason for hiding this comment

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

link needs to be updated to this repo, and pointed at the raw version

},
{
"$ref": "./Block.json#/transactionHash"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesnt look right combining headerObject with an array of tx hashes.

it should be combining type: 'object's together with the correct shape that you want to merge:

"allOf": [
      {
        "$ref": "./Header.json#/headerObject"
      },
      {
       "type: "object",
       "properties": {
           "transactions": {
              "$ref": "./Block.json#/transactionHash"
           }
       },
        {
       "type: "object",
       "properties": {
           "uncles": {
              "$ref": "./Block.json#/transactionHash"
           }
       }
]       

side note: the name transactionHash seems like an incorrect name for an array of transaction hashes.

also you can pass true to eth_getBlockByNumber and it returns the transactions filled out instead of just the hashes:

image

so it needs to take that into account too

@lightclient
Copy link
Member

I ended up splitting out the spec in #23. I recommend using that as the base for additional work on the method and schema definitions.

@alita-moore
Copy link
Author

Alright, thanks

mkalinin added a commit that referenced this pull request Oct 2, 2024
* Define `engine_getBlobsV1`

* Add `getBlobs` method and schema. (#1)

* engine: Add `getBlobsV1` method (#2)

* Fix spell check

* Fix typo and formatting

* Apply Mikhail's clarifications

Co-authored-by: Mikhail Kalinin <[email protected]>

* Add error to schema

Co-authored-by: Mikhail Kalinin <[email protected]>

* Update toc in cancun.md

---------

Co-authored-by: Jimmy Chen <[email protected]>
Co-authored-by: Mikhail Kalinin <[email protected]>
bomanaps pushed a commit to bomanaps/execution-apis that referenced this pull request Jul 2, 2025
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.

3 participants