feat: redesign approval editor

package.json

@@ -59,7 +59,7 @@
     "@safe-global/protocol-kit": "^3.0.2",
     "@safe-global/safe-apps-sdk": "^9.0.0-next.1",
     "@safe-global/safe-deployments": "^1.34.0",
-    "@safe-global/safe-gateway-typescript-sdk": "3.19.0",
+    "@safe-global/safe-gateway-typescript-sdk": "3.21.0",
     "@safe-global/safe-modules-deployments": "^1.2.0",
     "@sentry/react": "^7.91.0",
     "@spindl-xyz/attribution-lite": "^1.4.0",

src/components/theme/safeTheme.ts

@@ -57,6 +57,12 @@ declare module '@mui/material/Button' {
   }
 }
 
+declare module '@mui/material/IconButton' {
+  export interface IconButtonPropsColorOverrides {
+    border: true
+  }
+}
+
 const createSafeTheme = (mode: PaletteMode): Theme => {
   const isDarkMode = mode === 'dark'
   const colors = isDarkMode ? darkPalette : palette

src/components/tx/ApprovalEditor/ApprovalEditor.test.tsx

@@ -5,6 +5,8 @@ import { TokenType } from '@safe-global/safe-gateway-typescript-sdk'
 import { OperationType } from '@safe-global/safe-core-sdk-types'
 import * as approvalInfos from '@/components/tx/ApprovalEditor/hooks/useApprovalInfos'
 import { createMockSafeTransaction } from '@/tests/transactions'
+import { faker } from '@faker-js/faker'
+import { shortenAddress } from '@/utils/formatters'
 
 describe('ApprovalEditor', () => {
   beforeEach(() => {
@@ -18,7 +20,11 @@ describe('ApprovalEditor', () => {
   })
 
   it('returns null if there are no approvals', () => {
-    const mockSafeTx = createMockSafeTransaction({ to: '0x1', data: '0x', operation: OperationType.DelegateCall })
+    const mockSafeTx = createMockSafeTransaction({
+      to: faker.finance.ethereumAddress(),
+      data: '0x',
+      operation: OperationType.DelegateCall,
+    })
     jest.spyOn(approvalInfos, 'useApprovalInfos').mockReturnValue([[], undefined, false])
     const result = render(<ApprovalEditor safeTransaction={mockSafeTx} />)
 
@@ -29,7 +35,11 @@ describe('ApprovalEditor', () => {
     jest
       .spyOn(approvalInfos, 'useApprovalInfos')
       .mockReturnValue([undefined, new Error('Error parsing approvals'), false])
-    const mockSafeTx = createMockSafeTransaction({ to: '0x1', data: '0x', operation: OperationType.DelegateCall })
+    const mockSafeTx = createMockSafeTransaction({
+      to: faker.finance.ethereumAddress(),
+      data: '0x',
+      operation: OperationType.DelegateCall,
+    })
 
     const result = render(<ApprovalEditor safeTransaction={mockSafeTx} />)
 
@@ -46,18 +56,21 @@ describe('ApprovalEditor', () => {
   })
 
   it('renders a read-only view if the transaction contains signatures', async () => {
+    const tokenAddress = faker.finance.ethereumAddress()
+    const spenderAddress = faker.finance.ethereumAddress()
     const mockApprovalInfo = {
-      tokenInfo: { symbol: 'TST', decimals: 18, address: '0x3', type: TokenType.ERC20 },
-      tokenAddress: '0x1',
-      spender: '0x2',
+      tokenInfo: { symbol: 'TST', decimals: 18, address: tokenAddress, type: TokenType.ERC20 },
+      tokenAddress,
+      spender: spenderAddress,
       amount: '4200000',
       amountFormatted: '420.0',
       method: 'approve',
+      transactionIndex: 0,
     } as const
     jest.spyOn(approvalInfos, 'useApprovalInfos').mockReturnValue([[mockApprovalInfo], undefined, false])
     const mockSafeTx = safeTxBuilder()
       .with({
-        signatures: new Map().set('0x1', safeSignatureBuilder().build()),
+        signatures: new Map().set(faker.finance.ethereumAddress(), safeSignatureBuilder().build()),
       })
       .build()
 
@@ -66,22 +79,29 @@ describe('ApprovalEditor', () => {
     const amountInput = result.container.querySelector('input[name="approvals.0"]') as HTMLInputElement
 
     expect(amountInput).not.toBeInTheDocument()
-    expect(result.getByText('TST'))
-    expect(result.getByText('420'))
-    expect(result.getByText('0x2'))
+    expect(result.getByText('TST', { exact: false }))
+    expect(result.getByText('420', { exact: false }))
+    expect(result.getByText(shortenAddress(spenderAddress)))
   })
 
   it('renders a form if there are no signatures', async () => {
+    const tokenAddress = faker.finance.ethereumAddress()
+    const spenderAddress = faker.finance.ethereumAddress()
     const mockApprovalInfo = {
-      tokenInfo: { symbol: 'TST', decimals: 18, address: '0x3', type: TokenType.ERC20 },
-      tokenAddress: '0x1',
-      spender: '0x2',
+      tokenInfo: { symbol: 'TST', decimals: 18, address: tokenAddress, type: TokenType.ERC20 },
+      tokenAddress,
+      spender: spenderAddress,
       amount: '4200000',
       amountFormatted: '420.0',
       method: 'approve',
+      transactionIndex: 0,
     } as const
     jest.spyOn(approvalInfos, 'useApprovalInfos').mockReturnValue([[mockApprovalInfo], undefined, false])
-    const mockSafeTx = createMockSafeTransaction({ to: '0x1', data: '0x', operation: OperationType.DelegateCall })
+    const mockSafeTx = createMockSafeTransaction({
+      to: tokenAddress,
+      data: '0x',
+      operation: OperationType.DelegateCall,
+    })
 
     const result = render(<ApprovalEditor safeTransaction={mockSafeTx} />)
 
@@ -90,21 +110,28 @@ describe('ApprovalEditor', () => {
     expect(amountInput1).toBeInTheDocument
 
     expect(amountInput1).toHaveValue('420.0')
-    expect(result.getByText('TST'))
-    expect(result.getByText('0x2'))
+    expect(result.getByText('TST', { exact: false }))
+    expect(result.getByText(shortenAddress(spenderAddress)))
   })
 
   it('renders a form if there is an update callback', async () => {
+    const tokenAddress = faker.finance.ethereumAddress()
+    const spenderAddress = faker.finance.ethereumAddress()
     const mockApprovalInfo = {
-      tokenInfo: { symbol: 'TST', decimals: 18, address: '0x3', type: TokenType.ERC20 },
-      tokenAddress: '0x1',
-      spender: '0x2',
+      tokenInfo: { symbol: 'TST', decimals: 18, address: tokenAddress, type: TokenType.ERC20 },
+      tokenAddress,
+      spender: spenderAddress,
       amount: '4200000',
       amountFormatted: '420.0',
       method: 'approve',
+      transactionIndex: 0,
     } as const
     jest.spyOn(approvalInfos, 'useApprovalInfos').mockReturnValue([[mockApprovalInfo], undefined, false])
-    const mockSafeTx = createMockSafeTransaction({ to: '0x1', data: '0x', operation: OperationType.DelegateCall })
+    const mockSafeTx = createMockSafeTransaction({
+      to: tokenAddress,
+      data: '0x',
+      operation: OperationType.DelegateCall,
+    })
 
     const result = render(<ApprovalEditor safeTransaction={mockSafeTx} />)
 

src/components/tx/ApprovalEditor/ApprovalEditorForm.test.tsx

@@ -1,9 +1,10 @@
-import { fireEvent, render, waitFor } from '@/tests/test-utils'
+import { act, fireEvent, render, waitFor } from '@/tests/test-utils'
 import { toBeHex } from 'ethers'
 import { TokenType } from '@safe-global/safe-gateway-typescript-sdk'
 import { ApprovalEditorForm } from '@/components/tx/ApprovalEditor/ApprovalEditorForm'
 import { getAllByTestId, getAllByTitle } from '@testing-library/dom'
 import type { ApprovalInfo } from './hooks/useApprovalInfos'
+import { faker } from '@faker-js/faker'
 
 describe('ApprovalEditorForm', () => {
   beforeEach(() => {
@@ -15,23 +16,25 @@ describe('ApprovalEditorForm', () => {
   it('should render and edit multiple txs', async () => {
     const tokenAddress1 = toBeHex('0x123', 20)
     const tokenAddress2 = toBeHex('0x234', 20)
-
+    const spenderAddress = faker.finance.ethereumAddress()
     const mockApprovalInfos: ApprovalInfo[] = [
       {
         tokenInfo: { symbol: 'TST', decimals: 18, address: tokenAddress1, type: TokenType.ERC20 },
         tokenAddress: '0x1',
-        spender: '0x2',
+        spender: spenderAddress,
         amount: '4200000',
         amountFormatted: '420.0',
         method: 'approve',
+        transactionIndex: 0,
       },
       {
         tokenInfo: { symbol: 'TST', decimals: 18, address: tokenAddress2, type: TokenType.ERC20 },
         tokenAddress: '0x1',
-        spender: '0x2',
+        spender: spenderAddress,
         amount: '6900000',
         amountFormatted: '69.0',
         method: 'increaseAllowance',
+        transactionIndex: 1,
       },
     ]
 
@@ -41,22 +44,33 @@ describe('ApprovalEditorForm', () => {
     const approvalItems = getAllByTestId(result.container, 'approval-item')
     expect(approvalItems).toHaveLength(2)
 
-    // One button for each approval
-    const buttons = getAllByTitle(result.container, 'Save')
-    expect(buttons).toHaveLength(2)
+    // One edit button for each approval
+    const editButtons = getAllByTitle(result.container, 'Edit')
+    expect(editButtons).toHaveLength(2)
 
     // First approval value is rendered
     await waitFor(() => {
       const amountInput = result.container.querySelector('input[name="approvals.0"]') as HTMLInputElement
       expect(amountInput).not.toBeNull()
       expect(amountInput).toHaveValue('420.0')
       expect(amountInput).toBeEnabled()
+      expect(amountInput).toHaveAttribute('readOnly')
     })
 
     // Change value of first approval
-    const amountInput1 = result.container.querySelector('input[name="approvals.0"]') as HTMLInputElement
-    fireEvent.change(amountInput1!, { target: { value: '123' } })
-    fireEvent.click(buttons[0])
+    act(() => {
+      fireEvent.click(editButtons[0])
+      const amountInput1 = result.container.querySelector('input[name="approvals.0"]') as HTMLInputElement
+
+      fireEvent.change(amountInput1!, { target: { value: '123' } })
+    })
+    let saveButton = result.getByTitle('Save')
+    await waitFor(() => {
+      expect(saveButton).toBeEnabled()
+    })
+    act(() => {
+      fireEvent.click(saveButton)
+    })
 
     expect(updateCallback).toHaveBeenCalledWith(['123', '69.0'])
 
@@ -69,9 +83,19 @@ describe('ApprovalEditorForm', () => {
     })
 
     // Change value of second approval
-    const amountInput2 = result.container.querySelector('input[name="approvals.1"]') as HTMLInputElement
-    fireEvent.change(amountInput2!, { target: { value: '456' } })
-    fireEvent.click(buttons[1])
+    act(() => {
+      fireEvent.click(editButtons[1])
+      const amountInput2 = result.container.querySelector('input[name="approvals.1"]') as HTMLInputElement
+      fireEvent.change(amountInput2!, { target: { value: '456' } })
+    })
+
+    saveButton = result.getByTitle('Save')
+    await waitFor(() => {
+      expect(saveButton).toBeEnabled()
+    })
+    act(() => {
+      fireEvent.click(saveButton)
+    })
 
     expect(updateCallback).toHaveBeenCalledWith(['123', '456'])
   })
@@ -82,10 +106,11 @@ describe('ApprovalEditorForm', () => {
     const mockApprovalInfo: ApprovalInfo = {
       tokenInfo: { symbol: 'TST', decimals: 18, address: tokenAddress, type: TokenType.ERC20 },
       tokenAddress: '0x1',
-      spender: '0x2',
+      spender: faker.finance.ethereumAddress(),
       amount: '4200000',
       amountFormatted: '420.0',
       method: 'approve',
+      transactionIndex: 0,
     }
 
     const result = render(<ApprovalEditorForm approvalInfos={[mockApprovalInfo]} updateApprovals={updateCallback} />)
@@ -104,10 +129,21 @@ describe('ApprovalEditorForm', () => {
 
     // Change value and save
     const amountInput = result.container.querySelector('input[name="approvals.0"]') as HTMLInputElement
+    const editButton = result.getByTitle('Edit')
+
+    act(() => {
+      fireEvent.click(editButton)
+      fireEvent.change(amountInput!, { target: { value: '100' } })
+    })
+
     const saveButton = result.getByTitle('Save')
+    await waitFor(() => {
+      expect(saveButton).toBeEnabled()
+    })
 
-    fireEvent.change(amountInput!, { target: { value: '100' } })
-    fireEvent.click(saveButton)
+    act(() => {
+      fireEvent.click(saveButton)
+    })
 
     expect(updateCallback).toHaveBeenCalledWith(['100'])
   })

src/components/tx/ApprovalEditor/ApprovalEditorForm.tsx

@@ -1,13 +1,12 @@
-import { IconButton, SvgIcon, List, ListItem } from '@mui/material'
+import { Box, Divider, List, ListItem, Stack } from '@mui/material'
 import { FormProvider, useForm } from 'react-hook-form'
 import css from './styles.module.css'
-import CheckIcon from '@mui/icons-material/Check'
 import type { ApprovalInfo } from './hooks/useApprovalInfos'
-import { ApprovalValueField } from './ApprovalValueField'
-import { MODALS_EVENTS } from '@/services/analytics'
-import Track from '@/components/common/Track'
+
 import { useMemo } from 'react'
-import ApprovalItem from '@/components/tx/ApprovalEditor/ApprovalItem'
+import EditableApprovalItem from './EditableApprovalItem'
+import { groupBy } from 'lodash'
+import { SpenderField } from './SpenderField'
 
 export type ApprovalEditorFormData = {
   approvals: string[]
@@ -20,6 +19,8 @@ export const ApprovalEditorForm = ({
   approvalInfos: ApprovalInfo[]
   updateApprovals: (newApprovals: string[]) => void
 }) => {
+  const groupedApprovals = useMemo(() => groupBy(approvalInfos, (approval) => approval.spender), [approvalInfos])
+
   const initialApprovals = useMemo(() => approvalInfos.map((info) => info.amountFormatted), [approvalInfos])
 
   const formMethods = useForm<ApprovalEditorFormData>({
@@ -29,44 +30,36 @@ export const ApprovalEditorForm = ({
     mode: 'onChange',
   })
 
-  const {
-    formState: { errors, dirtyFields },
-    getValues,
-    reset,
-  } = formMethods
+  const { getValues, reset } = formMethods
 
   const onSave = () => {
     const formData = getValues('approvals')
     updateApprovals(formData)
     reset({ approvals: formData })
   }
 
+  let fieldIndex = 0
+
   return (
     <FormProvider {...formMethods}>
       <List className={css.approvalsList}>
-        {approvalInfos.map((tx, idx) => (
-          <ListItem
-            key={tx.tokenAddress + tx.spender}
-            className={0n === tx.amount ? css.zeroValueApproval : undefined}
-            disablePadding
-            data-testid="approval-item"
-          >
-            <ApprovalItem spender={tx.spender} method={tx.method}>
-              <>
-                <ApprovalValueField name={`approvals.${idx}`} tx={tx} />
-                <Track {...MODALS_EVENTS.EDIT_APPROVALS}>
-                  <IconButton
-                    className={css.iconButton}
-                    onClick={onSave}
-                    disabled={!!errors.approvals || !dirtyFields.approvals?.[idx]}
-                    title="Save"
-                  >
-                    <SvgIcon component={CheckIcon} />
-                  </IconButton>
-                </Track>
-              </>
-            </ApprovalItem>
-          </ListItem>
+        {Object.entries(groupedApprovals).map(([spender, approvals], spenderIdx) => (
+          <Box key={spender}>
+            <Stack gap={2}>
+              <SpenderField address={spender} />
+              {approvals.map((tx) => (
+                <ListItem
+                  key={tx.tokenAddress + tx.spender}
+                  className={0n === tx.amount ? css.zeroValueApproval : undefined}
+                  disablePadding
+                  data-testid="approval-item"
+                >
+                  <EditableApprovalItem approval={tx} name={`approvals.${fieldIndex++}`} onSave={onSave} />
+                </ListItem>
+              ))}
+            </Stack>
+            {spenderIdx !== Object.keys(groupedApprovals).length - 1 && <Divider />}
+          </Box>
         ))}
       </List>
     </FormProvider>