Deserialize boundedbtreeset

[ozgunozerk]

I followed BoundedVec's Deserialize implementation.

Tests are passing 🥳

[KiChjang]

This is weird. You insert before checking for the max, i.e. you add 1 more value over the limit before you check the limit?

[ozgunozerk]

I also found it weird. It was like that in BoundedVec's Deserialize implementation: https://github.com/paritytech/parity-common/blob/master/bounded-collections/src/bounded_vec.rs#L90-L92

I thought that was intentional and didn't put much thought tbh.

I can fix them both if you like?

[KiChjang]

Sure, but there's one more additional issue: checking values.len() > max doesn't sound right to me either, because it too is also waiting for 1 element over the max before the condition triggers. What should really happen is values.len() >= max.

It would be so great if you can write tests for these as well.

[ozgunozerk]

Since we are already discussing the implementation of Deserialize for BoundedVec as well, I had another thing which confused me:

https://github.com/paritytech/parity-common/blob/master/bounded-collections/src/bounded_vec.rs#L84-L85

So, if we are performing a check in here for the size, do we need another check in here:
https://github.com/paritytech/parity-common/blob/master/bounded-collections/src/bounded_vec.rs#L90-L92
?

I think the second check is not necessary and the error produced by the second check is unreachable. So, I don't know if it's possible to write a test for triggering that error. It will trigger the previous error instead.

I might be missing something, so feel free to correct me

[bkchr]

The first check is using size_hint() and that could also be None in which case the size is 0 as you see in the code.

[ozgunozerk]

I've added a new commit:

• insertion is now performed after the bound check
• new tests for bound check:
  • i've copied the deserializer_test and made the size equal to the bound
  • for the tests, size_hint() does indeed return None, so I'm positive I'm testing the inner check
  • if you prefer not to have an additional test, the same test can be done by changing the bound to the size of the deserialized string in the deserializer_test