Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tarcieri opened this issue Jan 9, 2024 · 5 comments · Fixed by #1515
Closed

Comments

@tarcieri
Copy link
Member

tarcieri commented Jan 9, 2024

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 #1112 (comment)

@fjarri
Copy link
Contributor

fjarri commented Jan 10, 2024

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
Copy link
Contributor

fjarri commented Sep 10, 2024

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?

@tarcieri
Copy link
Member Author

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()

@fjarri
Copy link
Contributor

fjarri commented Sep 10, 2024

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.

@tarcieri
Copy link
Member Author

tarcieri commented Sep 10, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants