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

Remove or move notification package from go-ethereum fork to status-go #655

Closed
pedropombeiro opened this issue Feb 12, 2018 · 4 comments
Closed
Assignees

Comments

@pedropombeiro
Copy link
Contributor

pedropombeiro commented Feb 12, 2018

Problem

According to some comments (here and here), regarding a package which deals with notifications in https://github.com/status-im/go-ethereum/tree/develop/whisper/notifications, it is not clear if this code is required, but at the very least, it is not necessary for this code to live there and it implies patches (https://github.com/status-im/status-go/blob/develop/_assets/patches/geth/0004-whisper-notifications.patch, https://github.com/status-im/status-go/blob/develop/_assets/patches/geth/0013-whisperv6-notifications.patch). Should the code still be needed, it should be moved to the status-go repo.

Implementation

Investigate whether code is needed. Remove it if not, move it to status-go if yes.

Acceptance Criteria

  • After removal push notifications work properly.
  • Chat performance and functionality is not deteriorated.

Notes

Code that initializes notifications: https://github.com/status-im/status-go/blob/develop/geth/node/node.go#L186-L192

@JekaMas
Copy link
Contributor

JekaMas commented Feb 12, 2018

The other issue is that the patch contains some useful modifications to whisper service. So it couldn't been removed all at once.
So additional criteria: chats are working fine with the same speed.

@adambabik
Copy link
Contributor

@pombeirp @JekaMas what do you think about splitting 0013-whisperv6-notifications.patch into two patches so that one encapsulates only whisper/notifications/ code and another one Whisper V6 changes? Even if whisper/notifications/ is needed, this issue would not require changing 0013-whisperv6-notifications.patch.

@JekaMas
Copy link
Contributor

JekaMas commented Feb 12, 2018

That'd be great. It should be in the issue.

@adambabik
Copy link
Contributor

One thing: watch out for not making circular imports. In whisper/whisperv5/whisper.go there is notificationServer NotificationServer attribute. So, we will end up with status-go importing go-ethereum and go-ethereum (after patching) will import status-go to get NotificationServer declaration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants