Add syncing_status query parameter to health API

[nflaig]

Problem description

Latest beacon API spec requires syncing_status query parameter

• Add syncing_status query parameter to GetHealth ethereum/beacon-APIs#251

Solution description

Add syncing_status query parameter to health API

• https://ethereum.github.io/beacon-APIs/#/Node/getHealth

Additional context

No response

[allwin199]

I would like to work on this if it is still open.

[allwin199]

Can you give more info about this issue?

[nflaig]

You can find more information in the spec PR ethereum/beacon-APIs#251 and this issue ethereum/beacon-APIs#147.

This requires to update the api definition here

lodestar/packages/api/src/beacon/routes/node.ts

Line 141 in cdca9fb

getHealth: {url: "/eth/v1/node/health", method: "GET"},

and further changes on beacon node side

lodestar/packages/beacon-node/src/api/impl/node/index.ts

Line 69 in cdca9fb

async getHealth(_req, res) {

lodestar/packages/api/src/beacon/server/node.ts

Line 20 in cdca9fb

handler: async (req, res) => {

Best to check other routes that use query parameters and see how it is implemented there.

You can find more about the reasoning how the API routes are defined here

lodestar/packages/api/src/beacon/routes/index.ts

Lines 33 to 35 in cdca9fb

	// Reasoning of the API definitions
	// ================================
	//

The validator client won't use this feature, so no changes required there.

[allwin199]

Thanks for the explanation! Will work on it.

[allwin199]

I tried fixing this issue, but I am unable to fix it.

If someone fixes this issue, I will learn from it.

Thanks for the opportunity.

[nflaig]

I tried fixing this issue, but I am unable to fix it.

All good, thanks for giving it a try.

It would be great if you could provide some feedback

• what were the obstacles you faced when trying to solve this?
• and is there anything we could do to make it easier to contribute?

[allwin199]

To fix the issue, I changed below lines

export type ReqTypes = {
   getHealth: {query: {syncing_status?: boolean}};
}

export function getReqSerializers = {
  getHealth: {
      writeReq: (syncing_status) => ({query: syncing_status || {}}),
      parseReq: ({query}) => [query],
      schema: {query: {syncing_status: Schema.Boolean}},
    },
}

Is the flow correct? I have also added type boolean in Schema.ts

If the flow is correct,
In the getReqSerializers(), I don't understand this parseReq part.
This is where I am stuck.

[nflaig]

Is the flow correct? I have also added type boolean in Schema.ts

First you need to update the types, this makes it easier to update all the other parts. syncing_status is an optional integer value not a boolean so the schema needs to be Schema.Uint.

In the getReqSerializers(), I don't understand this parseReq part.

That just parses the incoming request to the right format expected in the code.

lodestar/packages/api/src/beacon/routes/node.ts

Lines 178 to 182 in 1bc571b

	getHealth: {
	writeReq: (options) => ({query: {syncing_status: options?.syncingStatus}}),
	parseReq: ({query}) => [{syncingStatus: query.syncing_status}],
	schema: {query: {syncing_status: Schema.Uint}},
	},

I found an issue with the current implementation, it always returned 200 even if node is syncing, this made the task also not that great for someone new to codebase.

I opened a PR #5790 to implement the new query parameter and fix the issue. @worksofallwin Feel free to give this a review and ask questions if something is unclear.

[allwin199]

@nflaig Thanks for the reply. I will go through and I will post If I don't understand anything.