Rifeljm/#622 walletconnect bypass readonly commands #2055

rifeljm · 2023-08-31T16:08:54Z

No description provided.

paninaro · 2023-09-05T21:40:36Z

packages/gui/src/components/walletConnect/WalletConnectAddConnectionDialog.tsx

+        <Typography variant="body2">
+          <Trans>Bypass confirmation for all read-only commands</Trans>
+        </Typography>


Tweak wording and add an informative tooltip on hover.

⚠️ NOTE: TooltipIcon needs to be imported from @chia-network/core ⚠️

Suggested change

<Typography variant="body2">

<Trans>Bypass confirmation for all read-only commands</Trans>

</Typography>

<Flex flexDirection="row" alignItems="center" gap={1}>

<Typography variant="body2">

<Trans>Skip confirmation for all read-only commands</Trans>

</Typography>

<TooltipIcon>

<Trans>

By default, a prompt will be presented each time a WalletConnect command is invoked. Select this option if

you would like to skip the prompt for all read-only WalletConnect commands.

<p />

Commands that create a transaction or otherwise modify your wallet will still require confirmation.

</Trans>

</TooltipIcon>

</Flex>

</Flex>

packages/gui/src/components/settings/SettingsIntegration.tsx

paninaro · 2023-09-05T21:52:57Z

Can we move the new "Skip Confirmation For All Readonly Commands" into the same box as "Skip Confirmation for Commands"?
Let's be consistent with the "readonly"/"read-only" wording. In the pairing dialog we use "read-only" but here it's "readonly". Let's go with "read-only".
If "Always Skip" is selected, we should hide the commands that are listed under the "Skip Confirmation for Commands" section.

paninaro · 2023-09-05T21:56:19Z

The new bypass-readonly-commands localStorage item needs to be cleaned up properly when Disconnect, Restore, or Reset buttons are pressed:

⚠️ NOTE: The Disconnect and Restore buttons should only remove the selected dapp's topic from bypass-readonly-commands. The Reset button should completely delete the localStorage item. ⚠️

paninaro · 2023-09-05T22:00:52Z

packages/gui/src/components/settings/SettingsIntegration.tsx

@@ -45,6 +46,9 @@ export default function SettingsIntegration() {
    useWalletConnectPairs();

  const pairs = get();
+  const { data: fingerprint } = useGetLoggedInFingerprintQuery();
+
+  const [bypassReadonlyCommands, setBypassReadonlyCommands] = useLocalStorage<any>('bypass-readonly-commands', {});


Is there a reason not to use the useWalletConnectPairs or useWalletConnectPreferences hooks? Creating a new localStorage item is going to make maintenance more complex. At the very least we should make the naming of these localStorage items consistent -- maybe walletConnectBypassReadonlyCommands. But I would prefer using the existing hooks if possible.

paninaro · 2023-09-05T22:01:56Z

packages/gui/src/components/settings/SettingsIntegration.tsx

@@ -334,6 +341,31 @@ export default function SettingsIntegration() {
                </Box>
              )}
            </Flex>
+            <Box {...borderStyle}>
+              <Typography variant="h6">
+                <Trans>Skip Confirmation For All Readonly Commands</Trans>


"Read-only" for consistency

paninaro · 2023-09-05T22:03:19Z

packages/gui/src/components/settings/SettingsIntegration.tsx

+                      }}
+                    >
+                      <MenuItem value>
+                        <Trans>Always Skip</Trans>


What do you think about using "Always Allow" instead of "Always Skip"?

Suggested change

<Trans>Always Skip</Trans>

<Trans>Always Allow</Trans>

paninaro · 2023-09-05T22:25:40Z

packages/gui/src/components/walletConnect/WalletConnectAddConnectionDialog.tsx

+      let skipReadOnlyObject = {};
+      selectedFingerprints.forEach((f: number) => {
+        skipReadOnlyObject = { ...skipReadOnlyObject, [f]: true };
+      });
+      setBypassReadonlyCommands({ ...bypassReadonlyCommands, [topic.toString()]: skipReadOnlyObject });


It seems odd to create a nested data structure to hold a boolean. The data structure could be simplified to:

{ <topic_1>: [fingerprint_1, fingerprint_2], <topic_2>: [fingerprint_3] }

If the fingerprint is in the list, bypass confirmation is enabled. If it's not, then confirmation is required.

