NodeID is not emitted in ValidatorManager Events

[0xJohnnyGault]

In ValidatorManager.sol, NodeID is defined as bytes (a dynamically-sized type). I understand that this is probably done for future-proofing. However, when events are emitted like:

  event ValidationPeriodCreated(
    bytes32 indexed validationID,
    bytes indexed nodeID,
    bytes32 indexed registerValidationMessageID,
    uint256 weight,
    uint64 registrationExpiry
  );

Its misleading, because the actual NodeID is not emitted, as Solidity will keccak the bytes to get a 32-byte hash to put in the event. So at a minimum I would suggest renaming it to NodeIDHash which would make it clear its not the actual nodeid.

Alternatively, I think if indexed is removed, the full value would be placed in the event log.

[geoff-vball]

The relevant discussion for this is here.

I definitely see how this isn't super user-friendly. @cam-schultz would it make sense to use a bytes32 for all sizes? I think the bytes20 should just get padded?

[cam-schultz]

I still think that bytes is the most future-proofed option, as it's the most in line with the ACP-77 spec. Removing indexed (and indexing one of the other fixed size parameters instead) sounds like a reasonable workaround to me. The other suggestion to rename to NodeIDHash would clarify the output value, but wouldn't do much to make the event more useful.

[0xJohnnyGault]

Having the actual nodeID available in the log would be very useful, so I would vote for removing the indexed attr.