-
Notifications
You must be signed in to change notification settings - Fork 193
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
feat: Add Swap Settings #1210
feat: Add Swap Settings #1210
Changes from 19 commits
fa3c2cd
65f7462
39bebad
818f332
7735d76
719acc2
0893f30
ee25865
a684dc6
07b0f2d
7e99805
68b1e6f
550d020
5bdd025
b4992d1
03e0f86
73ac6df
7fbe2ec
fce027d
cab4738
c933993
3579bc7
787b238
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@coinbase/onchainkit': patch | ||
--- | ||
|
||
-**feat**: added custom slippage support settings sub-component in the `Swap` component. By @cpcramer #1210 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,9 @@ export function SwapProvider({ | |
const { address } = useAccount(); | ||
// Feature flags | ||
const { useAggregator } = experimental; | ||
|
||
const [maxSlippage, _setMaxSlippage] = useState( | ||
experimental.maxSlippage || 3, | ||
); | ||
// Core Hooks | ||
const config = useConfig(); | ||
const [loading, setLoading] = useState(false); | ||
|
@@ -56,6 +58,7 @@ export function SwapProvider({ | |
statusName: 'init', | ||
statusData: { | ||
isMissingRequiredField: true, | ||
maxSlippage, | ||
}, | ||
}); // Component lifecycle | ||
const [hasHandledSuccess, setHasHandledSuccess] = useState(false); | ||
|
@@ -120,10 +123,11 @@ export function SwapProvider({ | |
statusName: 'init', | ||
statusData: { | ||
isMissingRequiredField: false, | ||
maxSlippage, | ||
}, | ||
}); | ||
} | ||
}, [hasHandledSuccess, lifeCycleStatus.statusName]); | ||
}, [hasHandledSuccess, lifeCycleStatus.statusName, maxSlippage]); | ||
|
||
const handleToggle = useCallback(() => { | ||
from.setAmount(to.amount); | ||
|
@@ -153,6 +157,7 @@ export function SwapProvider({ | |
statusData: { | ||
amountFrom: from.amount, | ||
amountTo: to.amount, | ||
maxSlippage, | ||
tokenFrom: from.token, | ||
tokenTo: to.token, | ||
// token is missing | ||
|
@@ -175,6 +180,7 @@ export function SwapProvider({ | |
// amount is irrelevant | ||
amountFrom: type === 'from' ? amount : '', | ||
amountTo: type === 'to' ? amount : '', | ||
maxSlippage, | ||
tokenFrom: from.token, | ||
tokenTo: to.token, | ||
// when fetching quote, the destination | ||
|
@@ -189,7 +195,7 @@ export function SwapProvider({ | |
amountReference: 'from', | ||
from: source.token, | ||
to: destination.token, | ||
maxSlippage: experimental.maxSlippage?.toString(), | ||
maxSlippage: maxSlippage.toString(), | ||
useAggregator, | ||
}); | ||
// If request resolves to error response set the quoteError | ||
|
@@ -215,6 +221,7 @@ export function SwapProvider({ | |
statusData: { | ||
amountFrom: type === 'from' ? amount : formattedAmount, | ||
amountTo: type === 'to' ? amount : formattedAmount, | ||
maxSlippage, | ||
tokenFrom: from.token, | ||
tokenTo: to.token, | ||
// if quote was fetched successfully, we | ||
|
@@ -236,7 +243,7 @@ export function SwapProvider({ | |
destination.setLoading(false); | ||
} | ||
}, | ||
[from, experimental.maxSlippage, to, useAggregator], | ||
[from, maxSlippage, to, useAggregator], | ||
); | ||
|
||
const handleSubmit = useCallback(async () => { | ||
|
@@ -247,6 +254,7 @@ export function SwapProvider({ | |
statusName: 'init', | ||
statusData: { | ||
isMissingRequiredField: false, | ||
maxSlippage, | ||
}, | ||
}); | ||
|
||
|
@@ -257,7 +265,7 @@ export function SwapProvider({ | |
from: from.token, | ||
to: to.token, | ||
useAggregator, | ||
maxSlippage: experimental.maxSlippage?.toString(), | ||
maxSlippage: maxSlippage.toString(), | ||
}); | ||
if (isSwapError(response)) { | ||
setLifeCycleStatus({ | ||
|
@@ -272,6 +280,7 @@ export function SwapProvider({ | |
} | ||
await processSwapTransaction({ | ||
config, | ||
maxSlippage, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case was miss, let's cover this comment https://github.com/coinbase/onchainkit/pull/1210/files#r1744336194 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This link is broken, it is sending me to the top of the updated files page. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I don't think is clean to just pass the maxSlippage. Like think 7 steps ahead, statusData will contain other info that need to persist inside processSwapTransaction when setLifeCycleStatus is called. So we should pass lifeCycleStatus, inside processSwapTransaction. So that we can persit the info we need from statusData when we recall setLifeCycleStatus. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should useAggregator be handled the same way? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Zizzamia Updated to pass in And now within
|
||
sendTransactionAsync, | ||
setLifeCycleStatus, | ||
swapTransaction: response, | ||
|
@@ -297,10 +306,10 @@ export function SwapProvider({ | |
config, | ||
from.amount, | ||
from.token, | ||
maxSlippage, | ||
sendTransactionAsync, | ||
to.token, | ||
useAggregator, | ||
experimental.maxSlippage, | ||
]); | ||
|
||
const value = useValue({ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,180 @@ | ||
import '@testing-library/jest-dom'; | ||
import { fireEvent, render, screen, waitFor } from '@testing-library/react'; | ||
import { beforeEach, vi } from 'vitest'; | ||
import { useBreakpoints } from '../../useBreakpoints'; | ||
import { useIcon } from '../../wallet/hooks/useIcon'; | ||
import { SwapSettings } from './SwapSettings'; | ||
import { SwapSettingsSlippageDescription } from './SwapSettingsSlippageDescription'; | ||
import { SwapSettingsSlippageInput } from './SwapSettingsSlippageInput'; | ||
import { SwapSettingsSlippageTitle } from './SwapSettingsSlippageTitle'; | ||
|
||
vi.mock('../../wallet/hooks/useIcon', () => ({ | ||
useIcon: vi.fn(() => <svg data-testid="mock-icon" />), | ||
})); | ||
|
||
vi.mock('./SwapSettingsSlippageLayout', () => ({ | ||
SwapSettingsSlippageLayout: vi.fn(({ children }) => ( | ||
<div data-testid="mock-layout">{children}</div> | ||
)), | ||
})); | ||
|
||
vi.mock('./SwapSettingsSlippageLayoutBottomSheet', () => ({ | ||
SwapSettingsSlippageLayoutBottomSheet: vi.fn(({ children }) => ( | ||
<div data-testid="mock-bottom-sheet">{children}</div> | ||
)), | ||
})); | ||
|
||
vi.mock('./SwapSettingsSlippageTitle', () => ({ | ||
SwapSettingsSlippageTitle: vi.fn(() => <div>Title</div>), | ||
})); | ||
|
||
vi.mock('./SwapSettingsSlippageDescription', () => ({ | ||
SwapSettingsSlippageDescription: vi.fn(() => <div>Description</div>), | ||
})); | ||
|
||
vi.mock('./SwapSettingsSlippageInput', () => ({ | ||
SwapSettingsSlippageInput: vi.fn(() => <div>Input</div>), | ||
})); | ||
|
||
vi.mock('../../useBreakpoints', () => ({ | ||
useBreakpoints: vi.fn(), | ||
})); | ||
|
||
const useBreakpointsMock = useBreakpoints as vi.Mock; | ||
|
||
const renderComponent = (props = {}) => { | ||
return render( | ||
<SwapSettings {...props}> | ||
<SwapSettingsSlippageTitle /> | ||
<SwapSettingsSlippageDescription /> | ||
<SwapSettingsSlippageInput /> | ||
</SwapSettings>, | ||
); | ||
}; | ||
|
||
describe('SwapSettings', () => { | ||
beforeEach(() => { | ||
vi.clearAllMocks(); | ||
useBreakpointsMock.mockReturnValue('md'); | ||
}); | ||
|
||
it('should render with default props and handle dropdown toggle for desktop', async () => { | ||
renderComponent(); | ||
expect(screen.getByTestId('ockSwapSettings_Settings')).toBeInTheDocument(); | ||
expect(screen.getByTestId('mock-icon')).toBeInTheDocument(); | ||
const button = screen.getByRole('button', { | ||
name: /toggle swap settings/i, | ||
}); | ||
fireEvent.click(button); | ||
await waitFor(() => { | ||
expect(screen.getByTestId('ockSwapSettingsDropdown')).toBeInTheDocument(); | ||
expect(screen.getByTestId('mock-layout')).toBeInTheDocument(); | ||
expect(screen.getByText('Title')).toBeInTheDocument(); | ||
expect(screen.getByText('Description')).toBeInTheDocument(); | ||
expect(screen.getByText('Input')).toBeInTheDocument(); | ||
}); | ||
fireEvent.click(button); | ||
await waitFor(() => { | ||
expect( | ||
screen.queryByTestId('ockSwapSettingsDropdown'), | ||
).not.toBeInTheDocument(); | ||
}); | ||
}); | ||
|
||
it('should render with custom props and handle outside click', async () => { | ||
render( | ||
<div> | ||
<SwapSettings | ||
text="Custom Text" | ||
className="custom-class" | ||
icon="custom-icon" | ||
/> | ||
<div data-testid="outside">Outside</div> | ||
</div>, | ||
); | ||
expect(screen.getByText('Custom Text')).toBeInTheDocument(); | ||
expect(screen.getByTestId('ockSwapSettings_Settings')).toHaveClass( | ||
'custom-class', | ||
); | ||
expect(useIcon).toHaveBeenCalledWith({ icon: 'custom-icon' }); | ||
const button = screen.getByRole('button', { | ||
name: /toggle swap settings/i, | ||
}); | ||
fireEvent.click(button); | ||
await waitFor(() => { | ||
expect(screen.getByTestId('ockSwapSettingsDropdown')).toBeInTheDocument(); | ||
}); | ||
fireEvent.mouseDown(screen.getByTestId('outside')); | ||
await waitFor(() => { | ||
expect( | ||
screen.queryByTestId('ockSwapSettingsDropdown'), | ||
).not.toBeInTheDocument(); | ||
}); | ||
}); | ||
|
||
it('should handle non-valid React elements as children', async () => { | ||
render( | ||
<SwapSettings> | ||
<SwapSettingsSlippageTitle /> | ||
Plain text child | ||
<SwapSettingsSlippageInput /> | ||
</SwapSettings>, | ||
); | ||
const button = screen.getByRole('button', { | ||
name: /toggle swap settings/i, | ||
}); | ||
fireEvent.click(button); | ||
await waitFor(() => { | ||
expect(screen.getByTestId('ockSwapSettingsDropdown')).toBeInTheDocument(); | ||
expect(screen.getByText('Title')).toBeInTheDocument(); | ||
expect(screen.getByText('Plain text child')).toBeInTheDocument(); | ||
expect(screen.getByText('Input')).toBeInTheDocument(); | ||
}); | ||
}); | ||
|
||
it('renders SwapSettingsSlippageLayoutBottomSheet when breakpoint is "sm"', async () => { | ||
useBreakpointsMock.mockReturnValue('sm'); | ||
renderComponent(); | ||
const button = screen.getByRole('button', { | ||
name: /toggle swap settings/i, | ||
}); | ||
const initialBottomSheet = screen.getByTestId( | ||
'ockSwapSettingsSlippageLayoutBottomSheet_container', | ||
); | ||
expect(initialBottomSheet).toHaveClass('ease-in-out'); | ||
expect(initialBottomSheet).toHaveClass('group-[]:bottom-0'); | ||
fireEvent.click(button); | ||
await waitFor(() => { | ||
const parentDiv = screen | ||
.getByTestId('ockSwapSettings_Settings') | ||
.querySelector('div'); | ||
expect(parentDiv).toHaveClass('group'); | ||
const openBottomSheet = screen.getByTestId( | ||
'ockSwapSettingsSlippageLayoutBottomSheet_container', | ||
); | ||
expect(openBottomSheet).toHaveClass('group-[]:bottom-0'); | ||
}); | ||
fireEvent.click(button); | ||
await waitFor(() => { | ||
const parentDiv = screen | ||
.getByTestId('ockSwapSettings_Settings') | ||
.querySelector('div'); | ||
expect(parentDiv).not.toHaveClass('group'); | ||
const closedBottomSheet = screen.getByTestId( | ||
'ockSwapSettingsSlippageLayoutBottomSheet_container', | ||
); | ||
expect(closedBottomSheet).toHaveClass('ease-in-out'); | ||
}); | ||
}); | ||
|
||
it('removes event listener on unmount', () => { | ||
const removeEventListenerSpy = vi.spyOn(document, 'removeEventListener'); | ||
const { unmount } = renderComponent(); | ||
unmount(); | ||
expect(removeEventListenerSpy).toHaveBeenCalledWith( | ||
'mousedown', | ||
expect.any(Function), | ||
); | ||
removeEventListenerSpy.mockRestore(); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't want this in state do we? If the experimental prop changes, should that update here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The experimental prop won't change after the component is rendered.
It will set the initial
maxSlippage
value. Any updates will be handled byLifeCycleStatus.slippageChange.maxSlippage
propertyhttps://github.com/coinbase/onchainkit/pull/1210/files#diff-9bca97e90be8d6cfaa2c6813dbf1d684ed792b80b395f6b033d3d06963f23371R91-R94
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we update this value using
setLifeCycleStatus
'https://github.com/coinbase/onchainkit/pull/1210/files#diff-704c4aa565af1d2becb4f70ce49ac884069cdb8e451c6e3afe603a6d47de07bcR35-R38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some context on why we are setting it this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are only using this const to initialize lifecycle status, can it just be a const? or alternatively we could move the initiallifecycle status into a const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpcramer that sentence it's incorrect! We should always expect a prop can change, that's the all point of prop. :)