feat(containers): add function to decode Vec with given body len

[kskalski]

It's often useful to decode just vector's body with known length (which in hand-tuned serialization formats may be stored in other fields or implied from previously decoded data).

Add function wincode::containers::Vec::<T, SeqLen>::decode_body_with_len::<ConfigCore>(reader, len) that does:

• prealloc check based on SeqLen<ConfigCore>
• allocate vector with enough capacity
• copy into properly sized slice of space capacity
• set len of allocated vec

[kskalski]

maybe a better idea would be to provide Box<[T]> decoding instead - Vec should be trivial to create from box and with Box::<[T]>::new_uninit_slice we get exactly the requested capacity

[davidkleymann]

Shouldn't this use Len::read_prealloc_check::<T::Dst>(reader.by_ref())? ?

[kskalski]

No, read_prealloc_check reads the length from the reader and the point of this API is to use len provided in fn parameter instead of reading it. We still want to validate the length against the limit defined by Len: SeqLen<C>, the call here does exactly (and only) that.

[davidkleymann]

Ah sorry, I got confused I meant Len::prealloc_check::<T::Dst>(reader.by_ref())?

[kskalski]

Oh, it should be Len::prealloc_check::<T::Dst>(len)?, right.

[kskalski]

Calling the function is a bit verbose (anza-xyz/solana-sdk#630), since Vec<T, Len> doesn't include config in its signature and thus it needs to be passed as extra generic argument on the function, even though typically C will be a Config and Len would be taken from C::LengthEncoding.

It feels natural to put such function inside Vec, but possibly it should be free floating or in some extra module.

[cpubot]

I'm a bit hesitant to add this onto the containers::Vec. There is overlap between what this is trying to do and the proposed context system in #220. The Vec implementation in #220 doesn't do a preallocation check, but we could either have a separate context implementation that performs a preallocation check or just leave that up to the user.

My sense is that if someone is opting into this path, it's likely reasonable to expect them to do their own validation / checks. At the callsite, they could use one of the SeqLen implementations we provide, or simply perform their own (or just elide one entirely).

[kskalski]

My point of view is:

• having direct API is just simpler and easier to comprehend and use
• this function alone eliminates all uses of take_scoped from solana-sdk
• as for prealloc check - I can see arguments both ways:
  • as you pointed out earlier lack of pre-alloc check is the one against using helper functions like decode_into_slice_t,
  • but in fact it is easy for user to just add the check manually before decoding - they might have already did the validation of the len, maybe manually or by reading it earlier with LengthEncoding::read_prealloc_check - I'm leaning towards removing this part
  • without necessity to have len, maybe the API can be simplified further, requiring user to specify the len type is a pain
  • however, we could also provide a SeqLen type that skips prealloc check, this way the API would be flexible to support both modes, e.g. Vec::<T, NoAllocCheckLen>::decode_body_with_len::<C>(..)
• the Ctx proposal could be a generalization of this kinds of APIs, simply calling into the relevant direct function - it is a powerful mechanism, but I think it adds a burden of learning how to use it and given we just need to fix 1 single use-case, it feels like using sledgehammer to crack a nut
• the issue of where to place the new function remains - having it in containers::Vec is far from perfect

[kskalski]

Better API for this will probably involve defining some trait, e.g. containers::DecodeSequenceBody that we could implement for various containers.
In a way this would get really close to the Ctx proposal and given that #220 now uses a separate dedicated trait for decoding with context seems better to just go with that.