Skip to content

Commit

Permalink
fix: Fixes in NFT listing label and values (#29046)
Browse files Browse the repository at this point in the history
## **Description**

Fixes in NFT listing:
1. Fix order of listing state changes
2. Change label from `You receive` to `Listing price`
3. Show received value in gray background

## **Related issues**

Fixes: #28977

## **Manual testing steps**

1. Submit NFT listing confirmation
2. Check the confirmations page
3. Ensure that order of state changes is correct and label is also
correct

## **Screenshots/Recordings**
<img width="360" alt="Screenshot 2024-12-10 at 4 03 42 PM"
src="https://github.com/user-attachments/assets/ca286a6a-aca7-4ed2-9446-4056c154506e">

## **Pre-merge author checklist**

- [X] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [X] I've completed the PR template to the best of my ability
- [X] I’ve included tests if applicable
- [X] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [X] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
jpuri authored Jan 8, 2025
1 parent dabf173 commit 8d9fa1a
Show file tree
Hide file tree
Showing 6 changed files with 160 additions and 38 deletions.
3 changes: 3 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.

4 changes: 2 additions & 2 deletions test/e2e/tests/confirmations/signatures/nft-permit.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ describe('Confirmation Signature - NFT Permit @no-mmi', function (this: Suite) {
signatureType: 'eth_signTypedData_v4',
primaryType: 'Permit',
uiCustomizations: ['redesigned_confirmation', 'permit'],
decodingChangeTypes: ['RECEIVE', 'LISTING'],
decodingChangeTypes: ['LISTING', 'RECEIVE'],
decodingResponse: 'CHANGE',
});

Expand Down Expand Up @@ -115,7 +115,7 @@ describe('Confirmation Signature - NFT Permit @no-mmi', function (this: Suite) {
primaryType: 'Permit',
uiCustomizations: ['redesigned_confirmation', 'permit'],
location: 'confirmation',
decodingChangeTypes: ['RECEIVE', 'LISTING'],
decodingChangeTypes: ['LISTING', 'RECEIVE'],
decodingResponse: 'CHANGE',
});
},
Expand Down
8 changes: 4 additions & 4 deletions test/e2e/tests/confirmations/signatures/permit.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ describe('Confirmation Signature - Permit @no-mmi', function (this: Suite) {
signatureType: 'eth_signTypedData_v4',
primaryType: 'Permit',
uiCustomizations: ['redesigned_confirmation', 'permit'],
decodingChangeTypes: ['RECEIVE', 'LISTING'],
decodingChangeTypes: ['LISTING', 'RECEIVE'],
decodingResponse: 'CHANGE',
});

