Enforce right types for spec-reserved keys

[armaganyildirak]

Issue Addressed

Addresses #26

Proposed Changes

Checking key and value according to spec-reserved keys.

Additional Info

I have not finished the PR yet. I am still not sure about that I am following the correct idea.

[divagant-martian]

Hi @armaganyildirak just checked -after my review- that this PR is not finished. Two things that you can do to signal this in a more clear way (one, the other or both):

• Add "WIP" (as in work in progress) in the title as follows: [WIP] Enforce right types for spec-reserved keys. Remove the tag when you are happy with the code.
• Open the PR as a draft: I'll convert as a such shortly.

If you want a partial review, while still in progress, keep it as a draft/tagged as WIP and re-request or tag me/someone who you think could help, like age. Mark as ready once you are satisfied with it and re-request reviews.

Regarding the code itself, to begin with, you can simply follow the same path taken for the u16 keys. The logic is:

• Check if the key belongs to the reserved ones via comparison (as you are currently doing) and then attempt to decode as the expected type. For the u16 keys right now it looks like:

      if is_keyof_u16(key.as_ref()) {
          rlp::decode::<u16>(&value).map_err(|err| EnrError::InvalidRlpData(err.to_string()))?;
      }

  so you would, for and ipv6 key for example, do something like:

      if <here goes the check about the key being ipv6> {
          rlp::decode::<IPv6Addr>(&value).map_err(|err| EnrError::InvalidRlpData(err.to_string()))?;
      }

After this the code can probable be improved further, but this is a good first step

[divagant-martian]

It's going to take some iterations before we get this right @armaganyildirak but don't get discouraged.

Right now, you have changed most (if not all) the functions of the builder to return a Result. This is not very ergonomic. It would be better to accept the values in the builder in those functions and reject them if wrong in the build one.

We would get things like this:

assert!(EnrBuilder::new("v4").add_value("ip6", "most definitely not an ipv6! :O").build(&key).is_err());

Now onto the build function itself: the way it works right now is not ideal. As you've noticed, you had to add the verification in two places, in the enr and the builder as well. We should have it only in the enr and rely on that exclusively, included the builder function. For this part, I probably have to think a bit more how to do it correctly, hence the "multiple interations" ahead.

In the meantime, can you restore the functions that were not Result to how they were and move the validation to the build one?

Also, it would be better to add a match statement instead of multiple == comparisons for the reserved keys.

About clippy, don't bother dealing with errors that do not relate to your code, I added the fixes for those in #42

Thanks for hanging in there