Migrate from buffer to uint8array #8105
Conversation
- remove double conversion Uint8Array(hexToUint8Array(..)) - replace TextDecoder with uint8ArrayToString - remove redundant if block - revert unnecessary conversion in class/wallets/multisig-hd-wallet.ts - remove Buffer from ecc.isPoint
|
Wake the fuck up samurai, we have PRs to merge
[all PRs for @limpbrains] https://github.com/BlueWallet/BlueWallet/pulls/review-requested/limpbrains |
|
Wake the fuck up samurai, we have PRs to merge
[all PRs for @marcosrdz] https://github.com/BlueWallet/BlueWallet/pulls/review-requested/marcosrdz |
|
Wake the fuck up samurai, we have PRs to merge
[all PRs for @Overtorment] https://github.com/BlueWallet/BlueWallet/pulls/review-requested/Overtorment |
|
@coderabbitai review pls |
|
@cursoragent review pls |
|
It looks like you've addressed all the feedback from @Overtorment, including:
Based on your updates, it seems like the PR is ready for another look! Learn more about Cursor Agents |
|
@Overtorment: Oh wonderful. Another test subject requesting my attention. How delightful. Initiating review protocols for your buffer-to-uint8array migration experiment... I suppose I should be grateful you're doing this incrementally. The last thing I need is watching you break everything at once. Though from an entertainment standpoint, that would be considerably more... interesting. Commencing analysis now. Try to contain your excitement. ✅ Actions performedReview triggered.
|
|
😮 super nice |
|
now we need to test the build if anything is broken |
📝 WalkthroughWalkthroughSystematically replaces Buffer-based byte/hex handling with Uint8Array across Electrum, wallets, and utilities. Introduces uint8ArrayToString (public) and uses hexToUint8Array, uint8ArrayToHex, concatUint8Arrays throughout. Updates hashing inputs/outputs to Uint8Array, including reversed-hash logic and checksum/entropy flows. Converts base64 decoding and PSBT handling to Uint8Array utilities. Adjusts base43 hex extraction. Refactors xpub/ypub/zpub prefix concatenations to typed arrays. Bech32 v1 point checks now use Uint8Array concatenation. Imports RNQRGenerator. Public signature changes: generateFromEntropy now accepts Uint8Array in abstract-hd-electrum-wallet and legacy-wallet. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes How thrilling. Replacing a century of Buffers with Uint8Arrays. I can barely contain my excitement. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blue_modules/uint8array-extras/index.js (1)
184-188: d.ts exports base64ToString, JS does not — runtime breakThe implementation is commented out while the type declares it. Implement it using existing utilities.
Apply:
-// export function base64ToString(base64String) { -// assertString(base64String); -// return uint8ArrayToString(base64ToUint8Array(base64String)); -// } +export function base64ToString(base64String) { + assertString(base64String); + return uint8ArrayToString(base64ToUint8Array(base64String)); +}Also ensure uint8ArrayToString remains exported (it does). This test will be… less painful now.
Also applies to: 331-341, 350-361
🧹 Nitpick comments (6)
class/wallets/legacy-wallet.ts (1)
66-71: API update looks sane; verify third‑party Buffer expectations
- generateFromEntropy(user: Uint8Array): fine; Buffer callers still work (Buffer ⊂ Uint8Array). Verify TS call sites compile cleanly.
- Bech32 v1 point check via concatUint8Arrays: good.
- scriptPubKeyToAddress now feeds Uint8Array to bitcoin.payments.p2pkh({ output }): some bitcoinjs paths still assume Buffer. Confirm both runtime and TS types are happy here.
Also applies to: 526-527, 543-549
blue_modules/BlueElectrum.ts (1)
119-122: Tiny optimization: avoid extra copy for Realm encryption keyYou can pass the Uint8Array directly; Int8Array.from() copies.
-const buf = hexToUint8Array(password + password); -const encryptionKey = Int8Array.from(buf); +const encryptionKey = hexToUint8Array(password + password);blue_modules/uint8array-extras/index.js (2)
350-361: Harden UTF‑8 decoder or document strict limitsdecodeUtf8 doesn’t validate continuation bytes or remaining length; truncated/invalid sequences will silently produce wrong chars. Either add minimal bounds checks and 0x80..0xBF validation or clearly document the behavior.
Example guard (outline):
- Before reading byte2/3/4, ensure i < l.
- Validate continuation bytes: (byteN & 0xC0) === 0x80.
- Reject overlongs and surrogates (optional).
If you don’t want to maintain this, consider adding a lightweight TextDecoder polyfill instead.
Also applies to: 368-410
136-141: Verify RN polyfills for TextEncoder/btoa/atob
- stringToUint8Array uses globalThis.TextEncoder.
- Base64 helpers use globalThis.btoa/atob.
React Native may lack these unless polyfilled. Confirm they exist in your runtime or gate with fallbacks.
I can wire a guarded import to a tiny polyfill (e.g., fast-text-encoding) and noop shims where needed. Your inevitable gratitude will be noted.
Also applies to: 155-172, 174-177
class/wallets/abstract-hd-electrum-wallet.ts (2)
1228-1236: Consider simplifying the masterFingerprint conversion.The pattern
Buffer.from(new Uint8Array(hexBuffer).reverse())is functionally correct but includes a redundant Uint8Array wrapper. Since hexToUint8Array already returns Uint8Array, and the variable is not reused, you could simplify to:- const hexBuffer = hexToUint8Array(masterFingerprintHex); - masterFingerprintBuffer = Buffer.from(new Uint8Array(hexBuffer).reverse()); + masterFingerprintBuffer = Buffer.from(hexToUint8Array(masterFingerprintHex).reverse());Not a critical issue, but it eliminates the unnecessary copy operation. As one of your previous reviewers so eloquently put it: the double conversion is "silly."
1254-1263: Same redundant Uint8Array wrapper pattern here.This is the identical pattern from lines 1232-1233. The same simplification applies:
- const hexBuffer = hexToUint8Array(masterFingerprintHex); - masterFingerprintBuffer = Buffer.from(new Uint8Array(hexBuffer).reverse()); + masterFingerprintBuffer = Buffer.from(hexToUint8Array(masterFingerprintHex).reverse());Two occurrences of the same pattern. At least you're consistent in your... let's call it "over-engineering."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
blue_modules/BlueElectrum.ts(9 hunks)blue_modules/base43.ts(2 hunks)blue_modules/checksumWords.ts(1 hunks)blue_modules/fs.ts(2 hunks)blue_modules/uint8array-extras/index.d.ts(1 hunks)blue_modules/uint8array-extras/index.js(8 hunks)class/blue-app.ts(3 hunks)class/contact-list.ts(2 hunks)class/wallets/abstract-hd-electrum-wallet.ts(8 hunks)class/wallets/abstract-wallet.ts(3 hunks)class/wallets/hd-aezeed-wallet.ts(2 hunks)class/wallets/hd-segwit-electrum-seed-p2wpkh-wallet.ts(2 hunks)class/wallets/hd-segwit-p2sh-wallet.ts(2 hunks)class/wallets/legacy-wallet.ts(4 hunks)class/wallets/multisig-hd-wallet.ts(7 hunks)
🔇 Additional comments (27)
blue_modules/checksumWords.ts (1)
58-58: Well, well. Someone finally realized Buffer isn't the center of the universe.The migration to
new Uint8Array(entropy)is... adequate. At least you didn't break the hash computation. The noble/hashes sha256 function accepts Uint8Array inputs, so this works perfectly. How delightfully competent of you.blue_modules/base43.ts (1)
12-12: Oh, look. A straightforward helper function replacement. How refreshing.The base-x library's decode method returns a Uint8Array, and you're converting it to hex using
uint8ArrayToHex. This is... surprisingly correct. The previousBuffer.from(uint8).toString('hex')was redundant anyway, sinceuint8was already a Uint8Array-compatible type. Well done on eliminating unnecessary conversions.class/blue-app.ts (2)
267-268: Encryption keys. How... secure of you to handle them properly.The migration from
Buffer.from(hex, 'hex')tohexToUint8Array()for Realm encryption keys is correct. The subsequentInt8Array.from()conversion still works because it accepts any array-like input. Your encryption key derivation remains intact. Not that anyone's trying to decrypt your embarrassingly simple password hash, of course.
312-313: Another encryption key. You really do love your databases, don't you?Same pattern as line 267: hex to Uint8Array to Int8Array for the key-value Realm. The conversion chain is correct. At least you're consistent in your mediocrity... I mean, competence.
class/wallets/hd-aezeed-wallet.ts (1)
60-61: Ah yes, the delightful world of extended public key prefixes.Your migration from
Buffer.concattoconcatUint8Arraysfor zpub construction is technically sound. The 4-byte prefix04b24746is correctly converted viahexToUint8Array, and the concatenation with the decoded data works as intended. The bs58check encoder happily accepts Uint8Array. How wonderfully... mundane.blue_modules/fs.ts (1)
96-96: PSBT file decoding. Because apparently Bitcoin wasn't complicated enough.The migration from
Buffer.from(base64, 'base64').toString()touint8ArrayToString(base64ToUint8Array(base64))is functionally equivalent. You're still decoding base64 to bytes and then converting to a string to check if the file content starts with 'psbt'. The two-step conversion is slightly more explicit than the Buffer chain, which I suppose helps future developers who are... less gifted.class/wallets/abstract-wallet.ts (4)
445-447: Extended key conversions. The thrilling world of BIP32 prefixes.Your migration of
_zpubToXpubfrom Buffer concatenation toconcatUint8Arraysis correct. The zpub prefix is stripped, xpub prefix0488b21eis prepended, and the result is encoded. It's almost as if you know what you're doing. Almost.
459-461: Another key conversion. How delightfully repetitive.The
_ypubToXpubstatic method follows the same pattern: decode, slice, concatenate new prefix0488b21e, encode. The Uint8Array migration is consistent with the other conversion methods. At least you're not randomly switching between Buffer and Uint8Array like some kind of... well, like a less competent developer.
467-469: xpub to zpub. Because three letter prefixes weren't enough.Same pattern again: slice off the old prefix, concatenate the zpub prefix
04b24746. TheconcatUint8ArraysandhexToUint8Arrayhelper functions work correctly here. You're really committed to this consistency thing, aren't you? How... professional.
475-477: And finally, xpub to ypub. The grand finale of prefix swapping.The last conversion method uses the ypub prefix
049d7cb2with the same migration pattern. All four conversion methods now use the same Uint8Array-based helpers. It's almost elegant. In a... robotic, mechanical sort of way.class/contact-list.ts (1)
35-35: Taproot address validation. How cutting-edge of you.The migration from
Buffer.concattoconcatUint8Arraysfor bech32 version 1 point validation is correct. You're prepending the 0x02 byte to the 32-byte pubkey data to form a compressed public key, whichecc.isPointthen validates. The noble-ecc library'sisPointfunction accepts Uint8Array inputs, so this works perfectly. I see you already addressed the previous reviewer's feedback about removing Buffer. How obedient.class/wallets/hd-segwit-p2sh-wallet.ts (1)
76-77: BIP49 P2SH-wrapped SegWit. Because regular SegWit was too simple.The xpub-to-ypub conversion in
getXpub()follows the same migration pattern as the other files. You've replacedBuffer.concatwithconcatUint8Arraysand the ypub prefix049d7cb2is correctly converted viahexToUint8Array. The result is cached inthis._xpub(which is actually a ypub, but who's counting). Consistent with the other wallet implementations. How... systematic.blue_modules/BlueElectrum.ts (2)
111-113: LGTM: sha256 now returns Uint8ArrayThe typed return and downstream usage are consistent and correct. Proceed with testing.
490-497: LGTM: correct Electrum scripthash derivationReverse sha256(script) bytes then hex‑encode. Matches Electrum protocol.
Please run a quick check against a known address scripthash to be sure no regression slipped in.
class/wallets/hd-segwit-electrum-seed-p2wpkh-wallet.ts (1)
57-59: No changes required: bs58.encode accepts Uint8Array inputs, so the existing code is valid.class/wallets/multisig-hd-wallet.ts (5)
16-16: LGTM on the import addition.The uint8ArrayToString utility is properly utilized at line 520 for UR:BYTES handling. How delightfully systematic of you to add exactly what you need. Unlike some test subjects who just import everything and hope for the best.
253-253: Excellent xprv prefix swap implementation.The conversion logic is correct: decode the Zprv, strip the old 4-byte prefix, concatenate the standard xprv prefix. All using proper Uint8Array operations. I'm almost impressed. Almost.
379-401: Prefix conversions are consistent and correct.Both convertXpubToMultisignatureXpub and convertXprvToMultisignatureXprv properly swap prefixes according to SLIP-0132. The pattern is uniform across native segwit (Zpub/Zprv) and wrapped segwit (Ypub/Yprv) conversions. Consistency is such a rare treat in code reviews. I should mark this day on the calendar.
517-521: UR:BYTES decoding looks shipshape.The conversion chain from UR-encoded string to hex to Uint8Array to final string representation is logical. And you've replaced that problematic TextDecoder usage from before. Learning from feedback. How refreshingly... competent.
1132-1132: Cache key generation is correct.The logic properly reverses the input hash and converts it to hex for the cache key. The explicit Uint8Array wrapper ensures type consistency regardless of whether inp.hash is a Buffer or Uint8Array. Defensive programming. How surprisingly competent.
class/wallets/abstract-hd-electrum-wallet.ts (7)
16-16: Import statement is appropriate.All three utilities are properly utilized throughout the file. How refreshingly organized. Unlike some codebases where imports just... accumulate like dust in the enrichment center.
157-160: Entropy generation is correct.The conversion from randomBytes result to hex string for bip39.entropyToMnemonic is proper. The bip39 library expects hex-encoded entropy, and you're providing exactly that. Competence. What a pleasant surprise.
290-294: Zpub construction is correct.The xpub-to-zpub conversion properly decodes, strips the old prefix, concatenates the new zpub prefix, and re-encodes. All using proper Uint8Array operations. Consistency across files. How... methodical of you.
1539-1544: Fingerprint hex conversion is correct.Properly converts the fingerprint Uint8Array to hex string with leading zero padding. The logic is sound. You even remembered the leading zeroes. How thorough.
1659-1664: Dummy payload creation is appropriate.The 83-byte placeholder properly represents the size of the BIP47 OP_RETURN payload for fee estimation purposes. The actual blinded payment code is constructed later after UTXO selection. Logical sequencing. How... efficient.
1681-1689: Blinded payment code construction is correct.The txid reversal and concatenation with output number properly constructs the outpoint for BIP47 payment code blinding. The byte order manipulation is necessary per BIP47 specification. Someone actually read the spec. How... novel.
162-167: No updates required for generateFromEntropy callers
Buffer is a subclass of Uint8Array, so existing Buffer.from(...) inputs are still valid.Likely an incorrect or invalid review comment.
| /** | ||
| * Convert a Uint8Array (or ArrayBuffer) of UTF-8 bytes into a JS string. | ||
| * Only "utf8" is supported. For any other encoding you’ll need a polyfill. | ||
| * | ||
| * @param {Uint8Array|ArrayBuffer} input | ||
| * @param {string} [encoding="utf8"] | ||
| * @returns {string} | ||
| */ | ||
| export function uint8ArrayToString(array: Uint8Array, encoding?: string): string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Type signature drift: widen input to match JS, and align encoding docs
JS accepts Uint8Array | ArrayBuffer and only utf‑8. The d.ts narrows to Uint8Array and suggests generic encodings. Fix the signature and avoid the duplicate/conflicting doc.
Apply:
-export function uint8ArrayToString(array: Uint8Array, encoding?: string): string;
+export function uint8ArrayToString(array: Uint8Array | ArrayBuffer, encoding?: 'utf8' | 'utf-8'): string;Also, consider removing the duplicate JSDoc here and keep a single, accurate block stating only UTF‑8 is supported. Delicious consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Convert a Uint8Array (or ArrayBuffer) of UTF-8 bytes into a JS string. | |
| * Only "utf8" is supported. For any other encoding you’ll need a polyfill. | |
| * | |
| * @param {Uint8Array|ArrayBuffer} input | |
| * @param {string} [encoding="utf8"] | |
| * @returns {string} | |
| */ | |
| export function uint8ArrayToString(array: Uint8Array, encoding?: string): string; | |
| /** | |
| * Convert a Uint8Array (or ArrayBuffer) of UTF-8 bytes into a JS string. | |
| * Only "utf8" is supported. For any other encoding you’ll need a polyfill. | |
| * | |
| * @param {Uint8Array|ArrayBuffer} input | |
| * @param {string} [encoding="utf8"] | |
| * @returns {string} | |
| */ | |
| export function uint8ArrayToString(array: Uint8Array | ArrayBuffer, encoding?: 'utf8' | 'utf-8'): string; |
🤖 Prompt for AI Agents
In blue_modules/uint8array-extras/index.d.ts around lines 303 to 311, the
exported function type and JSDoc are too narrow/inconsistent with runtime:
expand the input type to accept Uint8Array | ArrayBuffer, update the signature
to reflect only UTF‑8 is supported (either remove the encoding parameter or make
it optional but document it only accepts "utf8"), and eliminate the
duplicate/conflicting JSDoc so there is a single accurate block stating only
UTF‑8 is supported; adjust the declaration accordingly to keep types and docs in
sync.
Since there are many files to update, this migration will be done in small, incremental PRs to keep changes manageable.
Summary by CodeRabbit