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.

Sign up for GitHub

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

Jump to bottom

feat(bridge-ui-v2): Fixed input validations and catching additional errors #14331

Merged
merged 3 commits into from
Aug 2, 2023
Merged

feat(bridge-ui-v2): Fixed input validations and catching additional errors #14331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
436b699
feat(bridge-ui-v2): fixed some input validations for ERC20 tokens, ca…
KorbinianK Aug 1, 2023
39e36dd
chore(bridge-ui-v2): fix linting
KorbinianK Aug 1, 2023
2a6a75c
Merge branch 'alpha-4' into fix/bridge-ui-v2--briding-validations
KorbinianK Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 46 additions & 16 deletions packages/bridge-ui-v2/src/components/Bridge/Amount.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,20 @@
<script lang="ts">
import type { FetchBalanceResult } from '@wagmi/core';
import { onMount } from 'svelte';
import { t } from 'svelte-i18n';
import { formatUnits, parseUnits } from 'viem';

import { FlatAlert } from '$components/Alert';
import { InputBox } from '$components/InputBox';
import { LoadingText } from '$components/LoadingText';
import { warningToast } from '$components/NotificationToast';
import { checkBalanceToBridge, getMaxAmountToBridge } from '$libs/bridge';
import { bridges, checkBalanceToBridge, getMaxAmountToBridge, type RequireAllowanceArgs } from '$libs/bridge';
import type { ERC20Bridge } from '$libs/bridge/ERC20Bridge';
import { chainContractsMap } from '$libs/chain';
import { InsufficientAllowanceError, InsufficientBalanceError, RevertedWithFailedError } from '$libs/error';
import { getBalance as getTokenBalance } from '$libs/token';
import { getAddress,getBalance as getTokenBalance } from '$libs/token';
import { debounce } from '$libs/util/debounce';
import { getConnectedWallet } from '$libs/util/getConnectedWallet';
import { truncateString } from '$libs/util/truncateString';
import { uid } from '$libs/util/uid';
import { account } from '$stores/account';
Expand All @@ -33,17 +37,19 @@
let inputBox: InputBox;
let computingMaxAmount = false;

onMount(() => {
clearAmount();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to clear on mount? If the reason is to clear the state, before we actually wanted to leave the amount on purpose when switching pages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not persisting (visibly) right now. We either clear it here or we make sure it is also displayed.

});

// Public API
export function clearAmount() {
inputBox.clear();
$enteredAmount = BigInt(0);
}

export async function validateAmount(token = $selectedToken, fee = $processingFee) {
$insufficientBalance = false;
$insufficientAllowance = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this fixing a problem? We're gonna validate these two, we start assuming there is no issue and then validate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it helps with the flickering of the buttons. In the future we could introduce a loading or disabled status


const to = $recipientAddress || $account?.address;
const walletClient = await getConnectedWallet();

// We need all these guys to validate
if (
Expand All @@ -53,8 +59,38 @@
!$destNetwork ||
!$tokenBalance ||
$enteredAmount === BigInt(0) // no need to check if the amount is 0
)
) {
$insufficientBalance = false;
return;
}

if ($enteredAmount > $tokenBalance.value) {
$insufficientBalance = true;
// no need to check any further if the amount is greater than the balance
return;
}
$insufficientBalance = false;

const erc20Bridge = bridges.ERC20 as ERC20Bridge;
const spenderAddress = chainContractsMap[$network.id].tokenVaultAddress;

const tokenAddress = await getAddress({
token,
srcChainId: $network.id,
destChainId: $destNetwork.id,
});

if (!tokenAddress) return;

// Check for allowance
$insufficientAllowance = await erc20Bridge.requireAllowance({
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to do this here?, we already get the error when running the estimation, don't we?, is this not redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed in the call, the gas estimation does not throw an error, so we need to call it.

amount: $enteredAmount,
tokenAddress,
ownerAddress: walletClient.account.address,
spenderAddress,
} as RequireAllowanceArgs);

if ($insufficientAllowance) return;

