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.

Sign up for GitHub

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

Jump to bottom

add from/toBech32 #846

Merged
merged 2 commits into from
Aug 21, 2017
Merged

add from/toBech32 #846

merged 2 commits into from
Aug 21, 2017

Conversation

dcousens
Copy link
Contributor

@dcousens dcousens commented Aug 14, 2017 •
edited
Loading

Reliant on bitcoinjs/bech32#10

TODO

  • Add toBech32/fromBech32
  • Add toString/fromString
  • Add toOutputScript and fromOutputScript support

@dcousens dcousens self-assigned this Aug 14, 2017
@dcousens dcousens requested review from afk11 and karelbilek August 14, 2017 07:16
@karelbilek
Copy link
Contributor

In the fixtures, what is the difference between bech32/invalid and invalid/frombech32? Why do these differ slightly?

Other than that, looks fine to me

@dcousens dcousens force-pushed the bech32 branch 3 times, most recently from 8420f95 to 2a8a9d5 Compare August 15, 2017 00:35
@dcousens
Copy link
Contributor Author

dcousens commented Aug 17, 2017 •
edited
Loading

Will add toOutputScript/fromOutputScript support soon.

toString/fromString is unnecessary and has a non-sensible return type.

@dcousens dcousens added this to the 3.1.0 milestone Aug 17, 2017
@kabutosamourai
Copy link

Is this work going to abstract the human-readable prefixes to networks, or will that be a future addition?

@dcousens
Copy link
Contributor Author

dcousens commented Aug 17, 2017 •
edited
Loading

@kabutosamourai yes.

I was thinking network.bech32 or network.bech32Prefix - any preference?

See 51fd6b0

@dcousens
Copy link
Contributor Author

PR ready to merge, ping reviewers

@dcousens dcousens requested review from jprichardson and a team August 17, 2017 04:21
@dcousens
Copy link
Contributor Author

dcousens commented Aug 17, 2017 •
edited
Loading

@kabutosamourai I added you to the org - no pressure, but your input is valuable 👍

I was going to see if having a reviewers team was worth it... but we have branch protection in play - so commit rights shouldn't be an issue.

@kabutosamourai
Copy link

network.bech32 makes sense.

The new internals of fromOutputScript and toOutputScript are really nice.

ACK

@dcousens dcousens merged commit b1272a1 into master Aug 21, 2017
@dcousens dcousens deleted the bech32 branch August 21, 2017 01:17
@Ashtonulliac Ashtonulliac mentioned this pull request Aug 1, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afk11 afk11 Awaiting requested review from afk11

@karelbilek karelbilek Awaiting requested review from karelbilek

@jprichardson jprichardson Awaiting requested review from jprichardson

Assignees

@dcousens dcousens

Labels
feature
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@dcousens @karelbilek @kabutosamourai