-
Notifications
You must be signed in to change notification settings - Fork 192
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: getSwap
#503
feat: getSwap
#503
Conversation
@@ -7,6 +7,7 @@ module.exports = { | |||
statements: 100, | |||
}, | |||
}, | |||
maxWorkers: 1, |
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.
this is to enable serialization for bigint
by jest
/** | ||
* Note: exported as public Type | ||
*/ | ||
export type TransactionParams = { |
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.
this is used to inject values from the wallet into the transaction
const swap = await getSwap({
fromAddress: account.address,
from: eth,
to: degen,
amount: '0.0001',
});
// Build the transaction
const nonce = await publicClient.getTransactionCount({ address: account.address });
const { maxFeePerGas, maxPriorityFeePerGas } = await publicClient.estimateFeesPerGas();
const params: TransactionParams = { nonce, maxFeePerGas, maxPriorityFeePerGas };
const tx = swap.transaction.withParams(params);
* Note: exported as public Type | ||
*/ | ||
export interface Transaction { | ||
transaction: TransactionData; |
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.
this is the object developers/users should pass into viem's signTransaction
(https://viem.sh/docs/actions/wallet/signTransaction.html)
export const CDP_LISTSWAPASSETS = 'cdp_listSwapAssets'; | ||
export const CDP_GETSWAPQUOTE = 'cdp_getSwapQuote'; | ||
export const CDP_GETSWAPTRADE = 'cdp_getSwapTrade'; |
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.
Thoughts on updating these to be snake case?
export const CDP_LISTSWAPASSETS = 'cdp_listSwapAssets'; | |
export const CDP_GETSWAPQUOTE = 'cdp_getSwapQuote'; | |
export const CDP_GETSWAPTRADE = 'cdp_getSwapTrade'; | |
export const CDP_LIST_SWAP_ASSETS = 'cdp_listSwapAssets'; | |
export const CDP_GET_SWAP_QUOTE = 'cdp_getSwapQuote'; | |
export const CDP_GET_SWAP_TRADE = 'cdp_getSwapTrade'; |
@@ -1,15 +1,17 @@ | |||
import { formatDecimals } from './formatDecimals'; | |||
import type { GetQuoteParams, GetQuoteAPIParams } from '../types'; | |||
import type { SwapParams, SwapAPIParams, GetSwapParams } from '../types'; |
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.
These seem very similar to each other in terms of naming
export type TransactionData = { | ||
chainId: number; // The chain ID | ||
data: `0x${string}`; // The data for the transaction | ||
gas: bigint; // The gas limit | ||
to: `0x${string}`; // The recipient address | ||
value: bigint; // The value of the transaction | ||
nonce?: number; // The nonce for the transaction | ||
maxFeePerGas?: bigint | undefined; // The maximum fee per gas | ||
maxPriorityFeePerGas?: bigint | undefined; // The maximum priority fee per gas | ||
}; |
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.
Should we document what each type is in the comment above the function?
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.
No next is fine, after all Types should feel easy to understand. Also a type works in pair on where it's used, so folks should be able to see where the Type is used.
@@ -1,10 +1,15 @@ | |||
// 🌲☀️🌲 | |||
export { getQuote } from './core/getQuote'; | |||
export { getSwap } from './core/getSwap'; |
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.
@0xAlec more a an open question, is getSwap
too short as name?
We wrote
Retrieves an unsigned transaction for a swap from Token A to Token B.
I wonder if get
is the right verb, and maybe something like prepareSwapTransaction
or buildSwapTransaction
.
Let's not change the name yet, but would love to reflect a second on this.
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.
I like buildSwapTransaction
!
warning: trade.quote.warning, | ||
}; | ||
} catch (error) { | ||
throw new Error(`getSwap: ${error}`); |
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.
Something we should write on, is also an Error page in our docs, and overtime re-align all our code to it.
* Note: exported as public Type | ||
*/ | ||
export type Swap = { | ||
approveTransaction?: Transaction; // The approval transaction |
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.
What a developer will do with the Approval Transaction? approve
is a verb, and it feels more something to do that to read.
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 response will contain an approveTransaction
(which needs to be broadcast before transaction
) in order to approve e.g. Token A
to be used by the contract in a swap between Token A
to Token B
. This is only applicable to ERC-20 tokens.
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.
/** | ||
* Note: exported as public Type | ||
*/ | ||
export type SwapParams = GetQuoteParams | GetSwapParams; |
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.
I think joinging them will might confuse folks, because now fromAddress: Address;
which is in Swap, it's not in Quote, but we allow it to have it now when use it in getQuote
.
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.
Should we just keep the two type seprated?
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.
Oh ok so you have getParamsForToken
that depends on both, interesting.
const { from, to, amount, amountReference, isAmountInDecimals } = params; | ||
const { fromAddress } = params as GetSwapParams; |
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.
Can we add more comments here, I am a bit confused. Like who needs fromAddress
and how they use it. And when is not there, will just be empty?
/** | ||
* Constructs an unsigned transaction. | ||
*/ | ||
export function getTransaction(tx: RawTransactionData, chainId: string): Transaction { |
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.
We probably want to change the name of this one, as I read this more get info from a particular transaction
. Which I think in this case is slighly different.
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.
Asks:
- add docs to the official docs pages
- make it easier to locally test
- start a new feature branch for swapkit so we can continue to push package updates without unfinished work
I'm default hesitant to be componentizing out of the gate without having a functional solution built, but I recognize it takes layers of iterations to get there. I would not be surprised if we fully rewrite the first 1-5 pieces of swapkit given how complex it is.
A lot of my comments are focused on typing, naming, and reusing existing libraries as much as possible to reduce our own code bloat.
export type GetQuoteParams = { | ||
from: Token; // The source token for the swap | ||
to: Token; // The destination token for the swap | ||
amount: string; // The amount to be swapped |
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.
To be more specific about naming conventions, is amount
the amount in or out? Is it a min or is it fixed on either side of in/out? Can you elaborate on why we have this as a string versus a bigint
?
from: Token; // The source token for the swap | ||
to: Token; // The destination token for the swap | ||
amount: string; // The amount to be swapped | ||
amountReference?: string; // The reference amount for the swap |
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.
I don't know what "reference amount for the swap" means, can you elaborate?
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.
Yeah, I get confused on this one always. Probably let's have a longer comment to explaining what's that about.
to: Token; // The destination token for the swap | ||
amount: string; // The amount to be swapped | ||
amountReference?: string; // The reference amount for the swap | ||
isAmountInDecimals?: boolean; // Whether the amount is in decimals |
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.
I think the code will be easier to follow and debug if the types are stricter/consistent. More often than not, I think we should pass data in the native bigint
type and apply decimals only for displaying formatted numbers.
export type RawTransactionData = { | ||
data: string; // The transaction data | ||
from: string; // The sender address | ||
gas: string; // The gas limit | ||
gasPrice: string; // The gas price | ||
to: string; // The recipient address | ||
value: string; // The value of the transaction | ||
}; |
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.
I feel like we should be able to grab all types like this from viem? To build our intuition, if we're ever building custom utilities for core functionality (eg transaction types), then I think we should double check nothing from viem/wagmi can serve us.
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.
@ilikesymmetry if there are types we can use from Viem, can you just linking them. Some of them are easy to use, other might need some refactoring as they need to reflect more our API responses.
approveTransaction?: Transaction; // The approval transaction | ||
fee: Fee; // The fee for the swap | ||
quote: Quote; // The quote for the swap | ||
transaction: Transaction; // The swap transaction |
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.
(nit, but I think it's important for our job as a library)
Technically this is not a transaction, but a "call". This only becomes a transaction once it it signed and sent to the mempool, at which point we have a transaction hash to uniquely identify it. Following the recent work on 5792, I think we should call this swapCall
and rename approveTransaction
->approveCall
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.
Oh great point
* Note: exported as public Type | ||
*/ | ||
export type Swap = { | ||
approveTransaction?: Transaction; // The approval transaction |
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.
Can you confirm that the approve transaction uses Permit2? It seems like this calls are prepared by our API which I don't have insight into how it's estimating and preparing these things
data: `0x${string}`; // The data for the transaction | ||
gas: bigint; // The gas limit | ||
to: `0x${string}`; // The recipient address |
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.
0x${string}
-> Address
/** | ||
* Note: exported as public Type | ||
*/ | ||
export type TransactionData = { |
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.
Same comment here on being able to use a viem
type
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.
Which one are you thinking about?
I am going to merge this PR, and follow up with all the comments. So I can help @0xAlec with some other work he is doing. cc @ilikesymmetry @kyhyco @abcrane123 |
What changed? Why?
getSwap
SwapParams
for bothgetQuote
andgetSwap
(shared params for allcore/swap
utils)Transaction
interface with a helper method for injectingnonce
and EIP-1559 gas valuesNotes to reviewers
gasPrice
(i.e. pre-EIP-1559 transactions)How has it been tested?
Checking for a warning
Example of a successful
ETH
->DEGEN
swap:Swap - https://basescan.org/tx/0xca2a5a324b3c011f35a9e5e182c2f9e11b1714b739093b66b7bc0cce949db7bf
Example with an ERC-20 approval
Approval - https://basescan.org/tx/0x044bf9ed67b1c22d7d2c203222d9a6b66576d0058780d222b30fe4377aaf32d2
Swap - https://basescan.org/tx/0xe15755021e051a4fcfc02cfa083728022eb9262b74c5b271aca1dbf147bdd2aa