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

Add setApprovalForAll confirmation view #15010

Merged
merged 3 commits into from
Jul 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 26 additions & 0 deletions app/_locales/en/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions shared/constants/transaction.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import { MESSAGE_TYPE } from './app';
* to ensure that the receiver is an address capable of handling with the token being sent.
* @property {'approve'} TOKEN_METHOD_APPROVE - A token transaction requesting an
* allowance of the token to spend on behalf of the user
* @property {'setapprovalforall'} TOKEN_METHOD_SET_APPROVAL_FOR_ALL - A token transaction requesting an
* allowance of all of a user's token to spend on behalf of the user
* @property {'incoming'} INCOMING - An incoming (deposit) transaction
* @property {'simpleSend'} SIMPLE_SEND - A transaction sending a network's native asset to a recipient
* @property {'contractInteraction'} CONTRACT_INTERACTION - A transaction that is
Expand Down Expand Up @@ -66,6 +68,7 @@ export const TRANSACTION_TYPES = {
TOKEN_METHOD_SAFE_TRANSFER_FROM: 'safetransferfrom',
TOKEN_METHOD_TRANSFER: 'transfer',
TOKEN_METHOD_TRANSFER_FROM: 'transferfrom',
TOKEN_METHOD_SET_APPROVAL_FOR_ALL: 'setapprovalforall',
};

/**
Expand Down
5 changes: 4 additions & 1 deletion shared/modules/transaction.utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { readAddressAsContract } from './contract-utils';
import { isEqualCaseInsensitive } from './string-utils';

/**
* @typedef { 'transfer' | 'approve' | 'transferfrom' | 'contractInteraction'| 'simpleSend' } InferrableTransactionTypes
* @typedef { 'transfer' | 'approve' | 'setapprovalforall' | 'transferfrom' | 'contractInteraction'| 'simpleSend' } InferrableTransactionTypes
*/

