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

RFC16: add version call #505

Merged
merged 3 commits into from
Jun 22, 2022
Merged

RFC16: add version call #505

merged 3 commits into from
Jun 22, 2022

Conversation

kaiserd
Copy link
Contributor

@kaiserd kaiserd commented May 16, 2022

Specifies a debug RPC call returning a Waku node's version. This call is implemented in nwaku accordingly.

@kaiserd kaiserd requested review from oskarth and jm-clius May 16, 2022 15:55
@oskarth oskarth added this to New in vac-research May 16, 2022
@oskarth oskarth moved this from New to Review/QA in vac-research May 16, 2022
### `get_waku_v2_debug_v1_version`

The `get_waku_v2_debug_v1_version` method retrieves the version of a Waku v2 node as a string.
The version SHOULD follow [semantic versioning](https://semver.org/).
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to specify the format of the response for the scenarios described here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 104 to 105
#### Response
- [**`WakuInfo`**](#wakuinfo) - information about a Waku v2 node
Copy link
Contributor

Choose a reason for hiding this comment

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

This section still belongs with the previous entry get_waku_v2_debug_v1_info

Could also add a simple Response subsection for this call, indicating a String response containing version / commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes :). Both done.


none


#### Response
- [**`WakuInfo`**](#wakuinfo) - information about a Waku v2 node
Copy link
Member

Choose a reason for hiding this comment

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

WakuInfo seems to be missing the attribute containing the version information

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The version is a separate string not contained in WakuInfo. I accidentally split the section as @jm-clius said.
Now it should be clear 😅

@oskarth oskarth removed their request for review May 18, 2022 11:07
@D4nte
Copy link
Contributor

D4nte commented May 23, 2022

LGTM, also agree with comments.

@kaiserd
Copy link
Contributor Author

kaiserd commented Jun 15, 2022

Feedback applied. Sorry for the delayed action.

@@ -81,6 +81,10 @@ The following structured types are defined for use on the Debug API:
| `listenAddresses` | `Array`[`String`] | mandatory | Listening addresses of the node |
| `enrUri` | `String` | optional | ENR URI of the node |

#### WakuInfo

`Version`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that a typo? I am not sure to understand why this text is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes 😅 . Removed in 1675b76

@kaiserd kaiserd requested a review from D4nte June 22, 2022 13:01
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

LGTM!

@kaiserd kaiserd merged commit d942b3b into master Jun 22, 2022
vac-research automation moved this from Review/QA to Done Jun 22, 2022
@kaiserd kaiserd deleted the 16/add-version-call branch June 22, 2022 15:40
kaiserd added a commit that referenced this pull request Jul 16, 2022
* master:
  RFC16: add version call (#505)
  fix(noise): update RFC to implementation (#508)
  fixup: 37/WAKU2-NOISE fix images paths (#506)
  New RFC: 37/WAKU2-NOISE-SESSIONS (#504)
  36/WAKU2-BINDINGS-API (#501)
  docs(16/WAKU2-RPC): add ENR to waku info (#502)
  Adding 35/WAKU2-NOISE to menu (#500)
  add RFC33 to index (#499)
  feat: 32/RLN raw spec
  New RFC: 35/WAKU2-NOISE (#496)
  Update on the rln registration figure to match the current spec (#497)
  33/WAKU-DISCV5: Add first raw version (#487)
  Add pubsubTopic field to index (#492)
  Fix markdown links (#493)
  Categorize 22 & 31 (#490)
  Changed PB Timestamp index to 10 (#491)
  13/14/16/21: Change in timestamp format (#483)
  add: RFC31 copyright statement (#489)
  17/WAKU-RLN-RELAY: Revise spec for its draft version (#484)
kaiserd added a commit that referenced this pull request Jul 25, 2022
* master:
  RFC16: add version call (#505)
  fix(noise): update RFC to implementation (#508)
  fixup: 37/WAKU2-NOISE fix images paths (#506)
  New RFC: 37/WAKU2-NOISE-SESSIONS (#504)
  36/WAKU2-BINDINGS-API (#501)
  docs(16/WAKU2-RPC): add ENR to waku info (#502)
  Adding 35/WAKU2-NOISE to menu (#500)
  add RFC33 to index (#499)
  feat: 32/RLN raw spec
  New RFC: 35/WAKU2-NOISE (#496)
  Update on the rln registration figure to match the current spec (#497)
  33/WAKU-DISCV5: Add first raw version (#487)
  Add pubsubTopic field to index (#492)
  Fix markdown links (#493)
  Categorize 22 & 31 (#490)
  Changed PB Timestamp index to 10 (#491)
  13/14/16/21: Change in timestamp format (#483)
  add: RFC31 copyright statement (#489)
  17/WAKU-RLN-RELAY: Revise spec for its draft version (#484)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

4 participants