refactor: c-bindings #619
refactor: c-bindings #619
Conversation
Jenkins BuildsClick to see older builds (51)
|
|
It looks very nice! I look forward to becoming a reviewer :) |
richard-ramos
force-pushed
the
c-bindings-2
branch
3 times, most recently
from
1ac6044
to
0660ffa
Compare
richard-ramos
force-pushed
the
c-bindings-2
branch
2 times, most recently
from
25d8f92
to
b7014e6
Compare
|
Finally done with this PR :) . It ended up being quite large due to moving a lot of files and extracting functions so they could be used both for the mobile bindings and the c bindings Here's the generated header if it makes it simpler to review, instead of focusing in mostly repetitive code changes in go: https://gist.github.com/richard-ramos/bb1a6b6a2ae59cb3949a4627ab0b139b . Notice that some functions in here receive a callback, while they don't in nwaku, ( Something else that I wanted to discuss with you is how are we going to handle functions that do not receive a string result. for example these: // Get number of connected peers
extern int waku_peer_cnt(WakuCallBack onOkCb, WakuCallBack onErrCb);
// Determine if there are enough peers to publish a message on a topic. Use NULL
// to verify the number of peers in the default pubsub topic
extern int waku_relay_enough_peers(char* topic, WakuCallBack onOkCb, WakuCallBack onErrCb);The peer count is supposed to be a number, and the second function is supposed to be a boolean. In this PR i'm returning the string representation of a number and a boolean, but IMO it's ugly. Do you think we should have instead different types of callbacks instead? something like this: Then |
|
@chaitanyaprem I just noticed the infinite number of codeclimate issues that were generated. I'll try to fix them :) |
richard-ramos
force-pushed
the
c-bindings-2
branch
3 times, most recently
from
3cd22a7
to
85ebcc5
Compare
richard-ramos
force-pushed
the
c-bindings-2
branch
from
85ebcc5
to
721c71b
Compare
|
Noticed that ReadMe intro section needs to be modified to change nwaku to go-waku. Line 9 in 721c71b |
library/signals.go
Outdated
| C.StatusServiceSignalEvent(str) | ||
| dataStr := string(data) | ||
| str := C.CString(dataStr) | ||
| C.StatusServiceSignalEvent(str, C.size_t(len(data))) |
There was a problem hiding this comment.
Maybe we should rename this to ServiceSignalEvent?
| } | ||
|
|
||
| // StoreLocalQuery is used to retrieve historic messages stored in the localDB using waku store protocol. | ||
| func StoreLocalQuery(queryJSON string) string { |
There was a problem hiding this comment.
Why are 2 different methods required?
I am thinking, wouldn't it be better to expose to the user a simple method called StoreQuery which checks localDB and if not present fetches from the peer?
There was a problem hiding this comment.
The problem is that you do not know if the data you have locally is complete. For an specific time period you might have n messages on your db, and the peer might have n+m message, so attempting to retrieve messages locally might return results and still end up missing messages. 🤔
There was a problem hiding this comment.
Ok, it's the users responsibility to identify what is missing and query peer accordingly.
Ah yes, i agree. The reason why it is like that is because when the bindings were created, the idea was for go-waku to only act as library, and not as a service node, hence the mention of nwaku, instead of go-waku |
Updates the c-bindings to match the style of using success and error callbacks defined by @Ivansete-status in nwaku's c-bindings.