Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc-schema: make address field in getinfo response required #5904

Merged

Conversation

RCasatta
Copy link
Contributor

@RCasatta RCasatta commented Jan 17, 2023

Since it is always returned even if empty this was false, see #5904 (comment) , there are cases where address field is not returned, but since there are also cases where is returned empty the issue remains.

alternative to #5902
Closes #5902
Changelog-None

@cdecker
Copy link
Member

cdecker commented Jan 17, 2023

Following the discussion on #5902 it feels like this breaks fewer things. Apologies for suggesting #5902 first :-)

@cdecker
Copy link
Member

cdecker commented Jan 17, 2023

To spell out the various combinations:

  • New client, new node: business as usual
  • Old client, old node: business as usual
  • New client, old node: node (cln-grpc with old proto) may skip serializing the empty array, client sees it missing, and initializes it with the default.
  • Old client, new node: node serializes empty array, which would have been omitted before, client deserializes empty array.

The only downside I can see with this is the couple of extra bytes for serializing the empty array, whereas the old code would just omit it. No big deal.

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 8da5c7f

I preferer this, but I do not have the big picture of the problem

@rustyrussell
Copy link
Contributor

But the code shows this is trivially untrue?

	/* Add network info */
	if (cmd->ld->listen) {
		/* These are the addresses we're announcing */
		count_announceable = tal_count(cmd->ld->announceable);
		json_array_start(response, "address");

So, run with --offline, and you'll see no address.

@RCasatta
Copy link
Contributor Author

RCasatta commented Jan 18, 2023

But the code shows this is trivially untrue?

Right, so I updated this to add an empty address array even if --offline is set
and I also updated this MR description

@RCasatta
Copy link
Contributor Author

but I do not have the big picture of the problem

The reason I am doing this is that If the field is optional and I have a getinfo json A with the address field empty,
if I do json A -> grpc -> json B the B json will not match A because it will skip serializing an empty optional field.

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2dc70b5

@rustyrussell
Copy link
Contributor

Trivial rebase for conflict.

Ack 297b5e4

There were cases where address it's empty, and this cases are not right if the
field is considered optional.
This makes it required and add the field also when `--offline` is set.

Changelog-Changed: JSON-RPC: `getinfo` `address` array is always present (though may be empty)
@rustyrussell
Copy link
Contributor

rustyrussell commented Jan 30, 2023

Trivial rebase for man page checksum conflict

Ack 58566cb

@rustyrussell rustyrussell merged commit 18397c0 into ElementsProject:master Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants