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

chore: update bindings version #317

Merged
merged 1 commit into from
Feb 29, 2024
Merged

Conversation

pete-eiger
Copy link
Contributor

@pete-eiger pete-eiger commented Feb 2, 2024

Updated waku bindings version and fixed conflicts. This update should fix the memory leak issue in the bindings, it also includes updates for deprecating the legacy filter protocol and moving to filter v2.

@pete-eiger pete-eiger requested a review from hopeyen February 2, 2024 10:23
@pete-eiger pete-eiger self-assigned this Feb 2, 2024
@pete-eiger pete-eiger force-pushed the update-waku-bindings-version branch from 48d9c3d to 7d59482 Compare February 2, 2024 10:24
@pete-eiger pete-eiger marked this pull request as draft February 5, 2024 18:28
@pete-eiger pete-eiger force-pushed the update-waku-bindings-version branch from 7d59482 to 0aec55b Compare February 20, 2024 12:00
@pete-eiger pete-eiger removed the request for review from hopeyen February 20, 2024 12:00
@pete-eiger pete-eiger force-pushed the update-waku-bindings-version branch 6 times, most recently from d3db0b0 to 00db6ef Compare February 20, 2024 16:22
@pete-eiger pete-eiger marked this pull request as ready for review February 20, 2024 16:23
@pete-eiger pete-eiger requested a review from hopeyen February 20, 2024 16:23
Copy link
Collaborator

@hopeyen hopeyen left a comment

Choose a reason for hiding this comment

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

did the rust binding deprecate filter protocol by 0.6.0?

@pete-eiger
Copy link
Contributor Author

did the rust binding deprecate filter protocol by 0.6.0?

Yes

@hopeyen
Copy link
Collaborator

hopeyen commented Feb 20, 2024

did the rust binding deprecate filter protocol by 0.6.0?

Yes

any updates on filter v2?

@pete-eiger
Copy link
Contributor Author

did the rust binding deprecate filter protocol by 0.6.0?

Yes

any updates on filter v2?

Do we need it?

@hopeyen
Copy link
Collaborator

hopeyen commented Feb 20, 2024

did the rust binding deprecate filter protocol by 0.6.0?

Yes

any updates on filter v2?

Do we need it?

arguably needed when a user wants to participate as a light client with limited load requirements. if v2 exists the filter protocol should have similar api if not the same, so it is better to include as we have the flow already. if v2 doesn't exist then it is fine to remove so we can proceed without memory leak

@pete-eiger
Copy link
Contributor Author

did the rust binding deprecate filter protocol by 0.6.0?

Yes

any updates on filter v2?

Do we need it?

arguably needed when a user wants to participate as a light client with limited load requirements. if v2 exists the filter protocol should have similar api if not the same, so it is better to include as we have the flow already. if v2 doesn't exist then it is fine to remove so we can proceed without memory leak

Ok I will spend some time looking at whether it's trivial to include it and keep the current filter functionality

@pete-eiger pete-eiger force-pushed the update-waku-bindings-version branch 8 times, most recently from b98d5a9 to a126223 Compare February 21, 2024 11:46
@pete-eiger pete-eiger requested a review from hopeyen February 21, 2024 12:03
@pete-eiger pete-eiger force-pushed the update-waku-bindings-version branch from a126223 to aa4ae1e Compare February 21, 2024 12:04
@pete-eiger pete-eiger marked this pull request as draft February 22, 2024 14:02
@pete-eiger pete-eiger removed the request for review from hopeyen February 22, 2024 14:02
@pete-eiger pete-eiger force-pushed the update-waku-bindings-version branch from aa4ae1e to 53eb639 Compare February 27, 2024 09:42
@pete-eiger pete-eiger marked this pull request as ready for review February 27, 2024 09:43
@pete-eiger pete-eiger requested a review from hopeyen February 27, 2024 09:44
@pete-eiger pete-eiger force-pushed the update-waku-bindings-version branch 3 times, most recently from 29fafc5 to 7bf9f8c Compare February 27, 2024 14:09
@@ -456,9 +458,6 @@ impl GraphcastAgent {
"Selected content topic from subscriptions"
);

// Check network before sending a message
self.network_check()
Copy link
Collaborator

Choose a reason for hiding this comment

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

would you please briefly explain why this function is no longer called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this function is very slow now that we're using waku's discovery ENR, it takes about 250-300s each time and it blocks message sending, I opted into calling it every 5 minutes in the Radio itself

Copy link
Collaborator

Choose a reason for hiding this comment

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

depending on whether network connectivity should be handled by the sdk, we can later on refactor the function to make sure it doesn't take that long, but okay here

src/graphcast_agent/waku_handling.rs Outdated Show resolved Hide resolved
@@ -192,6 +182,8 @@ fn node_config(
gossipsub_params: Some(gossipsub_params),
dns4_domain_name: None,
websocket_params: None,
dns_discovery_urls: vec![],
Copy link
Collaborator

Choose a reason for hiding this comment

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

should these 2 params be passed in from the radio since we have those configs iirc?

@pete-eiger pete-eiger force-pushed the update-waku-bindings-version branch from 7bf9f8c to dfb06e6 Compare February 28, 2024 13:20
@pete-eiger pete-eiger force-pushed the update-waku-bindings-version branch from dfb06e6 to 4d50b93 Compare February 28, 2024 13:27
@pete-eiger pete-eiger requested a review from hopeyen February 28, 2024 13:27
@pete-eiger pete-eiger merged commit 5db9ca6 into dev Feb 29, 2024
10 checks passed
@pete-eiger pete-eiger deleted the update-waku-bindings-version branch February 29, 2024 15:50
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.

2 participants