Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor CheckWallet to split up conditions #4210

Open
usame-algan opened this issue Sep 19, 2024 · 1 comment
Open

Refactor CheckWallet to split up conditions #4210

usame-algan opened this issue Sep 19, 2024 · 1 comment

Comments

@usame-algan
Copy link
Member

usame-algan commented Sep 19, 2024

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 usame-algan changed the title Refactor CheckWallet to simplify conditions Refactor CheckWallet to split up conditions Sep 19, 2024
@usame-algan
Copy link
Member Author

Another idea: If we can determine that there are a fix amount of variants we could also split up CheckWallet into multiple components like CheckWalletConnected etc. and use those more specific components instead of one big one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: New issues
Development

No branches or pull requests

1 participant