search

status-im / status-protocol-go

Status Protocol implementation in Go
Mozilla Public License 2.0
0 stars 1 forks source link

Prepare API to support other implementations of ETH nodes #88

Closed pedropombeiro closed 4 years ago

pedropombeiro commented 4 years ago

Closes #84

This PR tries to limit usage of Geth types to the strict minimum surface area, and instead expose its own types, or wrapper interfaces to the outside world so that other implementations like Nimbus can more easily be added.

For the time being, the envelope monitor has been excluded from the scope (commented out), since it is not strictly required for the MVP. Otherwise, it is a non-breaking change.

This PR is part of a group of 3 PRs (https://github.com/status-im/status-protocol-go/pull/88, https://github.com/status-im/status-go/pull/1627, https://github.com/status-im/status-console-client/pull/128) and we should only merge when we're happy with the 3 of them.

adambabik commented 4 years ago

I don't exactly see how it's gonna work with two implementations.

So, the idea is that nim-based implementation, which would interact with Go using Cgo, is gonna implement proposed interfaces and wrappers by kind of translating Cgo code to Go and vice versa? Maybe we can have some diagram or some snippet that would explain how this effort is solving the problem of multiple implementation? My main point is to introduce some more clarity and further verify assumptions (maybe they were already verified but I can't find any trace of that in this PR or a linked issue).

kdeme commented 4 years ago

Update: OK, after going through it a bit further and seeing the wrapper go files I see that you are indeed facilitating to swap at Whisper API level, sorry for ignorance :)

So it would e.g. the Post call would be swapped at return a.shhAPI.Post(ctx, *newMessage), right? And then specific func (w *gethPublicWhisperAPIWrapper) Post should be reimplemented in the Nimbus version?

pedropombeiro commented 4 years ago

So it would e.g. the Post call would be swapped at return a.shhAPI.Post(ctx, *newMessage), right? And then specific func (w *gethPublicWhisperAPIWrapper) Post should be reimplemented in the Nimbus version?

Exactly

kdeme commented 4 years ago

Looks good to me. I think any message or other remapping that needs to be done for Nimbus can now be done in the specific wrapper (or we can always do some minor changes to the Nimbus API itself).

While I think it will still be important that in the future the separation between the transport (Whisper) and the Status protocol (and then specifically here the concept of Public, private, etc. chats) is improved, I agree that it is not in scope of this PR. So ignore those comments for now.

pedropombeiro commented 4 years ago

OK, ready for final review.

pedropombeiro commented 4 years ago

Are we OK with merging this PR then?

pedropombeiro commented 4 years ago

@cammellos @adambabik @kdeme By the way, I think the future goal of status-protocol-go should be to not include neither geth nor Nimbus, but rather those should be adapter projects (e.g. status-transport-geth and status-transport-nimbus) that get plugged in to status-protocol-go. These wrappers should then be moved to the respective adapter projects.