Conversation
| #![no_std] | ||
| #![cfg_attr(docsrs, feature(doc_cfg))] | ||
|
|
||
| #[cfg(feature = "alloc")] |
There was a problem hiding this comment.
Maybe we should depend on alloc by default so we can avoid having so many #[cfg(feature = "alloc")]. Currently both types require alloc.
There was a problem hiding this comment.
That's fine by me, we could even call the crate spl-alloc if we want to try something else 😅
f48ca8f to
693a279
Compare
693a279 to
83a6398
Compare
|
The |
joncinque
left a comment
There was a problem hiding this comment.
Looks great overall! Mostly small points
| #![no_std] | ||
| #![cfg_attr(docsrs, feature(doc_cfg))] | ||
|
|
||
| #[cfg(feature = "alloc")] |
There was a problem hiding this comment.
That's fine by me, we could even call the crate spl-alloc if we want to try something else 😅
| [workspace] | ||
| resolver = "2" | ||
| members = [ | ||
| "collections", |
There was a problem hiding this comment.
To go with these changes, can you include the new package at
libraries/.github/workflows/main.yml
Lines 11 to 13 in ed9d43b
| while let Ok(item) = T::deserialize_reader(reader) { | ||
| items.push(item); | ||
| } |
There was a problem hiding this comment.
Shouldn't this only read prefix items and not all of them?
|
|
||
| #[inline(always)] | ||
| fn size_of(src: &Self::Src) -> WriteResult<usize> { | ||
| Ok(core::mem::size_of::<$prefix_type>() + src.0.len()) |
There was a problem hiding this comment.
Shouldn't this multiply the length by the size of T?
| Ok(core::mem::size_of::<$prefix_type>() + src.0.len()) | |
| Ok(core::mem::size_of::<$prefix_type>() + src.0.len() * core::mem::size_of::<T>()) |
|
|
||
| while let Ok(item) = T::get(&mut reader) { | ||
| items.push(item); | ||
| } |
There was a problem hiding this comment.
Same with this, shouldn't it only read prefix items?
There was a problem hiding this comment.
Considering String just wraps Vec<u8> under the hood, maybe TrailingString can just wrap TrailingVec<u8> and PrefixedString can just wrap PrefixedVec<u8>, and we can remove a lot of this code, except for a converter to a String and a &str. But if it's too complicated, don't worry about it, this code looks fine to me. What do you think?
| s.push_str( | ||
| core::str::from_utf8(&buffer[..bytes_read]).map_err(|_| ErrorKind::InvalidData)?, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Is this approach fallible? If the reader happens to fill half-way through a multi-byte character, won't this fail? Or is that not possible because BUFFER_SIZE is a nice power of 2?
Problem
Client generation need custom types to support
StringandVecto have different prefix length types – e.g., aStringwith au8length prefix or aVec<Address>without a prefix that consumes the remaining available bytes.Solution
Add a new crate to include custom
StringandVectypes with borsh and wincode serialization support.