serde(with = "SerHex::<StrictPfx>") NodeId

[KolbyML]

Without adding this if you tried to

        let mut responses: HashMap<NodeId, u16> = Default::default();
        responses.insert(discv5::enr::NodeId::random(), 1);
        let hi = json!(responses);

json!(...) will fail with "key must be a string"

[KolbyML]

@divagant-martian @AgeManning ping for review

[divagant-martian]

I see a couple issues with this suggestion, and with the PR itself

Regarding the PR, please check this compiles first. Right now it doesn't.

Regarding the suggestion itself, NodeId derives Serialize and Deserialize already, and there is a explicit test for json serialization. From what I can tell, you want to serialize the RawNodeId as a hex string unconditionally. Although this is a breaking change, it's one I find reasonable. To be clear, this would make it so that anyone using the NodeId serialization with a different serializer to the one you propose would now get a different output without option to configure it, so we need to decide if this is acceptable. I think it is

Now, even if we do this change, as it is right now, I suspect this won't solve your issue. Changing the serialization of the inner type won't make the outer type a string (which is the error you want to solve) it will still be serialized as a json object. That being said, if you want to get this change working so that we can discuss accepting the breaking change, you probably want/need to:

• leverage serde(transparent)
• a test

[KolbyML]

I see a couple issues with this suggestion, and with the PR itself

Regarding the PR, please check this compiles first. Right now it doesn't.

Regarding the suggestion itself, NodeId derives Serialize and Deserialize already, and there is a explicit test for json serialization. From what I can tell, you want to serialize the RawNodeId as a hex string unconditionally. Although this is a breaking change, it's one I find reasonable. To be clear, this would make it so that anyone using the NodeId serialization with a different serializer to the one you propose would now get a different output without option to configure it, so we need to decide if this is acceptable. I think it is

Now, even if we do this change, as it is right now, I suspect this won't solve your issue. Changing the serialization of the inner type won't make the outer type a string (which is the error you want to solve) it will still be serialized as a json object. That being said, if you want to get this change working so that we can discuss accepting the breaking change, you probably want/need to:

• leverage serde(transparent)
• a test

Ok I did some testing https://github.com/sigp/enr/pull/35

#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[derive(Clone, Copy, PartialEq, Eq, Hash)]
/// The `NodeId` of an ENR (a 32 byte identifier).
pub struct NodeId {
    raw: RawNodeId,
}

#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[derive(Clone, Copy, PartialEq, Eq, Hash)]
/// The `NodeIdSerde` of an ENR (a 32 byte identifier).
pub struct NodeIdSerde(RawNodeIdSerde);

#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[derive(Clone, Copy, PartialEq, Eq, Hash)]
/// The `NodeIdSerde3` of an ENR (a 32 byte identifier).
pub struct NodeIdSerde3 {
    #[serde(with = "SerHex::<StrictPfx>")] raw: RawNodeIdSerde3,
}

The three above all fail

but

