-
Notifications
You must be signed in to change notification settings - Fork 25
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
deps: modernization and upgrades #584
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Tested this locally, seems to work fine for all the broad/important cases. Some questions and comments below, and out of band.
- upgrade: keccak, keythereum, secp256k1, truffle, web3 - remove: react-hot-loader, @hot-loader/react-dom - update README with correct Node version
included changes: - converted some files to Typescript and added typings to aid debugging - reorganized some crypto utilities and constants - added our first test! ... currently being skipped for now (:
Use sha256 instead of keccak256 to ensure 32-byte output for ecdsaSign
- update Development doc - renamings - remove dead code
- update Testing note per fang's feedback
Add context to error message logging Co-authored-by: fang <github@fang.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed out of band, this now works just fine across flows and devices, as far as I can tell. Great work here! Can't wait to finally push out minimized builds again. (:
Context
The JS ecosystem moves quickly 🏎️ This PR brings Bridge up-to-date in several crypto libs, and removes unused development libs (since replaced by latest version of
react-scripts
). It also seems to resolve the issue we were having with unreliable builds.Changes
Testing
Since there are no automated tests (yet!), we manually tested these changes by following these steps:
rm -rf node_modules/
- first, remove existing depsnpm install
- reinstall JS packages and rebuild native extensionsnpm run build
- generate production-optimized buildnpm run pilot
- confirm that local development environment starts successfullyOnce confirming the app was building and loading successfully, we then tested the following flows:
Fixes #410