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

feat: v2 Swap API #878

Merged
merged 13 commits into from
Jul 25, 2024
Merged

feat: v2 Swap API #878

merged 13 commits into from
Jul 25, 2024

Conversation

0xAlec
Copy link
Contributor

@0xAlec 0xAlec commented Jul 24, 2024

What changed? Why?

  • add aggregators prop to <Swap/> to control V1 or V2 API
  • this will add the v2Enabled parameter in the request body, our API will route these to a beta implementation for serving quotes/trades directly from Uniswap V3

V1 API does not index pools with under <= $10,000 of liquidity, V2 API will make a direct call to the UniswapRouter contract

Notes to reviewers
slippage for the V2 API is hardcoded to 1000 bps (10%) - Tina is aligned for the initial launch and we'll fast-follow to add configurability to the UI

usage

<Swap address={address} experimental={{ useAggregator: false }}>
          ...
</Swap>

How has it been tested?
See Slack for demo

@@ -57,6 +58,7 @@ export type GetSwapAPIParams = GetQuoteAPIParams & {
export type GetSwapQuoteParams = {
from: Token; // The source token for the swap
to: Token; // The destination token for the swap
aggregator: boolean; // Whether to use a DEX aggregator
Copy link
Contributor

Choose a reason for hiding this comment

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

alphabetical

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -45,6 +45,7 @@ export type GetQuoteAPIParams = {
to: AddressOrETH | ''; // The destination address or 'ETH' for Ethereum
amount: string; // The amount to be swapped
amountReference?: string; // The reference amount for the swap
v2Enabled?: boolean; // Whether to use V2 of the API (default: false)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just enable it as default? After all it should not have a real change for the experience, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see Slack

Copy link
Contributor

Choose a reason for hiding this comment

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

use experimental

Copy link
Contributor

Choose a reason for hiding this comment

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

v2Enabled this is such a bad way to name a property for an API.

Copy link
Contributor

Choose a reason for hiding this comment

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

do we really need this year?


if (!params.aggregator) {
apiParams = {
v2Enabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

oh I see you want to test this on his own for a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, we'll do a switchover once the V2 service is more production ready

@@ -36,9 +36,11 @@ export function useSwapContext() {
export function SwapProvider({
address,
children,
useAggregator,
Copy link
Contributor

Choose a reason for hiding this comment

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

the provider too should have experimental

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't want to leak bad experience across too many components, and we want to keep experimental nice and neat.

isAmountInDecimals?: boolean; // Whether the amount is in decimals
to: Token; // The destination token for the swap
useAggregator: boolean; // Whether to use a DEX aggregator
Copy link
Contributor

Choose a reason for hiding this comment

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

use experimental

}) {
// Feature flags
Copy link
Contributor

Choose a reason for hiding this comment

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

Love the comment

@Zizzamia Zizzamia merged commit 7b4930c into main Jul 25, 2024
12 checks passed
@Zizzamia Zizzamia deleted the alec/v2-swap branch July 25, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants