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: eliminate bn.js and replace public key storage with JavaScript bigint #1142

Closed
wants to merge 3 commits into from

Conversation

steveluscher
Copy link
Collaborator

Problem

JavaScript bigint has been available in browsers since September 2020. Despite that we've implemented large value storage in PublicKey using the bn.js library. This library presently makes up ~13% of @solana/web3.js, or around 35K uncompressed.

image

Replacing this system with pure bigint storage would cut the size of this library down considerably.

Summary of Changes

  • Patch borsh to serialize to and from bigint rather than BN instances. A pull request has been sent to the upstream project.
  • Replace BN storage in PublicKey with bigint

Bundle size

Using package-build-stats:

const p = require('package-build-stats');
p.getPackageStats('~/solana/web3.js/').then(r => console.log(r))

Size before: 75299 gzipped bytes
Size after: 64191 gzipped bytes

Reduction of ~14%.

Notes

  • Anyone who used to rely on accessing the private variable PublicKey::_bn will find that its type has changed from BN to bigint. Such is the risk when you reach into private variables.
  • This change relies on patching borsh-js in a way that represents a breaking change. For this reason we modify the bundle script to bundle the modified version into the web3.js bundle. This has two implications:
    • An app that uses borsh outside of web3.js will end up bundling a second copy of it (presumably one that includes 'bn.js` which negates the benefits of this change)
    • Our patched version is accessible only within the confines of web3.js and will not cause a breaking change to the rest of the app.

Addresses #1103.

@codecov-commenter
Copy link

Codecov Report

Merging #1142 (2a08815) into master (34cf5d7) will decrease coverage by 0.29%.
The diff coverage is 70.83%.

@@            Coverage Diff             @@
##           master    #1142      +/-   ##
==========================================
- Coverage   76.35%   76.06%   -0.29%     
==========================================
  Files          56       56              
  Lines        3151     3159       +8     
  Branches      475      481       +6     
==========================================
- Hits         2406     2403       -3     
- Misses        578      585       +7     
- Partials      167      171       +4     
Impacted Files Coverage Δ
src/publickey.ts 81.81% <70.83%> (-13.84%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@wireless-anon
Copy link

will this break the old PublicKey.equals when you pass in a new PublicKey object (i.e. a bigint-based one)?

concern is that calling a function from a library that uses an older version of @solana/web3.js and passing in a bigint-based PublicKey may fail

@github-actions github-actions bot added the stale label Feb 20, 2023
@steveluscher
Copy link
Collaborator Author

steveluscher commented Feb 20, 2023

will this break the old PublicKey.equals when you pass in a new PublicKey object (i.e. a bigint-based one)?

Most certainly; good call-out.

Options:

  1. Breaking version bump.
  2. Add @solana/web3.js as a peerDependency of itself, meaning that when you install it, installing an older version of it somewhere else makes your project fail the peer dependency check.

I'm not sure if 2 would actually ‘work,’ but it does in my head. Thoughts, @joncinque, @jordansexton?

@github-actions
Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants