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

Verify ENS in the background #1824

Merged
merged 1 commit into from
Feb 5, 2020
Merged

Conversation

cammellos
Copy link
Contributor

@cammellos cammellos commented Jan 31, 2020

Currently ENS are verified explicitly by status-react, this is not ideal
as if that fails it will have to be explicilty retried in status-react.
This commits changes that behavior so that ENS are verified in a loop
and updated if new messages are received.

status: ready

@cammellos cammellos self-assigned this Jan 31, 2020
@status-github-bot
Copy link

Pull Request Checklist

  • Have you updated the documentation, if impacted (e.g. docs.status.im)?

@status-im-auto
Copy link
Member

status-im-auto commented Jan 31, 2020

Jenkins Builds

Click to see older builds (6)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 7eefde7 #1 2020-01-31 12:14:00 ~49 sec linux 📦zip
✔️ 7eefde7 #1 2020-01-31 12:18:20 ~5 min ios 📦zip
✔️ 7eefde7 #1 2020-01-31 12:21:25 ~8 min android 📦aar
✔️ 9c02047 #2 2020-01-31 14:44:38 ~46 sec linux 📦zip
✔️ 9c02047 #2 2020-01-31 14:49:09 ~5 min ios 📦zip
✔️ 9c02047 #2 2020-01-31 14:52:34 ~8 min android 📦aar
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ fb2f5ab #3 2020-02-04 10:57:05 ~39 sec linux 📦zip
✔️ fb2f5ab #3 2020-02-04 10:59:41 ~3 min ios 📦zip
✔️ fb2f5ab #3 2020-02-04 11:09:38 ~13 min android 📦aar
✔️ e153c60 #4 2020-02-04 13:54:17 ~45 sec linux 📦zip
✔️ e153c60 #4 2020-02-04 13:56:25 ~2 min ios 📦zip
✔️ e153c60 #4 2020-02-04 13:59:34 ~6 min android 📦aar

Copy link
Member

@jakubgs jakubgs left a comment

Choose a reason for hiding this comment

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

Overall looks good, just few small comments. My main one would be:

Why are we adding ENS stuff into protocol package?
Wouldn't it make sense to make a separate ens package?
Or at least protocol/ens?

protocol/ens.go Outdated Show resolved Hide resolved
protocol/message_handler.go Outdated Show resolved Hide resolved
params/config.go Show resolved Hide resolved
protocol/message_handler.go Outdated Show resolved Hide resolved
Currently ENS are verified explicitly by status-react, this is not ideal
as if that fails it will have to be explicilty retried in status-react.
This commits changes that behavior so that ENS are verified in a loop
and updated if new messages are received.
@cammellos cammellos force-pushed the feature/verify-ens-background branch from fb2f5ab to e153c60 Compare February 4, 2020 13:53
@cammellos
Copy link
Contributor Author

Thanks for the review, addressed the feedback:

Why are we adding ENS stuff into protocol package?

The protocol naming is not very descriptive of the responsibilities, basically the protocol/ package takes care of the handling and state management of the app, and this happens in reaction to a protocol event (receiving a message from a user).

I would move it to protocol/ens.go, but currently there would be some circular dependencies as most of the data structures are top level Contact for example, and ENS depends on Contact, while Messenger depends on both Contact and ENS, so we should move first Contact to his own namespace and then have ENS and Messenger both depend on it, but I'd rather handle this once adam is back, as he is the one who has a better feel on how things should be organized, if that's ok.

@cammellos cammellos merged commit f2eebd1 into develop Feb 5, 2020
@delete-merged-branch delete-merged-branch bot deleted the feature/verify-ens-background branch February 5, 2020 10:09
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.

3 participants