Skip to content

Commit

Permalink
Fix wrong messages during MWP Flow (#6094)
Browse files Browse the repository at this point in the history
* add logging for pkey errors

* fix keychain error handling

* checksum address before keychain lookup

* remove invalid chainId param from tx

* Update src/keychain/index.ts

* Update src/keychain/index.ts

Co-authored-by: Jin <jin.chung17@gmail.com>

* Explicitly return UserCanceled error for code strings 10 and 13

* Remove returning -3 explicit error code

* Use enum values instead of primitives for error codes for readability

---------

Co-authored-by: Jin <jin.chung17@gmail.com>
(cherry picked from commit 82dca43)
  • Loading branch information
brunobar79 authored and BrodyHughes committed Sep 10, 2024
1 parent 9ccbb6b commit 8e24521
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 22 deletions.
24 changes: 16 additions & 8 deletions src/keychain/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ export async function get(key: string, options: KeychainOptions = {}): Promise<R
error: e.toString(),
},
});

switch (e.toString()) {
/*
* Can happen if the user initially had biometrics enabled, installed
Expand Down Expand Up @@ -170,14 +171,21 @@ export async function get(key: string, options: KeychainOptions = {}): Promise<R
return _get(attempts + 1);
}
default: {
logger.error(new RainbowError(`[keychain]: _get() handled unknown error`), {
message: e.toString(),
});

return {
value: undefined,
error: ErrorType.Unknown,
};
// Avoid logging user cancelled operations
if (e.toString().includes('code: 10') || e.toString().includes('code: 13')) {
return {
value: undefined,
error: ErrorType.UserCanceled,
};
} else {
logger.error(new RainbowError(`[keychain]: _get() handled unknown error`), {
message: e.toString(),
});
return {
value: undefined,
error: ErrorType.Unknown,
};
}
}
}
}
Expand Down
52 changes: 39 additions & 13 deletions src/model/wallet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,10 @@ export const loadWallet = async <S extends Screen>({
privateKey = await loadPrivateKey(addressToUse, isHardwareWallet);
}

if (privateKey === -1 || privateKey === -2) {
// kc.ErrorType.UserCanceled means the user cancelled, so we don't wanna do anything
// kc.ErrorType.NotAuthenticated means the user is not authenticated (maybe removed biometrics).
// In this case we show an alert inside loadPrivateKey
if (privateKey === kc.ErrorType.UserCanceled || privateKey === kc.ErrorType.NotAuthenticated) {
return null;
}
if (isHardwareWalletKey(privateKey)) {
Expand Down Expand Up @@ -536,7 +539,10 @@ export const oldLoadSeedPhrase = async (): Promise<null | EthereumWalletSeed> =>

export const loadAddress = (): Promise<null | EthereumAddress> => keychain.loadString(addressKey) as Promise<string | null>;

export const loadPrivateKey = async (address: EthereumAddress, hardware: boolean): Promise<null | EthereumPrivateKey | -1 | -2> => {
export const loadPrivateKey = async (
address: EthereumAddress,
hardware: boolean
): Promise<null | EthereumPrivateKey | kc.ErrorType.UserCanceled | kc.ErrorType.NotAuthenticated> => {
try {
const isSeedPhraseMigrated = await keychain.loadString(oldSeedPhraseMigratedKey);

Expand All @@ -550,8 +556,8 @@ export const loadPrivateKey = async (address: EthereumAddress, hardware: boolean

if (!privateKey) {
const privateKeyData = await getKeyForWallet(address, hardware);
if (privateKeyData === -1) {
return -1;
if (privateKeyData === kc.ErrorType.UserCanceled || privateKeyData === kc.ErrorType.NotAuthenticated) {
return privateKeyData;
}
privateKey = privateKeyData?.privateKey ?? null;
}
Expand Down Expand Up @@ -911,9 +917,12 @@ export const saveKeyForWallet = async (
* @desc Gets wallet keys for the given address depending wallet type
* @param address The wallet address.
* @param hardware If the wallet is a hardware wallet.
* @return null | PrivateKeyData | -1
* @return null | PrivateKeyData | kc.ErrorType.UserCanceled | kc.ErrorType.NotAuthenticated
*/
export const getKeyForWallet = async (address: EthereumAddress, hardware: boolean): Promise<null | PrivateKeyData | -1> => {
export const getKeyForWallet = async (
address: EthereumAddress,
hardware: boolean
): Promise<null | PrivateKeyData | kc.ErrorType.UserCanceled | kc.ErrorType.NotAuthenticated> => {
if (hardware) {
return await getHardwareKey(address);
} else {
Expand Down Expand Up @@ -971,9 +980,11 @@ export const saveHardwareKey = async (
/**
* @desc Gets wallet private key for a given address.
* @param address The wallet address.
* @return null | PrivateKeyData | -1
* @return null | PrivateKeyData | kc.ErrorType.UserCanceled | kc.ErrorType.NotAuthenticated
*/
export const getPrivateKey = async (address: EthereumAddress): Promise<null | PrivateKeyData | -1> => {
export const getPrivateKey = async (
address: EthereumAddress
): Promise<null | PrivateKeyData | kc.ErrorType.UserCanceled | kc.ErrorType.NotAuthenticated> => {
try {
const key = `${address}_${privateKeyKey}`;
const options = { authenticationPrompt };
Expand All @@ -984,11 +995,26 @@ export const getPrivateKey = async (address: EthereumAddress): Promise<null | Pr
androidEncryptionPin,
});

if (error === -2) {
Alert.alert(lang.t('wallet.authenticate.alert.error'), lang.t('wallet.authenticate.alert.current_authentication_not_secure_enough'));
return null;
switch (error) {
case kc.ErrorType.UserCanceled:
// User Cancelled - We want to bubble up this error code. No need to track it.
return kc.ErrorType.UserCanceled;
case kc.ErrorType.NotAuthenticated:
// Alert the user and bubble up the error code.
Alert.alert(
lang.t('wallet.authenticate.alert.error'),
lang.t('wallet.authenticate.alert.current_authentication_not_secure_enough')
);
return kc.ErrorType.NotAuthenticated;
case kc.ErrorType.Unavailable:
// This means we couldn't find any matches for this key.
logger.error(new RainbowError('KC unavailable for PKEY lookup'), { error });
break;
default:
// This is an unknown error
logger.error(new RainbowError('KC unknown error for PKEY lookup'), { error });
break;
}

return pkey || null;
} catch (error) {
logger.error(new RainbowError('[wallet]: Error in getPrivateKey'), { error });
Expand Down Expand Up @@ -1043,7 +1069,7 @@ export const getSeedPhrase = async (
androidEncryptionPin,
});

if (error === -2) {
if (error === kc.ErrorType.NotAuthenticated) {
Alert.alert(lang.t('wallet.authenticate.alert.error'), lang.t('wallet.authenticate.alert.current_authentication_not_secure_enough'));
return null;
}
Expand Down
3 changes: 2 additions & 1 deletion src/screens/SignTransactionSheet.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ import { useNonceForDisplay } from '@/hooks/useNonceForDisplay';
import { useProviderSetup } from '@/hooks/useProviderSetup';
import { useTransactionSubmission } from '@/hooks/useSubmitTransaction';
import { useConfirmTransaction } from '@/hooks/useConfirmTransaction';
import { toChecksumAddress } from 'ethereumjs-util';

type SignTransactionSheetParams = {
transactionDetails: RequestData;
Expand Down Expand Up @@ -332,7 +333,7 @@ export const SignTransactionSheet = () => {
screen: SCREEN_FOR_REQUEST_SOURCE[source],
operation: TimeToSignOperation.KeychainRead,
})({
address: accountInfo.address,
address: toChecksumAddress(accountInfo.address),
provider: providerToUse,
timeTracking: {
screen: SCREEN_FOR_REQUEST_SOURCE[source],
Expand Down
2 changes: 2 additions & 0 deletions src/utils/ethereumUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,8 @@ const calculateL1FeeOptimism = async (tx: RainbowTransaction, provider: Provider
newTx.nonce = Number(await provider.getTransactionCount(newTx.from));
}

// @ts-expect-error operand should be optional
delete newTx?.chainId;
// @ts-expect-error operand should be optional
delete newTx?.from;
// @ts-expect-error gas is not in type RainbowTransaction
Expand Down

0 comments on commit 8e24521

Please sign in to comment.