#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
#[derive(Clone, Copy, PartialEq, Eq, Hash)]
/// The `NodeIdSerde2` of an ENR (a 32 byte identifier).
pub struct NodeIdSerde2(#[serde(with = "SerHex::<StrictPfx>")] RawNodeIdSerde2);

^ this will pass

When I get some time I will try and debug this and see what the root of the issue is. Thank you for your time and pointing a few things out to me :+1:

[divagant-martian]

To give you a hand here, this code works

diff --git a/Cargo.toml b/Cargo.toml
index 2ac529d..8f20730 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -14,7 +14,7 @@ exclude = [".gitignore", ".github/*"]
 [dependencies]
 base64 = "0.21.0"
 bytes = "1"
-hex = "0.4.2"
+hex = {version = "0.4.2", features = ["serde"]}
 log = "0.4.8"
 rand = "0.8"
 rlp = "0.5"
diff --git a/src/node_id.rs b/src/node_id.rs
index 6713b7f..4970b82 100644
--- a/src/node_id.rs
+++ b/src/node_id.rs
@@ -10,8 +10,10 @@ type RawNodeId = [u8; 32];
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 #[derive(Clone, Copy, PartialEq, Eq, Hash)]
 /// The `NodeId` of an ENR (a 32 byte identifier).
+#[serde(transparent)]
 pub struct NodeId {
-    #[cfg_attr(feature = "serde", serde(with = "SerHex::<StrictPfx>"))] raw: RawNodeId,
+    #[cfg_attr(feature = "serde", serde(with = "hex::serde"))]
+    raw: RawNodeId,
 }

 impl NodeId {

but it needs a bit more love

[KolbyML]

@divagant-martian thank you so much for the help :D. I added a testcase to this PR and it works!!!! I am not exactly sure what you meant by a bit more love, but I one lined the cfg_attr

Let me know if anything else is needed :)

Thank you very much for all the help!

[divagant-martian]

So this uses the hex::serde turning on the serde feature, a couple of points to note on this

• feature should be turned on conditionally, not all the time
• this does not do hex encoding prefix, we need to decide whether we want it to be 0x.. or not
• as my suggestion was, it was missing a test

That's the love it was (still partially) missing

[KolbyML]

So this uses the hex::serde turning on the serde feature, a couple of points to note on this

• feature should be turned on conditionally, not all the time

I think it is like that right now, but I could be wrong. Would I also need to add a cfg in the Config.toml for features=serde?

• this does not do hex encoding prefix, we need to decide whether we want it to be 0x.. or not

0x since it is a universal identifier, but I am not picky as long as NodeId can be used as a hashmap key :)

• as my suggestion was, it was missing a test

I just added a test, let me know if you would want another :+1:

That's the love it was (still partially) missing

Ah :+1:

[AgeManning]

Clippy seems to be failing at the moment.

[divagant-martian]

it's unrelated, fix here https://github.com/sigp/enr/pull/36

[KolbyML]

it's unrelated, fix here #36

it is unrelated to this PR, but I think we need to add all-targets or something to the clippy command since clippy locally is complaining about a lot of other things not being picked up by CI, which makes it unusable locally for me on this project. Could be a issue on my end don't know

[AgeManning]

Its probably a good idea to add all-targets to the CI. I'll add this in another PR to make sure we catch them in CI.

If you can merge in the latest master, CI should hopefully run fine now.

[KolbyML]

rebased

[AgeManning]

On second thought, the all-targets just makes clippy run over the tests. Not sure if we are concerned about whether tests are formatted with clippy lints or not.

Others have an opinion here? @divagant-martian ?

[KolbyML]

On second thought, the all-targets just makes clippy run over the tests. Not sure if we are concerned about whether tests are formatted with clippy lints or not.

Others have an opinion here? @divagant-martian ?

Yeah I am not sure what would resolve it, but I will send a snippet of what mine prints

error: unneeded sub `cfg` when there is only one condition
  --> src/keys/mod.rs:12:7
   |
12 | #[cfg(any(feature = "k256"))]
   |       ^^^^^^^^^^^^^^^^^^^^^ help: try: `feature = "k256"`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_minimal_cfg

error: unneeded sub `cfg` when there is only one condition
  --> src/keys/mod.rs:21:7
   |
21 | #[cfg(any(feature = "k256"))]
   |       ^^^^^^^^^^^^^^^^^^^^^ help: try: `feature = "k256"`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_minimal_cfg

This is using cargo clippy --all-features -- -D warnings, it isn't a big problem just makes it kind of annoying what to focus on.

[AgeManning]

What version of rust are you using. You might need to do a rustup update.

There are no errors locally for me.

[KolbyML]

rustc 1.72.0-nightly (e6d4725c7 2023-06-05)

Oh maybe this is why xD.

[KolbyML]

Thank you for the help Age :+1: