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

Rifeljm/#622 walletconnect bypass readonly commands #2055

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
32 changes: 32 additions & 0 deletions packages/gui/src/components/settings/SettingsIntegration.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { useLocalStorage, useGetLoggedInFingerprintQuery } from '@chia-network/api-react';
import { Flex, MenuItem, SettingsHR, SettingsSection, SettingsTitle, SettingsText } from '@chia-network/core';
import { t, Trans } from '@lingui/macro';
import {
Expand Down Expand Up @@ -45,6 +46,9 @@ export default function SettingsIntegration() {
useWalletConnectPairs();

const pairs = get();
const { data: fingerprint } = useGetLoggedInFingerprintQuery();

const [bypassReadonlyCommands, setBypassReadonlyCommands] = useLocalStorage<any>('bypass-readonly-commands', {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const refreshBypassCommands = React.useCallback(() => {
if (selectedPair.current) {
Expand Down Expand Up @@ -131,6 +135,9 @@ export default function SettingsIntegration() {
}
}, [topic, pairs, refreshBypassCommands]);

const readonlySkipValue =
bypassReadonlyCommands && topic && fingerprint ? bypassReadonlyCommands[topic][fingerprint] : false;
paninaro marked this conversation as resolved.
Show resolved Hide resolved

return (
<Grid container style={{ maxWidth: '624px' }} gap={3}>
<Grid item>
Expand Down Expand Up @@ -334,6 +341,31 @@ export default function SettingsIntegration() {
</Box>
)}
</Flex>
<Box {...borderStyle}>
<Typography variant="h6">
<Trans>Skip Confirmation For All Readonly Commands</Trans>
Copy link
Contributor

@paninaro paninaro Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Read-only" for consistency

<Grid item container xs marginTop="20px" marginRight="8px">
<FormControl size="small">
<Select
value={readonlySkipValue}
onChange={(evt: PointerEvent) => {
setBypassReadonlyCommands((localBypassReadonlyCommands) => ({
...localBypassReadonlyCommands,
[topic]: { [fingerprint]: evt.target.value },
}));
}}
>
<MenuItem value>
<Trans>Always Skip</Trans>
Copy link
Contributor

@paninaro paninaro Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
<Trans>Always Skip</Trans>
<Trans>Always Allow</Trans>

</MenuItem>
<MenuItem value={false} onClick={() => {}}>
<Trans>Require Confirmation</Trans>
</MenuItem>
</Select>
</FormControl>
</Grid>
</Typography>
</Box>

{topic && commands.length > 0 && (
<>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useGetKeysQuery, useGetLoggedInFingerprintQuery, usePrefs } from '@chia-network/api-react';
import { useGetKeysQuery, useGetLoggedInFingerprintQuery, usePrefs, useLocalStorage } from '@chia-network/api-react';
import {
ButtonLoading,
DialogActions,
Expand Down Expand Up @@ -51,6 +51,8 @@ export default function WalletConnectAddConnectionDialog(props: WalletConnectAdd
const { onClose = () => {}, open = false } = props;

const [step, setStep] = useState<Step>(Step.CONNECT);
const [bypassReadonlyCommands, setBypassReadonlyCommands] = useLocalStorage<any>('bypass-readonly-commands', {});
const [bypassCheckbox, toggleBypassCheckbox] = useState(false);
const { pair, isLoading: isLoadingWallet } = useWalletConnectContext();
const { data: keys, isLoading: isLoadingPublicKeys } = useGetKeysQuery({});
const [sortedWallets] = usePrefs('sortedWallets', []);
Expand Down Expand Up @@ -118,6 +120,13 @@ export default function WalletConnectAddConnectionDialog(props: WalletConnectAdd
}

const topic = await pair(uri, selectedFingerprints, mainnet);
if (bypassCheckbox) {
let skipReadOnlyObject = {};
selectedFingerprints.forEach((f: number) => {
skipReadOnlyObject = { ...skipReadOnlyObject, [f]: true };
});
setBypassReadonlyCommands({ ...bypassReadonlyCommands, [topic.toString()]: skipReadOnlyObject });
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

onClose(topic);
}

Expand Down Expand Up @@ -212,6 +221,23 @@ export default function WalletConnectAddConnectionDialog(props: WalletConnectAdd
return null;
}

function renderQuietModeOption() {
return (
<Flex
sx={{ cursor: 'pointer' }}
alignItems="center"
onClick={() => {
toggleBypassCheckbox(!bypassCheckbox);
}}
>
<Checkbox checked={bypassCheckbox} disableRipple sx={{ paddingLeft: 0 }} />
<Typography variant="body2">
<Trans>Bypass confirmation for all read-only commands</Trans>
</Typography>
</Flex>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

);
}

return (
<Dialog onClose={handleClose} maxWidth="xs" open={open} fullWidth>
<DialogTitle>
Expand Down Expand Up @@ -259,6 +285,7 @@ export default function WalletConnectAddConnectionDialog(props: WalletConnectAdd
{renderSelectedAsPills()}
</Flex>
{renderKeysMultiSelect()}
{renderQuietModeOption()}
</Flex>
)}
</Flex>
Expand Down
47 changes: 27 additions & 20 deletions packages/gui/src/hooks/useWalletConnectCommand.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import api, { store, useGetLoggedInFingerprintQuery } from '@chia-network/api-react';
import api, { store, useGetLoggedInFingerprintQuery, useLocalStorage } from '@chia-network/api-react';
import { useOpenDialog, useAuth } from '@chia-network/core';
import { Trans } from '@lingui/macro';
import debug from 'debug';
Expand Down Expand Up @@ -80,6 +80,8 @@ export default function useWalletConnectCommand(options: UseWalletConnectCommand

const isLoading = isLoadingLoggedInFingerprint;

const [bypassReadonlyCommands] = useLocalStorage<any>('bypass-readonly-commands', {});

async function confirm(props: {
topic: string;
message: ReactNode;
Expand Down Expand Up @@ -139,8 +141,9 @@ export default function useWalletConnectCommand(options: UseWalletConnectCommand

const { fingerprint } = requestedParams;

const pair = getPairBySession(topic);
const pairedTopic = pair?.topic!;
if (command === 'showNotification') {
const pair = getPairBySession(topic);
if (!pair) {
throw new Error('Invalid session topic');
}
Expand Down Expand Up @@ -172,24 +175,28 @@ export default function useWalletConnectCommand(options: UseWalletConnectCommand
values = newValues;
}

const confirmed = await confirm({
topic,
message:
!allFingerprints && isDifferentFingerprint ? (
<Trans>
Do you want to log in to {fingerprint} and execute command {command}?
</Trans>
) : (
<Trans>Do you want to execute command {command}?</Trans>
),
params: definitionParams,
values,
fingerprint,
isDifferentFingerprint,
command,
bypassConfirm,
onChange: handleChangeParam,
});
const isReadOnlyEnabled = bypassReadonlyCommands?.[pairedTopic]?.[fingerprint];

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename this to something like skipConfirmation?

const confirmed =
(isReadOnlyEnabled && definition.bypassConfirm) ||
(await confirm({
topic,
message:
!allFingerprints && isDifferentFingerprint ? (
<Trans>
Do you want to log in to {fingerprint} and execute command {command}?
</Trans>
) : (
<Trans>Do you want to execute command {command}?</Trans>
),
params: definitionParams,
values,
fingerprint,
isDifferentFingerprint,
command,
bypassConfirm,
onChange: handleChangeParam,
}));

if (!confirmed) {
throw new Error(`User cancelled command ${requestedCommand}`);
Expand Down
Loading