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.

Sign up for GitHub

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

feat: redesign approval editor #3614

Merged
merged 8 commits into from
May 7, 2024
Merged

feat: redesign approval editor #3614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2622482
feat: redesign approval editor
schmanu Apr 25, 2024
ebc696e
fix: store position in transaction within approval infos
schmanu Apr 26, 2024
5546787
fix: review issues and bug with increaseAllowance
schmanu Apr 29, 2024
27fac17
fix: tests
schmanu Apr 29, 2024
17d0a42
fix: suggested changes
schmanu May 2, 2024
3ecb731
fix: shortAddress for small screens only
schmanu May 3, 2024
4e64644
fix: show validation errors in approval editor
schmanu May 3, 2024
2084973
test: fix test expectation
schmanu May 7, 2024
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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
6 changes: 6 additions & 0 deletions src/components/theme/safeTheme.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
297 changes: 265 additions & 32 deletions src/components/tx/ApprovalEditor/ApprovalEditor.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,35 @@
import { safeSignatureBuilder, safeTxBuilder } from '@/tests/builders/safeTx'
import { render } from '@/tests/test-utils'
import { act, fireEvent, getAllByTitle, render, waitFor } from '@/tests/test-utils'
import ApprovalEditor from '.'
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 { encodeMultiSendData } from '@safe-global/protocol-kit'
import { ERC20__factory, Multi_send__factory } from '@/types/contracts'
import { getAndValidateSafeSDK } from '@/services/tx/tx-sender/sdk'
import { parseUnits } from 'ethers'
import { checksumAddress } from '@/utils/addresses'

jest.mock('@/services/tx/tx-sender/sdk', () => ({
getAndValidateSafeSDK: jest.fn().mockReturnValue({
createTransaction: jest.fn(),
}),
}))

jest.mock('@safe-global/safe-gateway-typescript-sdk', () => ({
...jest.requireActual('@safe-global/safe-gateway-typescript-sdk'),
getContract: jest.fn(() => undefined),
__esModule: true,
}))

const ERC20_INTERFACE = ERC20__factory.createInterface()
const MULTISEND_INTERFACE = Multi_send__factory.createInterface()

describe('ApprovalEditor', () => {
beforeEach(() => {
jest.clearAllMocks()
jest.restoreAllMocks()
})

it('returns null if there is no safe transaction', () => {
Expand All @@ -18,8 +39,11 @@ describe('ApprovalEditor', () => {
})

it('returns null if there are no approvals', () => {
const mockSafeTx = createMockSafeTransaction({ to: '0x1', data: '0x', operation: OperationType.DelegateCall })
jest.spyOn(approvalInfos, 'useApprovalInfos').mockReturnValue([[], undefined, false])
const mockSafeTx = createMockSafeTransaction({
to: faker.finance.ethereumAddress(),
data: '0x',
operation: OperationType.DelegateCall,
})
const result = render(<ApprovalEditor safeTransaction={mockSafeTx} />)

expect(result.container).toBeEmptyDOMElement()
Expand All @@ -29,7 +53,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} />)

Expand All @@ -46,18 +74,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()

Expand All @@ -66,22 +97,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(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} />)

Expand All @@ -90,26 +128,221 @@ 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(spenderAddress))
})

