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

Replace Uint8Array.from with new Uint8Array #553

Merged
merged 3 commits into from
Jul 13, 2020
Merged

Conversation

sisou
Copy link
Member

@sisou sisou commented Apr 30, 2020

  • All tests pass. Demo project builds and runs.
  • I have resolved any merge conflicts.

What's in this pull request?

In the voting app, we ran into an issue on iOS Safari, which threw an error of
TypedArray.from requires its this argument subclass a TypedArray constructor when trying to initialise a GenesisConfig. The stack trace traces it back to BufferUtils.fromHex().

Eventually, the issue was that an external library that was bundled with the app overwrote the browser's native Uint8Array constructor with a shim, which triggered the error in Safari.

While we will fix it in the voting app by removing this shim, we can also make the Nimiq Core library more resilient to this problem by simply not using Uint8Array.from(), but using new UintArray() instead. This PR does exactly that. It does the byte-mapping before handing it to the Uint8Array constructor, avoiding the problem.

@sisou sisou requested a review from jeffesquivels April 30, 2020 11:51
@sisou sisou self-assigned this Apr 30, 2020
@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@362d677). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #553   +/-   ##
=========================================
  Coverage          ?   72.91%           
=========================================
  Files             ?      206           
  Lines             ?    14094           
  Branches          ?        0           
=========================================
  Hits              ?    10276           
  Misses            ?     3818           
  Partials          ?        0           
Impacted Files Coverage Δ
src/main/generic/utils/buffer/BufferUtils.js 88.46% <100.00%> (ø)
...ain/generic/consensus/base/primitive/Commitment.js 77.77% <0.00%> (ø)
src/main/generic/miner/MinerWorker.js 100.00% <0.00%> (ø)
...us/base/transaction/index/TransactionStoreEntry.js 100.00% <0.00%> (ø)
...in/generic/consensus/base/account/PrunedAccount.js 77.77% <0.00%> (ø)
...form/browser/network/websocket/WebSocketFactory.js 33.33% <0.00%> (ø)
src/main/generic/api/Client.js 61.73% <0.00%> (ø)
...c/main/generic/network/message/SubscribeMessage.js 92.85% <0.00%> (ø)
...in/generic/consensus/base/blockchain/BlockChain.js 54.02% <0.00%> (ø)
src/main/generic/utils/array/HashMap.js 83.33% <0.00%> (ø)
... and 197 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 362d677...b888bb8. Read the comment docs.

@sisou sisou force-pushed the soeren/from-to-new branch from 8bf0c3a to a898c7b Compare July 6, 2020 08:38
@styppo styppo force-pushed the soeren/from-to-new branch from a898c7b to b888bb8 Compare July 13, 2020 14:33
@styppo styppo merged commit b888bb8 into master Jul 13, 2020
@styppo styppo deleted the soeren/from-to-new branch July 14, 2020 07:39
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.

2 participants