try {
await checkBalanceToBridge({
Expand Down Expand Up @@ -208,16 +244,10 @@
{$t('amount_input.button.max')}
</button>
</div>

{#if $insufficientBalance}
<FlatAlert type="error" message={$t('amount_input.error.insufficient_balance')} class="absolute bottom-[-26px]" />
{/if}

{#if $insufficientAllowance}
<FlatAlert
type="warning"
message={$t('amount_input.error.insufficient_allowance')}
class="absolute bottom-[-26px]" />
{#if $insufficientBalance && $enteredAmount > 0}
<FlatAlert type="error" message={$t('error.insufficient_balance')} class="absolute bottom-[-26px]" />
{:else if $insufficientAllowance && $enteredAmount > 0}
<FlatAlert type="warning" message={$t('error.insufficient_allowance')} class="absolute bottom-[-26px]" />
{/if}
</div>
</div>
25 changes: 21 additions & 4 deletions packages/bridge-ui-v2/src/components/Bridge/Bridge.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<script lang="ts">
import { t } from 'svelte-i18n';
import { UserRejectedRequestError } from 'viem';
import { TransactionExecutionError, UserRejectedRequestError } from 'viem';

import { Card } from '$components/Card';
import { ChainSelector } from '$components/ChainSelector';
Expand All @@ -13,7 +13,13 @@
import { type BridgeArgs, bridges, type ERC20BridgeArgs, type ETHBridgeArgs } from '$libs/bridge';
import type { ERC20Bridge } from '$libs/bridge/ERC20Bridge';
import { chainContractsMap, chains } from '$libs/chain';
import { ApproveError, NoAllowanceRequiredError, SendERC20Error, SendMessageError } from '$libs/error';
import {
ApproveError,
InsufficientAllowanceError,
NoAllowanceRequiredError,
SendERC20Error,
SendMessageError,
} from '$libs/error';
import { ETHToken, getAddress, isDeployedCrossChain, tokens, TokenType } from '$libs/token';
import { getConnectedWallet } from '$libs/util/getConnectedWallet';
import { type Account, account } from '$stores/account';
Expand Down Expand Up @@ -107,6 +113,9 @@
case err instanceof NoAllowanceRequiredError:
errorToast($t('bridge.errors.no_allowance_required'));
break;
case err instanceof InsufficientAllowanceError:
errorToast($t('bridge.errors.insufficient_allowance'));
break;
case err instanceof ApproveError:
// TODO: see contract for all possible errors
errorToast($t('bridge.errors.approve_error'));
Expand Down Expand Up @@ -210,8 +219,8 @@
console.error(err);

switch (true) {
case err instanceof UserRejectedRequestError:
warningToast($t('bridge.errors.rejected'));
case err instanceof InsufficientAllowanceError:
errorToast($t('bridge.errors.insufficient_allowance'));
break;
case err instanceof SendMessageError:
// TODO: see contract for all possible errors
Expand All @@ -221,6 +230,14 @@
// TODO: see contract for all possible errors
errorToast($t('bridge.errors.send_erc20_error'));
break;
case err instanceof UserRejectedRequestError:
// Todo: viem does not seem to detect UserRejectError
warningToast($t('bridge.errors.rejected'));
break;
case err instanceof TransactionExecutionError && err.shortMessage === 'User rejected the request.':
Copy link
Contributor

@jscriptcoder jscriptcoder Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not quite understand why this is needed? denying tx signature is handled in the bridge method which re-throws UserRejectedRequestError getting caught here already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, we can move this logic to the ERC20Bridge but for some reason the wrong error is thrown and we need to catch it

//Todo: so we catch it by string comparison below, suboptimal
warningToast($t('bridge.errors.rejected'));
break;
default:
errorToast($t('bridge.errors.unknown_error'));
}
Expand Down
7 changes: 4 additions & 3 deletions packages/bridge-ui-v2/src/components/Bridge/SwitchChainsButton.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<script lang="ts">
import { switchNetwork } from '@wagmi/core';
import { t } from 'svelte-i18n';
import { UserRejectedRequestError } from 'viem';
import { SwitchChainError, UserRejectedRequestError } from 'viem';

import { Icon } from '$components/Icon';
import { warningToast } from '$components/NotificationToast';
Expand All @@ -15,8 +15,9 @@
await switchNetwork({ chainId: $destNetwork.id });
} catch (err) {
console.error(err);

if (err instanceof UserRejectedRequestError) {
if (err instanceof SwitchChainError) {
warningToast($t('messages.network.pending'));
} else if (err instanceof UserRejectedRequestError) {
warningToast($t('messages.network.rejected'));
}
}
Expand Down
6 changes: 4 additions & 2 deletions packages/bridge-ui-v2/src/i18n/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
"send_message_error": "Failed to send message",
"approve_error": "Failed to approve token",
"rejected": "Request to bridge rejected.",
"no_allowance_required": "You need to add allowance to bridge this token.",
"no_allowance_required": "You already have enough allowance.",
"insufficient_allowance": "You need to increase your allowance in order to be able to bridge.",
"unknown_error": "An error occured"
},
"button": {
Expand Down Expand Up @@ -174,7 +175,8 @@
},
"network": {
"switching": "Switching network",
"rejected": "Request to switch chain rejected."
"rejected": "Request to switch chain rejected.",
"pending": "A network switch is still pending, open your wallet to check the status."
}
},
"common": {
Expand Down
15 changes: 11 additions & 4 deletions packages/bridge-ui-v2/src/libs/bridge/checkBalanceToBridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { getPublicClient } from '@wagmi/core';
import { type Address, zeroAddress } from 'viem';

import { chainContractsMap } from '$libs/chain';
Expand Down Expand Up @@ -61,10 +62,16 @@ export async function checkBalanceToBridge({
throw new RevertedWithFailedError('BLL token doing its thing', { cause: err });
}
}
if (estimatedCost > balance - amount) {
throw new InsufficientBalanceError('you do not have enough balance to bridge');
}
} else {
const { tokenVaultAddress } = chainContractsMap[srcChainId];
const tokenAddress = await getAddress({ token, srcChainId, destChainId });

// since we are briding a token, we need the ETH balance of the wallet
balance = await getPublicClient().getBalance(wallet.account);

if (!tokenAddress || tokenAddress === zeroAddress) return false;

const isTokenAlreadyDeployed = await isDeployedCrossChain({
Expand All @@ -88,9 +95,9 @@ export async function checkBalanceToBridge({
throw new InsufficientAllowanceError(`insufficient allowance for the amount ${amount}`, { cause: err });
}
}
}

if (estimatedCost > balance - amount) {
throw new InsufficientBalanceError('you do not have enough balance to bridge');
// no need to deduct the amount we want to bridge from the balance as we pay in ETH
if (estimatedCost > balance) {
throw new InsufficientBalanceError('you do not have enough balance to bridge');
}
}
}
Loading