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

big-integer shim testing on android-jsc #6221

Closed
wants to merge 14 commits into from
Closed

big-integer shim testing on android-jsc #6221

wants to merge 14 commits into from

Conversation

leotm
Copy link
Member

@leotm leotm commented Apr 19, 2023

Description

Testing big-integer shim on problematic android-jsc cc @Gudahtt

since iOS JSC already natively fully supports big integers (BigInt and big integer literals like 1n)
thus compat with @ethereumjs/util

whereas vanilla android-jsc (even on latest RN 0.71.6) throws

  • e.g. BigInt(1): ReferenceError: Can't find variable: BigInt
    • fixed by big-integer shim
  • e.g. 1n: SyntaxError: No identifiers allowed directly after numeric literal
    • not fixed by big-integer shim

Screenshots/Recordings

If applicable, add screenshots and/or recordings to visualize the before and after of your change

Issue

Progresses #???

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

[SyntaxError: No identifiers allowed directly after numeric literal]

Emu stacktrace: No identifiers allowed after numeric literal (x2), no stack (x2)
This reverts commit 17d3788.
Received: 5
Expected: 5n
Methods isObject and hasProperty behaving as expected
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@socket-security
Copy link

New dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
@metamask/utils@5.0.1 None +8 metamaskbot

resolved "https://registry.yarnpkg.com/@ethereumjs/util/-/util-8.0.5.tgz#b9088fc687cc13f0c1243d6133d145dfcf3fe446"
integrity sha512-259rXKK3b3D8HRVdRmlOEi6QFvwxdt304hhrEAmpZhsj7ufXEOTIc9JRZPMnXatKjECokdLNBcDOFBeBSzAIaw==
dependencies:
"@chainsafe/ssz" "0.9.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

@leotm in case you're not aware and it's relevant, this will pull in a WASM dependency. MetaMask/eth-sig-util#302 (comment)

My understanding is that @chainsafe/ssz is runtime-agnostic as of recent 0.11.1, which will be incorporated in an imminent @ethereumjs/* release.

Copy link
Member Author

@leotm leotm Apr 27, 2023

Choose a reason for hiding this comment

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

thanks again for bringing up earlier ^ and the link
gotcha so the shim could be incompat w @ethereumjs/util>@ChainSafe/ssz>@chainsafe/as-sha256's wasm serialisation, which @ethereumjs/util bump might fix (replacing w e.g. js-only noble-hashes)

Copy link
Member Author

@leotm leotm Apr 27, 2023

Choose a reason for hiding this comment

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

the shim's unsupported big int literals on android-jsc (e.g. 1n) remain
but yeah not rly a prob long as @ethereumjs/util doesn't complain

Copy link
Member Author

@leotm leotm Apr 27, 2023

Choose a reason for hiding this comment

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

testing @ethereumjs/util 8.0.6 with arrToBufArr 64fb902 throws (screenshot)

error: Error: Unable to resolve module zlib from .../metamask-mobile/node_modules/micro-ftch/index.js: zlib could not be found within the project or in these directories:
  node_modules

which is true node_modules/zlib doesn't exist 🤔

and noticed
@ethereumjs/util/src/index.ts also exports * from './provider'
'./provider' imports fetch from 'micro-ftch'
'./provider' exports fetchFromProvider


edit: ah zlib is a node API we'd need to shim too for arrToBufArr
looks like https://github.com/tradle/rn-nodeify/blob/master/shims.js#L3 already has browserify-zlib to potentially add to our postinstall.sh (otherwise shim manually again like big-integer)
if going down this potential whack-a-mole route, to get all @ethereumjs/util methods like arrToBufArr working

also we have a Metro warning which could be related

warn Package @metamask/eth-sig-util has been ignored because it contains invalid configuration. Reason: Package subpath './package.json' is not defined by "exports" in /Users/leo/Documents/GitHub/metamask-mobile/node_modules/@metamask/eth-sig-util/package.json

Copy link
Member Author

Choose a reason for hiding this comment

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

just checked a fresh (RN 0.71.6) as well
switched off Hermes (for android-jsc)
added and ran rn-nodeify --install http,https,zlib,events,util --hack --yarn (minimal shims)
added and manually shimmed big-integer
added @ethereumjs/util 8.0.6 and tested arrToBufArr
which gets us past node missing shim issues like zlib
onto a new issue (potentially more):

rn-nodeify --install http,https,zlib,events,util --hack --yarn

And create a new example Uint8Array
Error: Unable to resolve module zlib from .../metamask-mobile/node_modules/micro-ftch/index.js: zlib could not be found within the project or in these directories: node_modules

@ethereumjs/util/src/index.ts exports * from './provider'
'./provider' imports fetch from 'micro-ftch'
'./provider' exports fetchFromProvider
@leotm leotm changed the title big-integer shim big-integer shim testing on android-jsc Apr 27, 2023
leotm added a commit that referenced this pull request May 2, 2023
- react-native-community/jsc-android-buildscripts#159 (comment)
- facebook/react-native#35504 (comment)
- vanilla RN 0.71.6 tested working: 1n+2n, BigInt(1)+BigInt(2)
- package.json>resolution not required (result: resolution field incompatible)
- org.webkit:android-jsc-intl:+ required (which we're already using)

294992.0.0 prevents need for e.g. `big-integer` shim, till RN 0.71.6 shipped
#6221

294992.0.0 prevents need for BE patches, till RN 0.71.6 upgrade shipped
e.g. #6305 cc @Cal-L

But (the catch) 294992.0.0 results in repo-specific dep build error,
possibly more deps to resolve, likely via patch(es):

> Configure project :react-native-reanimated
No AAR for react-native-reanimated found. Attempting to build from source.
Android gradle plugin: 4.2.2
Gradle: 6.9

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.9/userguide/command_line_interface.html#sec:command_line_warnings
Error: Command failed: ./gradlew app:installProdDebug -PreactNativeDevServerPort=8081
OpenJDK 64-Bit Server VM warning: Ignoring option MaxPermSize; support was removed in 8.0
Warning: Mapping new ns http://schemas.android.com/repository/android/common/02 to old ns http://schemas.android.com/repository/android/common/01
Warning: Mapping new ns http://schemas.android.com/repository/android/generic/02 to old ns http://schemas.android.com/repository/android/generic/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/02 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/03 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/02 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/03 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/03 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/02 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01

FAILURE: Build failed with an exception.

* Where:
Build file '/Users/leo/Documents/GitHub/metamask-mobile/node_modules/react-native-reanimated/android/build.gradle' line: 1059

* What went wrong:
A problem occurred evaluating project ':react-native-reanimated'.
> Expected directory '/Users/leo/Documents/GitHub/metamask-mobile/node_modules/react-native/../jsc-android/dist/org/webkit/android-jsc' to contain exactly one file, however, it contains no files.
@leotm leotm mentioned this pull request Jun 6, 2023
3 tasks
@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 7 days. Thank you for your contributions.

@github-actions github-actions bot added the stale Issues that have not had activity in the last 90 days label Jul 26, 2023
@Gudahtt
Copy link
Member

Gudahtt commented Jul 26, 2023

This should no longer be relevant now that we've upgraded React Native

@Gudahtt Gudahtt closed this Jul 26, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
stale Issues that have not had activity in the last 90 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants