safe-wallet-web icon indicating copy to clipboard operation
safe-wallet-web copied to clipboard

Refactor `CheckWallet` to split up conditions

Open usame-algan opened this issue 5 months ago • 1 comments

What is the feature about

The CheckWallet component is a critical component that wraps almost every button in the app to decide whether that button should be enabled or not. It checks for several conditions like the user wallet, the safe setup and multiple permissions like delegates and spending limits. This component grew over time, has a high complexity and is hard to extend and maintain.

In some cases we have buttons where we only want to ensure that any wallet is connected. In order to achieve this we have additional flags that are passed to CheckWallet like allowNonOwner

The list of requirements

I suggest to split the condition up into multiple hooks that are composed and conditionally used inside CheckWallet e.g.

const CheckWallet = ({
  checkWalletConnection = true,
  checkSafeStatus = true,
  checkPermissions = true,
}) => {
  const walletMessage = useWalletCheck(checkWalletConnection)
  const safeStatusMessage = useSafeStatusCheck(checkSafeStatus)
  const permissionMessage = usePermissionCheck(checkPermissions)

  const message = walletMessage || safeStatusMessage || permissionMessage

  const isDisabled = Boolean(message)

  return ...
}

With the above and wanting to skip permission and safe status check it would look like this:

<CheckWallet checkSafeStatus={false} checkPermissions={false}>...</CheckWallet>

Furthermore, having to change something about permissions would only touch a single hook instead of the whole component where the rest of the logic lies.

Designs/sketches

Current conditions Screenshot 2024-09-19 at 13 49 09

usame-algan avatar Sep 19 '24 11:09 usame-algan