update base58 to have simpler logic and more tests#1
Merged
Conversation
Contributor
|
look good to me. Would you want to add typespec and implement encode for Base58Check? |
leishman
force-pushed
the
leishman--add-base-58-tests
branch
from
518a149 to
249d883
Compare
leishman
changed the title
Member
Author
|
@bruteforcecat I have updated the branch with a lot of improvements and all tests are passing. |
leishman
changed the title
kafaichoi
reviewed
May 27, 2019
| |> encode_base() | ||
| end | ||
|
|
||
| @spec encode_base(binary) :: binary |
Contributor
There was a problem hiding this comment.
overlapping typespec should change this to @spec encode_base(binary) :: String.t() and remove the one below
| end | ||
|
|
||
| @doc """ | ||
| Encode a byte array into a base58 encoded string |
Contributor
There was a problem hiding this comment.
it should be a binary instead byte array which is list(byte)
| @type byte_list :: list(byte()) | ||
|
|
||
| @doc """ | ||
| Decode a base58 check encoded string into a byte array and validate checksum |
Contributor
There was a problem hiding this comment.
binary instead of byte array
| end | ||
|
|
||
| @doc """ | ||
| Decode a base58 encoded string into a byte array |
Contributor
There was a problem hiding this comment.
binary instead of byte array
Contributor
|
Look super awesome! just leave some comment about doc and typespec. please merge it after fixing them |
… and comment fixes
leishman
force-pushed
the
leishman--add-base-58-tests
branch
from
7d4a256 to
6a94cef
Compare
Member
Author
|
fixed. thanks for the great comments. will merge |
kafaichoi
pushed a commit
to kafaichoi/bitcoinex
that referenced
this pull request
Apr 21, 2021
Sachin add privkey module
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this PR I update the base58 module to rely more on built-in Elixir functions inspired by the approach taken here, which I think is very elegant (https://github.com/comboy/bitcoin-elixir/blob/develop/lib/bitcoin/base58_check.ex).
This is still a WIP with more tests to write and cleanups around the public functions, along with documentation and fixing some compile bugs due to usage in other modules.
@bruteforcecat what do you think about these changes?