From 2b06f0b65de492b87900fbd273e2df9814b2f1e8 Mon Sep 17 00:00:00 2001 From: Ryan Berckmans Date: Mon, 15 Jul 2024 14:37:45 -0500 Subject: [PATCH] Disable suggestAutoExecute and autoClickIfNeverClicked suggestAutoExecute and autoClickIfNeverClicked are two transactions features that help ensure the user only has to click the button once, regardless of any needed EIP-712 signatures or network additions or switches. These features offer a delightful UX where almost all users only have to click the button once. However, these features have a critical bug since the recent upgrade to wagmi v2 where unexpected ExecuteTokenTransfer renders can cause execute() to be called twice in a row, leading to the user being prompted to sign duplicate payment transactions (and duplicate payment if they sign both). This disables suggestAutoExecute and autoClickIfNeverClicked until they can be rebuilt safely. --- packages/interface/src/transactions.tsx | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/packages/interface/src/transactions.tsx b/packages/interface/src/transactions.tsx index 0e2fb56..ffe9aad 100644 --- a/packages/interface/src/transactions.tsx +++ b/packages/interface/src/transactions.tsx @@ -147,7 +147,7 @@ type ExecuteTokenTransferButtonUIProps = Pick = ({ tt, onClickPassthrough, autoReset, autoClickIfNeverClicked, loadForeverOnTransactionFeeUnaffordableError, label, successLabel, warningLabel, errorLabel, disabled, showLoadingSpinnerWhenDisabled, className, disabledClassName, enabledClassName, errorClassName, warningClassName, loadingSpinnerClassName, observer, isExecuteTokenTransferRendered, }) => { +const ExecuteTokenTransferButtonUI: React.FC = ({ tt, onClickPassthrough, autoReset, loadForeverOnTransactionFeeUnaffordableError, label, successLabel, warningLabel, errorLabel, disabled, showLoadingSpinnerWhenDisabled, className, disabledClassName, enabledClassName, errorClassName, warningClassName, loadingSpinnerClassName, observer, isExecuteTokenTransferRendered, }) => { const { connector: activeConnector } = useAccount(); const statusFromObserver: ExecuteTokenTransferStatus | undefined = useObservedValue(observer); const status = (isExecuteTokenTransferRendered && statusFromObserver) || defaultExecuteTokenTransferStatus; // if the underlying ExecuteTokenTransfer is not currently being rendered, then any statusFromObserver is by definition stale as it's from an unmounted ExecuteTokenTransfer, so we mustn't use it. NB when ExecuteTokenTransfer is unmounted it sends a final undefined status, but statusFromObserver is updated async based on effect timers, whereas isExecuteTokenTransferRendered is computed synchronously on each render, so when ExecuteTokenTransfer is unmounted, isExecuteTokenTransferRendered flips to false before statusFromObserver becomes the final undefined status @@ -155,15 +155,17 @@ const ExecuteTokenTransferButtonUI: React.FC const connectedWalletAutoSigns: boolean = activeConnector ? activeConnector.id.includes("web3auth") : false; // true iff the connected wallet does not ask the user to sign the transaction and instead signs it on their behalf. NB we don't actually use web3auth anymore and it was entirely removed from the codebase, but we retained this as an example of how to handle a wallet that auto-signs transactions - useEffect(() => { - if (!onClickPassthrough && status.suggestAutoExecute) status.execute(); // NB here we never auto execute if onClickPassthrough is defined because passthrough clicks are intended to supercede the button's internal functionality. Eg. if onClickPassthrough was defined to support some client feature, it would be jarring and potentially inconsistent to begin auto executing a token transfer - }, [onClickPassthrough, status]); - - const se = status.execute; // use a local so that the useEffect dependency isn't on the entire object - const seclo = status.executeCalledAtLeastOnce; // use a local so that the useEffect dependency isn't on the entire object - useEffect(() => { // handle autoClickIfNeverClicked - if (!onClickPassthrough && autoClickIfNeverClicked && se && !seclo) se(); // NB here we never auto execute if onClickPassthrough is defined because passthrough clicks are intended to supercede the button's internal functionality. Eg. if onClickPassthrough was defined to support some client feature, it would be jarring and potentially inconsistent to begin auto executing a token transfer - }, [onClickPassthrough, autoClickIfNeverClicked, se, seclo]); + // WARNING this commented-out handler implements automatic execution for suggestAutoExecute. Howver, after the upgrade to wagmi v2, it seems that ExecuteTokenTransfer may rerender "sooner" than expected due to hook internals and produce a next status before suggestAutoExecute is properly unset, leading to this effect handler running twice in a row and duplicate execution, causing the transaction to potentially be signed twice by the user and duplicate payment. TODO rebuild suggestAutoExecute control flow to ensure this can't happen and then re-enable the feature. + // useEffect(() => { + // if (!onClickPassthrough && status.suggestAutoExecute) status.execute(); // NB here we never auto execute if onClickPassthrough is defined because passthrough clicks are intended to supercede the button's internal functionality. Eg. if onClickPassthrough was defined to support some client feature, it would be jarring and potentially inconsistent to begin auto executing a token transfer + // }, [onClickPassthrough, status]); + + // WARNING this commented-out handler implements automatic execution for autoClickIfNeverClicked. Howver, after the upgrade to wagmi v2, it seems that ExecuteTokenTransfer may rerender "sooner" than expected due to hook internals and produce a next status before executeCalledAtLeastOnce is properly set, leading to this effect handler running twice in a row and duplicate execution, causing the transaction to potentially be signed twice by the user and duplicate payment. TODO rebuild autoClickIfNeverClicked control flow to ensure this can't happen and then re-enable the feature. + // const se = status.execute; // use a local so that the useEffect dependency isn't on the entire object + // const seclo = status.executeCalledAtLeastOnce; // use a local so that the useEffect dependency isn't on the entire object + // useEffect(() => { // handle autoClickIfNeverClicked + // if (!onClickPassthrough && autoClickIfNeverClicked && se && !seclo) se(); // NB here we never auto execute if onClickPassthrough is defined because passthrough clicks are intended to supercede the button's internal functionality. Eg. if onClickPassthrough was defined to support some client feature, it would be jarring and potentially inconsistent to begin auto executing a token transfer + // }, [onClickPassthrough, autoClickIfNeverClicked, se, seclo]); // @eslint-no-use-below[se] // @eslint-no-use-below[seclo]