fix(wallet): Ignore chain down notifications when connection is lost #5820
Conversation
Jenkins BuildsClick to see older builds (85)
|
friofry
force-pushed
the
ab/issue-21071-chain-down-notifications
branch
from
1caf1ad
to
0de4099
Compare
friofry
force-pushed
the
ab/issue-21071-chain-down-notifications
branch
from
0de4099
to
44159b1
Compare
services/wallet/market/market.go
Outdated
| Message: message, | ||
| At: time.Now().Unix(), | ||
| }) | ||
| if pm.onlineChecker == nil || pm.onlineChecker.Online() { |
There was a problem hiding this comment.
hmm I'm not really sure about this change...
- I don't think we've got a guarantee that the
onlinechecker.Online()state will change before the providers are detected as offline/offline. This extra check will potentially inhibit the last provider down notifications and the first providers up ones. - The providers ARE indeed down from the client's perspective (simply due to the fact that there's no internet connection), shouldn't the client decide what to do with that piece of info instead of not communicating the individual states?
There was a problem hiding this comment.
-
Agree, the solution is not robust at all. The peer count only becomes zero after a timeout (but after the connection is re-established, it becomes greater than zero quite quickly).
-
Users still see the "please check your internet connection" banner. So the chain-down notifications might be excessive.
if 2 is OK. I see two options here:
- Run a background loop that accesses the provider's server with a delay (maybe with a fake key).
- Simply skip notifications for
net.Error,net.DNSError,net.OpError,tls.RecordHeaderError,context.DeadlineExceeded,http.ErrServerClosedand "connection refused", "i/o timeout" intoggleConnectionState.
and I'm leaning towards the 2nd, I've done a few tests on desktop and it seems to work good
There was a problem hiding this comment.
Alternatively, we can move this logic to the client side. I can add errors (raw or as an enum) to the "down" status, so the client can decide if it wants to display the notification. I would suggest doing this as a separate task.
friofry
force-pushed
the
ab/issue-21071-chain-down-notifications
branch
2 times, most recently
from
932744e
to
24fe8af
Compare
friofry
force-pushed
the
ab/issue-21071-chain-down-notifications
branch
from
24fe8af
to
bb304ae
Compare
There was a problem hiding this comment.
please add this to the mock section of Makefile
rpc/chain/client.go
Outdated
| if !isNotFoundError(err) && !isVMError(err) && !errors.Is(err, ErrRequestsOverLimit) && !errors.Is(err, context.Canceled) { | ||
| if network_utils.IsConnectionError(err) { | ||
| connected = false | ||
| skipNotification = true |
There was a problem hiding this comment.
I'm still not happy about not sending the notifications in this case...
Example, some providers could be inaccessible due to firewall rules, while others could be working just fine. The client should be able to say "hey, you cannot access these providers so you cannot do this and this operations, but you can do these other ones for now".
What I would do, if you feel it's reasonable, is add some enum field to the notification with more detail about the cause for the "provider down" event. One of the values could be "Network Down" which I feel would have the same value as the skipNotification flag here. Then, if the Mobile client wants to not raise banners when that's the reason, you guys can do that.
There was a problem hiding this comment.
As I mentioned here #5820 (comment)
What do you think about making it a separate task?
Because it will require some clarification from the UI team.
There was a problem hiding this comment.
Sorry, perhaps I'm being stubborn and I don't want to block you unnecessarily so I'd like to hear some other folks' opinions.
The way I see it right now, this could happen after these changes:
- Chain providers are down for whatever reason (firewall, providers are really down, etc), so we cannot refresh balances. We inhibit the notification.
- The completely independent "Internet connection down" state (I think it's tied to the number of Waku peers being 0 or something) does not get triggered
- The client shows no banner about providers being down, internet connection being down or balances being out of date
There was a problem hiding this comment.
After discussion on the status-go guild, we agreed to add a field to the wallet notification event to determine the type of error to avoid breaking desktop logic.
There was a problem hiding this comment.
Added some findings while I was working on this task #5854
friofry
force-pushed
the
ab/issue-21071-chain-down-notifications
branch
2 times, most recently
from
5960c4a
to
c6cfa03
Compare
|
Summary of discussion with @dlipicar
|
friofry
force-pushed
the
ab/issue-21071-chain-down-notifications
branch
7 times, most recently
from
6625522
to
3b2948a
Compare
|
added a PR for a health-manager: #5924 |
friofry
force-pushed
the
ab/issue-21071-chain-down-notifications
branch
from
3b2948a
to
f4143cd
Compare
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## develop #5820 +/- ##
============================================
- Coverage 47.02% 10.51% -36.51%
============================================
Files 832 832
Lines 137725 136989 -736
============================================
- Hits 64763 14404 -50359
- Misses 65429 120728 +55299
+ Partials 7533 1857 -5676
Flags with carried forward coverage won't be shown. Click here to find out more.
|
friofry
force-pushed
the
ab/issue-21071-chain-down-notifications
branch
from
f4143cd
to
cd6dc2b
Compare
Description.
network_errors.go(+tests)market.goandchain/client.gowill not trigger a notification event if there is a connection error.Future improvements:.
market.gosimilar tochain/client.go. (Use WalletNotifier in market.go)fixes status-im/status-mobile#21071
refs status-im/status-mobile#21056
refs status-im/status-mobile#20736