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

Fix: add new type to allow toBase58Check to get string in browser #2069

Closed

Conversation

fwx5618177
Copy link

@fwx5618177 fwx5618177 commented Apr 6, 2024

Issue:
fixes #2070

Aim:

  • Allow toBase58Check to get string param.

Reason:

  1. I can't use toBase58Check in the browser. It mentioned this error: Error: Expected property "0" of type Buffer, got Uint8Array
    image

Link: https://jasonandjay.github.io/bitcoinjs-lib/functions/address.fromBase58Check.html
Code:

// You can test it here and find more case in test/address.spec.ts
const encoder = new TextEncoder();
const hash = encoder.encode('1BgGZ9tcN4rm9KBzDn7KprQz87SZ26SAMH')
const decode = address.toBase58Check(hash)

console.log(decode.version) // 0

console.log(decode.hash.toString('hex')) // 751e76e8199196d454941c45d1b3a323f1433bd6

@fwx5618177
Copy link
Author

fwx5618177 commented Apr 6, 2024

@jasonandjay @johngame @junderw PTAL.

Copy link

@Jem256 Jem256 left a comment

Choose a reason for hiding this comment

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

Hello, thank you for this. Maybe you could consider editing your PR description to mention that this PR fixes #openissue so that it can indicate automatically what issue the PR is closing.

@fwx5618177 fwx5618177 changed the title feat: add new type to allow toBase58Check to get string in browser Fix: add new type to allow toBase58Check to get string in browser Apr 19, 2024
@fwx5618177
Copy link
Author

Hello, thank you for this. Maybe you could consider editing your PR description to mention that this PR fixes #openissue so that it can indicate automatically what issue the PR is closing.

Thanks, I've updated the description. Do we recently plan to upgrade it to esm module? Maybe I could join our community.

@fwx5618177 fwx5618177 requested a review from Jem256 April 19, 2024 10:13
@Jem256
Copy link

Jem256 commented Apr 19, 2024

Hello, thank you for this. Maybe you could consider editing your PR description to mention that this PR fixes #openissue so that it can indicate automatically what issue the PR is closing.

Thanks, I've updated the description. Do we recently plan to upgrade it to esm module? Maybe I could join our community.

Hey, I am not sure about this. I think one of the maintainers will provide better insight into that.

@junderw
Copy link
Member

junderw commented Apr 20, 2024

I don't understand what you're trying to do.

// This is your code
const encoder = new TextEncoder();
const hash = encoder.encode('1BgGZ9tcN4rm9KBzDn7KprQz87SZ26SAMH')
/*
hash is Uint8Array(34) [
  49,  66, 103,  71, 90,  57, 116, 99,  78,
  52, 114, 109,  57, 75,  66, 122, 68, 110,
  55,  75, 112, 114, 81, 122,  56, 55,  83,
  90,  50,  54,  83, 65,  77,  72
] */

The input to toBase58Check is a Buffer that contains the scriptPubkey's binary representation.

Why are you creating a UTF8 array with the UTF8 data for a string address?

This whole PR makes zero sense to me. I don't understand what you're trying to do, I don't understand why the random github io link is relevant, and I don't understand why the PR is taking a hex string when your example is trying to input a Uint8Array.......

Please fix the PR, or your body to make sense, and then I'll look at it again.

If I ignore your PR body and only look at the code, I would reject this because the API isn't for handling strings, and Buffer.from(xyz, 'hex') has weird behavior when an invalid string is input.

@fwx5618177
Copy link
Author

I don't understand what you're trying to do.

// This is your code
const encoder = new TextEncoder();
const hash = encoder.encode('1BgGZ9tcN4rm9KBzDn7KprQz87SZ26SAMH')
/*
hash is Uint8Array(34) [
  49,  66, 103,  71, 90,  57, 116, 99,  78,
  52, 114, 109,  57, 75,  66, 122, 68, 110,
  55,  75, 112, 114, 81, 122,  56, 55,  83,
  90,  50,  54,  83, 65,  77,  72
] */

The input to toBase58Check is a Buffer that contains the scriptPubkey's binary representation.

Why are you creating a UTF8 array with the UTF8 data for a string address?

This whole PR makes zero sense to me. I don't understand what you're trying to do, I don't understand why the random github io link is relevant, and I don't understand why the PR is taking a hex string when your example is trying to input a Uint8Array.......

Please fix the PR, or your body to make sense, and then I'll look at it again.

If I ignore your PR body and only look at the code, I would reject this because the API isn't for handling strings, and Buffer.from(xyz, 'hex') has weird behavior when an invalid string is input.

Underlying Reason:

  1. The current package lacks perfect compatibility in both browser and Node environments and is not the simplest to use. This issue is related to the handling of Buffers.

Reason for this modification:

  1. The function toBase58Check in the linked code requires processing strings into Buffer types for testing.
  2. Due to strict type checks, only Buffers can be passed, but Buffers are not available in the browser environment. We would have to introduce a buffer package to manage this, which complicates the usage unnecessarily.
  3. The essence of the buffer package is to convert strings into TypeArrays, which is why we are using new TextEncoder(). The appended code demonstrates a test of the current method, which proved non-functional and necessitates modification.
  4. Therefore, in response to the errors mentioned in the PR, I attempted an extensible fix that allows the input of both Buffer and string, thus ensuring broader applicability of this method.

Handling hexadecimal strings:

hash = Buffer.isBuffer(hash) ? hash : Buffer.from(hash, 'hex');

This revised code snippet and explanation should address the concerns raised. By clarifying the need for flexibility in input types and simplifying integration across different environments, we ensure that the PR aligns better with user expectations and practical functionality.

@junderw
Copy link
Member

junderw commented Apr 20, 2024

Buffer.from(hash, 'hex'); is not sufficient to guarantee that the input is a properly formatted hex string.

> Buffer.from('abcdef', 'hex')
<Buffer ab cd ef>
> Buffer.from('abc def', 'hex')
<Buffer ab>
> Buffer.from('abZZZZZZ', 'hex')
<Buffer ab>

strings are not how you should deal with binary data.

Closing in favor of #1855 which is currently being worked on by a few Summer of Bitcoin participants.

@junderw junderw closed this Apr 20, 2024
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.

Mention error toBase58Check on Browser
3 participants