it('renders a form if there is an update callback', async () => {
const mockApprovalInfo = {
tokenInfo: { symbol: 'TST', decimals: 18, address: '0x3', type: TokenType.ERC20 },
tokenAddress: '0x1',
spender: '0x2',
amount: '4200000',
amountFormatted: '420.0',
method: 'approve',
} as const
jest.spyOn(approvalInfos, 'useApprovalInfos').mockReturnValue([[mockApprovalInfo], undefined, false])
const mockSafeTx = createMockSafeTransaction({ to: '0x1', data: '0x', operation: OperationType.DelegateCall })
it('should modify approvals on save', async () => {
const tokenAddress = checksumAddress(faker.finance.ethereumAddress())
const multiSendAddress = checksumAddress(faker.finance.ethereumAddress())
const spenderAddress = checksumAddress(faker.finance.ethereumAddress())

const result = render(<ApprovalEditor safeTransaction={mockSafeTx} />)
const mockSafeTx = createMockSafeTransaction({
to: multiSendAddress,
data: MULTISEND_INTERFACE.encodeFunctionData('multiSend', [
encodeMultiSendData([
{
to: tokenAddress,
data: ERC20_INTERFACE.encodeFunctionData('approve', [spenderAddress, '420000000000000000000']),
value: '0',
operation: OperationType.Call,
},
{
to: tokenAddress,
data: ERC20_INTERFACE.encodeFunctionData('transfer', [spenderAddress, '25']),
value: '0',
operation: OperationType.Call,
},
{
to: tokenAddress,
data: ERC20_INTERFACE.encodeFunctionData('increaseAllowance', [spenderAddress, '690000000000']),
value: '0',
operation: OperationType.Call,
},
]),
]),
operation: OperationType.DelegateCall,
})

const amountInput = result.container.querySelector('input[name="approvals.0"]') as HTMLInputElement
const result = render(<ApprovalEditor safeTransaction={mockSafeTx} />, {
initialReduxState: {
balances: {
loading: false,
data: {
fiatTotal: '0',
items: [
{
balance: '10',
tokenInfo: {
address: tokenAddress,
decimals: 18,
logoUri: 'someurl',
name: 'Test token',
symbol: 'TST',
type: TokenType.ERC20,
},
fiatBalance: '10',
fiatConversion: '1',
},
],
},
},
},
})

await waitFor(() => {
const amountInput1 = result.container.querySelector('input[name="approvals.0"]') as HTMLInputElement
const amountInput2 = result.container.querySelector('input[name="approvals.1"]') as HTMLInputElement
expect(amountInput1).toBeInTheDocument()
expect(amountInput2).toBeInTheDocument()
})

// One edit button for each approval
const editButtons = getAllByTitle(result.container, 'Edit')
expect(editButtons).toHaveLength(2)

// Edit and Save first value
act(() => {
fireEvent.click(editButtons[0])
const amountInput1 = result.container.querySelector('input[name="approvals.0"]') as HTMLInputElement
fireEvent.change(amountInput1!, { target: { value: '100' } })
})

let saveButton = result.getByTitle('Save')
await waitFor(() => {
expect(saveButton).toBeEnabled()
})

act(() => {
fireEvent.click(saveButton)
})
const mockSafe = getAndValidateSafeSDK()
expect(mockSafe.createTransaction).toHaveBeenCalledWith({
onlyCalls: true,
transactions: [
{
to: tokenAddress,
data: ERC20_INTERFACE.encodeFunctionData('approve', [spenderAddress, parseUnits('100', 18)]),
value: '0',
},
{
to: tokenAddress,
data: ERC20_INTERFACE.encodeFunctionData('transfer', [spenderAddress, '25']),
value: '0',
},
{
to: tokenAddress,
data: ERC20_INTERFACE.encodeFunctionData('increaseAllowance', [spenderAddress, '690000000000']),
value: '0',
},
],
})
})

it('should modify increaseAllowance on save', async () => {
const tokenAddress = checksumAddress(faker.finance.ethereumAddress())
const multiSendAddress = checksumAddress(faker.finance.ethereumAddress())
const spenderAddress = checksumAddress(faker.finance.ethereumAddress())

const mockSafeTx = createMockSafeTransaction({
to: multiSendAddress,
data: MULTISEND_INTERFACE.encodeFunctionData('multiSend', [
encodeMultiSendData([
{
to: tokenAddress,
data: ERC20_INTERFACE.encodeFunctionData('approve', [spenderAddress, '420000000000000000000']),
value: '0',
operation: OperationType.Call,
},
{
to: tokenAddress,
data: ERC20_INTERFACE.encodeFunctionData('transfer', [spenderAddress, '25']),
value: '0',
operation: OperationType.Call,
},
{
to: tokenAddress,
data: ERC20_INTERFACE.encodeFunctionData('increaseAllowance', [spenderAddress, '690000000000']),
value: '0',
operation: OperationType.Call,
},
]),
]),
operation: OperationType.DelegateCall,
})

const result = render(<ApprovalEditor safeTransaction={mockSafeTx} />, {
initialReduxState: {
balances: {
loading: false,
data: {
fiatTotal: '0',
items: [
{
balance: '10',
tokenInfo: {
address: tokenAddress,
decimals: 18,
logoUri: 'someurl',
name: 'Test token',
symbol: 'TST',
type: TokenType.ERC20,
},
fiatBalance: '10',
fiatConversion: '1',
},
],
},
},
},
})

await waitFor(() => {
const amountInput1 = result.container.querySelector('input[name="approvals.0"]') as HTMLInputElement
const amountInput2 = result.container.querySelector('input[name="approvals.1"]') as HTMLInputElement
expect(amountInput1).toBeInTheDocument()
expect(amountInput2).toBeInTheDocument()
})

// One edit button for each approval
const editButtons = getAllByTitle(result.container, 'Edit')
expect(editButtons).toHaveLength(2)

// Edit and Save second value
act(() => {
fireEvent.click(editButtons[1])
const amountInput2 = result.container.querySelector('input[name="approvals.1"]') as HTMLInputElement
fireEvent.change(amountInput2!, { target: { value: '300' } })
})

let saveButton = result.getByTitle('Save')
await waitFor(() => {
expect(saveButton).toBeEnabled()
})

expect(amountInput).toBeInTheDocument()
act(() => {
fireEvent.click(saveButton)
})
const mockSafe = getAndValidateSafeSDK()
expect(mockSafe.createTransaction).toHaveBeenCalledWith({
onlyCalls: true,
transactions: [
{
to: tokenAddress,
data: ERC20_INTERFACE.encodeFunctionData('approve', [spenderAddress, '420000000000000000000']),
value: '0',
},
{
to: tokenAddress,
data: ERC20_INTERFACE.encodeFunctionData('transfer', [spenderAddress, '25']),
value: '0',
},
{
to: tokenAddress,
data: ERC20_INTERFACE.encodeFunctionData('increaseAllowance', [spenderAddress, parseUnits('300', 18)]),
value: '0',
},
],
})
})
})
Loading
Loading