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

Large nums as strings #225

Merged
merged 4 commits into from
Sep 18, 2024
Merged

Conversation

matthew1001
Copy link
Contributor

@matthew1001 matthew1001 commented Sep 18, 2024

Relates to hyperledger/firefly#1568

With large JSON numbers supported in FF (see hyperledger/firefly#1569 and hyperledger/firefly#1581) the FireFly UI received large numbers correctly but then lost precision when rendering them.

For example the following Data viewer:

image

which was rendered from the following JSON payload from FF:

...
  "datatype": {
    "name": "mattdata",
    "version": "1.0.0"
  },
  "value": {
    "id": 1000000000000000001
  }
...

This PR uses the MIT-licenses package lossless-json to replace the JSON.parse step in all fetch(...) calls. It stringifies numbers that are too large to be represented as a Javascript number e.g.

image

which is from the same JSON payload as above.

The parser leaves all other data untouched (i.e. it returns the same JSON as JSON.parse except for the case where a number is determined to be a number that cannot be safely represented as a Javascript number).

Here is an example of a small number, which is represented exactly as it would be before this PR:

image

Here is an example of a string, also represented exactly as it would be before this PR:

image

I think representing numbers in this way as strings is definitely preferable to showing incorrect data, but it might be useful to FF users to show some indication in UI panels that data has been manipulated to render it correctly. I'm considering that work out of scope for this fix but mentioning here for discussion.

Signed-off-by: Matthew Whitehead <[email protected]>
Signed-off-by: Matthew Whitehead <[email protected]>
Copy link
Contributor

@EnriqueL8 EnriqueL8 left a comment

Choose a reason for hiding this comment

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

Great stuff - I think just documenting that long number might appear as strings in the docs is fine for now

Comment on lines +25 to +30
export function parseLargeNumbersAsStrings(value: any) {
return isSafeNumber(value, { approx: false })
? parseFloat(value) // Smaller numbers are kept as Javascript numbers
: new LosslessNumber(value).toString(); // Large numbers are safely stringified
}

Copy link
Contributor

Choose a reason for hiding this comment

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

actually based on this example https://github.com/josdejong/lossless-json?tab=readme-ov-file#lossless-json couldn't we just call parse() and that's it?

Copy link
Contributor

@EnriqueL8 EnriqueL8 Sep 18, 2024

Choose a reason for hiding this comment

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

Or is it because lower down in the react component we are actually doing some manipulation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with just calling parse() is that small numbers all end up as LosslessNumbers as well and are therefore stringified in the UI. Which I don't think we want. So the extra number parser allows us to retain Javascript numbers for smaller numbers.

Copy link
Contributor

Choose a reason for hiding this comment

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

But they don't look like stringified here

// LosslessJSON.parse will preserve all numbers and even the formatting:
console.log(stringify(parse(text)))
// '{"decimal":2.370,"long":9123372036854000123,"big":2.3e+500}'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it's because we want the large numbers as a string (instead of a BigInt which would render in the UI as 1000000000000001n). In their example they are using their own stringify to render it as a large number without a string, but we would need to refactor a lot of the codebase to use that instead of our existing JSON serialisation logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got caught out that stringify is actually their own function

@EnriqueL8 EnriqueL8 merged commit 831071f into hyperledger:main Sep 18, 2024
3 checks passed
@EnriqueL8 EnriqueL8 deleted the large-nums-as-strings branch September 18, 2024 12:58
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.

2 participants