Expand Down Expand Up @@ -107,7 +107,7 @@ describe('Confirmation Signature - Permit @no-mmi', function (this: Suite) {
primaryType: 'Permit',
uiCustomizations: ['redesigned_confirmation', 'permit'],
location: 'confirmation',
decodingChangeTypes: ['RECEIVE', 'LISTING'],
decodingChangeTypes: ['LISTING', 'RECEIVE'],
decodingResponse: 'CHANGE',
});
},
Expand All @@ -126,12 +126,12 @@ describe('Confirmation Signature - Permit @no-mmi', function (this: Suite) {
const simulationSection = driver.findElement({
text: 'Estimated changes',
});
const receiveChange = driver.findElement({ text: 'You receive' });
const receiveChange = driver.findElement({ text: 'Listing price' });
const listChange = driver.findElement({ text: 'You list' });
const listChangeValue = driver.findElement({ text: '#2101' });

assert.ok(await simulationSection, 'Estimated changes');
assert.ok(await receiveChange, 'You receive');
assert.ok(await receiveChange, 'Listing price');
assert.ok(await listChange, 'You list');
assert.ok(await listChangeValue, '#2101');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ import {
import { getMockTypedSignConfirmStateForRequest } from '../../../../../../../../../test/data/confirmations/helper';
import { renderWithConfirmContextProvider } from '../../../../../../../../../test/lib/confirmations/render-helpers';
import { permitSignatureMsg } from '../../../../../../../../../test/data/confirmations/typed_sign';
import PermitSimulation, { getStateChangeToolip } from './decoded-simulation';
import PermitSimulation, {
getStateChangeType,
getStateChangeToolip,
StateChangeType,
} from './decoded-simulation';

const decodingData = {
stateChanges: [
Expand Down Expand Up @@ -57,21 +61,40 @@ const decodingDataListingERC1155: DecodingDataStateChanges = [
tokenID: '2233',
},
];
const decodingDataBidding: DecodingDataStateChanges = [

const nftListing: DecodingDataStateChanges = [
{
assetType: 'ERC721',
changeType: DecodingDataChangeType.Listing,
address: '',
amount: '',
contractAddress: '0x922dC160f2ab743312A6bB19DD5152C1D3Ecca33',
tokenID: '189',
},
{
assetType: 'ERC20',
changeType: DecodingDataChangeType.Receive,
address: '',
amount: '900000000000000000',
contractAddress: '',
amount: '950000000000000000',
contractAddress: '0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2',
},
];

const nftBidding: DecodingDataStateChanges = [
{
assetType: 'Native',
assetType: 'ERC20',
changeType: DecodingDataChangeType.Bidding,
address: '',
amount: '',
contractAddress: '0xafd4896984CA60d2feF66136e57f958dCe9482d5',
tokenID: '2101',
contractAddress: '0x922dC160f2ab743312A6bB19DD5152C1D3Ecca33',
tokenID: '189',
},
{
assetType: 'ERC721',
changeType: DecodingDataChangeType.Receive,
address: '',
amount: '950000000000000000',
contractAddress: '0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2',
},
];

Expand Down Expand Up @@ -137,7 +160,7 @@ describe('DecodedSimulation', () => {
);

expect(await findByText('Estimated changes')).toBeInTheDocument();
expect(await findByText('You receive')).toBeInTheDocument();
expect(await findByText('Listing price')).toBeInTheDocument();
expect(await findByText('You list')).toBeInTheDocument();
expect(await findByText('#2101')).toBeInTheDocument();
});
Expand Down Expand Up @@ -174,24 +197,74 @@ describe('DecodedSimulation', () => {
expect(await findByText('Unavailable')).toBeInTheDocument();
});

it('renders label only once if there are multiple state changes of same changeType', async () => {
const state = getMockTypedSignConfirmStateForRequest({
...permitSignatureMsg,
decodingLoading: false,
decodingData: {
stateChanges: [
decodingData.stateChanges[0],
decodingData.stateChanges[0],
decodingData.stateChanges[0],
],
},
});
const mockStore = configureMockStore([])(state);

const { findAllByText } = renderWithConfirmContextProvider(
<PermitSimulation />,
mockStore,
);

expect(await findAllByText('Spending cap')).toHaveLength(1);
});

it('for NFT permit label for receive should be "Listing price"', async () => {
const state = getMockTypedSignConfirmStateForRequest({
...permitSignatureMsg,
decodingLoading: false,
decodingData: {
stateChanges: nftListing,
},
});
const mockStore = configureMockStore([])(state);

const { findAllByText } = renderWithConfirmContextProvider(
<PermitSimulation />,
mockStore,
);

expect(await findAllByText('Listing price')).toHaveLength(1);
});

describe('getStateChangeToolip', () => {
it('return correct tooltip when permit is for listing NFT', async () => {
const tooltip = getStateChangeToolip(
decodingDataListing,
decodingDataListing?.[0],
StateChangeType.NFTListingReceive,
(str: string) => str,
);
expect(tooltip).toBe('signature_decoding_list_nft_tooltip');
});

it('return correct tooltip when permit is for bidding NFT', async () => {
const tooltip = getStateChangeToolip(
StateChangeType.NFTBiddingReceive,
(str: string) => str,
);
expect(tooltip).toBe('signature_decoding_bid_nft_tooltip');
});
});

it('return correct tooltip when permit is for bidding NFT', async () => {
const tooltip = getStateChangeToolip(
decodingDataBidding,
decodingDataBidding?.[0],
(str: string) => str,
);
expect(tooltip).toBe('signature_decoding_bid_nft_tooltip');
describe('getStateChangeType', () => {
it('return correct state change type for NFT listing receive', async () => {
const stateChange = getStateChangeType(nftListing, nftListing[1]);
expect(stateChange).toBe(StateChangeType.NFTListingReceive);
});

it('return correct state change type for NFT bidding receive', async () => {
const stateChange = getStateChangeType(nftBidding, nftBidding[1]);
expect(stateChange).toBe(StateChangeType.NFTBiddingReceive);
});
});

it('renders label only once if there are multiple state changes of same changeType', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,15 @@ import StaticSimulation from '../../../shared/static-simulation/static-simulatio
import TokenValueDisplay from '../value-display/value-display';
import NativeValueDisplay from '../native-value-display/native-value-display';

export const getStateChangeToolip = (
export enum StateChangeType {
NFTListingReceive = 'NFTListingReceive',
NFTBiddingReceive = 'NFTBiddingReceive',
}

export const getStateChangeType = (
stateChangeList: DecodingDataStateChanges | null,
stateChange: DecodingDataStateChange,
t: ReturnType<typeof useI18nContext>,
): string | undefined => {
): StateChangeType | undefined => {
if (stateChange.changeType === DecodingDataChangeType.Receive) {
if (
stateChangeList?.some(
Expand All @@ -29,32 +33,58 @@ export const getStateChangeToolip = (
change.assetType === TokenStandard.ERC721,
)
) {
return t('signature_decoding_list_nft_tooltip');
return StateChangeType.NFTListingReceive;
}
if (
stateChange.assetType === TokenStandard.ERC721 &&
stateChangeList?.some(
(change) => change.changeType === DecodingDataChangeType.Bidding,
)
) {
return t('signature_decoding_bid_nft_tooltip');
return StateChangeType.NFTBiddingReceive;
}
}
return undefined;
};

export const getStateChangeToolip = (
nftTransactionType: StateChangeType | undefined,
t: ReturnType<typeof useI18nContext>,
): string | undefined => {
if (nftTransactionType === StateChangeType.NFTListingReceive) {
return t('signature_decoding_list_nft_tooltip');
} else if (nftTransactionType === StateChangeType.NFTBiddingReceive) {
return t('signature_decoding_bid_nft_tooltip');
}
return undefined;
};

const stateChangeOrder = {
[DecodingDataChangeType.Transfer]: 1,
[DecodingDataChangeType.Listing]: 2,
[DecodingDataChangeType.Approve]: 3,
[DecodingDataChangeType.Revoke]: 4,
[DecodingDataChangeType.Bidding]: 5,
[DecodingDataChangeType.Receive]: 6,
};

const getStateChangeLabelMap = (
t: ReturnType<typeof useI18nContext>,
changeType: string,
) =>
({
stateChangeType?: StateChangeType,
) => {
return {
[DecodingDataChangeType.Transfer]: t('permitSimulationChange_transfer'),
[DecodingDataChangeType.Receive]: t('permitSimulationChange_receive'),
[DecodingDataChangeType.Receive]:
stateChangeType === StateChangeType.NFTListingReceive
? t('permitSimulationChange_nft_listing')
: t('permitSimulationChange_receive'),
[DecodingDataChangeType.Approve]: t('permitSimulationChange_approve'),
[DecodingDataChangeType.Revoke]: t('permitSimulationChange_revoke'),
[DecodingDataChangeType.Bidding]: t('permitSimulationChange_bidding'),
[DecodingDataChangeType.Listing]: t('permitSimulationChange_listing'),
}[changeType]);
}[changeType];
};

const StateChangeRow = ({
stateChangeList,
Expand All @@ -70,14 +100,19 @@ const StateChangeRow = ({
const t = useI18nContext();
const { assetType, changeType, amount, contractAddress, tokenID } =
stateChange;
const tooltip = getStateChangeToolip(stateChangeList, stateChange, t);
const nftTransactionType = getStateChangeType(stateChangeList, stateChange);
const tooltip = getStateChangeToolip(nftTransactionType, t);
const canDisplayValueAsUnlimited =
assetType === TokenStandard.ERC20 &&
(changeType === DecodingDataChangeType.Approve ||
changeType === DecodingDataChangeType.Revoke);
return (
<ConfirmInfoRow
label={shouldDisplayLabel ? getStateChangeLabelMap(t, changeType) : ''}
label={
shouldDisplayLabel
? getStateChangeLabelMap(t, changeType, nftTransactionType)
: ''
}
tooltip={tooltip}
>
{(assetType === TokenStandard.ERC20 ||
Expand All @@ -88,7 +123,10 @@ const StateChangeRow = ({
value={amount}
chainId={chainId}
tokenId={tokenID}
credit={changeType === DecodingDataChangeType.Receive}
credit={
nftTransactionType !== StateChangeType.NFTListingReceive &&
changeType === DecodingDataChangeType.Receive
}
debit={changeType === DecodingDataChangeType.Transfer}
canDisplayValueAsUnlimited={canDisplayValueAsUnlimited}
/>
Expand All @@ -97,7 +135,10 @@ const StateChangeRow = ({
<NativeValueDisplay
value={amount}
chainId={chainId}
credit={changeType === DecodingDataChangeType.Receive}
credit={
nftTransactionType !== StateChangeType.NFTListingReceive &&
changeType === DecodingDataChangeType.Receive
}
debit={changeType === DecodingDataChangeType.Transfer}
/>
)}
Expand All @@ -112,8 +153,13 @@ const DecodedSimulation: React.FC<object> = () => {
const { decodingLoading, decodingData } = currentConfirmation;

const stateChangeFragment = useMemo(() => {
const orderedStateChanges = decodingData?.stateChanges?.sort((c1, c2) =>
stateChangeOrder[c1.changeType] > stateChangeOrder[c2.changeType]
? 1
: -1,
);
const stateChangesGrouped: Record<string, DecodingDataStateChange[]> = (
decodingData?.stateChanges ?? []
orderedStateChanges ?? []
).reduce<Record<string, DecodingDataStateChange[]>>(
(result, stateChange) => {
result[stateChange.changeType] = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ describe('PermitSimulation', () => {
mockStore,
);

expect(await findByText('You receive')).toBeInTheDocument();
expect(await findByText('Listing price')).toBeInTheDocument();
expect(await findByText('You list')).toBeInTheDocument();
});
});
Expand Down

0 comments on commit 8d9fa1a

Please sign in to comment.