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.

Sign up for GitHub

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

Jump to bottom

36/WAKU2-BINDINGS-API #501

Merged
merged 21 commits into from
May 17, 2022
Merged

36/WAKU2-BINDINGS-API #501

merged 21 commits into from
May 17, 2022

Conversation

D4nte
Copy link
Contributor

@D4nte D4nte commented Mar 22, 2022 •
edited

Helps with waku-org/nwaku#904

TODO:

  • Tidy up the subscription API (subscription id was removed)
  • Clarify JsonMessage type with new publish/publish enc APIs

Will do light push, store, filter in a separate PR. Let's get this PR merged with relay once todo items above are done.

@oskarth oskarth added this to New in vac-research Mar 22, 2022
@oskarth oskarth moved this from New to Review/QA in vac-research Mar 22, 2022
D4nte
D4nte commented Mar 22, 2022
View reviewed changes
content/docs/rfcs/36/README.md Outdated Show resolved Hide resolved
D4nte
D4nte commented Mar 22, 2022
View reviewed changes
content/docs/rfcs/36/README.md Outdated Show resolved Hide resolved
D4nte
D4nte commented Mar 23, 2022
View reviewed changes
content/docs/rfcs/36/README.md Outdated Show resolved Hide resolved
D4nte
D4nte commented Mar 23, 2022
View reviewed changes
content/docs/rfcs/36/README.md Outdated Show resolved Hide resolved
D4nte
D4nte commented Mar 23, 2022
View reviewed changes
content/docs/rfcs/36/README.md Outdated Show resolved Hide resolved
@D4nte
Copy link
Contributor Author

D4nte commented Mar 23, 2022

Is the subscription id a number or string? I saw both in the spec I think.

Changes I have done:

  • Define types JsonMessage, etc
  • Use proper camelCase: Id
  • rename topic to pubsubTopic to avoid possible confusion.
  • Examples for all Returns
  • Remove go in function name.
  • Use title anchors to cross-reference methods
  • Rename waku_encode_data to waku_encode_payload.

@richard-ramos
Copy link
Member

Is the subscription id a number or string?

It's a UUID. Should we use an int instead?

@D4nte
Copy link
Contributor Author

D4nte commented Mar 24, 2022

Comments looks good. Will action changes tomorrow.

@jm-clius
Copy link
Contributor

jm-clius commented Apr 4, 2022

Note that /35 is already taken: https://rfc.vac.dev/spec/35/

@oskarth
Copy link
Contributor

oskarth commented Apr 6, 2022

What's the state of this?

D4nte
D4nte commented Apr 13, 2022
View reviewed changes
content/docs/rfcs/36/README.md Outdated Show resolved Hide resolved
D4nte
D4nte commented Apr 13, 2022
View reviewed changes
content/docs/rfcs/36/README.md Outdated Show resolved Hide resolved
D4nte
D4nte commented Apr 13, 2022
View reviewed changes
content/docs/rfcs/36/README.md Outdated Show resolved Hide resolved
@D4nte D4nte self-assigned this Apr 13, 2022
@D4nte
Copy link
Contributor Author

D4nte commented Apr 13, 2022

What's the state of this?

Updated. Some comments are unresolved and need further discussion.

@D4nte D4nte marked this pull request as ready for review April 13, 2022 12:19
@richard-ramos
Copy link
Member

Current state of c-bindings in go-waku can be seen here: https://gist.github.com/richard-ramos/abb1540f7fa1b5b7fb21d28a5c00e183

@D4nte D4nte changed the title 35/WAKU2-BINDINGS-API 36/WAKU2-BINDINGS-API May 12, 2022
@D4nte
Copy link
Contributor Author

D4nte commented May 13, 2022

I believe this is ready.

@richard-ramos @jm-clius if you would be kind enough to do a last pass and approve we can then merge.

jm-clius
jm-clius approved these changes May 13, 2022
View reviewed changes
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! Thanks for this.

Only comment is to find and replace "nim-waku" with "nwaku" :P

D4nte and others added 19 commits May 17, 2022 18:17
@D4nte
More
fe27308
@D4nte
More corrections
f7b901c
@D4nte
Typo
478bef9
@D4nte
Fix metadata
6e44121
@D4nte
Sort out type
fc0af91 
- Define `JsonSignal`
- Use `Json` as a prefix
@D4nte
Specify that the multiaddress needs a peer id
b4cc9ed
@D4nte
Remove waku_close_peer
8faf7f8 
An API with a peer id is sufficient.
@D4nte
Improve dial/close terminology
04086cf
@D4nte
35 already taken
a5a7917
@D4nte
Remove nodeid
d964bc4 
We assume that an app would only want to manage one waku node.
This would enable to make the API stateless (if the same is done for subscription id).
@D4nte
Return a boolean instead of null when the JsonResponse is trivial
623ca08
@richard-ramos @D4nte
fix: subscriptions and decoding
0b5155e 
- remove mentions to subscriptionId
- use waku_relay_unsubscribe to close subscriptions to pubsub topics
- added functions to decode messages
@D4nte
Remove waku_decode_payload
2139388
@D4nte
Define DecodedPayload, clean MD errors
fff2270
@D4nte
Clarify expectation around WakuMessage version
916b63d
@D4nte
Add enc publish functions
6396b17
@D4nte
Clarify default behaviour for advertiseAddr
c10fefd
@D4nte
Replace nim-waku with nwaku
2f91ec9
@D4nte
Adding menu link
7c3fa06
@D4nte D4nte merged commit 3ed40ff into master May 17, 2022
vac-research automation moved this from Review/QA to Done May 17, 2022
@D4nte D4nte deleted the c-bindings branch May 17, 2022 08:18
kaiserd added a commit that referenced this pull request Jul 16, 2022
@kaiserd
Merge branch 'master' into add/waku-rfc-template
d452c13 
* 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
@kaiserd
Merge branch 'master' into add/waku-rfc-template
ba23a38 
* 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)
@richard-ramos richard-ramos mentioned this pull request Nov 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richard-ramos richard-ramos richard-ramos left review comments

@jm-clius jm-clius jm-clius approved these changes

Assignees

@D4nte D4nte

Labels
None yet
Projects
No open projects
vac-research
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@D4nte @richard-ramos @jm-clius @oskarth