encoding/blockchain: Encapsulate "varstrlist" and "extensible string" patterns #376
Conversation
|
Carves off part of #349 for easier review. |
|
PTAL |
|
👀 |
| defer bufpool.Put(buf) | ||
| t.WriteInputCommitment(buf) | ||
| blockchain.WriteVarstr31(w, buf.Bytes()) | ||
| blockchain.WriteExtensibleString(w, func(w io.Writer) error { |
There was a problem hiding this comment.
Can this be
blockchain.WriteExtensibleString(w, t.WriteInputCommitment)
and same for writeInputWitness below?
There was a problem hiding this comment.
Yes. I had to revise those functions to return errors first, but nice.
|
|
||
| // WriteExtensibleString sends the output of the given function, | ||
| // together with a varint31 length prefix, to w. | ||
| func WriteExtensibleString(w io.Writer, f func(io.Writer) error) (int, error) { |
There was a problem hiding this comment.
Can/should we document that WriteExtensibleString always provides a *bytes.Buffer to f, and that the signature of f is this way mainly so that it matches io.WriterTo? That way, f can safely ignore errors from its Writer (which is guaranteed to be a *bytes.Buffer, which never returns an error from Write). That would let us clean up some (or maybe just one) of the call sites.
There was a problem hiding this comment.
Eh, I don't find that suggestion especially compelling. Eliminating two or three error checks in a Go program feels like spitting into the ocean.
|
Revised, PTAL |
|
LGTM |
functions (Read|Write)(VarstrList|ExtensibleString) and use them in appropriate places.
Use the new functions throughout
protocol/bc.