status-im / status-mobile

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

Missing wallet accounts are not hidden from "Addresses for permissions" screen #20624

Closed pavloburykh closed 2 months ago

pavloburykh commented 3 months ago

Currently missing accounts are not hidden from "Addresses for permissions" screen. It means that user can select such accounts during joining the community which causes join request to fail. сс @smohamedjavid

Steps:

  1. Login the User who has missing key pair
  2. Open link of any community which is not joined yet by the user
  3. Proceed joining community flow.
  4. See if there is possibility to select missing account for sharing during join

Expected result: we should hide missing accounts from "Addresses for permissions" list so user is unable to select such account. Solution is approved by Alisher here

Actual result: missing accounts are not hidden from "Addresses for permissions" screen. User can select such accounts during joining the community which results in join request to fail.

Status-debug-logs - 2024-07-02T144114.267.zip

https://github.com/status-im/status-mobile/assets/97245802/963c5355-0fec-4de7-900a-9e6f39effa19

Additional Information

smohamedjavid commented 3 months ago

@pavloburykh - I removed the non-operable accounts from the list in the Addresses for permission and Airdrop address selections while joining a community in PR #20636


@ilmotta @cammellos

I checked the logs attached to this issue and found that there is a crash happening in the CheckPermissionsToJoinCommunity RPC if the addresses contain any non-operable account addresses. (Not sure if it's due to the key store file not being found)

ERROR[07-02|11:37:27.552|github.com/status-im/status-go/vendor/github.com/ethereum/go-ethereum/rpc/service.go:200]                          RPC method wakuext_checkPermissionsToJoinCommunity crashed: runtime error: invalid memory address or nil pointer dereference
goroutine 3156 [running]:
github.com/status-im/status-go/vendor/github.com/ethereum/go-ethereum/rpc.(*callback).call.func1()
    github.com/status-im/status-go/vendor/github.com/ethereum/go-ethereum/rpc/service.go:199 +0x74
panic({0x7bf40d00c0?, 0x7bf5b61bc0?})
    runtime/panic.go:914 +0x218
github.com/status-im/status-go/protocol.(*Messenger).CheckPermissionsToJoinCommunity(0x0, 0x4002632e70)
    github.com/status-im/status-go/protocol/messenger_communities.go:4494 +0x70
github.com/status-im/status-go/services/ext.(*PublicAPI).CheckPermissionsToJoinCommunity(...)
    github.com/status-im/status-go/services/ext/api.go:1603
reflect.Value.call({0x4001301c20?, 0x4004336500?, 0x7f35215e10?}, {0x7bf1adbe9c, 0x4}, {0x4003eb5360, 0x2, 0x7bf2bc9058?})
    reflect/value.go:596 +0x994

In the fallback case where the addresses are empty in request, we use all the non-watch account addresses. It's best we use IsWalletAccountReadyForTransaction instead of IsWalletNonWatchOnlyAccount as it also checks for the operable state of the account. WDYT?

https://github.com/status-im/status-go/blob/a535aedad5d9e1468b1d9d133c30cbbc13c91144/protocol/messenger_communities.go#L4499-L4503

pavloburykh commented 3 months ago

@pavloburykh - I removed the non-operable accounts

Thank you for such a quick fix @smohamedjavid.

ilmotta commented 3 months ago

Thank you @smohamedjavid.

It's best we use IsWalletAccountReadyForTransaction instead of IsWalletNonWatchOnlyAccount as it also checks for the operable state of the account. WDYT?

I don't know what's best, but your suggestion makes sense @smohamedjavid. Although I'm just a bit confused from the comment in status-go saying that IsWalletNonWatchOnlyAccount Returns true if an account is a wallet account that logged in user has a control over, otherwise returns false., which would imply that if the user has control over the account it's good to be shared while joining.

Based on your suggestion, we will also need to apply a few changes on the client, because we're using status-im.contexts.communities.utils/sorted-non-watch-only-accounts in a few places.

smohamedjavid commented 3 months ago

Although I'm just a bit confused from the comment in status-go saying that IsWalletNonWatchOnlyAccount Returns true if an account is a wallet account that logged in user has a control over, otherwise returns false., which would imply that if the user has control over the account it's good to be shared while joining.

@ilmotta - The comment is partially correct. Fundamentally, any wallet account which is not a watched address can be controlled by the user.

On the other hand, when the user restores a profile from a backup with one or more key pairs (apart from the profile key pair), all those generated wallet accounts will be in the non-operable state, as the seed phrase or private key is missing for those key pairs. Technically, the user has NO control over it. He/She can't perform any transaction from it. The user has to key in the seed phrase or private key of the key pair to make it operable and have full control over it.

Hence, I suggested the IsWalletAccountReadyForTransaction method as it checks for the operable state of the account as well.

Based on your suggestion, we will also need to apply a few changes on the client, because we're using status-im.contexts.communities.utils/sorted-non-watch-only-accounts in a few places.

No worries, I already applied it 👍