From 6e31fb03ae5d5acf57b1709eb2c72db557aeb1b9 Mon Sep 17 00:00:00 2001 From: Matthew Russell Date: Fri, 3 Nov 2023 08:12:40 -0700 Subject: [PATCH] fix(accounts): fix key selection and validation, and add use max to transfers (#5184) --- libs/accounts/src/lib/transfer-container.tsx | 16 +- libs/accounts/src/lib/transfer-form.spec.tsx | 61 +++---- libs/accounts/src/lib/transfer-form.tsx | 157 +++++++++---------- 3 files changed, 102 insertions(+), 132 deletions(-) diff --git a/libs/accounts/src/lib/transfer-container.tsx b/libs/accounts/src/lib/transfer-container.tsx index 15cd5063d..ba339a73a 100644 --- a/libs/accounts/src/lib/transfer-container.tsx +++ b/libs/accounts/src/lib/transfer-container.tsx @@ -1,5 +1,5 @@ import * as Schema from '@vegaprotocol/types'; -import { addDecimal, truncateByChars } from '@vegaprotocol/utils'; +import { truncateByChars } from '@vegaprotocol/utils'; import { t } from '@vegaprotocol/i18n'; import { NetworkParams, @@ -12,7 +12,6 @@ import { useVegaWallet } from '@vegaprotocol/wallet'; import { useCallback } from 'react'; import { accountsDataProvider } from './accounts-data-provider'; import { TransferForm } from './transfer-form'; -import sortBy from 'lodash/sortBy'; import { Lozenge } from '@vegaprotocol/ui-toolkit'; export const ALLOWED_ACCOUNTS = [ @@ -42,18 +41,6 @@ export const TransferContainer = ({ assetId }: { assetId?: string }) => { ? data.filter((account) => ALLOWED_ACCOUNTS.includes(account.type)) : []; - const assets = accounts - // Theres only one general account for each asset, this will give us a list - // of assets the user has accounts for - .filter((a) => a.type === Schema.AccountType.ACCOUNT_TYPE_GENERAL) - .map((account) => ({ - id: account.asset.id, - symbol: account.asset.symbol, - name: account.asset.name, - decimals: account.asset.decimals, - balance: addDecimal(account.balance, account.asset.decimals), - })); - return ( <>

@@ -71,7 +58,6 @@ export const TransferContainer = ({ assetId }: { assetId?: string }) => { pk.publicKey) : null} - assets={sortBy(assets, 'name')} assetId={assetId} feeFactor={param} submitTransfer={transfer} diff --git a/libs/accounts/src/lib/transfer-form.spec.tsx b/libs/accounts/src/lib/transfer-form.spec.tsx index 886bfe556..032a18df9 100644 --- a/libs/accounts/src/lib/transfer-form.spec.tsx +++ b/libs/accounts/src/lib/transfer-form.spec.tsx @@ -3,7 +3,7 @@ import userEvent from '@testing-library/user-event'; import BigNumber from 'bignumber.js'; import { AddressField, TransferFee, TransferForm } from './transfer-form'; import { AccountType } from '@vegaprotocol/types'; -import { addDecimal, formatNumber, removeDecimal } from '@vegaprotocol/utils'; +import { removeDecimal } from '@vegaprotocol/utils'; describe('TransferForm', () => { const submit = async () => { @@ -14,7 +14,6 @@ describe('TransferForm', () => { const selectAsset = async (asset: { id: string; - balance: string; name: string; decimals: number; }) => { @@ -28,9 +27,6 @@ describe('TransferForm', () => { expect(await screen.findByTestId('select-asset')).toHaveTextContent( asset.name ); - expect(await screen.findByTestId('asset-balance')).toHaveTextContent( - formatNumber(asset.balance, asset.decimals) - ); }; const amount = '100'; @@ -41,7 +37,6 @@ describe('TransferForm', () => { symbol: '€', name: 'EUR', decimals: 2, - balance: addDecimal(100000, 2), // 1000 }; const props = { pubKey, @@ -49,19 +44,18 @@ describe('TransferForm', () => { pubKey, 'a4b6e3de5d7ef4e31ae1b090be49d1a2ef7bcefff60cccf7658a0d4922651cce', ], - assets: [asset], feeFactor: '0.001', submitTransfer: jest.fn(), accounts: [ { type: AccountType.ACCOUNT_TYPE_GENERAL, asset, - balance: '100', + balance: '100000', }, { type: AccountType.ACCOUNT_TYPE_VESTED_REWARDS, asset, - balance: '100', + balance: '100000', }, ], }; @@ -75,7 +69,7 @@ describe('TransferForm', () => { render(); // Select a pubkey await userEvent.selectOptions( - screen.getByLabelText('Vega key'), + screen.getByLabelText('To Vega key'), props.pubKeys[1] ); @@ -121,13 +115,19 @@ describe('TransferForm', () => { // 1003-TRAN-004 render(); await submit(); - expect(await screen.findAllByText('Required')).toHaveLength(4); + expect(await screen.findAllByText('Required')).toHaveLength(3); // pubkey is set as default value const toggle = screen.getByText('Enter manually'); await userEvent.click(toggle); // has switched to input expect(toggle).toHaveTextContent('Select from wallet'); - expect(screen.getByLabelText('Vega key')).toHaveAttribute('type', 'text'); - await userEvent.type(screen.getByLabelText('Vega key'), 'invalid-address'); + expect(screen.getByLabelText('To Vega key')).toHaveAttribute( + 'type', + 'text' + ); + await userEvent.type( + screen.getByLabelText('To Vega key'), + 'invalid-address' + ); expect(screen.getAllByTestId('input-error-text')[0]).toHaveTextContent( 'Invalid Vega key' ); @@ -142,7 +142,7 @@ describe('TransferForm', () => { render(); // check current pubkey not shown - const keySelect = screen.getByLabelText('Vega key'); + const keySelect = screen.getByLabelText('To Vega key'); expect(keySelect.children).toHaveLength(3); expect(Array.from(keySelect.options).map((o) => o.value)).toEqual([ '', @@ -151,25 +151,17 @@ describe('TransferForm', () => { ]); await submit(); - expect(await screen.findAllByText('Required')).toHaveLength(4); + expect(await screen.findAllByText('Required')).toHaveLength(3); // pubkey is set as default value // Select a pubkey await userEvent.selectOptions( - screen.getByLabelText('Vega key'), + screen.getByLabelText('To Vega key'), props.pubKeys[1] ); // Select asset await selectAsset(asset); - // assert rich select as updated - expect(await screen.findByTestId('select-asset')).toHaveTextContent( - asset.name - ); - expect(await screen.findByTestId('asset-balance')).toHaveTextContent( - formatNumber(asset.balance, asset.decimals) - ); - await userEvent.selectOptions( screen.getByLabelText('From account'), AccountType.ACCOUNT_TYPE_VESTED_REWARDS @@ -217,7 +209,7 @@ describe('TransferForm', () => { render(); // check current pubkey not shown - const keySelect = screen.getByLabelText('Vega key'); + const keySelect = screen.getByLabelText('To Vega key'); const pubKeyOptions = ['', pubKey, props.pubKeys[1]]; expect(keySelect.children).toHaveLength(pubKeyOptions.length); expect(Array.from(keySelect.options).map((o) => o.value)).toEqual( @@ -225,11 +217,11 @@ describe('TransferForm', () => { ); await submit(); - expect(await screen.findAllByText('Required')).toHaveLength(4); + expect(await screen.findAllByText('Required')).toHaveLength(3); // pubkey set as default value // Select a pubkey await userEvent.selectOptions( - screen.getByLabelText('Vega key'), + screen.getByLabelText('To Vega key'), props.pubKeys[1] ); @@ -286,7 +278,7 @@ describe('TransferForm', () => { render(); // check current pubkey not shown - const keySelect: HTMLSelectElement = screen.getByLabelText('Vega key'); + const keySelect: HTMLSelectElement = screen.getByLabelText('To Vega key'); const pubKeyOptions = ['', pubKey, props.pubKeys[1]]; expect(keySelect.children).toHaveLength(pubKeyOptions.length); expect(Array.from(keySelect.options).map((o) => o.value)).toEqual( @@ -294,11 +286,11 @@ describe('TransferForm', () => { ); await submit(); - expect(await screen.findAllByText('Required')).toHaveLength(4); + expect(await screen.findAllByText('Required')).toHaveLength(3); // pubkey set as default value // Select a pubkey await userEvent.selectOptions( - screen.getByLabelText('Vega key'), + screen.getByLabelText('To Vega key'), props.pubKeys[1] ); @@ -324,7 +316,6 @@ describe('TransferForm', () => { describe('AddressField', () => { const props = { - pubKeys: ['pubkey-1', 'pubkey-2'], select:

select
, input:
input
, onChange: jest.fn(), @@ -334,7 +325,7 @@ describe('TransferForm', () => { const mockOnChange = jest.fn(); render(); - // select should be shown as multiple pubkeys provided + // select should be shown by default expect(screen.getByText('select')).toBeInTheDocument(); expect(screen.queryByText('input')).not.toBeInTheDocument(); await userEvent.click(screen.getByText('Enter manually')); @@ -346,12 +337,6 @@ describe('TransferForm', () => { expect(screen.queryByText('input')).not.toBeInTheDocument(); expect(mockOnChange).toHaveBeenCalledTimes(2); }); - - it('Does not provide select option if there is only a single key', () => { - render(); - expect(screen.getByText('input')).toBeInTheDocument(); - expect(screen.queryByText('Select from wallet')).not.toBeInTheDocument(); - }); }); describe('TransferFee', () => { diff --git a/libs/accounts/src/lib/transfer-form.tsx b/libs/accounts/src/lib/transfer-form.tsx index ce7ea5dd6..7e7151cc4 100644 --- a/libs/accounts/src/lib/transfer-form.tsx +++ b/libs/accounts/src/lib/transfer-form.tsx @@ -1,3 +1,4 @@ +import sortBy from 'lodash/sortBy'; import { minSafe, maxSafe, @@ -28,28 +29,19 @@ import { AssetOption, Balance } from '@vegaprotocol/assets'; import { AccountType, AccountTypeMapping } from '@vegaprotocol/types'; interface FormFields { - toAddress: string; + toVegaKey: string; asset: string; amount: string; fromAccount: AccountType; } -interface Asset { - id: string; - symbol: string; - name: string; - decimals: number; - balance: string; -} - interface TransferFormProps { pubKey: string | null; pubKeys: string[] | null; - assets: Array; accounts: Array<{ type: AccountType; balance: string; - asset: { id: string; symbol: string; decimals: number }; + asset: { id: string; symbol: string; name: string; decimals: number }; }>; assetId?: string; feeFactor: string | null; @@ -59,7 +51,6 @@ interface TransferFormProps { export const TransferForm = ({ pubKey, pubKeys, - assets, assetId: initialAssetId, feeFactor, submitTransfer, @@ -75,16 +66,50 @@ export const TransferForm = ({ } = useForm({ defaultValues: { asset: initialAssetId, + toVegaKey: pubKey || '', }, }); - const selectedPubKey = watch('toAddress'); + const assets = sortBy( + accounts + .filter((a) => a.type === AccountType.ACCOUNT_TYPE_GENERAL) + .map((account) => ({ + ...account.asset, + balance: addDecimal(account.balance, account.asset.decimals), + })), + 'name' + ); + + const selectedPubKey = watch('toVegaKey'); const amount = watch('amount'); + const fromAccount = watch('fromAccount'); const assetId = watch('asset'); + const asset = assets.find((a) => a.id === assetId); + const account = accounts.find( + (a) => a.asset.id === assetId && a.type === fromAccount + ); + const accountBalance = + account && addDecimal(account.balance, account.asset.decimals); + + // General account for the selected asset + const generalAccount = accounts.find((a) => { + return ( + a.asset.id === assetId && a.type === AccountType.ACCOUNT_TYPE_GENERAL + ); + }); + const [includeFee, setIncludeFee] = useState(false); + // Min viable amount given asset decimals EG for WEI 0.000000000000000001 + const min = asset + ? new BigNumber(addDecimal('1', asset.decimals)) + : new BigNumber(0); + + // Max amount given selected asset and from account + const max = accountBalance ? new BigNumber(accountBalance) : new BigNumber(0); + const transferAmount = useMemo(() => { if (!amount) return undefined; if (includeFee && feeFactor) { @@ -112,7 +137,7 @@ export const TransferForm = ({ throw new Error('Submitted transfer with no amount selected'); } const transfer = normalizeTransfer( - fields.toAddress, + fields.toVegaKey, transferAmount, fields.fromAccount, AccountType.ACCOUNT_TYPE_GENERAL, // field is readonly in the form @@ -126,19 +151,6 @@ export const TransferForm = ({ [asset, submitTransfer, transferAmount] ); - const min = useMemo(() => { - // Min viable amount given asset decimals EG for WEI 0.000000000000000001 - const minViableAmount = asset - ? new BigNumber(addDecimal('1', asset.decimals)) - : new BigNumber(0); - return minViableAmount; - }, [asset]); - - const max = useMemo(() => { - const maxAmount = asset ? new BigNumber(asset.balance) : new BigNumber(0); - return maxAmount; - }, [asset]); - // reset for placeholder workaround https://github.com/radix-ui/primitives/issues/1569 useEffect(() => { if (!pubKey) { @@ -146,51 +158,38 @@ export const TransferForm = ({ } }, [setValue, pubKey]); - // General account for the selected asset - const generalAccount = accounts.find((a) => { - return ( - a.asset.id === assetId && a.type === AccountType.ACCOUNT_TYPE_GENERAL - ); - }); - return (
- + setValue('toAddress', '')} + onChange={() => setValue('toVegaKey', '')} select={ - + - {pubKeys?.length && - pubKeys.map((pk) => { - const text = pk === pubKey ? t('Current key: ') + pk : pk; + {pubKeys?.map((pk) => { + const text = pk === pubKey ? t('Current key: ') + pk : pk; - return ( - - ); - })} + return ( + + ); + })} } input={ } /> - {errors.toAddress?.message && ( - - {errors.toAddress.message} + {errors.toVegaKey?.message && ( + + {errors.toVegaKey.message} )} @@ -322,15 +321,24 @@ export const TransferForm = ({ maxSafe: (v) => { const value = new BigNumber(v); if (value.isGreaterThan(max)) { - return t( - 'You cannot transfer more than your available collateral' - ); + return t('You cannot transfer more than available'); } return maxSafe(max)(v); }, }, })} /> + {accountBalance && ( + + )} {errors.amount?.message && ( {errors.amount.message} @@ -442,40 +450,31 @@ export const TransferFee = ({ }; interface AddressInputProps { - pubKeys: string[] | null; select: ReactNode; input: ReactNode; onChange: () => void; } export const AddressField = ({ - pubKeys, select, input, onChange, }: AddressInputProps) => { - const [isInput, setIsInput] = useState(() => { - if (pubKeys && pubKeys.length <= 1) { - return true; - } - return false; - }); + const [isInput, setIsInput] = useState(false); return ( <> {isInput ? input : select} - {pubKeys && pubKeys.length > 1 && ( - - )} + ); };