/**
Expand Down Expand Up @@ -150,6 +150,7 @@ export async function determineTransactionType(txParams, query) {

const tokenMethodName = [
TRANSACTION_TYPES.TOKEN_METHOD_APPROVE,
TRANSACTION_TYPES.TOKEN_METHOD_SET_APPROVAL_FOR_ALL,
TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER,
TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER_FROM,
TRANSACTION_TYPES.TOKEN_METHOD_SAFE_TRANSFER_FROM,
Expand Down Expand Up @@ -181,6 +182,7 @@ export async function determineTransactionType(txParams, query) {

const INFERRABLE_TRANSACTION_TYPES = [
TRANSACTION_TYPES.TOKEN_METHOD_APPROVE,
TRANSACTION_TYPES.TOKEN_METHOD_SET_APPROVAL_FOR_ALL,
TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER,
TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER_FROM,
TRANSACTION_TYPES.CONTRACT_INTERACTION,
Expand Down Expand Up @@ -220,6 +222,7 @@ export async function determineTransactionAssetType(
// method to get the asset type.
const isTokenMethod = [
TRANSACTION_TYPES.TOKEN_METHOD_APPROVE,
TRANSACTION_TYPES.TOKEN_METHOD_SET_APPROVAL_FOR_ALL,
TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER,
TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER_FROM,
].find((methodName) => methodName === inferrableType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ const ConfirmPageContainerSummary = (props) => {
contractAddress =
transactionType === TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER ||
transactionType === TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER_FROM ||
transactionType === TRANSACTION_TYPES.TOKEN_METHOD_SAFE_TRANSFER_FROM
transactionType === TRANSACTION_TYPES.TOKEN_METHOD_SAFE_TRANSFER_FROM ||
transactionType === TRANSACTION_TYPES.TOKEN_METHOD_SET_APPROVAL_FOR_ALL
? tokenAddress
: toAddress;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,10 @@ export default class TransactionListItemDetails extends PureComponent {
<div className="transaction-list-item-details__cards-container">
<TransactionBreakdown
nonce={transactionGroup.initialTransaction.txParams.nonce}
isTokenApprove={type === TRANSACTION_TYPES.TOKEN_METHOD_APPROVE}
isTokenApprove={
type === TRANSACTION_TYPES.TOKEN_METHOD_APPROVE ||
type === TRANSACTION_TYPES.TOKEN_METHOD_SET_APPROVAL_FOR_ALL
}
transaction={transaction}
primaryCurrency={primaryCurrency}
className="transaction-list-item-details__transaction-breakdown"
Expand Down
3 changes: 3 additions & 0 deletions ui/helpers/constants/routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ const CONFIRM_SEND_ETHER_PATH = '/send-ether';
const CONFIRM_SEND_TOKEN_PATH = '/send-token';
const CONFIRM_DEPLOY_CONTRACT_PATH = '/deploy-contract';
const CONFIRM_APPROVE_PATH = '/approve';
const CONFIRM_SET_APPROVAL_FOR_ALL_PATH = '/set-approval-for-all';
const CONFIRM_TRANSFER_FROM_PATH = '/transfer-from';
const CONFIRM_SAFE_TRANSFER_FROM_PATH = '/safe-transfer-from';
const CONFIRM_TOKEN_METHOD_PATH = '/token-method';
Expand Down Expand Up @@ -145,6 +146,7 @@ const PATH_NAME_MAP = {
[`${CONFIRM_TRANSACTION_ROUTE}/:id${CONFIRM_SEND_TOKEN_PATH}`]: 'Confirm Send Token Transaction Page',
[`${CONFIRM_TRANSACTION_ROUTE}/:id${CONFIRM_DEPLOY_CONTRACT_PATH}`]: 'Confirm Deploy Contract Transaction Page',
[`${CONFIRM_TRANSACTION_ROUTE}/:id${CONFIRM_APPROVE_PATH}`]: 'Confirm Approve Transaction Page',
[`${CONFIRM_TRANSACTION_ROUTE}/:id${CONFIRM_SET_APPROVAL_FOR_ALL_PATH}`]: 'Confirm Set Approval For All Transaction Page',
[`${CONFIRM_TRANSACTION_ROUTE}/:id${CONFIRM_TRANSFER_FROM_PATH}`]: 'Confirm Transfer From Transaction Page',
[`${CONFIRM_TRANSACTION_ROUTE}/:id${CONFIRM_SAFE_TRANSFER_FROM_PATH}`]: 'Confirm Safe Transfer From Transaction Page',
[`${CONFIRM_TRANSACTION_ROUTE}/:id${SIGNATURE_REQUEST_PATH}`]: 'Signature Request Page',
Expand Down Expand Up @@ -206,6 +208,7 @@ export {
CONFIRM_SEND_TOKEN_PATH,
CONFIRM_DEPLOY_CONTRACT_PATH,
CONFIRM_APPROVE_PATH,
CONFIRM_SET_APPROVAL_FOR_ALL_PATH,
CONFIRM_TRANSFER_FROM_PATH,
CONFIRM_SAFE_TRANSFER_FROM_PATH,
CONFIRM_TOKEN_METHOD_PATH,
Expand Down
1 change: 1 addition & 0 deletions ui/helpers/constants/transactions.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export const PRIORITY_STATUS_HASH = {

export const TOKEN_CATEGORY_HASH = {
[TRANSACTION_TYPES.TOKEN_METHOD_APPROVE]: true,
[TRANSACTION_TYPES.TOKEN_METHOD_SET_APPROVAL_FOR_ALL]: true,
[TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER]: true,
[TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER_FROM]: true,
};
Expand Down
4 changes: 4 additions & 0 deletions ui/helpers/utils/token-util.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,10 @@ export function getTokenValueParam(tokenData = {}) {
return tokenData?.args?._value?.toString();
}

export function getTokenApprovedParam(tokenData = {}) {
return tokenData?.args?._approved;
}

export function getTokenValue(tokenParams = []) {
const valueData = tokenParams.find((param) => param.name === '_value');
return valueData && valueData.value;
Expand Down
4 changes: 4 additions & 0 deletions ui/helpers/utils/transactions.util.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ export function isTokenMethodAction(type) {
return [
TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER,
TRANSACTION_TYPES.TOKEN_METHOD_APPROVE,
TRANSACTION_TYPES.TOKEN_METHOD_SET_APPROVAL_FOR_ALL,
TRANSACTION_TYPES.TOKEN_METHOD_TRANSFER_FROM,
TRANSACTION_TYPES.TOKEN_METHOD_SAFE_TRANSFER_FROM,
].includes(type);
Expand Down Expand Up @@ -217,6 +218,9 @@ export function getTransactionTypeTitle(t, type, nativeCurrency = 'ETH') {
case TRANSACTION_TYPES.TOKEN_METHOD_APPROVE: {
return t('approve');
}
case TRANSACTION_TYPES.TOKEN_METHOD_SET_APPROVAL_FOR_ALL: {
return t('setApprovalForAll');
}
case TRANSACTION_TYPES.SIMPLE_SEND: {
return t('sendingNativeAsset', [nativeCurrency]);
}
Expand Down
6 changes: 6 additions & 0 deletions ui/hooks/useTransactionDisplayData.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,12 @@ export function useTransactionDisplayData(transactionGroup) {
title = t('approveSpendLimit', [token?.symbol || t('token')]);
subtitle = origin;
subtitleContainsOrigin = true;
} else if (type === TRANSACTION_TYPES.TOKEN_METHOD_SET_APPROVAL_FOR_ALL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this type actually get set anywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

@digiwand when you tested this, did you see the setApprovalForAllTitle in the transaction history list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-07-11 at 2 41 32 PM

Should be able to! I see it. cc @digiwand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its set here in the method determineTransactionType

category = TRANSACTION_GROUP_CATEGORIES.APPROVAL;
prefix = '';
title = t('setApprovalForAllTitle', [token?.symbol || t('token')]);
subtitle = origin;
subtitleContainsOrigin = true;
} else if (type === TRANSACTION_TYPES.CONTRACT_INTERACTION) {
category = TRANSACTION_GROUP_CATEGORIES.INTERACTION;
const transactionTypeTitle = getTransactionTypeTitle(t, type);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ export default class ConfirmApproveContent extends Component {
assetName: PropTypes.string,
tokenId: PropTypes.string,
assetStandard: PropTypes.string,
isSetApproveForAll: PropTypes.bool,
setApproveForAllArg: PropTypes.bool,
};

state = {
Expand Down Expand Up @@ -184,7 +186,7 @@ export default class ConfirmApproveContent extends Component {

renderERC721OrERC1155PermissionContent() {
const { t } = this.context;
const { origin, toAddress, isContract } = this.props;
const { origin, toAddress, isContract, isSetApproveForAll } = this.props;

const titleTokenDescription = this.getTitleTokenDescription();

Expand All @@ -201,7 +203,9 @@ export default class ConfirmApproveContent extends Component {
{t('approvedAsset')}:
</div>
<div className="confirm-approve-content__medium-text">
{titleTokenDescription}
{isSetApproveForAll
? `${t('allOfYour', [titleTokenDescription])} `
: titleTokenDescription}
</div>
</div>
<div className="flex-row">
Expand Down Expand Up @@ -299,12 +303,19 @@ export default class ConfirmApproveContent extends Component {

renderDataContent() {
const { t } = this.context;
const { data } = this.props;
const { data, isSetApproveForAll, setApproveForAllArg } = this.props;
return (
<div className="flex-column">
<div className="confirm-approve-content__small-text">
{t('functionApprove')}
{isSetApproveForAll
? t('functionSetApprovalForAll')
: t('functionApprove')}
</div>
{isSetApproveForAll && setApproveForAllArg !== undefined ? (
<div className="confirm-approve-content__small-text">
{t('parameters')}: {setApproveForAllArg}
</div>
) : null}
<div className="confirm-approve-content__small-text confirm-approve-content__data__data-block">
{data}
</div>
Expand Down Expand Up @@ -509,6 +520,41 @@ export default class ConfirmApproveContent extends Component {
return titleTokenDescription;
}

renderTitle() {
const { t } = this.context;
const { isSetApproveForAll, setApproveForAllArg } = this.props;
const titleTokenDescription = this.getTitleTokenDescription();

let title;
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be let title = t('allowSpendToken', [titleTokenDescription]);, and we avoid the || in the return statement, but I won't block on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol this is what I had before and Ariella suggested this way: #15010 (comment)

I don't have much of an opinion on this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

haha sorry @adonesky1 and @darkwing. I actually liked the || to possibly do one less translation here. I can switch the preference to @darkwing 's for future implementations


if (isSetApproveForAll) {
title = t('approveAllTokensTitle', [titleTokenDescription]);
if (setApproveForAllArg === false) {
title = t('revokeAllTokensTitle', [titleTokenDescription]);
}
}
return title || t('allowSpendToken', [titleTokenDescription]);
}

renderDescription() {
const { t } = this.context;
const { isContract, isSetApproveForAll, setApproveForAllArg } = this.props;
const grantee = isContract
? t('contract').toLowerCase()
: t('account').toLowerCase();

let description = t('trustSiteApprovePermission', [grantee]);

if (isSetApproveForAll && setApproveForAllArg === false) {
description = t('revokeApproveForAllDescription', [
grantee,
this.getTitleTokenDescription(),
]);
}

return description;
}

render() {
const { t } = this.context;
const {
Expand All @@ -534,8 +580,6 @@ export default class ConfirmApproveContent extends Component {
} = this.props;
const { showFullTxDetails } = this.state;

const titleTokenDescription = this.getTitleTokenDescription();

return (
<div
className={classnames('confirm-approve-content', {
Expand Down Expand Up @@ -575,14 +619,10 @@ export default class ConfirmApproveContent extends Component {
</Box>
</Box>
<div className="confirm-approve-content__title">
{t('allowSpendToken', [titleTokenDescription])}
{this.renderTitle()}
</div>
<div className="confirm-approve-content__description">
{t('trustSiteApprovePermission', [
isContract
? t('contract').toLowerCase()
: t('account').toLowerCase(),
])}
{this.renderDescription()}
</div>
<Box className="confirm-approve-content__address-display-content">
<Box display={DISPLAY.FLEX}>
Expand Down
15 changes: 14 additions & 1 deletion ui/pages/confirm-approve/confirm-approve.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ import {
updateCustomNonce,
getNextNonce,
} from '../../store/actions';
import { calcTokenAmount } from '../../helpers/utils/token-util';
import {
calcTokenAmount,
getTokenApprovedParam,
} from '../../helpers/utils/token-util';
import { readAddressAsContract } from '../../../shared/modules/contract-utils';
import { GasFeeContextProvider } from '../../contexts/gasFee';
import { TransactionModalContextProvider } from '../../contexts/transaction-modal';
Expand All @@ -34,6 +37,7 @@ import EditGasFeePopover from '../../components/app/edit-gas-fee-popover';
import EditGasPopover from '../../components/app/edit-gas-popover/edit-gas-popover.component';
import Loading from '../../components/ui/loading-screen';
import { ERC20, ERC1155, ERC721 } from '../../helpers/constants/common';
import { parseStandardTokenTransactionData } from '../../../shared/modules/transaction.utils';
import { getCustomTxParamsData } from './confirm-approve.util';
import ConfirmApproveContent from './confirm-approve-content';

Expand All @@ -57,6 +61,7 @@ export default function ConfirmApprove({
ethTransactionTotal,
fiatTransactionTotal,
hexTransactionTotal,
isSetApproveForAll,
}) {
const dispatch = useDispatch();
const { txParams: { data: transactionData } = {} } = transaction;
Expand Down Expand Up @@ -150,6 +155,11 @@ export default function ConfirmApprove({
})
: null;

const parsedTransactionData = parseStandardTokenTransactionData(
transactionData,
);
const setApproveForAllArg = getTokenApprovedParam(parsedTransactionData);

return tokenSymbol === undefined && assetName === undefined ? (
<Loading />
) : (
Expand All @@ -162,6 +172,8 @@ export default function ConfirmApprove({
contentComponent={
<TransactionModalContextProvider>
<ConfirmApproveContent
isSetApproveForAll={isSetApproveForAll}
setApproveForAllArg={setApproveForAllArg}
decimals={decimals}
siteImage={siteImage}
setCustomAmount={setCustomPermissionAmount}
Expand Down Expand Up @@ -290,4 +302,5 @@ ConfirmApprove.propTypes = {
ethTransactionTotal: PropTypes.string,
fiatTransactionTotal: PropTypes.string,
hexTransactionTotal: PropTypes.string,
isSetApproveForAll: PropTypes.bool,
};
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ export default class ConfirmTransactionBase extends Component {
};

const hasSimulationError = Boolean(txData.simulationFails);

const renderSimulationFailureWarning =
hasSimulationError && !userAcknowledgedGasMissing;
const networkName = NETWORK_TO_NAME_MAP[txData.chainId];
Expand Down