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.

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

Fix RSK EIP1191 checksum issues #4151

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions src/components/GeneralLookupField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { DomainStatus, InlineMessage } from '@components';
import {
isValidENSName,
isValidETHAddress,
isValidETHRecipientAddress,
isValidRecipientAddress,
ProviderHandler
} from '@services/EthService';
import { createENSResolutionError, isResolutionError } from '@services/EthService/ens';
Expand Down Expand Up @@ -76,7 +76,7 @@ const GeneralLookupField = ({
useEffect(() => {
// Run validation if possible
if (setFieldError) {
const validationResult = isValidETHRecipientAddress(value.value, resolutionError);
const validationResult = isValidRecipientAddress(value.value, resolutionError, network);
setFieldError(name, validationResult.success ? undefined : validationResult.message);
}
}, [value, resolutionError]);
Expand Down
3 changes: 2 additions & 1 deletion src/features/InteractWithContracts/components/Interact.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
InputField,
NetworkSelector
} from '@components';
import { DEFAULT_NETWORK } from '@config';
import { getNetworkById, isValidENSName, isValidETHAddress, useNetworks } from '@services';
import { BREAK_POINTS, COLORS } from '@theme';
import { translateRaw } from '@translations';
Expand Down Expand Up @@ -154,7 +155,7 @@ const FormSchema = object().shape({
address: object({
value: string().test(
'check-eth-address',
translateRaw('TO_FIELD_ERROR'),
translateRaw('TO_FIELD_ERROR', { $network: DEFAULT_NETWORK }),
Copy link
Author

@7alip 7alip Sep 30, 2021

Choose a reason for hiding this comment

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

I provided network to avoid it to be displayed as Enter a valid $network address, ENS name, or blockchain domain

Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't you move the schema inside the component so it can access the network prop and use that instead?

(value) => isValidETHAddress(value) || isValidENSName(value)
)
}).required(translateRaw('REQUIRED'))
Expand Down
13 changes: 9 additions & 4 deletions src/features/SendAssets/components/SendAssetsForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import isEmpty from 'lodash/isEmpty';
import mergeDeepWith from 'ramda/src/mergeDeepWith';
import styled from 'styled-components';
import { ValuesType } from 'utility-types';
import { number, object, string } from 'yup';
import { number, object, ObjectSchema, string } from 'yup';

import {
AccountSelector,
Expand Down Expand Up @@ -41,6 +41,7 @@ import { useGasForm } from '@hooks';
import { getNonce, useRates } from '@services';
import {
isBurnAddress,
isValidAddress,
isValidETHAddress,
isValidPositiveNumber
} from '@services/EthService/validators';
Expand Down Expand Up @@ -279,9 +280,13 @@ export const SendAssetsForm = ({ txConfig, onComplete, protectTxButton }: ISendF
account: object().required(translateRaw('REQUIRED')),
address: object()
.required(translateRaw('REQUIRED'))
.test('valid', translateRaw('TO_FIELD_ERROR'), function (value) {
return value && value.value && isValidETHAddress(value.value);
})
.when('network', (network: Network, schema: ObjectSchema) =>
schema.test('valid', translateRaw('TO_FIELD_ERROR', { $network: network.name }), function (
value
) {
return value && value.value && isValidAddress(value.value, network.chainId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks to work, but I am not sure it will be possible to send without fixing the issues in Ethers. Balance scanning etc seems to be failing on RSK right now. Have you talked to ricmoo or is there an open issue?

})
)
// @ts-expect-error Hack as Formik doesn't officially support warnings
// tslint:disable-next-line
.test('check-sending-to-burn', translateRaw('SENDING_TO_BURN_ADDRESS'), function (value) {
Expand Down
12 changes: 8 additions & 4 deletions src/features/Settings/components/AddToAddressBook.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@
import { Button } from '@mycrypto/ui';
import { Field, FieldProps, Form, Formik } from 'formik';
import styled from 'styled-components';
import { object, string } from 'yup';
import { object, ObjectSchema, string } from 'yup';

import backArrowIcon from '@assets/images/icn-back-arrow.svg';
import { DashboardPanel, InputField, NetworkSelector } from '@components';
import GeneralLookupField from '@components/GeneralLookupField';
import { DEFAULT_NETWORK } from '@config/constants';
import { useToasts } from '@features/Toasts';
import { useContacts, useNetworks } from '@services';
import { isValidETHAddress } from '@services/EthService';
import { isValidAddress } from '@services/EthService';
import { translateRaw } from '@translations';
import { Contact, NetworkId } from '@types';

Expand Down Expand Up @@ -63,8 +63,12 @@
const Schema = object().shape({
label: string().required(translateRaw('REQUIRED')),
address: object()
.test('check-eth-address', translateRaw('TO_FIELD_ERROR'), (value) =>
isValidETHAddress(value.value)
.when('network', (network: NetworkId, schema: ObjectSchema) =>
schema.test(

Check warning on line 67 in src/features/Settings/components/AddToAddressBook.tsx

View check run for this annotation

Codecov / codecov/patch

src/features/Settings/components/AddToAddressBook.tsx#L67

Added line #L67 was not covered by tests
'check-eth-address',
translateRaw('TO_FIELD_ERROR', { $network: network ?? DEFAULT_NETWORK }),
(value) => isValidAddress(value.value, getNetworkById(network).chainId)

Check warning on line 70 in src/features/Settings/components/AddToAddressBook.tsx

View check run for this annotation

Codecov / codecov/patch

src/features/Settings/components/AddToAddressBook.tsx#L70

Added line #L70 was not covered by tests
)
)
.test('doesnt-exist', translateRaw('ADDRESS_ALREADY_ADDED'), function (value) {
const contact = getContactByAddress(value.value);
Expand Down
55 changes: 42 additions & 13 deletions src/services/EthService/__tests__/validators.spec.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { ResolutionError } from '@unstoppabledomains/resolution';

import { NETWORKS_CONFIG } from '@database/data';
import { bigify } from '@utils';

import {
isENSLabelHash,
isTransactionFeeHigh,
isValidAddress,
isValidETHRecipientAddress,
isValidMixedCaseETHAddress,
isValidRecipientAddress,
isValidUpperOrLowerCaseETHAddress,
TxFeeResponseType,
validateTxFee
Expand Down Expand Up @@ -115,35 +116,51 @@ describe('validateTxFee', () => {
});
});

describe('isValidETHRecipientAddress', () => {
describe('isValidRecipientAddress', () => {
it('returns true for a valid checksummed address', () => {
const expected = { success: true };
const testAddress = '0x4bbeEB066eD09B7AEd07bF39EEe0460DFa261520';
const returned = isValidETHRecipientAddress(testAddress, undefined);
const returned = isValidRecipientAddress(testAddress, undefined);
expect(returned.success).toBe(expected.success);
});
it('returns true for a valid RSK checksummed address', () => {
const expected = { success: true };
const testAddress = '0x4bbEEb066Ed09b7aED07BF39eeE0460DFa261520';
const returned = isValidRecipientAddress(testAddress, undefined, NETWORKS_CONFIG.RSK);
expect(returned.success).toBe(expected.success);
});
it('returns true for a valid RSK Testnet checksummed address', () => {
const expected = { success: true };
const testAddress = '0x4BbEEb066eD09B7aEd07bf39eeE0460dfa261520';
const returned = isValidRecipientAddress(testAddress, undefined, NETWORKS_CONFIG.RSK_TESTNET);
expect(returned.success).toBe(expected.success);
});
it('returns true for a valid all-lowercase address', () => {
const expected = { success: true };
const testAddress = '0x4bbeeb066ed09b7aed07bf39eee0460dfa261520';
const returned = isValidETHRecipientAddress(testAddress, undefined);
expect(returned.success).toBe(expected.success);
const eth = isValidRecipientAddress(testAddress, undefined);
const rsk = isValidRecipientAddress(testAddress, undefined, NETWORKS_CONFIG.RSK);
const rskTestnet = isValidRecipientAddress(testAddress, undefined, NETWORKS_CONFIG.RSK_TESTNET);
expect(eth.success && rsk.success && rskTestnet.success).toBe(expected.success);
});
it('returns true for a valid all-uppercase address', () => {
const expected = { success: true };
const testAddress = '0X4BBEEB066ED09B7AED07BF39EEE0460DFA261520';
const returned = isValidETHRecipientAddress(testAddress, undefined);
expect(returned.success).toBe(expected.success);
const eth = isValidRecipientAddress(testAddress, undefined);
const rsk = isValidRecipientAddress(testAddress, undefined, NETWORKS_CONFIG.RSK);
const rskTestnet = isValidRecipientAddress(testAddress, undefined, NETWORKS_CONFIG.RSK_TESTNET);
expect(eth.success && rsk.success && rskTestnet.success).toBe(expected.success);
});
it('returns true for a valid ens name', () => {
const expected = { success: true };
const testAddress = 'mycryptoid.eth';
const returned = isValidETHRecipientAddress(testAddress, undefined);
const returned = isValidRecipientAddress(testAddress, undefined);
expect(returned.success).toBe(expected.success);
});
it('returns false for an invalid ens name', () => {
const expected = { success: false };
const testAddress = 'mycryptoid.ethhhhh';
const returned = isValidETHRecipientAddress(
const returned = isValidRecipientAddress(
testAddress,
('Domain mycryptoid.ethhhh is not supported' as unknown) as ResolutionError
);
Expand All @@ -152,7 +169,7 @@ describe('isValidETHRecipientAddress', () => {
it('returns false for an unresolved ens name', () => {
const expected = { success: false };
const testAddress = 'a.eth';
const returned = isValidETHRecipientAddress(
const returned = isValidRecipientAddress(
testAddress,
('Domain a.eth is not registered' as unknown) as ResolutionError
);
Expand All @@ -161,21 +178,33 @@ describe('isValidETHRecipientAddress', () => {
it('returns false for an invalid checksummed address', () => {
const expected = { success: false };
const testAddress = '0x4bbeEB066eD09B7AEd07bF39EEe0460DFA261520';
const returned = isValidETHRecipientAddress(testAddress, undefined);
const returned = isValidRecipientAddress(testAddress, undefined);
expect(returned.success).toBe(expected.success);
});
it('returns false for an invalid RSK checksummed address', () => {
const expected = { success: false };
const testAddress = '0x4BbEEb066eD09B7aEd07bf39eeE0460dfa261520';
const returned = isValidRecipientAddress(testAddress, undefined, NETWORKS_CONFIG.RSK);
expect(returned.success).toBe(expected.success);
});
it('returns false for an invalid RSK Testnet checksummed address', () => {
const expected = { success: false };
const testAddress = '0x4bbEEb066Ed09b7aED07BF39eeE0460DFa261520';
const returned = isValidRecipientAddress(testAddress, undefined, NETWORKS_CONFIG.RSK_TESTNET);
expect(returned.success).toBe(expected.success);
});

it('returns false for an invalid too-short address', () => {
const expected = { success: false };
const testAddress = '0x4bbeEB066eD09B7AEd07bF39E';
const returned = isValidETHRecipientAddress(testAddress, undefined);
const returned = isValidRecipientAddress(testAddress, undefined);
expect(returned.success).toBe(expected.success);
});

it('returns false for an invalid too-long address', () => {
const expected = { success: false };
const testAddress = '0x4bbeEB066eD09B7AEd07bF39EEe0460DFa2615200000';
const returned = isValidETHRecipientAddress(testAddress, undefined);
const returned = isValidRecipientAddress(testAddress, undefined);
expect(returned.success).toBe(expected.success);
});
});
Expand Down
2 changes: 1 addition & 1 deletion src/services/EthService/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export {
isTransactionFeeHigh,
isChecksumAddress,
isBurnAddress,
isValidETHRecipientAddress
isValidRecipientAddress
} from './validators';
export { ProviderHandler, getDPath, getDPaths } from './network';
export { isValidENSName } from './ens';
24 changes: 19 additions & 5 deletions src/services/EthService/validators.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { Network } from '@ethersproject/networks';
import { ALL_DERIVATION_PATHS } from '@mycrypto/wallets';
import { ResolutionError } from '@unstoppabledomains/resolution';
import BigNumber from 'bignumber.js';
Expand All @@ -12,6 +13,7 @@ import {
GAS_PRICE_GWEI_LOWER_BOUND,
GAS_PRICE_GWEI_UPPER_BOUND
} from '@config';
import { NETWORKS_CONFIG } from '@database/data';
import translate, { translateRaw } from '@translations';
import { InlineMessageType } from '@types';
import {
Expand Down Expand Up @@ -81,17 +83,29 @@ export function isValidETHLikeAddress(address: string, isChecksumValid: boolean)
return true;
}

export const isValidETHRecipientAddress = (
export const isValidRecipientAddress = (
address: string,
resolutionErr: ResolutionError | undefined
resolutionErr: ResolutionError | undefined,
network: Network = NETWORKS_CONFIG.Ethereum
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would prefer having network be undefined by default here. You can use ?? DEFAULT_NETWORK in cases where it is undefined. NETWORK_CONFIG is not the correct type.

) => {
if (isValidENSName(address) && resolutionErr) {
if (network.chainId === 30 || network.chainId === 31) {
// TODO: Consider checking RSN name if it will be supported by unstoppabledomains
// Is a invalid EIP-1191 checksum when trying to send through RSK network
return isValidRSKAddress(address, network.chainId)
? { success: true }
: {
success: false,
name: 'ValidationError',
type: InlineMessageType.ERROR,
message: translate('CHECKSUM_RSK_ERROR', { $network: network.name })
};
} else if (isValidENSName(address) && resolutionErr) {
// Is a valid ENS name, but it couldn't be resolved or there is some other issue.
return {
success: false,
name: 'ValidationError',
type: InlineMessageType.ERROR,
message: translateRaw('TO_FIELD_ERROR')
message: translateRaw('TO_FIELD_ERROR', { $network: network.name })
};
} else if (isValidENSName(address) && !resolutionErr) {
// Is a valid ENS name, and it can be resolved!
Expand Down Expand Up @@ -132,7 +146,7 @@ export const isValidETHRecipientAddress = (
success: false,
name: 'ValidationError',
type: InlineMessageType.ERROR,
message: translateRaw('TO_FIELD_ERROR')
message: translateRaw('TO_FIELD_ERROR', { $network: network.name })
};
}
return { success: true };
Expand Down
3 changes: 2 additions & 1 deletion src/translations/lang/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@
"CHECKOUT_CTA": "Next: Purchase for MyCrypto account",
"CHECKOUT_OTHER_CTA": "Next: Purchase for a different account",
"CHECKSUM_ERROR": "This address is not [checksummed](https://support.mycrypto.com/troubleshooting/sending/not-checksummed-shows-when-i-enter-an-address). Please ensure you have copied & pasted the address. Never hand-type addresses.",
"CHECKSUM_RSK_ERROR": "This address is not [$network checksummed](https://developers.rsk.co/rsk/architecture/account-based). Please ensure you have copied & pasted the address. Never hand-type addresses.",
"COMPLETE_PURCHASE": "Complete Purchase",
"COMPLETE_SWAP": "Complete Swap",
"CONFIRM_AND_SEND": "Confirm and Send",
Expand Down Expand Up @@ -701,7 +702,7 @@
"TOKEN_SEND_TOOLTIP": "Whenever you send a token on the Ethereum blockchain, you’re actually sending a request to the token contract to move the tokens around. This doesn’t require any extra action from you, but this is why you see this additional information.",
"TOKEN_SYMBOL": "Token Symbol ",
"TOTAL": "Total",
"TO_FIELD_ERROR": "Enter a valid address, ENS name, or blockchain domain",
"TO_FIELD_ERROR": "Enter a valid $network address, ENS name, or blockchain domain",
"TRANSACTION_BROADCASTED": "Transaction Broadcasted",
"TRANSACTION_BROADCASTED_BACK_TO_DASHBOARD": "Back to Dashboard",
"TRANSACTION_BROADCASTED_DESC": "Your transaction has been sent to the network and will be confirmed in the next few minutes unless [the network is busy](https://support.mycrypto.com/general-knowledge/ethereum-blockchain/why-is-gas) or there's [an error in the transaction](https://support.mycrypto.com/troubleshooting/sending/transactions-not-showing-or-pending).",
Expand Down