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

feat(fw,docs): Add eof/t8n tools specs, fix JSON schema generation in docs #549

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

winsvega
Copy link
Collaborator

@winsvega winsvega commented May 9, 2024

πŸ—’οΈ Description

Specs for eof tool implementation,

πŸ”— Related Issues

βœ… Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

@winsvega winsvega added scope:docs Scope: Documentation needs-discussion Needs discussion before proceeding labels May 9, 2024
@winsvega winsvega marked this pull request as draft May 9, 2024 08:36
@marioevz marioevz changed the title eof/t8n specs WIP feat(fw,docs): Add eof/t8n tools specs, fix JSON schema generation in docs Jun 10, 2024
@marioevz
Copy link
Member

Hey @winsvega, I revamped the specs and introduced the JSON schemas for all types that the tools are expected to parse or output, including examples for most JSON types.

Use mkdocs serve to see the changes and let me know what you think.

@marioevz marioevz marked this pull request as ready for review June 11, 2024 00:13
@winsvega
Copy link
Collaborator Author

winsvega commented Jun 12, 2024

I am getting lost now. there was a nice description with examples
why 600 lines of py code, is it now making automatically from the code structures?

the auto docs gen is ok, but I am not the type of a person who could read that.
to me my initial version was much preferable. basically a short example in console on how to use it is the best type of doc for me.

looks like the generated doc is about eof test parser, not the eof tool.

with t8n however need more examples on all the types. yes.
hm

currentNumber": "1",
  "currentTimestamp": "1000",
  "currentRandom": "0",
  "currentDifficulty": "1",
  "currentBaseFee": "2",
  "currentExcessBlobGas": "3",
  "parentDifficulty": "4",
  "parentTimestamp": "5",
  "parentBaseFee": "6",
  "parentGasUsed": "7",
  "parentGasLimit": "8",
  "currentBlobGasUsed": "9",

this can also be hex, but not padded. "0x1", "0x2"
another tricky part here that depending on the fork, this fields could be different.
geth report an error if currentBaseFee provided but fork is not supporting the field.

also we have a notation here that if currentDifficulty not set, then there must be parentDifficulty and parentTimestamp so the tool will calculate currentDifficulty itself to be used for difficulty tests.

same with bsefee

@marioevz
Copy link
Member

the auto docs gen is ok, but I am not the type of a person who could read that.
to me my initial version was much preferable. basically a short example in console on how to use it is the best type of doc for me.

Yes we now have auto-gen of these specs, I think it's important because with each fork we introduce new fields to the Environment, so it's easy to link implementers to this page for them to look at an example.

I really like the example parts of the new auto-gen doc, more than the parameters, I think the types are one of the most important aspects.

this can also be hex, but not padded. "0x1", "0x2"

Yes I think it should only be hex, not decimal, more so when the other fields like withdrawals already have hex numbers in their fields.
If you're ok with this, I can update the environment class to not produce anything but unpadded hex numbers as you mention.

another tricky part here that depending on the fork, this fields could be different.

This could definitely be added to the schema, I think we could have something like this:

  currentBaseFee:
    anyOf:
    - pattern: ^([0-9]|[1-9][0-9]+)$
      title: number
      type: string
    - type: 'null'
    default: null
    title: Currentbasefee
    fork: London

Although this is not within the standard of the JSON schema: https://json-schema.org/understanding-json-schema/reference/annotations

@marioevz
Copy link
Member

@winsvega Updated Environment to use hex number exclusively in the latest commit, and if you try mkdocs serve again, the types and examples are automatically updated by the auto-gen:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion Needs discussion before proceeding scope:docs Scope: Documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants