Update Buffer ser/de variant indices

[mickvangelderen]

No description provided.

[andreisilviudragnea]

There needs to be a unit test which verifies if the serde serialization code before and after #259 depends on the values of these numbers or not.

[mickvangelderen]

There needs to be a unit test which verifies if the serde serialization code before and after #259 depends on the values of these numbers or not.

@andreisilviudragnea Do you mean if the serialized value depends on the variant indices? It probably does, given that you need to supply them as well in the deserialization implementation.

@anton-lisanin mentioned that we are free to change the serialized format. It does not need to be backwards compatible.

I do think it would be a good idea to assert that serializing and deserializing gives back something equivalent to the original, if we don't already have those tests.

[andreisilviudragnea]

I know from previous experience that changing serialization code without unit tests is dangerous.

The concrete serialization framework which should be tested here is bincode.

[mickvangelderen]

I know from previous experience that changing serialization code without unit tests is dangerous.

That goes for pretty much any code 🤡 .

So to your knowledge there are no unit tests for this. I suppose our integration tests should catch problems with the serialization but having it unit tested would give a better guarantee and earlier feedback.

[mickvangelderen]

@andreisilviudragnea FYI added tests.

[anton-lisanin]

@anton-lisanin mentioned that we are free to change the serialized format. It does not need to be backwards compatible.

We need to keep backward compatibility for deserialization.
It's ok to change serialization format, but deserialization function should handle both old and new formats.

[anton-lisanin]

Rejected.
With this code, it is not possible to deserialize from the buffer that was serialized in the previous version.

[mickvangelderen]

We need to keep backward compatibility for deserialization. It's ok to change serialization format, but deserialization function should handle both old and new formats.

Ah I misunderstood you then. We should have tests to assert we can deserialize the old format in that case. This also means that develop is currently broken since we error on unknown variants including the previously valid 0 variant (buffer::Inner::Empty).

[mickvangelderen]

This PR was made based on the assumption that we can make backwards incompatible changes to the serialized format which is not the case. I've opened #263 to implement unit tests which guarantee that the serialized data looks a certain way, and that serialized data from previous and the current version can be successfully deserialized.