From 394a9d188da5a6bc8e2ffdd80121cb18471b3f08 Mon Sep 17 00:00:00 2001 From: Francisco Ramos Date: Wed, 19 Apr 2023 15:35:03 +0200 Subject: [PATCH] fix(bridge-ui): pending transactions custom store with better error handling (#13581) Co-authored-by: jeff <113397187+cyberhorsey@users.noreply.github.com> --- packages/bridge-ui/src/App.svelte | 24 +------ .../Transactions/Transaction.svelte | 21 +++--- .../src/components/form/BridgeForm.svelte | 29 ++++---- .../src/components/modals/FaucetModal.svelte | 12 ++-- .../bridge-ui/src/store/transactions.spec.ts | 67 +++++++++++++++++ packages/bridge-ui/src/store/transactions.ts | 71 +++++++++++++++++-- packages/bridge-ui/src/utils/Deferred.spec.ts | 25 +++++++ packages/bridge-ui/src/utils/Deferred.ts | 12 ++++ 8 files changed, 203 insertions(+), 58 deletions(-) create mode 100644 packages/bridge-ui/src/store/transactions.spec.ts create mode 100644 packages/bridge-ui/src/utils/Deferred.spec.ts create mode 100644 packages/bridge-ui/src/utils/Deferred.ts diff --git a/packages/bridge-ui/src/App.svelte b/packages/bridge-ui/src/App.svelte index f420d4c672..09b2a91dd9 100644 --- a/packages/bridge-ui/src/App.svelte +++ b/packages/bridge-ui/src/App.svelte @@ -8,11 +8,7 @@ import { MetaMaskConnector } from '@wagmi/core/connectors/metaMask'; import { setupI18n } from './i18n'; - import { - pendingTransactions, - transactioner, - transactions, - } from './store/transactions'; + import { transactioner, transactions } from './store/transactions'; import Navbar from './components/Navbar.svelte'; import Toast, { successToast } from './components/Toast.svelte'; import { signer } from './store/signer'; @@ -130,24 +126,6 @@ } }); - pendingTransactions.subscribe((store) => { - (async () => { - const confirmedPendingTxIndex = await Promise.race( - store.map((tx, index) => { - return new Promise((resolve) => { - $signer.provider - .waitForTransaction(tx.hash, 1) - .then(() => resolve(index)); - }); - }), - ); - successToast('Transaction completed!'); - let s = store; - s.splice(confirmedPendingTxIndex, 1); - pendingTransactions.set(s); - })(); - }); - const transactionToIntervalMap = new Map(); transactions.subscribe((store) => { diff --git a/packages/bridge-ui/src/components/Transactions/Transaction.svelte b/packages/bridge-ui/src/components/Transactions/Transaction.svelte index 43a9df9b04..4566f31306 100644 --- a/packages/bridge-ui/src/components/Transactions/Transaction.svelte +++ b/packages/bridge-ui/src/components/Transactions/Transaction.svelte @@ -108,15 +108,16 @@ srcBridgeAddress: chains[bridgeTx.fromChainId].bridgeAddress, }); - pendingTransactions.update((store) => { - store.push(tx); - return store; - }); - successToast($_('toast.transactionSent')); + + await pendingTransactions.add(tx, $signer); + // TODO: keep the MessageStatus as contract and use another way. transaction.status = MessageStatus.ClaimInProgress; + + successToast('Transaction completed!'); } catch (e) { + // TODO: handle potential transaction failure console.error(e); errorToast($_('toast.errorSendingTransaction')); } finally { @@ -153,13 +154,13 @@ srcTokenVaultAddress: tokenVaults[bridgeTx.fromChainId], }); - pendingTransactions.update((store) => { - store.push(tx); - return store; - }); - successToast($_('toast.transactionSent')); + + pendingTransactions.add(tx, $signer); + + successToast('Transaction completed!'); } catch (e) { + // TODO: handle potential transaction failure console.error(e); errorToast($_('toast.errorSendingTransaction')); } finally { diff --git a/packages/bridge-ui/src/components/form/BridgeForm.svelte b/packages/bridge-ui/src/components/form/BridgeForm.svelte index 89bf7b9fca..545c3bc87e 100644 --- a/packages/bridge-ui/src/components/form/BridgeForm.svelte +++ b/packages/bridge-ui/src/components/form/BridgeForm.svelte @@ -171,17 +171,18 @@ spenderAddress: tokenVaults[$fromChain.id], }); - pendingTransactions.update((store) => { - store.push(tx); - return store; - }); - successToast($_('toast.transactionSent')); - await $signer.provider.waitForTransaction(tx.hash, 1); + + await pendingTransactions.add(tx, $signer); requiresAllowance = false; + + successToast('Transaction completed!'); } catch (e) { console.error(e); + // TODO: if we have TransactionReceipt here means the tx failed + // We might want to give the user a link to etherscan + // to see the tx details errorToast($_('toast.errorSendingTransaction')); } finally { loading = false; @@ -262,12 +263,17 @@ ); if (!doesUserHaveEnoughBalance) { + // TODO: about custom errors and catch it in the catch block? errorToast('Insufficient ETH balance'); return; } const tx = await $activeBridge.Bridge(bridgeOpts); + successToast($_('toast.transactionSent')); + + await pendingTransactions.add(tx, $signer); + // tx.chainId is not set immediately but we need it later. set it // manually. tx.chainId = $fromChain.id; @@ -284,6 +290,7 @@ hash: tx.hash, status: MessageStatus.New, }; + if (!transactions) { transactions = [bridgeTransaction]; } else { @@ -292,11 +299,6 @@ $transactioner.updateStorageByAddress(userAddress, transactions); - pendingTransactions.update((store) => { - store.push(tx); - return store; - }); - const allTransactions = $transactionsStore; // get full BridgeTransaction object @@ -307,11 +309,12 @@ transactionsStore.set([bridgeTransaction, ...allTransactions]); - successToast($_('toast.transactionSent')); - await $signer.provider.waitForTransaction(tx.hash, 1); memo = ''; + + successToast('Transaction completed!'); } catch (e) { console.error(e); + // TODO: Same as in approve() errorToast($_('toast.errorSendingTransaction')); } finally { loading = false; diff --git a/packages/bridge-ui/src/components/modals/FaucetModal.svelte b/packages/bridge-ui/src/components/modals/FaucetModal.svelte index c3806d42c0..a82f5c384b 100644 --- a/packages/bridge-ui/src/components/modals/FaucetModal.svelte +++ b/packages/bridge-ui/src/components/modals/FaucetModal.svelte @@ -73,17 +73,17 @@ const address = await $signer.getAddress(); const tx = await contract.mint(address); - pendingTransactions.update((store) => { - store.push(tx); - return store; - }); - successToast($_('toast.transactionSent')); + + await pendingTransactions.add(tx, $signer); + isOpen = false; - await $signer.provider.waitForTransaction(tx.hash, 1); + + successToast('Transaction completed!'); await onMint(); } catch (e) { + // TODO: handle potential transaction failure console.error(e); errorToast($_('toast.errorSendingTransaction')); } diff --git a/packages/bridge-ui/src/store/transactions.spec.ts b/packages/bridge-ui/src/store/transactions.spec.ts new file mode 100644 index 0000000000..4c7392db31 --- /dev/null +++ b/packages/bridge-ui/src/store/transactions.spec.ts @@ -0,0 +1,67 @@ +import { get } from 'svelte/store'; +import type { Signer, Transaction, ethers } from 'ethers'; +import { pendingTransactions } from './transactions'; + +jest.mock('../constants/envVars'); + +// Transaction we're going to add to the store +const tx = { hash: '0x789' } as Transaction; + +// These are the pending transactions we'll have initially in the store +const initialTxs = [{ hash: '0x123' }, { hash: '0x456' }] as Transaction[]; + +const mockSigner = (receipt: ethers.providers.TransactionReceipt) => { + const waitForTransaction = jest + .fn() + .mockImplementation(() => Promise.resolve(receipt)); + + return { + provider: { waitForTransaction }, + } as unknown as Signer; +}; + +describe('transaction stores', () => { + beforeEach(() => { + pendingTransactions.set(initialTxs); + }); + + it('tests a successful pendingTransactions', () => { + const txTeceipt = { status: 1 } as ethers.providers.TransactionReceipt; + const signer = mockSigner(txTeceipt); + + pendingTransactions + .add(tx, signer) + .then((receipt) => { + // The transaction should have been removed from the store + expect(get(pendingTransactions)).toStrictEqual(initialTxs); + + expect(receipt).toEqual(txTeceipt); + }) + .catch(() => { + throw new Error('Should not have thrown'); + }); + + // The transaction should have added to the store + expect(get(pendingTransactions)).toStrictEqual([...initialTxs, tx]); + }); + + it('tests a failed pendingTransactions custom store', () => { + const txTeceipt = { status: 0 } as ethers.providers.TransactionReceipt; + const signer = mockSigner(txTeceipt); + + pendingTransactions + .add(tx, signer) + .then(() => { + throw new Error('Should have thrown'); + }) + .catch((receipt) => { + // The transaction should have been removed from the store + expect(get(pendingTransactions)).toStrictEqual(initialTxs); + + expect(receipt).toEqual(txTeceipt); + }); + + // The transaction should have added to the store + expect(get(pendingTransactions)).toStrictEqual([...initialTxs, tx]); + }); +}); diff --git a/packages/bridge-ui/src/store/transactions.ts b/packages/bridge-ui/src/store/transactions.ts index 1f69a5358b..ca062eb62a 100644 --- a/packages/bridge-ui/src/store/transactions.ts +++ b/packages/bridge-ui/src/store/transactions.ts @@ -1,10 +1,69 @@ import { writable } from 'svelte/store'; - -import type { Transaction } from 'ethers'; +import type { Signer, Transaction, ethers } from 'ethers'; import type { BridgeTransaction, Transactioner } from '../domain/transactions'; +import { Deferred } from '../utils/Deferred'; + +export const transactions = writable([]); +export const transactioner = writable(); + +// Custom store: pendingTransactions +const { subscribe, set, update } = writable([]); +export const pendingTransactions = { + /** + * We're creating here a custom store, which is a writable store. + * We must stick to the store contract, which is: + */ + set, + subscribe, + // update, // this method is optional. + + /** + * Custom method, which will help us add a new transaction to the store + * and get it removed onces the transaction is mined. + */ + add: (tx: Transaction, signer: Signer) => { + const deferred = new Deferred(); + + update((txs: Transaction[]) => { + // New array with the new transaction appended + const newPendingTransactions = [...txs, tx]; + + // Save the index of the new transaction to later on remove it + // from the list of pending transactions. + const idxAppendedTransaction = newPendingTransactions.length - 1; + + // Next step is to wait for the transaction to be mined + // before removing it from the store. + + /** + * Returns a Promise which will not resolve until transactionHash is mined. + * If confirms is 0, this method is non-blocking and if the transaction + * has not been mined returns null. Otherwise, this method will block until + * the transaction has confirms blocks mined on top of the block in which + * is was mined. + * See https://docs.ethers.org/v5/api/providers/provider/#Provider-waitForTransaction + */ + signer.provider.waitForTransaction(tx.hash, 1).then((receipt) => { + // The transaction has been mined. + + // Removes the transaction from the store + update((txs: Transaction[]) => { + const copyPendingTransactions = [...txs]; + copyPendingTransactions.splice(idxAppendedTransaction, 1); + return copyPendingTransactions; + }); + + // Resolves or rejects the promise depending on the transaction status. + if (receipt.status === 1) { + deferred.resolve(receipt); + } else { + deferred.reject(receipt); + } + }); -const pendingTransactions = writable([]); -const transactions = writable([]); -const transactioner = writable(); + return newPendingTransactions; + }); -export { pendingTransactions, transactions, transactioner }; + return deferred.promise; + }, +}; diff --git a/packages/bridge-ui/src/utils/Deferred.spec.ts b/packages/bridge-ui/src/utils/Deferred.spec.ts new file mode 100644 index 0000000000..8ddebab418 --- /dev/null +++ b/packages/bridge-ui/src/utils/Deferred.spec.ts @@ -0,0 +1,25 @@ +import { Deferred } from './Deferred'; + +describe('Deferred', () => { + it('should resolve', async () => { + const deferred = new Deferred(); + deferred.resolve('test'); + + try { + await expect(deferred.promise).resolves.toBe('test'); + } catch (e) { + throw Error('This should never happen'); + } + }); + + it('should reject', async () => { + const deferred = new Deferred(); + deferred.reject('test'); + + try { + await deferred.promise; + } catch (err) { + expect(err).toBe('test'); + } + }); +}); diff --git a/packages/bridge-ui/src/utils/Deferred.ts b/packages/bridge-ui/src/utils/Deferred.ts new file mode 100644 index 0000000000..e48a37349a --- /dev/null +++ b/packages/bridge-ui/src/utils/Deferred.ts @@ -0,0 +1,12 @@ +export class Deferred { + public promise: Promise; + public resolve: (value?: T) => void; + public reject: (reason?: T) => void; + + constructor() { + this.promise = new Promise((resolve, reject) => { + this.resolve = resolve; + this.reject = reject; + }); + } +}