-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Convert witnessToHex
into a method ToHexStrings
on TxWitness
#1991
Conversation
Pull Request Test Coverage Report for Build 8420439676Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
ce89bf6
to
f96021d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I think we should just have the helper method (ToHexStrings()
and perhaps a ParseWitness()
function) but skip the JSON (un)marshal support (see inline comment for the reason).
ToHexStrings
to TxWitness
ToHexStrings
to TxWitness
witnessToHex
a method ToHexStrings
on TxWitness
f96021d
to
c5de509
Compare
I've reduced the scope of this PR to just converting |
c5de509
to
75fe7e4
Compare
witnessToHex
a method ToHexStrings
on TxWitness
witnessToHex
into a method ToHexStrings
on TxWitness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, LGTM 🎉
Something that's also on my wish list is a simple Serialize() ([]byte, error)
method for TxWitness
, if you feel like adding that as well (new PR or this one, your call).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add a method
ToHexStrings
toTxWitness
. Remove stand alone functionwitnessToHex(witness wire.TxWitness) []string
.Motivation:
witnessToHex
is hard to access and to find.