Refactor PSBT module & add new PSBT fields #44
Conversation
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
SachinMeier
commented
Nov 2, 2021
kafaichoi
reviewed
Nov 2, 2021
lib/extendedkey.ex
Outdated
| def to_bin(%__MODULE__{child_nums: child_nums}) do | ||
| try do | ||
| {:ok, %__MODULE__{child_nums: tfrom_string(String.split(pathstr, "/"))}} | ||
| {:ok, tto_bin(child_nums, <<>>)} |
Contributor
There was a problem hiding this comment.
what's meaning of the prefix t for private function like tfrom_string, tto_bin?
Contributor
Author
There was a problem hiding this comment.
I named the recursive helper functions differently, but there's no real reason for that. I'll remove and make them all the same.
Contributor
philipglazman
left a comment
philipglazman
left a comment
There was a problem hiding this comment.
Same Q as @bruteforcecat. Otherwise, lgtm
kafaichoi
reviewed
Jan 1, 2023
lib/extendedkey.ex
Outdated
| (used for PSBT encoding) | ||
| """ | ||
| @spec serialize(t(), atom) :: binary | ||
| def serialize(xkey = %__MODULE__{}, :no_checksum) do |
Contributor
There was a problem hiding this comment.
nit. it will be great to merge &serialize/1 and &serialize/2 into one &serialize/2 with type signature like
@spec serialize(t(), list({:with_checksum?, boolean}) :: binary
def serialize(xkey, opts \\ []) do
with_checksum? = Keyword.get(opts, :with_checksum?, true)
extended_key_without_checksum_bin = (xkey.prefix <>
xkey.depth <> xkey.parent_fingerprint <> xkey.child_num <> xkey.chaincode <> xkey.key)
case with_checksum? do
true ->
Base58.append_checksum(extended_key_without_checksum_bin)
false ->
extended_key_without_checksum_bin
end
end
I think it's more elixir-idiomatic, better typing for user using dialyzer and less code duplication
kafaichoi
reviewed
Jan 1, 2023
SachinMeier
commented
Feb 17, 2023
| pathstr | ||
| |> String.split("/") | ||
| |> path_from_string([]) | ||
| |> Enum.reverse() |
Contributor
Author
There was a problem hiding this comment.
move split & reverse into path_from_string,
SachinMeier
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
#42
Make PSBT use Derivation Path & Xpub modules
Add Taproot fields & PSBT v2 fields
Refactor some but not all odd logic
TODO:
add_<field>funcs