formats icon indicating copy to clipboard operation
formats copied to clipboard

serdect: `slice::deserialize_hex_or_bin` needs to return the amount of deserialized data

Open tarcieri opened this issue 1 year ago • 1 comments

This is tracking a regression from #1112, where slice::deserialize_hex_or_bin was changed to return Result<(), D::Error> instead of Result<&[u8], D::Error>.

Notably this API is intended for use cases where the amount of deserialized data can vary, and may be shorter than the provided buffer parameter. The ability to handle messages shorter than buffer has been lost.

See https://github.com/RustCrypto/formats/pull/1112#discussion_r1446473939

tarcieri avatar Jan 09 '24 18:01 tarcieri

Ah, my bad, didn't realize from the docstring it was a part of the contract, and I don't think there was a test for it either. But the functionality does sound necessary now that I think about it.

fjarri avatar Jan 10 '24 08:01 fjarri

Making a PR to fix this now, but I wonder: if it's the length of the deserialized data we're interested in, should we just return that and not the buffer reference?

fjarri avatar Sep 10 '24 17:09 fjarri

IMO it's more idiomatic in Rust to return the slice whenever possible. You can always get the length if that's all you're interested in by calling len()

tarcieri avatar Sep 10 '24 17:09 tarcieri

I understand that the slice provides the information we want, I was wondering whether we should return extra information (the actual contents of the slice), or try to limit the return value to just the information we want. But I have no strong feelings about it, I'll go with the slice.

fjarri avatar Sep 10 '24 17:09 fjarri

There are many existing usages of the let slice = slice::deserialize_hex_or_bin(...) pattern (see e.g. the ecdsa and rsa crates), so IMO we should definitely keep it. Otherwise it's a breaking change.

tarcieri avatar Sep 10 '24 17:09 tarcieri