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

feat: use CommonService in peerConnector #737

Merged
merged 7 commits into from
Sep 19, 2023
Merged

feat: use CommonService in peerConnector #737

merged 7 commits into from
Sep 19, 2023

Conversation

harsh-98
Copy link
Contributor

Description

Refactor peerConnector to include CommonService

Changes

Tests

@status-im-auto
Copy link

status-im-auto commented Sep 14, 2023 •
edited
Loading

Jenkins Builds

Click to see older builds (24)
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ 70f555e #1 2023-09-14 11:57:14 ~1 min nix-flake 📄log
❌ 70f555e #1 2023-09-14 11:57:16 ~1 min linux 📄log
✖️ 70f555e #1 2023-09-14 11:57:37 ~1 min tests 📄log
✖️ 70f555e #1 2023-09-14 11:57:55 ~1 min tests 📄log
❌ 70f555e #1 2023-09-14 11:58:36 ~2 min ios 📄log
❌ 70f555e #1 2023-09-14 11:58:49 ~2 min android 📄log
✔️ 41d2e1c #2 2023-09-14 11:59:40 ~1 min nix-flake 📄log
✔️ 41d2e1c #2 2023-09-14 11:59:56 ~1 min tests 📄log
✔️ 41d2e1c #2 2023-09-14 12:00:04 ~2 min linux 📦deb
✔️ 41d2e1c #2 2023-09-14 12:00:47 ~3 min tests 📄log
✔️ 41d2e1c #2 2023-09-14 12:02:10 ~3 min android 📦tgz
✔️ 41d2e1c #2 2023-09-14 12:07:44 ~10 min ios 📦tgz
✔️ 089cfd3 #3 2023-09-18 07:44:15 ~1 min linux 📦deb
✔️ 089cfd3 #3 2023-09-18 07:44:51 ~1 min tests 📄log
✔️ 089cfd3 #3 2023-09-18 07:45:00 ~1 min nix-flake 📄log
✔️ 089cfd3 #3 2023-09-18 07:45:23 ~2 min tests 📄log
✔️ 089cfd3 #3 2023-09-18 07:46:35 ~3 min android 📦tgz
✔️ 089cfd3 #3 2023-09-18 07:46:39 ~3 min ios 📦tgz
✔️ 089cfd3 #4 2023-09-18 09:43:35 ~1 min linux 📦deb
✔️ 089cfd3 #4 2023-09-18 09:44:05 ~2 min nix-flake 📄log
✔️ 089cfd3 #4 2023-09-18 09:45:08 ~3 min tests 📄log
✔️ 089cfd3 #4 2023-09-18 09:45:09 ~3 min tests 📄log
✔️ 089cfd3 #4 2023-09-18 09:45:23 ~3 min android 📦tgz
✔️ 089cfd3 #4 2023-09-18 09:45:27 ~3 min ios 📦tgz
❔ Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 70f636b #5 2023-09-19 00:24:30 ~1 min linux 📦deb
✖️ 70f636b #5 2023-09-19 00:24:33 ~1 min tests 📄log
✔️ 70f636b #5 2023-09-19 00:25:23 ~1 min tests 📄log
✔️ 70f636b #5 2023-09-19 00:25:37 ~2 min nix-flake 📄log
✔️ 70f636b #5 2023-09-19 00:27:28 ~4 min android 📦tgz
✔️ 70f636b #5 2023-09-19 00:27:37 ~4 min ios 📦tgz
✔️ f65a59e #6 2023-09-19 00:29:15 ~1 min linux 📦deb
✔️ f65a59e #6 2023-09-19 00:29:30 ~1 min tests 📄log
✔️ f65a59e #6 2023-09-19 00:30:02 ~1 min tests 📄log
✔️ f65a59e #6 2023-09-19 00:30:05 ~1 min nix-flake 📄log
✔️ f65a59e #6 2023-09-19 00:31:33 ~3 min ios 📦tgz
✔️ f65a59e #6 2023-09-19 00:31:56 ~3 min android 📦tgz

@harsh-98 harsh-98 force-pushed the PeerConnectorCommonService branch from 70f555e to 41d2e1c Compare September 14, 2023 11:57
@harsh-98 harsh-98 marked this pull request as ready for review September 14, 2023 19:09
@harsh-98 harsh-98 requested review from richard-ramos and chaitanyaprem and removed request for richard-ramos September 15, 2023 04:12
@chaitanyaprem
Copy link
Collaborator

I don't think we need to use generics here.
Look at the discussion in #735 . Better to go ahead with a different approach.

Generics make code hard to maintain and read. So if there is an alternative approach to generics, i would always prefer that. Looks like @richard-ramos also agrees with this.

@harsh-98 harsh-98 force-pushed the PeerConnectorCommonService branch from 41d2e1c to 089cfd3 Compare September 18, 2023 07:42
chaitanyaprem
chaitanyaprem approved these changes Sep 18, 2023
View reviewed changes
Copy link
Collaborator

@chaitanyaprem chaitanyaprem left a comment

Choose a reason for hiding this comment

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

LGTM

But, i would suggest to do some testing by running service-node locally and trying to connect from multiple nodes to ensure nothing is broken wrt peer connectivity and management.

Base automatically changed from Discv5CommonService to master September 18, 2023 09:41
@harsh-98 harsh-98 merged commit a650469 into master Sep 19, 2023
2 checks passed
@harsh-98 harsh-98 deleted the PeerConnectorCommonService branch September 19, 2023 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chaitanyaprem chaitanyaprem chaitanyaprem approved these changes

@richard-ramos richard-ramos richard-ramos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Waku
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@harsh-98 @status-im-auto @chaitanyaprem @richard-ramos