Skip to content

Conversation

@ilbertt
Copy link
Contributor

@ilbertt ilbertt commented Dec 10, 2025

Uses BigInt for numbers that are actually supposed to be big integers according to the API.

Unfortunately, this fix required using a custom JSON parser and serializer, provided by the json-with-bigint package. Reimplementing it was not worth it. I've pinned the version of the package so that it minimizes the risk of introducing any vulnerabilities with future patches of the package.

Additionally, fixes #193.

@ilbertt ilbertt requested a review from a team as a code owner December 10, 2025 10:28
viviveevee
viviveevee previously approved these changes Dec 10, 2025
@ilbertt ilbertt changed the title fix: set time using bigints fix!: use bigint for addCycles and getCyclesBalance Dec 10, 2025
@ilbertt ilbertt requested a review from viviveevee December 10, 2025 10:48
@peterpeterparker
Copy link
Member

JSON parser and serializer, provided by the json-with-bigint package.

Why not using @dfinity/utils JSON serializers?

viviveevee
viviveevee previously approved these changes Dec 10, 2025
@ilbertt
Copy link
Contributor Author

ilbertt commented Dec 10, 2025

Why not using @dfinity/utils JSON serializers?

@peterpeterparker the PocketIC API doesn't support parsing BigInt object format used by @dfinity/utils ({ "__bigint__": string }), it just expects plain numbers

@peterpeterparker
Copy link
Member

Gotcha

@ilbertt
Copy link
Contributor Author

ilbertt commented Dec 10, 2025

@peterpeterparker when parsing a PocketIC server JSON response that contains a bigint, e.g. EncodedGetTimeResponse, we cannot use the jsonReviver from @dfinity/utils because that assumes the bigint field (in this case nanos_since_epoch) has shape { "__bigint__": string }, which is not the case for the PocketIC server API.
The nanos_since_epoch field is just a number.

@peterpeterparker
Copy link
Member

Yeah took more time to understand the all PR and third party dependencies and not just the parsing. Thanks for the feedback.

@ilbertt ilbertt enabled auto-merge December 10, 2025 13:29
@ilbertt ilbertt added this pull request to the merge queue Dec 10, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 10, 2025
@ilbertt ilbertt added this pull request to the merge queue Dec 10, 2025
Merged via the queue into main with commit 728bcbb Dec 10, 2025
12 checks passed
@ilbertt ilbertt deleted the luca/fix-set-time branch December 10, 2025 13:54
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.

Time rounded to milliseconds instead of nanoseconds

3 participants