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

feat: add getAddressLookupTable method to Connection #27127

Merged
merged 1 commit into from
Aug 14, 2022

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented Aug 14, 2022

Problem

No way to fetch and decode address lookup tables with web3

Summary of Changes

  • Add getAddressLookupTable method to Connection
  • Add deserialization logic for decoding raw lookup table account data buffers

Fixes #

@codecov
Copy link

codecov bot commented Aug 14, 2022

Codecov Report

Merging #27127 (a460255) into master (1165a7f) will decrease coverage by 4.9%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #27127       +/-   ##
===========================================
- Coverage    81.9%    76.9%     -5.0%     
===========================================
  Files         631       48      -583     
  Lines      174252     2506   -171746     
  Branches        0      355      +355     
===========================================
- Hits       142728     1928   -140800     
+ Misses      31524      448    -31076     
- Partials        0      130      +130     

@jstarry jstarry force-pushed the web3/get-address-lookup-table branch from 80b3df5 to a460255 Compare August 14, 2022 10:05
@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Aug 14, 2022
@mergify mergify bot merged commit dcef8ec into solana-labs:master Aug 14, 2022
@jstarry jstarry deleted the web3/get-address-lookup-table branch August 14, 2022 11:08
xiangzhu70 pushed a commit to xiangzhu70/solana that referenced this pull request Aug 17, 2022
}

isActive(): boolean {
const U64_MAX = 2n ** 64n - 1n;
Copy link
Contributor

Choose a reason for hiding this comment

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

Fun fact: this change made web3.js incompatible with React Native because:

  • JavaScript Core (JSC) doesn't support the exponentiation operator (**) so there's a transform that converts 2n ** 64n to Math.pow(2n, 64n) and then you're in trouble because Math.pow() doesn't work with bigint (runtime fatal)
  • Hermes supports the exponentiation operator but doesn't support bigint yet.

No change necessary – I'm working on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ouch, thanks for the heads up. Seems worthwhile to use a literal here then, or do you think it's not worth the trouble?

Copy link
Contributor

@steveluscher steveluscher Aug 29, 2022

Choose a reason for hiding this comment

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

There are now three places in web3.js that use BigInt

  • This, with exponentiation
  • @noble/ed25519, with exponentiation
  • @noble/secp256k1, with exponentiation

If we inline the computation and convince the noble libs to do the same (eg. just literally change this to BigInt('18446744073709551616') and comment it) then:

  • Less CPU overhead (nice)
  • JSC: still fucked because JSC doesn't support BigInt
  • JSC with BigInt polyfill: still fucked because polyfills don't permit arithmetic (require('big-integer')(1) + require('big-integer')(1) is actually nonsense, because it's basically Object + Object)
  • Pre 0.70 Hermes: still fucked because of no BigInt support
  • >=0.70 Hermes: Works, but so does the original exponentiated code.

I'm going to post about this on September 1st, but my recommendation going forward for anyone who wants to use Solana libraries in React Native going forward will be to upgrade to React Native 0.70 and switch to Hermes. Big integers are critical for most crypto use cases, Hermes is now the default in React Native from 0.70 onward, and the way out of this mess is forward.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, really appreciate the breakdown on this. I'll leave as-is then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants