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

Replace bn.js with BigInt #1223

Merged
merged 11 commits into from
Mar 12, 2024
Merged

Replace bn.js with BigInt #1223

merged 11 commits into from
Mar 12, 2024

Conversation

gtsonevv
Copy link
Collaborator

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • Commit messages follow the conventional commits spec
  • If this is a code change: I have written unit tests.
  • If this changes code in a published package: I have run pnpm changeset to create a changeset JSON document appropriate for this change.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Reduce amount of dependencies.

Test Plan

Related issues/PRs

#1217
#1200

@gtsonevv gtsonevv requested review from frol and gagdiez December 21, 2023 13:26
Copy link

changeset-bot bot commented Dec 21, 2023

🦋 Changeset detected

Latest commit: f7c541e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
Name Type
near-api-js Major
@near-js/accounts Minor
@near-js/transactions Minor
@near-js/types Minor
@near-js/utils Minor
@near-js/biometric-ed25519 Patch
@near-js/cookbook Patch
@near-js/crypto Patch
@near-js/providers Patch
@near-js/wallet-account Patch
@near-js/keystores Patch
@near-js/keystores-browser Patch
@near-js/keystores-node Patch
@near-js/signers Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@gagdiez gagdiez left a comment

Choose a reason for hiding this comment

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

Please see all my comments above

@gagdiez
Copy link
Contributor

gagdiez commented Jan 3, 2024

How is that the tests are not catching problems here? for example, the change of BN(10).exp(24) to BigInt(10**24) should have broke a bunch of stuff (since it is in the utils.js module, that every other module uses).

Wrongly changing it to BigInt(10**24) == 999999999999999983222784n should have broke a lot of tests... or is just that we do not have thorough testing? (in which case, we should implement it)

@gtsonevv gtsonevv requested review from gagdiez and vikinatora January 4, 2024 07:52
vikinatora
vikinatora previously approved these changes Jan 5, 2024
Copy link
Collaborator

@vikinatora vikinatora left a comment

Choose a reason for hiding this comment

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

Some nitpicks but looks very good

packages/transactions/src/create_transaction.ts Outdated Show resolved Hide resolved
packages/utils/src/validators.ts Outdated Show resolved Hide resolved
packages/utils/src/validators.ts Outdated Show resolved Hide resolved
vikinatora
vikinatora previously approved these changes Jan 8, 2024
@gtsonevv
Copy link
Collaborator Author

Hey @gagdiez , this PR is ready for review. Merging is blocked until you approve it because you requested changes.

@gagdiez
Copy link
Contributor

gagdiez commented Jan 10, 2024

@gtsonevv

I am still puzzled about the tests, and why they didn't fail. Specially on the format file.

We need to find an explanation, and add more tests if necessary.

If I am being picky with this PR is because a rounding error could lead to real people losing real money.

gagdiez
gagdiez previously approved these changes Jan 10, 2024
@gagdiez
Copy link
Contributor

gagdiez commented Jan 10, 2024

@gtsonevv LGTM

@vikinatora
Copy link
Collaborator

Delaying the merge of this PR intentionally, because we want to batch these breaking changes with other oustanding breaking changes, in order to not increment major versions unnecessarily.

@gtsonevv gtsonevv dismissed stale reviews from gagdiez and vikinatora via f7c541e March 12, 2024 11:57
@gtsonevv gtsonevv requested a review from vikinatora March 12, 2024 12:02
@gtsonevv gtsonevv merged commit e8c7e48 into near:master Mar 12, 2024
3 checks passed
@gtsonevv gtsonevv deleted the remove-bnjs branch March 12, 2024 12:03
This was referenced Mar 12, 2024
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.

3 participants