From ce1b6ac81f2e5b7d8edcc5dbc48d5ca4c5ea0289 Mon Sep 17 00:00:00 2001 From: theborakompanioni Date: Mon, 11 Sep 2023 11:12:31 +0200 Subject: [PATCH 1/5] fix(import): apply upper limit to gaplimit value on import --- src/components/ImportWallet.tsx | 31 +++++++++++++++++++++++----- src/i18n/locales/en/translation.json | 2 +- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/components/ImportWallet.tsx b/src/components/ImportWallet.tsx index 2dd571f15..759aca017 100644 --- a/src/components/ImportWallet.tsx +++ b/src/components/ImportWallet.tsx @@ -16,7 +16,13 @@ import PreventLeavingPageByMistake from './PreventLeavingPageByMistake' import { WalletInfo, WalletInfoSummary } from './WalletCreationConfirmation' import { isDevMode, isDebugFeatureEnabled } from '../constants/debugFeatures' import { routes, Route } from '../constants/routes' -import { SEGWIT_ACTIVATION_BLOCK, DUMMY_MNEMONIC_PHRASE, JM_WALLET_FILE_EXTENSION, walletDisplayName } from '../utils' +import { + SEGWIT_ACTIVATION_BLOCK, + DUMMY_MNEMONIC_PHRASE, + JM_WALLET_FILE_EXTENSION, + walletDisplayName, + isValidNumber, +} from '../utils' import { JM_GAPLIMIT_DEFAULT, JM_GAPLIMIT_CONFIGKEY } from '../constants/config' type ImportWalletDetailsFormValues = { @@ -32,6 +38,15 @@ const GAPLIMIT_SUGGESTIONS = { const MIN_BLOCKHEIGHT_VALUE = 0 const MIN_GAPLIMIT_VALUE = 1 +/** + * Maximum gaplimit value for importing an existing wallet. + * This value represents an upper limit based on declining performance of JM when many + * addresses have to be monitored. On network `regtest`, importing 10_000 addresses in + * an empty wallet takes ~10min and requesting the `/display` endpoint takes another + * ~10min. At this point, JM becomes practically unusable. However, goal is to find a + * balance between usability and freedom of users to do what they are trying to do. + */ +const MAX_GAPLIMIT_VALUE = 10_000 const initialImportWalletDetailsFormValues: ImportWalletDetailsFormValues = isDevMode() ? { @@ -69,14 +84,19 @@ const ImportWalletDetailsForm = ({ errors.mnemonicPhrase = t('import_wallet.import_details.feedback_invalid_menmonic_phrase') } - if (typeof values.blockheight !== 'number' || values.blockheight < MIN_BLOCKHEIGHT_VALUE) { + if (!isValidNumber(values.blockheight) || values.blockheight < MIN_BLOCKHEIGHT_VALUE) { errors.blockheight = t('import_wallet.import_details.feedback_invalid_blockheight', { - min: MIN_BLOCKHEIGHT_VALUE, + min: MIN_BLOCKHEIGHT_VALUE.toLocaleString(), }) } - if (typeof values.gaplimit !== 'number' || values.gaplimit < MIN_GAPLIMIT_VALUE) { + if ( + !isValidNumber(values.gaplimit) || + values.gaplimit < MIN_GAPLIMIT_VALUE || + values.gaplimit > MAX_GAPLIMIT_VALUE + ) { errors.gaplimit = t('import_wallet.import_details.feedback_invalid_gaplimit', { - min: MIN_GAPLIMIT_VALUE, + min: MIN_GAPLIMIT_VALUE.toLocaleString(), + max: MAX_GAPLIMIT_VALUE.toLocaleString(), }) } return errors @@ -192,6 +212,7 @@ const ImportWalletDetailsForm = ({ isValid={touched.gaplimit && !errors.gaplimit} isInvalid={touched.gaplimit && !!errors.gaplimit} min={MIN_GAPLIMIT_VALUE} + max={MAX_GAPLIMIT_VALUE} step={1} required /> diff --git a/src/i18n/locales/en/translation.json b/src/i18n/locales/en/translation.json index 6f5a99605..b1b4b7f00 100644 --- a/src/i18n/locales/en/translation.json +++ b/src/i18n/locales/en/translation.json @@ -168,7 +168,7 @@ "feedback_invalid_blockheight": "Please provide a valid blockheight value greater than or equal to {{ min }}.", "label_gaplimit": "Address import limit", "description_gaplimit": "The amount of addresses that are imported per jar. Set to the highest address index used in any of the jars. Increase this number if your wallet is heavily used.", - "feedback_invalid_gaplimit": "Please provide a valid gaplimit value greater than or equal to {{ min }}.", + "feedback_invalid_gaplimit": "Please provide a valid gaplimit value between {{ min }} and {{ max }}.", "text_button_submit": "Review", "text_button_submitting": "Review" }, From 592bbffcc14a20a6ba0c0d1b3697cd2220a996c2 Mon Sep 17 00:00:00 2001 From: theborakompanioni Date: Mon, 11 Sep 2023 11:34:18 +0200 Subject: [PATCH 2/5] fix(import): apply upper limit to blockheight value on import --- src/components/ImportWallet.tsx | 17 ++++++++++++++++- src/i18n/locales/en/translation.json | 4 ++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/components/ImportWallet.tsx b/src/components/ImportWallet.tsx index 759aca017..00c36b671 100644 --- a/src/components/ImportWallet.tsx +++ b/src/components/ImportWallet.tsx @@ -37,6 +37,16 @@ const GAPLIMIT_SUGGESTIONS = { } const MIN_BLOCKHEIGHT_VALUE = 0 +/** + * Maximum blockheight value. + * Value choosen based on estimation of blockheight in tge year 2140 (plus some buffer): + * 365 × 144 × (2140 - 2009) = 6_885_360 = ~7_000_000 + * This is necessary because javascript does not handle large values too well, + * and the `/rescanblockchain` errors. Not to mention that a value beyond the current + * height does not make any sense in the first place. + */ +const MAX_BLOCKHEIGHT_VALUE = 10_000_000 + const MIN_GAPLIMIT_VALUE = 1 /** * Maximum gaplimit value for importing an existing wallet. @@ -84,7 +94,11 @@ const ImportWalletDetailsForm = ({ errors.mnemonicPhrase = t('import_wallet.import_details.feedback_invalid_menmonic_phrase') } - if (!isValidNumber(values.blockheight) || values.blockheight < MIN_BLOCKHEIGHT_VALUE) { + if ( + !isValidNumber(values.blockheight) || + values.blockheight < MIN_BLOCKHEIGHT_VALUE || + values.blockheight > MAX_BLOCKHEIGHT_VALUE + ) { errors.blockheight = t('import_wallet.import_details.feedback_invalid_blockheight', { min: MIN_BLOCKHEIGHT_VALUE.toLocaleString(), }) @@ -182,6 +196,7 @@ const ImportWalletDetailsForm = ({ isValid={touched.blockheight && !errors.blockheight} isInvalid={touched.blockheight && !!errors.blockheight} min={MIN_BLOCKHEIGHT_VALUE} + max={MAX_BLOCKHEIGHT_VALUE} step={1_000} required /> diff --git a/src/i18n/locales/en/translation.json b/src/i18n/locales/en/translation.json index b1b4b7f00..d22b8b77f 100644 --- a/src/i18n/locales/en/translation.json +++ b/src/i18n/locales/en/translation.json @@ -165,10 +165,10 @@ "import_options": "Import options", "label_blockheight": "Rescan height", "description_blockheight": "The blockheight at which the rescan process starts to search for your funds. The earlier the wallet has been created, the lower this value should be.", - "feedback_invalid_blockheight": "Please provide a valid blockheight value greater than or equal to {{ min }}.", + "feedback_invalid_blockheight": "Please provide a valid value between {{ min }} and the current blockheight.", "label_gaplimit": "Address import limit", "description_gaplimit": "The amount of addresses that are imported per jar. Set to the highest address index used in any of the jars. Increase this number if your wallet is heavily used.", - "feedback_invalid_gaplimit": "Please provide a valid gaplimit value between {{ min }} and {{ max }}.", + "feedback_invalid_gaplimit": "Please provide a valid value between {{ min }} and {{ max }}.", "text_button_submit": "Review", "text_button_submitting": "Review" }, From aa144f2875b35f9b952ddeacb32ead579656fade Mon Sep 17 00:00:00 2001 From: theborakompanioni Date: Mon, 11 Sep 2023 13:56:49 +0200 Subject: [PATCH 3/5] ui(import): display warning on high gaplimit values on import --- src/components/ImportWallet.tsx | 302 ++++++++++++++++----------- src/i18n/locales/en/translation.json | 1 + 2 files changed, 184 insertions(+), 119 deletions(-) diff --git a/src/components/ImportWallet.tsx b/src/components/ImportWallet.tsx index 00c36b671..9525097c7 100644 --- a/src/components/ImportWallet.tsx +++ b/src/components/ImportWallet.tsx @@ -57,6 +57,21 @@ const MIN_GAPLIMIT_VALUE = 1 * balance between usability and freedom of users to do what they are trying to do. */ const MAX_GAPLIMIT_VALUE = 10_000 +/** + * A gaplimit threshold at which a warning is displayed that with the given value a + * decline in performance is to be expected. Importing 500 addresses (per jar!) leads to + * the `/display` endpoint taking more than ~15s. + */ +const GAPLIMIT_WARN_THRESHOLD = 250 + +const GaplimitWarning = () => { + const { t } = useTranslation() + return ( + + {t('import_wallet.import_details.alert_high_gaplimit_value')} + + ) +} const initialImportWalletDetailsFormValues: ImportWalletDetailsFormValues = isDevMode() ? { @@ -130,129 +145,155 @@ const ImportWalletDetailsForm = ({ errors, isSubmitting, submitCount, - }) => ( - - setFieldValue('mnemonicPhrase', val, true)} - isDisabled={(_) => isSubmitting} - /> - {!!errors.mnemonicPhrase && ( - <> -
{ + const hasImportDetailsSectionErrors = !!errors.blockheight || !!errors.gaplimit + const showGaplimitWarning = !errors.gaplimit && values.gaplimit > GAPLIMIT_WARN_THRESHOLD + return ( + + setFieldValue('mnemonicPhrase', val, true)} + isDisabled={(_) => isSubmitting} + /> + {!!errors.mnemonicPhrase && ( + <> +
+ {errors.mnemonicPhrase} +
+ + )} + {__dev_showFillerButton && ( + setFieldValue('mnemonicPhrase', DUMMY_MNEMONIC_PHRASE, true)} + disabled={isSubmitting} > - {errors.mnemonicPhrase} -
- - )} - {__dev_showFillerButton && ( - setFieldValue('mnemonicPhrase', DUMMY_MNEMONIC_PHRASE, true)} - disabled={isSubmitting} + Fill with dummy mnemonic phrase + + dev + + + )} + + {(hasImportDetailsSectionErrors || showGaplimitWarning) && ( +
+ +
+ )} + {t('import_wallet.import_details.import_options')} + + } + defaultOpen={true} > - Fill with dummy mnemonic phrase - - dev - - - )} - - {t('import_wallet.import_details.import_options')} - - } - defaultOpen={true} - > - - {t('import_wallet.import_details.label_blockheight')} - - {t('import_wallet.import_details.description_blockheight')} - - - - - - - {errors.blockheight} - - - - {t('import_wallet.import_details.label_gaplimit')} - - - {t('import_wallet.import_details.description_gaplimit')} - - - - - - - {errors.gaplimit} - - - - -
- {isSubmitting && ( -
-
-
-
-
- )} +
+ +
+ + ) + }} ) } @@ -288,6 +329,8 @@ const ImportWalletConfirmation = ({ [walletDetails, importDetails], ) + const showGaplimitWarning = useMemo(() => importDetails.gaplimit > GAPLIMIT_WARN_THRESHOLD, [importDetails]) + return ( - + + {showGaplimitWarning && ( +
+ +
+ )} + {t('import_wallet.import_details.import_options')} + + } + >
{t('import_wallet.import_details.label_blockheight')}
{t('import_wallet.import_details.description_blockheight')}
@@ -310,6 +368,12 @@ const ImportWalletConfirmation = ({
{t('import_wallet.import_details.label_gaplimit')}
{t('import_wallet.import_details.description_gaplimit')}
{values.importDetails.gaplimit}
+ + {showGaplimitWarning && ( + + {t('import_wallet.import_details.alert_high_gaplimit_value')} + + )}
diff --git a/src/i18n/locales/en/translation.json b/src/i18n/locales/en/translation.json index d22b8b77f..1efc5292d 100644 --- a/src/i18n/locales/en/translation.json +++ b/src/i18n/locales/en/translation.json @@ -169,6 +169,7 @@ "label_gaplimit": "Address import limit", "description_gaplimit": "The amount of addresses that are imported per jar. Set to the highest address index used in any of the jars. Increase this number if your wallet is heavily used.", "feedback_invalid_gaplimit": "Please provide a valid value between {{ min }} and {{ max }}.", + "alert_high_gaplimit_value": "The given value results in many addresses being imported and an extreme decline in performance and responsiveness is to be expected.", "text_button_submit": "Review", "text_button_submitting": "Review" }, From 85a3b51b6252d623f00930e41cfa108359d1579a Mon Sep 17 00:00:00 2001 From: theborakompanioni Date: Mon, 11 Sep 2023 14:13:26 +0200 Subject: [PATCH 4/5] refactor(ui): add variant to Accordion component --- src/components/Accordion.tsx | 34 +++++++++++++++++++++--- src/components/ImportWallet.tsx | 46 +++------------------------------ 2 files changed, 34 insertions(+), 46 deletions(-) diff --git a/src/components/Accordion.tsx b/src/components/Accordion.tsx index 4da9a7327..5e5ede36a 100644 --- a/src/components/Accordion.tsx +++ b/src/components/Accordion.tsx @@ -1,15 +1,23 @@ -import React, { PropsWithChildren, useState } from 'react' +import { ReactNode, PropsWithChildren, useState } from 'react' +import classNames from 'classnames' import { useSettings } from '../context/SettingsContext' import * as rb from 'react-bootstrap' import Sprite from './Sprite' interface AccordionProps { - title: string | React.ReactNode + title: ReactNode | string defaultOpen?: boolean disabled?: boolean + variant?: 'warning' | 'danger' } -const Accordion = ({ title, defaultOpen = false, disabled = false, children }: PropsWithChildren) => { +const Accordion = ({ + title, + defaultOpen = false, + disabled = false, + variant, + children, +}: PropsWithChildren) => { const settings = useSettings() const [isOpen, setIsOpen] = useState(defaultOpen) @@ -21,7 +29,25 @@ const Accordion = ({ title, defaultOpen = false, disabled = false, children }: P onClick={() => setIsOpen((current) => !current)} disabled={disabled} > - {title} +
+ {variant && ( +
+ +
+ )} + {title} +

diff --git a/src/components/ImportWallet.tsx b/src/components/ImportWallet.tsx index 9525097c7..04751c1d3 100644 --- a/src/components/ImportWallet.tsx +++ b/src/components/ImportWallet.tsx @@ -64,15 +64,6 @@ const MAX_GAPLIMIT_VALUE = 10_000 */ const GAPLIMIT_WARN_THRESHOLD = 250 -const GaplimitWarning = () => { - const { t } = useTranslation() - return ( - - {t('import_wallet.import_details.alert_high_gaplimit_value')} - - ) -} - const initialImportWalletDetailsFormValues: ImportWalletDetailsFormValues = isDevMode() ? { mnemonicPhrase: new Array(12).fill(''), @@ -180,25 +171,8 @@ const ImportWalletDetailsForm = ({ )} - {(hasImportDetailsSectionErrors || showGaplimitWarning) && ( -
- -
- )} - {t('import_wallet.import_details.import_options')} - - } + title={t('import_wallet.import_details.import_options')} + variant={hasImportDetailsSectionErrors ? 'danger' : showGaplimitWarning ? 'warning' : undefined} defaultOpen={true} > @@ -344,20 +318,8 @@ const ImportWalletConfirmation = ({ - {showGaplimitWarning && ( -
- -
- )} - {t('import_wallet.import_details.import_options')} - - } + title={t('import_wallet.import_details.import_options')} + variant={showGaplimitWarning ? 'warning' : undefined} >
{t('import_wallet.import_details.label_blockheight')}
From a39d78db187bb798ab7690a17ea1efc81e71aef6 Mon Sep 17 00:00:00 2001 From: theborakompanioni Date: Tue, 12 Sep 2023 11:33:29 +0200 Subject: [PATCH 5/5] chore(i18n): adapt gaplimit warning --- src/i18n/locales/en/translation.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/i18n/locales/en/translation.json b/src/i18n/locales/en/translation.json index 1efc5292d..2681dbb86 100644 --- a/src/i18n/locales/en/translation.json +++ b/src/i18n/locales/en/translation.json @@ -169,7 +169,7 @@ "label_gaplimit": "Address import limit", "description_gaplimit": "The amount of addresses that are imported per jar. Set to the highest address index used in any of the jars. Increase this number if your wallet is heavily used.", "feedback_invalid_gaplimit": "Please provide a valid value between {{ min }} and {{ max }}.", - "alert_high_gaplimit_value": "The given value results in many addresses being imported and an extreme decline in performance and responsiveness is to be expected.", + "alert_high_gaplimit_value": "The given value causes many addresses to be imported, which can lead to a decline in performance and responsiveness.", "text_button_submit": "Review", "text_button_submitting": "Review" },