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

fix: bigint reviver #805

Merged
merged 1 commit into from
Dec 13, 2024
Merged

fix: bigint reviver #805

merged 1 commit into from
Dec 13, 2024

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Dec 12, 2024

Motivation

During manual testing with the updated wallet-headless, I found a bug in the logic used to revive BigInts. We should check using JS Number.MAX_SAFE_INTEGER, instead.

Acceptance Criteria

  • Make bigIntReviver() a standalone function in the JSONBigInt object.
  • Fix logic bigIntReviver() uses to determine when it should return a BigInt.
  • Add more tests.

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@glevco glevco added the bug Something isn't working label Dec 12, 2024
@glevco glevco requested a review from r4mmer December 12, 2024 17:18
@glevco glevco self-assigned this Dec 12, 2024
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 80.68%. Comparing base (50f9ff5) to head (0f5faa3).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/utils/bigint.ts 76.92% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #805      +/-   ##
==========================================
- Coverage   80.72%   80.68%   -0.05%     
==========================================
  Files          85       85              
  Lines        6549     6549              
  Branches     1420     1419       -1     
==========================================
- Hits         5287     5284       -3     
- Misses       1249     1252       +3     
  Partials       13       13              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@glevco glevco force-pushed the fix/bigint-reviver branch 2 times, most recently from d5aea0b to a0a6702 Compare December 12, 2024 19:19
@glevco glevco force-pushed the fix/bigint-reviver branch from a0a6702 to 0f5faa3 Compare December 12, 2024 19:20
@glevco glevco marked this pull request as ready for review December 12, 2024 19:47
__tests__/utils/bigint.test.js.ts Show resolved Hide resolved
__tests__/utils/bigint.test.js.ts Show resolved Hide resolved
@glevco glevco merged commit 3304f67 into master Dec 13, 2024
5 checks passed
@glevco glevco deleted the fix/bigint-reviver branch December 13, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants