sdk: Add try_from_slice_unchecked for Borsh #16098
Conversation
| @@ -54,3 +60,10 @@ pub fn get_packed_len<S: BorshSchema>() -> usize { | |||
| let schema_container = S::schema_container(); | |||
| get_declaration_packed_len(&schema_container.declaration, &schema_container.definitions) | |||
| } | |||
|
|
|||
| /// Deserializes without checking that the entire slice has been consumed | |||
| pub fn try_from_slice_unchecked<T: BorshDeserialize>(data: &[u8]) -> Result<T, Error> { | |||
There was a problem hiding this comment.
How about a little test for this guy?
There was a problem hiding this comment.
Unchecked sounds potentially dangerous, should folks be cautious using this function and if so what should they pay attention to?
There was a problem hiding this comment.
Hm, that's a good point. The main difference is that this function eliminates a check that the slice has been totally read, so it lets you work with overallocated buffers. The normal try_from_slice returns an error if there's data left in the buffer after deserialization. How could we clarify that?
There was a problem hiding this comment.
Is there a downside to always using the "unchecked" version?
There was a problem hiding this comment.
The length check ensures that whatever you're expecting to deserialize uses up all of the bytes in the buffer, so you could potentially run into an issue if you always use unchecked. Any buffer that's big enough could work to deserialize your type, so if you pass the wrong buffer with unchecked, the error wouldn't get caught.
There was a problem hiding this comment.
Any buffer greater than or equal to the expected size would work, correct? Maybe call it "unsized", "unchecked" is fine but should probably include something to effect of the description you have above as what the "unchecked" means practically to the developer
There was a problem hiding this comment.
Correct -- I'll rename the function and add the description then
There was a problem hiding this comment.
With the good description you have provided I think "unchecked" makes sense, I can go either way but "unchecked" is more consistent with rust norms
There was a problem hiding this comment.
It's also consistent with the PR that I put in for Borsh JS :-D
Codecov Report
@@ Coverage Diff @@
## master #16098 +/- ##
=======================================
Coverage 80.0% 80.1%
=======================================
Files 410 410
Lines 109161 109212 +51
=======================================
+ Hits 87422 87508 +86
+ Misses 21739 21704 -35 |
* sdk: Add try_from_slice_unchecked for Borsh * Add tests * Rename + clarify comment * Rename back to unchecked (cherry picked from commit cffa851)
Problem
If we're using variable-sized structures on-chain, for example
Vec<>, and we allocate 1000 bytes and don't immediately use all of them, Borsh fails deserialization because it doesn't read the entire slice.Summary of Changes
Add a helper function. This is a copy of https://github.com/near/borsh-rs/blob/3223286bae05d29162eaf37512966b9fa7d9cfdb/borsh/src/de/mod.rs#L30 which just removes the
is_empty()check.Fixes #