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

feat: serde helpers for hashmaps and btreemaps with quantity key types #1579

Merged
merged 6 commits into from
Nov 2, 2024

Conversation

stevencartavia
Copy link
Contributor

@stevencartavia stevencartavia commented Oct 29, 2024

Resolves #1502

Motivation

Existing mod vec implementation

Solution

Created the same for hashmaps/btreemaps with quantity key types and tests.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@stevencartavia stevencartavia changed the title feat: serde helpers for hashmaps and btreemaps w/ quantity key types feat: serde helpers for hashmaps and btreemaps with quantity key types Oct 29, 2024
Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

some comment nits,

this is also not quite right: we are only supposed to encode the keys as quantity, the value can be anything

@@ -164,6 +164,133 @@ pub mod u128_vec_vec_opt {
}
}

/// Serde functions for encoding a hashmap of primitive numbers using the Ethereum "quantity"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Serde functions for encoding a hashmap of primitive numbers using the Ethereum "quantity"
/// Serde functions for encoding a `HashMap` of primitive numbers using the Ethereum "quantity"

use serde::{de::MapAccess, ser::SerializeMap, Deserializer, Serializer};
use std::collections::HashMap;

/// Serializes a hashmap of primitive numbers as a "quantity" hex string.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Serializes a hashmap of primitive numbers as a "quantity" hex string.
/// Serializes a `HashMap` of primitive numbers as a "quantity" hex string.

map_ser.end()
}

/// Deserializes a hashmap of primitive numbers from a "quantity" hex string.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Deserializes a hashmap of primitive numbers from a "quantity" hex string.
/// Deserializes a `HashMap` of primitive numbers from a "quantity" hex string.

}
}

/// Serde functions for encoding a BTreeMap of primitive numbers using the Ethereum "quantity"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Serde functions for encoding a BTreeMap of primitive numbers using the Ethereum "quantity"
/// Serde functions for encoding a `BTreeMap` of primitive numbers using the Ethereum "quantity"

where
A: MapAccess<'de>,
{
let mut values = HashMap::new();
Copy link
Member

Choose a reason for hiding this comment

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

this can pre allocate with the map's size hint

use std::collections::HashMap;

/// Serializes a hashmap of primitive numbers as a "quantity" hex string.
pub fn serialize<K, V, S>(map: &HashMap<K, V>, serializer: S) -> Result<S::Ok, S::Error>
Copy link
Member

Choose a reason for hiding this comment

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

}

/// Deserializes a hashmap of primitive numbers from a "quantity" hex string.
pub fn deserialize<'de, K, V, D>(deserializer: D) -> Result<HashMap<K, V>, D::Error>
Copy link
Member

Choose a reason for hiding this comment

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

this should also relax the hasher

@stevencartavia
Copy link
Contributor Author

@onbjerg @mattsse, done with the requested changes! :)

@mattsse mattsse merged commit 1bfa38c into alloy-rs:main Nov 2, 2024
26 checks passed
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 this pull request may close these issues.

[Feature] Add serde helpers for handling maps with quantity keys
3 participants