Remove length prefix from the BytesEncoder
#5103
Conversation
Instead of having the length prefix handled by the `BytesEncoder` we can instead use an `Encoder2` to make the presences of the prefix explicit. With this applied there are no more usages of the `BytesEncoder::with_length_prefix` constructor. It will be removed in a separate patch.
Users can use an `Encoder2` to achive this at the cost of a little more code with the benefit of being more explicit and hence clearer.
|
Nice. Like to see all this red.
I would say "the idea is to remove unnecessary/redundant constructors to give us maximum API flexibility for 1.0". But ok, we agree on the code. |
| // Should have length prefix chunk, then data chunk, then exhausted. | ||
| let obj = [1u8, 2, 3]; | ||
| let test_bytes = TestBytes(&obj, true); | ||
| let mut encoder = test_bytes.encoder(); |
There was a problem hiding this comment.
I think we can still keep this test and use composition
let mut encoder =
Encoder2::new(CompactSizeEncoder::new(obj.len() as u64), BytesEncoder::without_length_prefix(&obj));| let enc1 = TestBytes(&[0xAA, 0xBB], false).encoder(); | ||
| let enc2 = TestBytes(&[0xCC], true).encoder(); |
There was a problem hiding this comment.
can be replaced with
let enc1 = TestBytes(&[0xAA, 0xBB]).encoder();
let enc2 = Encoder2::new(CompactSizeEncoder::new(1u64), BytesEncoder::without_length_prefix(&[0xCC]));| let test_bytes = TestBytes(&obj, true); | ||
| let mut encoder = test_bytes.encoder(); |
There was a problem hiding this comment.
same here we can use BytesEncoder composition with CompactSizeEncoder
|
hmm, I'm inclined to just clobber the tests even though you are correct @jrakibi. @apoelstra do you have an opinion? Or maybe I should just close this and get out of you way @jrakibi, you seem to have this under control and it was your idea first anyways. |
|
After reading #5090 (comment) I'm going to update this with 'please merge now' with my usual 'we can re-visit the test code during the RC cycle'. |
|
ACK c9c41f7 |
3 participants
First remove usage of
BytesEncoder::with_length_prefixwhen encoding a script. Then remove the constructor and the length prefix all together from theBytesEncoder.This is based on discussion in #5097, as suggested originally by jrakibi, and is a cleaner alternative to my first effort in #5098 (which tried to remove the other constructor).
The idea here is to have only one constructor per encoder so an encoders usage is unambiguous.