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

GraphQL Support for withdrawals #5496

Merged
merged 6 commits into from
May 30, 2023
Merged

Conversation

shemnon
Copy link
Contributor

@shemnon shemnon commented May 25, 2023

Add support for withdrawals in GraphQL, including needed changes to testing infrastructure for shanghai-era blocks.
Also align existing adapters with graphql schema optionality.

PR description

Fixed Issue(s)

Add support for withdrawals in GraphQL, including needed changes to
testing infrastructure for shanghai-era blocks.
Also align existing adapters with graphql schema optionality.

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
@shemnon
Copy link
Contributor Author

shemnon commented May 25, 2023

partial work to address ethereum/execution-apis#400

@github-actions
Copy link

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I have considered running ./gradlew acceptanceTestNonMainnet locally if my PR affects non-mainnet modules.
  • I thought about the changelog and included a changelog update if required.
  • If my PR includes database changes (e.g. KeyValueSegmentIdentifier) I have thought about compatibility and performed forwards and backwards compatibility tests

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm aware that it's a current issue we have but bringing into the context of this PR, the withdrawals fields should probably be hexa encoded as well as any other fields that report quantity.
We currently fail a few hive tests because we're a little out of spec regarding that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a followup for hex encoding that I'll post today.

For the record, the EIP was integer encoded, as were the initial hive tests. The execution tests transition to hex encoding was never subject to public discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Migration to strings - #5505 - Note that it builds off of this PR. It is only really a change to Scalars.java and most of the unit test vectors.

Copy link

@s1na s1na May 30, 2023

Choose a reason for hiding this comment

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

For the record, the EIP was integer encoded, as were the initial hive tests.

The first geth implementation by Nick Johnson used hex-encoding. At some point half of the fields were changed to integer encoding (including hive tests for them) and half were kept as hex which was weird. I proposed on the specs repo to change it to hex to stay consistent also with JSON-RPC.

The execution tests transition to hex encoding was never subject to public discussion.

I took this comment ethereum/execution-apis#389 (comment) as an approval from the Besu side. I'm sorry if you thought there wasn't enough discussion around it. I'd like to make more changes to the hive tests, how do you prefer to discuss them?

Signed-off-by: Danno Ferrin <danno.ferrin@swirldslabs.com>
@shemnon shemnon enabled auto-merge (squash) May 26, 2023 18:43
Copy link
Contributor

@gfukushima gfukushima left a comment

Choose a reason for hiding this comment

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

lgtm

@shemnon shemnon merged commit cc2150d into hyperledger:main May 30, 2023
18 of 19 checks passed
elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023
Add support for withdrawals in GraphQL, including needed changes to
testing infrastructure for shanghai-era blocks.
Also align existing adapters with graphql schema optionality.

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
Add support for withdrawals in GraphQL, including needed changes to
testing infrastructure for shanghai-era blocks.
Also align existing adapters with graphql schema optionality.

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
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.

None yet

3 participants