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

fix(serde): encode optional types as Some #1348

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

shekhirin
Copy link
Contributor

Motivation

There are binary formats such as bincode that care whether an Option is serialized using serialize_some or just serialize, because they prepend the enum variant of Option as a starting byte.

The tests were passing because JSON doesn't care about this.

Solution

Serialize quantity::opt as Some when the value is Some.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@shekhirin shekhirin added the bug Something isn't working label Sep 24, 2024
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

crates/serde/src/quantity.rs Outdated Show resolved Hide resolved
@shekhirin
Copy link
Contributor Author

It made me think, if that's a bug, do we want to have it on other modules too such as

pub mod u8_opt_via_ruint {
use alloy_primitives::U8;
use serde::{Deserialize, Deserializer, Serialize, Serializer};
/// Serializes u64 as hex string
pub fn serialize<S: Serializer>(value: &Option<u8>, s: S) -> Result<S::Ok, S::Error> {
match value {
Some(val) => U8::from(*val).serialize(s),
None => s.serialize_none(),
}
}

@mattsse
Copy link
Member

mattsse commented Sep 24, 2024

nice catch we should actually use the serialize some here, this doesn't matter for json but I guess it does for bincode

@shekhirin shekhirin changed the title fix(serde): encode optional quantity as Some fix(serde): encode optional types as Some Sep 24, 2024
@mattsse mattsse merged commit ec30a65 into alloy-rs:main Sep 24, 2024
26 checks passed
lwedge99 pushed a commit to sentioxyz/alloy that referenced this pull request Oct 8, 2024
* fix(serde): encode optional quantity as Some

* use serialize_some

* fix in other types too
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants