Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Safe in read-only mode even though correct owner is connected #1685

Closed
lukasschor opened this issue Dec 1, 2020 · 14 comments · Fixed by #1710 or #1847
Closed

Safe in read-only mode even though correct owner is connected #1685

lukasschor opened this issue Dec 1, 2020 · 14 comments · Fixed by #1710 or #1847
Assignees
Labels
Bug 🐛 Something isn't working Major Needs to be fixed for immediate next public release.

Comments

@lukasschor
Copy link
Member

lukasschor commented Dec 1, 2020

Title/Description

We've recently had multiple users report that they are confused about seeing a "Read only" mode label. Initially I thought it was just users connecting the wrong wallets to the app. But now I have experienced the same issue.

When opening a Safe via URL in a new browser tab and blocknative automatically connects to my Metamask account that is set on the correct owner, I still end up in read-only mode.

image

Also a few other issues appear:

  • Settings shows 0 owners
  • Safe name changes to LOADED SAFE
  • Threshold in settings shows "2 out of 0"

Maybe this is related to the onboard.js implementation.

Environment

  • Browser: Chrome
  • App Version: 2.16.0.
  • Environment:
    • production (mainnet)
  • Wallet: Metamask

Steps

  1. Open Safe in one tab (-> Settings, owners etc are loaded correctly.)
  2. Open the same safe via direct link in another tab

Expected

  • Usual Safe name
  • Safe is not read only.

Observed

  • Safe name changes to LOADED SAFE
  • Safe is read only
  • 0 owners is displayed in settings

Updates

  • Dec 3, Tobi: Added steps and more details.
@tschubotz tschubotz added the Bug 🐛 Something isn't working label Dec 2, 2020
@Agupane
Copy link
Contributor

Agupane commented Dec 2, 2020

The cause of this bug seems to be a race condition between useLoadSafe and useSafeScheduledUpdates

Seems that in some place of the code we are fetching the owners ok:

  const [[, remoteThreshold, remoteNonce, remoteOwners = []], safeInfo, localSafe] = await Promise.all([
    generateBatchRequests<[undefined, string | undefined, string | undefined, string[]]>({
      abi: GnosisSafeSol.abi as AbiItem[],
      address: safeAddress,
      methods: safeParams,
    }),
    getSafeInfo(safeAddress),
    getLocalSafe(safeAddress),
  ])

but then we are assigning [] to the owners list, I think the empty is coming from the localStorage, but I'm not 100% sure. Will need to do more research

@tschubotz
Copy link
Member

tschubotz commented Dec 2, 2020

@lukasschor just making sure: You don't have a reliable way to reproduce this yet, right? (I did encounter this problem also before, but can't seem to find a way to reproduce this 100%.

@tschubotz tschubotz added the Major Needs to be fixed for immediate next public release. label Dec 2, 2020
@lukasschor
Copy link
Member Author

Yes, for me it's reliably reproducible. I think a requirement is to already have the interface open in one tap and open in one tab and opening it in another one using a deep-link.

@tschubotz
Copy link
Member

Yes, for me it's reliably reproducible. I think a requirement is to already have the interface open in one tap and open in one tab and opening it in another one using a deep-link.

That's it! Nice one, I'll add the steps to the ticket. Thanks!

@liliya-soroka that's the key to #1454 as well :)

@fernandomg
Copy link
Contributor

Reopen as there are reported accounts that switch owner's connection status while connected to the safe (without any user interaction). No network or user switch, nor page refresh.

@fernandomg fernandomg reopened this Dec 28, 2020
@Agupane
Copy link
Contributor

Agupane commented Jan 14, 2021

I'm moving this to "requirements in progress" because I wasn't able to reproduce the issue. It's possible that's already fixed or it just a race condition that is not easy to reproduce. I asked Leandro to let me know in case he founds the bug again
CC: @fernandomg @alfetopito

@tschubotz
Copy link
Member

I'm also not able to reproduce this. We had reports of an unstable RPC endpoint. @alfetopito which RPC endpoint are you using? I'm on https://rpc.xdaichain.com/

I would actually close this one as long as it is not reproducible and/or the issue doesn't persist.

fyi @dasanra in case you had ideas on what the issue might be here.

@alfetopito
Copy link

I was testing most of the time with Nifty wallet on chrome https://www.poa.network/for-users/nifty-wallet
Not sure what endpoint they are using though.
Would assume they use the default's xdai endpoint? https://www.xdaichain.com/for-users/converting-xdai-via-bridge/use-alternate-or-custom-json-rpc-endpoints

@tschubotz
Copy link
Member

Thanks for the info, I'll try with nifty wallet today then!

@tschubotz
Copy link
Member

Seems like Nifty wallet uses https://dai.poa.network.
The alternative is https://rpc.xdaichain.com/ which is what they recommend here: https://www.xdaichain.com/for-developers/developer-resources

I wasn't able to reproduce with Nifty wallet on Chrome. But I still think it's related to the rpc.

@Agupane
Copy link
Contributor

Agupane commented Feb 2, 2021

I'm reopening this issue because I found the issue while fixing #1845 on volta. Seems that when this line runs:

  const [[, remoteThreshold, remoteNonce, remoteOwners = []], safeInfo, localSafe] = await Promise.all([
    generateBatchRequests<[undefined, string | undefined, string | undefined, string[]]>({
      abi: GnosisSafeSol.abi as AbiItem[],
      address: safeAddress,
      methods: safeParams,
    }),
    getSafeInfo(safeAddress),
    getLocalSafe(safeAddress),
  ])

on checkAndUpdateSafe the batch call hangs from time to time and returns undefined

@iafrincu
Copy link

iafrincu commented May 6, 2021

I also have this issue while trying to add the 4th owner of a safe using the Ledger wallet. I had a safe with 3 owners. I added the 4th owner and they have only read rights. I thought there was a random issue so I deleted them and added another wallet (a totally different ledger) to it and I have the same issue.

@dasanra
Copy link
Collaborator

dasanra commented May 10, 2021

@iafrincu we released v3.6.0 today that is including some improvements on how we check this parameters. Could you check now if you still have issues with the Ledger owners?

@1000F
Copy link

1000F commented Aug 13, 2021

currently having this issue on mobile, iOS 15 beta

@5afe 5afe locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug 🐛 Something isn't working Major Needs to be fixed for immediate next public release.
Projects
None yet
9 participants