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: specify Long input and output formats #389

Merged
merged 2 commits into from
Apr 21, 2023

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Mar 14, 2023

TL;DR After this PR every number field will be hex-formatted. Numeric inputs are flexible and can be provided as number or string.

The spec is vague in defining which inputs are accepted for Long and how outputs are formatted. I suggest that Long behave similar to BigInts in this sense. I.e. be flexible when taking input, but output hex-encoded values. My reasoning is that:

  • JSON-RPC API returns all the long values as hex.
  • Even though BigInt is a thing in JS-land, parsing a big uint64 literal in JS will still be inaccurate. Case in point this happens also in the GraphiQL UI which both Besu and Geth use this issue exists. There's a proposal which will help with this however it is not accepted for being included into the EcmaScript spec.
  • That said we can improve UX by accepting number literals as well as strings for inputs.

Every field with type Int has been changed to be a Long for consistency.

Hive tests: ethereum/hive#746

@shemnon
Copy link
Contributor

shemnon commented Mar 23, 2023

Looks reasonable. Any idea of the scope of the downstream impact? i.e. do you know of anyone we will break?

@shemnon
Copy link
Contributor

shemnon commented Mar 23, 2023

I also think that if we are going to hex for the longs, we should consider all-hex, or at least align with what the json-rpc results send on a per-field basis.

@s1na
Copy link
Contributor Author

s1na commented Mar 27, 2023

Any idea of the scope of the downstream impact? i.e. do you know of anyone we will break?
Unfortunately not.

I also think that if we are going to hex for the longs, we should consider all-hex, or at least align with what the json-rpc results send on a per-field basis.

Fair enough. json-rpc result sends back hex for every number AFAIK. At least checked all cases where graphql currently returns decimal and they are all returned as hex in json-rpc.

Currently their type is Int in the schema. I will change them to Long since Int is a graphql defined type and I'd rather we don't override its behavior.

@s1na
Copy link
Contributor Author

s1na commented Apr 21, 2023

Updated the hive tests as well, PTAL.

@lightclient lightclient merged commit 4c826f9 into ethereum:main Apr 21, 2023
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