ethereum / portal-network-specs

Official repository for specifications for the Portal Network
291 stars 80 forks source link

Co-locate ContentKey definitions in the specifications #129

Open pipermerriam opened 2 years ago

pipermerriam commented 2 years ago

What is wrong

Currently, all of the ContentKey definitions are defined within the individual specifications for each of the different portal network.

This presents 3 problems

  1. Inconsistent formatting
  2. Duplicate definitions for any ContentKey used in multiple networks
  3. Duplication of selector values across networks, or inconsistent selector values for any key used across multiple networks.

How to fix

I am going to move all of the ContentKey definitions into a common document and adjusting the specifications to simply reference which content keys are supported within an individual network.

kdeme commented 2 years ago

3. Duplication of selector values across networks, or inconsistent selector values for any key used across multiple networks.

Not sure I understand this one. Is it not fine to have different selector values for a certain type when it is about a different Union definition?

pipermerriam commented 2 years ago

Good point. I see two paths here (assuming we want to do this at all):

A: Separate Union and ContentKey definitions

Define the content keys int content-key-spec.md but define the individual Union definitions in each individual specification. That allows each spec to specify which content key definitions it uses and in which order. This would remove the selector from the content key definition in favor of each specification having some sort of definition like the following:

# content-keys.md
HeaderContentKey = Container[...]
BlockBodyContentKey = Container[...]
ReceiptsBundleContentKey = Container[...]
...

# history-network.md
ContentKeyUnion = Union[HeaderContentKey, BlockBodyContentKey, ReceiptsBundleContentKey]

This retains the benefit of co-locating the definitions while still having each network define a minimal Union type for the key types supported by that network. Under this model, SSZ libraries will "implicitely" reject unsupported key types since they won't be part of the Union definition.

B: Global Union definition with rejection of unsupported key types.

Define a single global Union in content-key-spec.md and require implementations reject unsupported keys.

This seems inferior. It gives us something that looks nice of the surface, a single global Union type which encapsulates every content key supported by any network.... but it also implicitly requires each network to implement logic to catch unsupported key types and reject them.... :vomiting_face:

kdeme commented 2 years ago

Yeah, I'd also opt for A. for the reason you mention. Also sounds better when updating/adding types for a specific network or adding a full new network with its new types.