Safe-react: Safe Apps section disappears

[rmeissner]

Title/Description

In some cases the Safe Apps section disappears and the user is send back to the asset view.

Note: The collectible tab also disappears

This is most likely related to https://github.com/gnosis/safe-react/blob/ce1ab8b039c9603014ca33ee32b6d199daa1d12f/src/logic/safe/store/actions/fetchSafe.ts#L53-L67

More information can be found in the Slack thread linked in the references section of this issue.

Environment

• Browser: Chrome
• Wallet: MM
• App Version: 3.1.2
• Environment:
  
  
  
  • production (mainnet)
  
  

Steps to reproduce

• Go to the Safe apps tab and use a Safe app that peforms quite some rpc requests. (e.g. Uniswap or ENS)

Expected result

• Safe apps should not appear if Safe state did not change

Obtained result

• Safe apps randomly disappear

References

See https://gnosisinc.slack.com/archives/C63LQTGNB/p1615482085121900

Possible fix

• Load Safe info only from client gateway (don't load Safe info from chain)
• If we have an existing state don't hide all feature in case of an error fetching updated information

[nicosampler]

The last time I reviewed the code that handles what is shown depending on the safe version and if the connected wallet is an owner was a little messy. I'm pretty sure this is happening because of a race condition.

It would be good to take a new look at this and perhaps refactor the code a little bit.

[fernandomg]

I want to propose a different approach to this issue. Can we convert it to an epic (or an _epic bug_ :grimacing:)?

Something around:
v1) refactor fetchSafe and buildSafe
v2) refactor Load Safe flow
v3) refactor fetchTokens and tokens balances

...and after each one, do a code cleanup, removing legacy code.

That's what I did for the v1 (#2122 with a code cleanup #2123)

The motivation

The issue is there and certainly is due to a race condition as Nico pointed out (mixed with a predefined set of steps to meet that condition, I'll add). Because of this, and as we weren't able to _deterministically_ reproduce it, I opted for the proposed fixes that Richard listed, along with a refactor and code-cleanup.

If you give me green light, tomorrow will do the update.

/cc @dasanra, @rmeissner, @lukasschor

[fernandomg]

update: I followed a lead that Dani suggested about the enabledFeatures prop that may be the cause of the issue. And it looks like it is, especially if we look at the currentVersion value, which if we fail to retrieve it from RPC calls, will

1. fall back to an empty value,
2. then that data will be persisted to the localStorage with an empty featuresEnabled value,
3. which will update the in-memory prop with empty data.

I'm quite positive that this scenario won't happen in the #2122 fix, as that intricate logic was replaced with a more clean and straightforward flow.

[dasanra]

@fernandomg I'm ok to handle this ticket in smaller ones. The PR simplifying how we fetch safe information is a real improvement. I think that doing it step by step will allow us to get a better solution for this. We need to think carefully about this part because it's still having some legacy there and it's easier to split in different sprints if we can separate in smaller tasks

[fernandomg]

update: as pointed by Franco (see: https://github.com/gnosis/safe-react/issues/2048#ref-pullrequest-854742757), we found there was an undesired behavior with locally persisted information. It has been written/erased constantly. I'll continue digging on it.

[fernandomg]

update: I've been trying to reproduce the latter issue in #2122. The only issue found is that AddressBook middleware is buggy, and fails to properly filter what needs to be stored or not. Other than that, everything seems to be working fine and _no 'Apps' or 'Collectibles' sections were harmed during the running of the app_.