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: add bigint support to system transfers #24975

Merged
merged 1 commit into from
May 5, 2022
Merged

fix: add bigint support to system transfers #24975

merged 1 commit into from
May 5, 2022

Conversation

steveluscher
Copy link
Contributor

@steveluscher steveluscher commented May 4, 2022

Note: this PR was made by @konytech, but @steveluscher is an idiot and accidentally corrupted #24797; reopened it as this PR.

Problem

System transfers only support the 'number' type for lamports. For transferring large amounts, supporting 'bigint' like it's done in the spl-token library is required.

This follows the same encoding/decoding approach used by the spl-token library:
https://github.com/solana-labs/solana-program-library/blob/master/token/js/src/instructions/transfer.ts#L56

Summary of Changes

  • Add bigint support to system transfers
  • Add @solana/buffer-layout-utils package required to decode u64 to bigint
  • Fix tests with this new type support

@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

Merging #24975 (1dd345a) into master (eae9a66) will decrease coverage by 6.8%.
The diff coverage is n/a.

❗ Current head 1dd345a differs from pull request most recent head 9a02b52. Consider uploading reports for the commit 9a02b52 to get more accurate results

@@             Coverage Diff             @@
##           master   #24975       +/-   ##
===========================================
- Coverage    82.0%    75.1%     -6.9%     
===========================================
  Files         598       38      -560     
  Lines      165953     2244   -163709     
  Branches        0      323      +323     
===========================================
- Hits       136170     1687   -134483     
+ Misses      29783      444    -29339     
- Partials        0      113      +113     

@steveluscher
Copy link
Contributor Author

@jordansexton, are you OK with this?

bigint support (relatively bad in Safari; click ‘date relative’): https://caniuse.com/bigint

image

Only other thing I could do would be to write a layout adapter for BN (ie. bn.js, the BigNum polyfill`).

@jordaaash
Copy link
Contributor

Yep, fine with this. Last 1 or 2 version support is pretty typical for crypto libs.

@steveluscher steveluscher merged commit 9089909 into solana-labs:master May 5, 2022
@steveluscher steveluscher deleted the add-bigint-to-system-transfers branch May 5, 2022 02:14
@konytech
Copy link

konytech commented May 5, 2022

Thanks for taking care of this @steveluscher

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