[GraphQL][RFC] Introduce UInt53 scalar #18552
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amnn
requested review from
wlmyng,
stefan-mysten,
emmazzz and
suiwombat
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
amnn
changed the title
wlmyng
approved these changes
Jul 12, 2024
| _ => None, | ||
| }; | ||
| } | ||
|
|
||
| let GqlValue::Number(value) = &value?.node else { | ||
| return None; | ||
| }; | ||
| value.as_u64() | ||
| value.as_u64().map(|v| v as u32) |
There was a problem hiding this comment.
Took me a second to wrap my head around this but I think it's fine because u64 maps to Int which on graphQL is capped at u32, so it shouldn't be possible for a graphQL client to provide > u32
There was a problem hiding this comment.
The query limits checker needs a deeper overhaul in any case so your confusion is warranted -- this should be clearer!
## Description Whilst working on #18337, I noticed that we were over-using the `Int` scalar -- using it to represent values that could exceed 2^31 - 1 -- when the GraphQL spec states that `Int` must be a 32-bit signed integer. We made this decision at the time (a) because `async-graphql` allowed converting `u64`s to `Int` and we were primarily concerned with the fact that although JSON doesn't specify a precision for its numeric types, JS (among other languages), assumes it is an IEEE double-precision floating point number, so can only represent integral values precisely below 2^53. `cynic` (a Rust GraphQL client library) is (correctly) stricter, however, and maps an `Int` to an `i32`, always. There may be other similarly strict client libraries for other languages. This PR introduces a new scalar, `UInt`, that maps to a JSON number literal, just like `Int`, but allows us to ascribe our own meaning (in this case, it will be an unsigned number, and it can be as large as 2^53). This scalar has been used in many cases where we had previously used `Int`: sequence numbers, counts of objects, checkpoints, transactions, etc. While other uses continue to use `Int` (pagination limits, service limits, values bounded by the number of validators). In some cases, we have switched from `BigInt` to using this scalar notably: - the db cost estimate, which was previously a `BigInt` because we were unsure of its scale, but in hindsight, after benchmarking, it is unlikely that we would want to set a limit greater than 2^31 - 1. - the number of checkpoints in an epoch, as the number of transactions in an epoch (a number that is guaranteed to be greater) is being represented using an `Int` at the moment (and soon a `UInt`). This will be a breaking change, so will only go out with the new major version. Hopefully, this change will be minimal as the format of this scalar over the wire is the same as for `Int`, but it will require existing clients to register a new scalar in most cases. ## Test plan Existing tests: ``` sui-graphql-rpc$ cargo nextest run sui-graphql-e2e-tests$ cargo nextest run --features pg_integration ```
tx-tomcat
pushed a commit
to tx-tomcat/sui-network
that referenced
this pull request
Jul 29, 2024
## Description Whilst working on MystenLabs#18337, I noticed that we were over-using the `Int` scalar -- using it to represent values that could exceed 2^31 - 1 -- when the GraphQL spec states that `Int` must be a 32-bit signed integer. We made this decision at the time (a) because `async-graphql` allowed converting `u64`s to `Int` and we were primarily concerned with the fact that although JSON doesn't specify a precision for its numeric types, JS (among other languages), assumes it is an IEEE double-precision floating point number, so can only represent integral values precisely below 2^53. `cynic` (a Rust GraphQL client library) is (correctly) stricter, however, and maps an `Int` to an `i32`, always. There may be other similarly strict client libraries for other languages. This PR introduces a new scalar, `UInt`, that maps to a JSON number literal, just like `Int`, but allows us to ascribe our own meaning (in this case, it will be an unsigned number, and it can be as large as 2^53). This scalar has been used in many cases where we had previously used `Int`: sequence numbers, counts of objects, checkpoints, transactions, etc. While other uses continue to use `Int` (pagination limits, service limits, values bounded by the number of validators). In some cases, we have switched from `BigInt` to using this scalar notably: - the db cost estimate, which was previously a `BigInt` because we were unsure of its scale, but in hindsight, after benchmarking, it is unlikely that we would want to set a limit greater than 2^31 - 1. - the number of checkpoints in an epoch, as the number of transactions in an epoch (a number that is guaranteed to be greater) is being represented using an `Int` at the moment (and soon a `UInt53`). This will be a breaking change, so will only go out with the new major version. Hopefully, this change will be minimal as the format of this scalar over the wire is the same as for `Int`, but it will require existing clients to register a new scalar in most cases. ## Test plan Existing tests: ``` sui-graphql-rpc$ cargo nextest run sui-graphql-e2e-tests$ cargo nextest run --features pg_integration ``` --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [x] GraphQL: Introduces a new scalar -- `UInt53` -- to represent unsigned 53 bit integer values. Some uses of `Int` in the existing schema have been replaced with `UInt53`. All clients will need to register the new scalar and clients for statically typed languages will also need to use a wider (e.g. 64 bit), unsigned type to hold the value. - [ ] CLI: - [ ] Rust SDK:
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Whilst working on #18337, I noticed that we were over-using the
Intscalar -- using it to represent values that could exceed 2^31 - 1 -- when the GraphQL spec states thatIntmust be a 32-bit signed integer.We made this decision at the time (a) because
async-graphqlallowed convertingu64s toIntand we were primarily concerned with the fact that although JSON doesn't specify a precision for its numeric types, JS (among other languages), assumes it is an IEEE double-precision floating point number, so can only represent integral values precisely below 2^53.cynic(a Rust GraphQL client library) is (correctly) stricter, however, and maps anIntto ani32, always. There may be other similarly strict client libraries for other languages.This PR introduces a new scalar,
UInt, that maps to a JSON number literal, just likeInt, but allows us to ascribe our own meaning (in this case, it will be an unsigned number, and it can be as large as 2^53).This scalar has been used in many cases where we had previously used
Int: sequence numbers, counts of objects, checkpoints, transactions, etc. While other uses continue to useInt(pagination limits, service limits, values bounded by the number of validators).In some cases, we have switched from
BigIntto using this scalar notably:the db cost estimate, which was previously a
BigIntbecause we were unsure of its scale, but in hindsight, after benchmarking, it is unlikely that we would want to set a limit greater than 2^31 - 1.the number of checkpoints in an epoch, as the number of transactions in an epoch (a number that is guaranteed to be greater) is being represented using an
Intat the moment (and soon aUInt53).This will be a breaking change, so will only go out with the new major version. Hopefully, this change will be minimal as the format of this scalar over the wire is the same as for
Int, but it will require existing clients to register a new scalar in most cases.Test plan
Existing tests:
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.
UInt53-- to represent unsigned 53 bit integer values. Some uses ofIntin the existing schema have been replaced withUInt53. All clients need to register the new scalar, and clients for statically typed languages also need to use a wider (64 bit, for instance), unsigned type to hold the value.