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

Balance is not updated when go from offline to online (until reopening app) #8951

Closed
churik opened this issue Sep 9, 2019 · 21 comments · Fixed by #9157
Closed

Balance is not updated when go from offline to online (until reopening app) #8951

churik opened this issue Sep 9, 2019 · 21 comments · Fixed by #9157
Assignees

Comments

@churik
Copy link
Member

churik commented Sep 9, 2019

Description

Type: Bug
Summary: when you restore/create account with poor connection or offline and then go online - balance is not fetched until you relogin to app. And if you get some funds when you were offline and then go to online - balance is not updated as well until manual app restart.
It may be very confusing to users.

Expected behavior

balance is updated when go to online

Actual behavior

balance is updated only after reopening the app

Reproduction

Scenario 1:

  • Open Status
  • Turn on airplane mode
  • Restore account
  • Turn off airplane mode
  • Wait for a while (3 mins)

Scenario 2:

  • Open Status
  • Restore account
  • Turn on airplane mode
  • Receive some funds to acount (check that balance is updated on etherscan.io)
  • Turn off airplane mode
  • Wait for a while (3 mins)

Additional Information

  • Status version: nightly 09/09/19
  • Operating System: Android, iOS
@yenda
Copy link
Contributor

yenda commented Sep 9, 2019

This is odd because status-go should signal the new transaction when coming back online.
A bad fix would be to just ask for balance again on status-react side every time we go back online, but the proper fix would be to figure out why status-go doesn't signal the new transaction once back online

@acolytec3
Copy link
Contributor

acolytec3 commented Oct 8, 2019

@yenda, I've reproduced scenario and have a question about trying to solve. You said above a "bad fix" would be to have status-react ask for balance once online connectivity is established. In looking at re-frisk when in this scenario, it looks like the :error-unable-to-fetch-balance error is just never cleared from the wallet subscriptions, even though we're clearly connected (seeing new block events continuously). As far as I can tell, the only place that error is ever cleared is in the "update-balances" function in wallet/core and is supposed to be triggered whenever network-status changes. I need to do some more digging on the subscriptions around network-status and offline? but that seems like the likely culprit, in that somehow offline status isn't getting properly set. Does that feel like a more likely route than something with status-go not correctly signaling new transactions that have an impact on balance?

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 60.0 DAI (60.0 USD @ $1.0/DAI) attached to it.

@gitcoinbot
Copy link

gitcoinbot commented Oct 8, 2019

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work has been started.

These users each claimed they can complete the work by 1 week, 4 days from now.
Please review their action plans below:

1) acolytec3 has started work.

Identify source of bug and code appropriate solution

Learn more on the Gitcoin Issue Details page.

@acolytec3
Copy link
Contributor

acolytec3 commented Oct 9, 2019

@rachelhamlin @yenda I'm stuck on this one and wondering if I should pass it off to someone else. As best I can tell, the issue lies mainly with the fact that the listener set to detect network changes under the net-info module is never firing off any events, and therefore is never updating the network-status subscription, which in turn means the update balances/prices events are never firing when you switch from offline to online. I only see the :status-im.network.net-info/network-info-changed event firing on app start-up and not when I actually force a network change (i.e. go to airplane mode and then back on).
Any idea if this sounds like the right track? I'm at a loss on next steps.

@yenda
Copy link
Contributor

yenda commented Oct 10, 2019

@acolytec3 how do you check if :status-im.network.net-info/network-info-changed is fired? If you use re-frisk you won't see it because going to airplane mode will also cut the connection for re-frisk. I just tried on real device while looking at adb logs and the events is indeed fired.

@acolytec3
Copy link
Contributor

@yenda That explains what I was seeing as I was just using the emulator. I'll try some further debugging on an actual device and see if I see anything else that looks out of place. Maybe this is a dumb question but if I'm testing it on an actual device, will re-frisk keep it's connection even if I switch to airplane mode (since the device maintains connectivity via USB?

@yenda
Copy link
Contributor

yenda commented Oct 10, 2019

@acolytec3 There is a bug anyway in handle-network-info-change: change is-connected to isConnected

maybe that fixes the whole thing or at least it will get you closer to the solution

@yenda
Copy link
Contributor

yenda commented Oct 10, 2019

@acolytec3 and yes on a real device re-frisk keeps the connection, but you need to use make android-ports to make sure that the ports are open for the repl and re-frisk

@acolytec3
Copy link
Contributor

@yenda Thanks for the guidance! That explains why network-status was always nil in the subscriptions. I now see it changing back and forth correctly in re-frisk on my device. That doesn't appear to solve the update-balance problem by itself but I think it's part of the problem. I've made that specific change locally and will add it to my PR (unless you've already got it going in another one to merge into develop).

