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: add field yParity #471

Merged
merged 2 commits into from
Nov 8, 2023
Merged

graphql: add field yParity #471

merged 2 commits into from
Nov 8, 2023

Conversation

jsvisa
Copy link
Contributor

@jsvisa jsvisa commented Sep 21, 2023

No description provided.

Signed-off-by: jsvisa <[email protected]>
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.

LGTM

@s1na
Copy link
Contributor

s1na commented Nov 2, 2023

@shemnon Please take a look. This is consistent with the JSON-RPC. We have to return yParity for type-1 and type2 transactions instead of v. (More context: paradigmxyz/reth#3798)

@shemnon
Copy link
Contributor

shemnon commented Nov 2, 2023

Looks reasonable.

Why Long over BigInt? JSON-RPC encodes yParity as a uint not a uint64, and Long limits to 64. It's not an input parameter so queries passing in numbers shouldn't matter.

@s1na
Copy link
Contributor

s1na commented Nov 2, 2023

Given that r, s and v are also BigInt I tend to agree 👍

graphql.json Outdated Show resolved Hide resolved
schema.graphqls Outdated Show resolved Hide resolved
@s1na
Copy link
Contributor

s1na commented Nov 2, 2023

Come to think of it, v should become nullable. It will be null for type-1 and type-2 txes.

@shemnon
Copy link
Contributor

shemnon commented Nov 3, 2023

we also need to merge #468 which impacts the graphql apis.

@fjl
Copy link
Collaborator

fjl commented Nov 3, 2023

The value of yParity is always 0 or 1. So it makes no sense for it to be a BigInt. There are only two possible values in responses: 0x0 or 0x1. Not sure how this could be encoded into the schema.

@shemnon
Copy link
Contributor

shemnon commented Nov 3, 2023

The argument for BigInt is for consistent handling with mappings from the JSON-RPC api, because it is just uint there which maps to BigInt for r and s. Long is to much as well. What is really needed is a type that means "hex integer output" which is what BigInt fulfills.

@fjl fjl merged commit 0c18fb0 into ethereum:main Nov 8, 2023
3 checks passed
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.

5 participants