search

status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.9k stars 988 forks source link

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

Closed churik closed 5 years ago

churik commented 5 years ago

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:

Scenario 2:

Additional Information

yenda commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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.

gitcoinbot commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

Empty account. Then, I took the same account, went to airplane mode, sent a transaction to it and waited for it to mine, switched off airplane mode and the account balance updated ~15-30 seconds later.

On Thu, Oct 10, 2019, 8:16 AM yenda notifications@github.com wrote:

@acolytec3 https://github.com/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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/status-im/status-react/issues/8951?email_source=notifications&email_token=AEENFXE5W7N2BWQXLNK7NYDQN4MJBA5CNFSM4IUZYN62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA4AZYQ#issuecomment-540544226, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEENFXAN323OVVKIQJXTPMDQN4MJBANCNFSM4IUZYN6Q .

acolytec3 commented 5 years ago

I can check it again this afternoon once I get home from work.

On Thu, Oct 10, 2019, 9:31 AM Andrew Day konjou@gmail.com wrote:

Empty account. Then, I took the same account, went to airplane mode, sent a transaction to it and waited for it to mine, switched off airplane mode and the account balance updated ~15-30 seconds later.

On Thu, Oct 10, 2019, 8:16 AM yenda notifications@github.com wrote:

@acolytec3 https://github.com/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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/status-im/status-react/issues/8951?email_source=notifications&email_token=AEENFXE5W7N2BWQXLNK7NYDQN4MJBA5CNFSM4IUZYN62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA4AZYQ#issuecomment-540544226, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEENFXAN323OVVKIQJXTPMDQN4MJBANCNFSM4IUZYN6Q .

acolytec3 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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.

gitcoinbot commented 5 years ago

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.