BitVec: Improve the encoding and consolidate the implementations

[bkchr]

No description provided.

[bkchr]

This technically is semver breaking, but I would go this route. BitVec already required the same bound.

[jsdw]

In general, the tests don't really look like they test that the bitvec is still being encoded in the same binary format as previously, only that it can be encoded and decoded back and still be equal. Is it worth adding a couple of tests to check that the SCALE encoded bytes are what we expect, too?

[dvdplm]

A before/after test like James suggests is a good idea to add.

[dvdplm]

Could this be moved outside the loop perhaps and re-used? Or will the compiler re-use element anyway?

[bkchr]

This would be wrong. As the last chunk could have less bits than the ones before, we could end up with an element having bits set that should not be set. I mean that should be fixed on the decoding side. However, I also don't see any real performance issue with having the primitive type inside the loop.

[bkchr]

In general, the tests don't really look like they test that the bitvec is still being encoded in the same binary format as previously, only that it can be encoded and decoded back and still be equal. Is it worth adding a couple of tests to check that the SCALE encoded bytes are what we expect, too?

The old encode was wrong. It would have lead to decode decoding junk. This is now fixed and the expected encoding format didn't changed.

[jsdw]

Yup, of course in some cases it was broken previously. But it would be nice to have a couple of tests to check that the encoding format is what we are expecting it to be in other places, for example that bitvec![u8, Lsb0, 0, 0, 1, 1] encodes to the same as (Compact<u32>(4), 3u8) (or whatever the correct encoding would be).

Actually, I had a play myself and this naive test:

#[test]
fn bitvec_u8_encodes_as_expected() {
	let cases = vec![
		(bitvec![u8, Lsb0; 0, 0, 1, 1].encode(), (Compact(4u32), 0b00001100u8).encode()),
		(bitvec![u8, Lsb0; 0, 1, 1, 1].encode(), (Compact(4u32), 0b00001110u8).encode()),
		(bitvec![u8, Lsb0; 1, 1, 1, 1].encode(), (Compact(4u32), 0b00001111u8).encode()),
		(bitvec![u8, Lsb0; 1, 1, 1, 1, 1].encode(), (Compact(5u32), 0b00011111u8).encode()),
		(bitvec![u8, Lsb0; 1, 1, 1, 1, 1, 0].encode(), (Compact(6u32), 0b00011111u8).encode()),
	];

	for (idx, (actual, expected)) in cases.into_iter().enumerate() {
		assert_eq!(actual, expected, "case at index {idx} failed; encodings differ");
	}
}

seemed to pass equally before and after the change, which gives me some confidence that the SCALE format is identical (of course it could be extended towork with different orderings and store types) :)

[bkchr]

CI is green.

[bkchr]

seemed to pass equally before and after the change, which gives me some confidence that the SCALE format is identical (of course it could be extended towork with different orderings and store types) :)

I added the test. However, it was already ensured that the format didn't changed, because we don't have changed the Decode implementation.

[bkchr]

bot merge