ethereum / beacon-APIs

Collection of RESTful APIs provided by Ethereum Beacon nodes
https://ethereum.github.io/beacon-APIs/
Creative Commons Zero v1.0 Universal
335 stars 168 forks source link

Reason of allOf usage? #233

Closed dapplion closed 2 years ago

dapplion commented 2 years ago

Some properties are defined with allOf notation

https://github.com/ethereum/beacon-APIs/blob/9cab46ad3c94a4a2779b42fa21f6bb1955b60b56/types/primitive.yaml#L40-L44

instead of just

ExecutionOptimistic: 
   type: boolean 
   example: false 
   description: "True if the response references an unverified execution payload. Optimistic information may be invalidated at a later time. If the field is not present, assume the False value." 

The former renders ugly in the final JSON making it harder for tooling to process. Any reason for this? Asking before I do a PR fixing it

                  "title": "GetBlockHeadersResponse",
                  "type": "object",
                  "properties": {
                    "execution_optimistic": {
                      "allOf": [
                        {
                          "type": "boolean"
                        },
                        {
                          "example": false
                        },
                        {
                          "description": "True if the response references an unverified execution payload. Optimistic information may be invalidated at a later time. If the field is not present, assume the False value."
                        }
                      ]
                    },
mpetrunic commented 2 years ago

It is mostly used in objects that are using "$ref" because "allOf" allows to override certain referenced properties. ExecutionOptimistic is probably just copy pasta mistake

dapplion commented 2 years ago

It is mostly used in objects that are using "$ref" because "allOf" allows to override certain referenced properties. ExecutionOptimistic is probably just copy pasta mistake

At least in AJV it's creating invalid schema, for example here

          {
            "name": "status",
            "description": "[Validator status specification](https://hackmd.io/ofFJ5gOmQpu1jjHilHbdQQ)",
            "in": "query",
            "required": false,
            "schema": {
              "type": "array",
              "uniqueItems": true,
              "items": {
                "allOf": [
                  {
                    "description": "Possible statuses:\n- **pending_initialized** - When the first deposit is processed, but not enough funds are available (or not yet the end of the first epoch) to get validator into the activation queue.\n- **pending_queued** - When validator is waiting to get activated, and have enough funds etc. while in the queue, validator activation epoch keeps changing until it gets to the front and make it through (finalization is a requirement here too).\n- **active_ongoing** - When validator must be attesting, and have not initiated any exit.\n- **active_exiting** - When validator is still active, but filed a voluntary request to exit.\n- **active_slashed** - When validator is still active, but have a slashed status and is scheduled to exit.\n- **exited_unslashed** - When validator has reached reguler exit epoch, not being slashed, and doesn't have to attest any more, but cannot withdraw yet.\n- **exited_slashed** - When validator has reached reguler exit epoch, but was slashed, have to wait for a longer withdrawal period.\n- **withdrawal_possible** - After validator has exited, a while later is permitted to move funds, and is truly out of the system.\n- **withdrawal_done** - (not possible in phase0, except slashing full balance) - actually having moved funds away\n\n[Validator status specification](https://hackmd.io/ofFJ5gOmQpu1jjHilHbdQQ)\n",
                    "enum": [
                      "pending_initialized",
                      "pending_queued",
                      "active_ongoing",
                      "active_exiting",
                      "active_slashed",
                      "exited_unslashed",
                      "exited_slashed",
                      "withdrawal_possible",
                      "withdrawal_done"
                    ],
                    "example": "active_ongoing"
                  },
                  {
                    "enum": ["active", "pending", "exited", "withdrawal"]
                  }
                ]
              }
            }
          }

It's impossible to satisfy this allOf because the list of enum does not intersect

dapplion commented 2 years ago

Also, why need to override the example here?

https://github.com/ethereum/beacon-APIs/blob/9cab46ad3c94a4a2779b42fa21f6bb1955b60b56/apis/beacon/states/committee.yaml#L16-L19

The example is useful

https://github.com/ethereum/beacon-APIs/blob/9cab46ad3c94a4a2779b42fa21f6bb1955b60b56/types/primitive.yaml#L27-L29

rolfyone commented 2 years ago

anywhere we're using allOf to just remove the example or something like that is probably not needed...

If we want to use primitives though, the allOf enables that...

Your example of the enum being broken is a point at which we should probably not be using the primitive that's in use :) It looks like we should define a different primitive there..

rolfyone commented 2 years ago

from discussions at devcon, this issue can be closed?