paninaro · 2023-09-05T22:26:54Z

packages/gui/src/hooks/useWalletConnectCommand.tsx

-      bypassConfirm,
-      onChange: handleChangeParam,
-    });
+    const isReadOnlyEnabled = bypassReadonlyCommands?.[pairedTopic]?.[fingerprint];


Can we rename this to something like skipConfirmation?

paninaro

Some edge cases and UX refinement to be ironed-out.

…ommands

paninaro · 2023-09-08T19:40:15Z

I'm seeing issues with the latest changes. Setting the checkbox to bypass readonly commands during pairing doesn't seem to take effect -- the walletConnectPreferences localStorage item isn't updated.

…ommands

github-actions · 2023-11-11T11:03:34Z

'This PR has been flagged as stale due to no activity for over 60
days. It will not be automatically closed, but it has been given
a stale-pr label and should be manually reviewed.'

Added "quiet mode" option when creating a WalletConnect pairing

7fe7391

rifeljm requested a review from paninaro August 31, 2023 16:08

"Skip Confirmation For All Readonly Commands" into Settings

7e8243e

rifeljm force-pushed the rifeljm/#622-walletconnect-bypass-readonly-commands branch from d9f0824 to 7e8243e Compare August 31, 2023 16:44

paninaro reviewed Sep 5, 2023

View reviewed changes

packages/gui/src/components/settings/SettingsIntegration.tsx Outdated Show resolved Hide resolved

paninaro reviewed Sep 5, 2023

View reviewed changes

paninaro suggested changes Sep 5, 2023

View reviewed changes

rifeljm added 2 commits September 8, 2023 12:32

wip

4e33099

Merge branch 'main' into rifeljm/#622-walletconnect-bypass-readonly-c…

32cabb7

…ommands

rifeljm and others added 4 commits September 11, 2023 16:47

Fixing bugs

cb438b6

bugfix

4f5ab53

Merge branch 'main' into rifeljm/#622-walletconnect-bypass-readonly-c…

d50221a

…ommands

Fix setting WC command override to "Require Confirmation"

6963175

github-actions bot added the stale-pr label Nov 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rifeljm/#622 walletconnect bypass readonly commands #2055

Rifeljm/#622 walletconnect bypass readonly commands #2055

rifeljm commented Aug 31, 2023

paninaro Sep 5, 2023

paninaro commented Sep 5, 2023

paninaro commented Sep 5, 2023

paninaro Sep 5, 2023

paninaro Sep 5, 2023 •

edited

Loading

paninaro Sep 5, 2023 •

edited

Loading

paninaro Sep 5, 2023

paninaro Sep 5, 2023

paninaro left a comment

paninaro commented Sep 8, 2023

github-actions bot commented Nov 11, 2023

-        <Typography variant="body2">
-          <Trans>Bypass confirmation for all read-only commands</Trans>
-        </Typography>
+        <Flex flexDirection="row" alignItems="center" gap={1}>
+          <Typography variant="body2">
+            <Trans>Skip confirmation for all read-only commands</Trans>
+          </Typography>
+          <TooltipIcon>
+            <Trans>
+              By default, a prompt will be presented each time a WalletConnect command is invoked. Select this option if
+              you would like to skip the prompt for all read-only WalletConnect commands.
+              <p />
+              Commands that create a transaction or otherwise modify your wallet will still require confirmation.
+            </Trans>
+          </TooltipIcon>
+        </Flex>
+      </Flex>

Rifeljm/#622 walletconnect bypass readonly commands #2055

Are you sure you want to change the base?

Rifeljm/#622 walletconnect bypass readonly commands #2055

Conversation

rifeljm commented Aug 31, 2023

paninaro Sep 5, 2023

Choose a reason for hiding this comment

paninaro commented Sep 5, 2023

paninaro commented Sep 5, 2023

paninaro Sep 5, 2023

Choose a reason for hiding this comment

paninaro Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

paninaro Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

paninaro Sep 5, 2023

Choose a reason for hiding this comment

paninaro Sep 5, 2023

Choose a reason for hiding this comment

paninaro left a comment

Choose a reason for hiding this comment

paninaro commented Sep 8, 2023

github-actions bot commented Nov 11, 2023

paninaro Sep 5, 2023 •

edited

Loading

paninaro Sep 5, 2023 •

edited

Loading