@acolytec3
Copy link
Contributor

@yenda Further update, subsequent testing shows that the above isConnected change does appear to solve scenario 2 that @churik outlined above. It doesn't fix scenario 1 since the :get-balances effect is only called in 3 scenarios

  1. When the wallet is initialized by the initialize-wallet effect - this is what causes the first issue, since the device isn't connected so it can't retrieve the balance
  2. When status-go detects a new transfer, this is resolved by the bug fix noted above.
  3. When the safe-account effect is triggered, and this looks like it only happens when you generate/restore an account from a seedphrase or keycard so same out come as 1) above.

I know you said it was a "bad" solution to have status-react call update-balance again but that seems like it's the only way to solve @churik's first issue given the above. Any preference on how to solve for that? Seems like we could add a subscription to the wallet screens (portfolio and account views) that call get-prices only if network-status == :online and there's a balance-update error set. This solution is based on the assumption that the balance-update error mostly or always stems from a root cause of network connectivity issues. Either that, or we could add a new error condition that is set if the update-balance fails while network-status is :offline and base the subscription purely on that.

@acolytec3 acolytec3 mentioned this issue Oct 10, 2019
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work for 60.0 DAI (60.0 USD @ $1.0/DAI) has been submitted by:

  1. @acolytec3

@StatusSceptre please take a look at the submitted work:


@yenda
Copy link
Contributor

yenda commented Oct 10, 2019

@acolytec3 just to clarify for scenario 1 did you try with an empty account or restoring an account which has transaction?

If you had transactions on the account do they start to show up in the transaction history once you are back online? If that is not the case there might be a bug on status-go side

@acolytec3
Copy link
Contributor

acolytec3 commented Oct 10, 2019 via email

@acolytec3
Copy link
Contributor

acolytec3 commented Oct 10, 2019 via email

@acolytec3
Copy link
Contributor

One follow up here, I was poking around the issue log and I wonder if #9061 might be related to scenario 1 above. Since there aren't any transactions on the account that @churik described there, seems like it could be the same root issue where it's not updating the balance of the given account (maybe because there's a :balance-update error on the account). I'll dig through status-im.ethereum.core and see if there's something I missed in my earlier review.

@yenda
Copy link
Contributor

yenda commented Oct 10, 2019

@acolytec3 so I think it's okay to update the balance when back online if the balance was not previously set, in that case that wouldn't be a bad fix because the status-go won't notifiy you of any transaction since there is none.

@acolytec3
Copy link
Contributor

acolytec3 commented Oct 10, 2019

@yenda So interestingly, I checked this again in the following way and here's what I see.

  1. Open Status app
  2. Turn on airplane mode
  3. Login with account with previous balance
  4. Turn off airplane mode the
  5. Wait a few minutes
  6. The account value never updates and remains "..."
  7. Check the transaction history and it shows the previous inbound transaction.

This is a different scenario than @churik's Scenario 2 in that the transaction is now considered "historical" (I think). That's why the account balance isn't updating since status-go only calls update-balances when it sees new transfers. It's more or less a variation on Scenario 1 where the app starts in off-line status when you login and then there's no trigger for update-balances.
So we resolved only the scenario where an account is offline when a new transfer is sent. I think my general proposal (i.e. call update-balances if network-status goes from offline to online and we have an balance-update error) will still work. All that to say, I think status-go is working fine with regard to this particular issue. I'm going to work on where it makes sense to call update-balance again (thinking most likely from the portfolio view code) but still figuring out how to layer it in.

@yenda
Copy link
Contributor

yenda commented Oct 10, 2019

@acolytec3 In your scenario did you send a transaction while offline or do you mean that there is only transaction that have been there for a while? If that is the case I think it still confirms my previous comment, and the correct way to fix it afaict would be to update the balance when back online only if the current balance is still nil (meaning there was no successful balance check yet)

@acolytec3
Copy link
Contributor

@yenda In this most recent attempt, I did the latter and agree that it does confirm. Will work on some changes tonight and tomorrow and hopefully update my PR with an appropriate trigger for update-balance by the weekend.

acolytec3 added a commit to acolytec3/status-react that referenced this issue Oct 13, 2019
acolytec3 added a commit to acolytec3/status-react that referenced this issue Oct 13, 2019
acolytec3 added a commit to acolytec3/status-react that referenced this issue Oct 23, 2019
acolytec3 added a commit to acolytec3/status-react that referenced this issue Oct 24, 2019
rasom pushed a commit to acolytec3/status-react that referenced this issue Oct 28, 2019
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


The funding of 60.0 DAI (60.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @acolytec3.

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 a pull request may close this issue.

6 participants