graphql: fix return types to be compatible with schema definition

[Shiti]

According to the schema definition and spec defined here, the transaction fields gas, nonce and the block field timestamp should be decimal.
This is inline with the other Long values like transaction status, etc

[rjl493456442]

Thanks, LGTM!

[holiman]

LGTM

[holiman]

@s1na or @gballet PTAL as codeowners

[fjl]

Just noting here that this is a compatibility-breaking change. We've been returning hex-encoded strings from graphql for years, so there will likely be issues in some client software trying to parse these results. Maybe the schema should be changed instead?

[Shiti]

I think that we should update the version and release notes to indicate this.
Changing schema definition will result in incompatibility with the ethereum api-spec

[fjl]

@s1na is in charge of the spec, so he'll be able to make a good decision here.

[holiman]

Just noting here that this is a compatibility-breaking change.

IMO, if there is a spec, and we are not following it, then we're in the wrong and need to change it.

[s1na]

I agree we should change these to decimal. Right now we have some fields that return a hex-encoded value, some that return decimal, all with a Long type in the schema. To stay consistent we should change one of these groups which would be a breaking change anyway.

From discussion:

The intention is that longs are read and formatted as numbers; GraphQL only specifies a 31 bit integer type, and it’s useful to be able to use a longer (52 bit, safely in Javascript) numeric type.

I think this intention prompted PRs such as #21883. At the time not all fields were converted. IMO we should do this now.

In addition to the ones you converted there are also the SyncState fields that currently return hexutil.Uint64. Can you please change those too?

[s1na]

It's weird, many of the methods defined for SyncState have no corresponding field in the schema. I.e. they're not queryable. We should either drop them or add them to schema.

[Shiti]

The api-spec does not mention or refer to SyncState. Based on the code, it looks like those types and methods are not exposed in graphql either.
I think it would be best to create a separate PR for changes to that code, be it cleanup or refactoring since it is unrelated to these changes. I can create that PR depending on how we want to deal with that code.

[fjl]

The API specification (https://github.com/ethereum/execution-apis) was created only very recently, and we literally copied the existing GraphQL schema there without changing it. It would be good to get to the point where schema and responses align again, but keeping the de-facto API outputs stable should be preferred over changing the output to match the existing (but apparently wrong) schema.

The GraphQL stance on versioning seems to be that

new capabilities can be added via new types and new fields on those types without creating a breaking change. This has led to a common practice of always avoiding breaking changes and serving a versionless API.

i.e. they recommend just adding new fields with a different type, which could then be requested. I guess, if we take that literally, the correct solution is adding new fields like timestampNumeric.

[holiman]

We literally copied it over means it was not created recently. It's an old standard, and we have not been compliant. Graphql has always been schema-first, as opposed to out old json-rpc. Imo we are at liberty to ruthlessly pursue standard compliance for graphql, since it's newer and more schema-centric. So I still think we should just fix our erroneous behavior. 

[MariusVanDerWijden]

sgtm (breaking users is fun)

[karalabe]

Account.TransactionCount seems to be wrong too

[Shiti]

@karalabe thanks for catching that. I've updated the PR

[Shiti]

The following job is failing but I dont think it is related to these changes. Could someone guide me on how to resolve this ?

Image: Visual Studio 2019; Environment: GETH_ARCH=386, GETH_MINGW=C:\msys64\mingw32

[MariusVanDerWijden]

Thats a flaky test, we can just ignore it. Not a showstopper for your pr

[fjl]

Discussion result:

Going back to the spec, we find that the definition of Long does not explicitly state the scalar type. It can be interpreted to mean that Long values should be integers, but strings are also permitted by "kind": "SCALAR".

So a follow-up task is defining this better in the spec. If we choose to do it the way it's implemented in the PR, we should explicitly define Long as an integer.

[Shiti]

We have already defined the custom scalar Long as an integer in our graphql server setup - https://github.com/ethereum/go-ethereum/blob/master/graphql/graphql.go#L45-L76

Do we wish to modify this and throw an error when base 10 strings are passed to unmarshal ? Or are you referring to something else ?
Am I missing something ?

[fjl]

My comment was mostly for @s1na, and was referring to the execution-apis spec: https://github.com/ethereum/execution-apis/blob/f33432b3a3f3d6de6ff5e7977f580376df9b57d9/graphql.json#L1226-L1235

[Shiti]

@fjl thanks for the clarification.

I think the ethereum API spec is correct as its the implementation's responsibility to ensure type validation

In most GraphQL service implementations, there is also a way to specify custom scalar types. For example, we could define a Date type:

scalar Date

Then it's up to our implementation to define how that type should be serialized, deserialized, and validated. For example, you could specify that the Date type should always be serialized into an integer timestamp, and your client should know to expect that format for any date fields.

https://graphql.org/learn/schema/#scalar-types

[Shiti]

@s1na @fjl Is there anything I can do to help with this ?

[holiman]

@s1na, since #26894 is merged, should this PR be closed, or what is the status?

[s1na]

Yes, closing this as resolved.