-
Notifications
You must be signed in to change notification settings - Fork 112
feat(v4-sdk): additional options when migrating #276
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
ce3bb3c
ad62981
81c86a1
616bc64
88c1c4c
b3e1f44
b669682
eb869ab
af3398f
034362c
74b11e4
542273f
40df03d
08c1256
aad7a8c
ab18592
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 |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import { BigintIsh, Percent, validateAndParseAddress, NativeCurrency } from '@uniswap/sdk-core' | ||
import { BigintIsh, Percent, validateAndParseAddress, NativeCurrency, Currency } from '@uniswap/sdk-core' | ||
import { TypedDataDomain, TypedDataField } from '@ethersproject/abstract-signer' | ||
import JSBI from 'jsbi' | ||
import { Position } from './entities/position' | ||
|
@@ -62,9 +62,9 @@ export interface MintSpecificOptions { | |
sqrtPriceX96?: BigintIsh | ||
|
||
/** | ||
* Whether the mint is part of a migration from V3 to V4. | ||
* Whether the mint is part of a migration from V3 to V4 and the additional currency and amount to send if needed | ||
*/ | ||
migrate?: boolean | ||
migrateOptions?: MigrateOptions | ||
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. looks like we are pulling out migrate into its own object; might need to change trading api impl |
||
} | ||
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. was thinking if we like this format, we could add the recipient field here too inside of MigrateOptions |
||
|
||
/** | ||
|
@@ -114,21 +114,19 @@ export interface CollectSpecificOptions { | |
recipient: string | ||
} | ||
|
||
export interface TransferOptions { | ||
export interface MigrateOptions { | ||
dianakocsis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* The account sending the NFT. | ||
* Whether the mint is part of a migration from V3 to V4. | ||
*/ | ||
sender: string | ||
|
||
migrate?: boolean | ||
/** | ||
* The account that should receive the NFT. | ||
* The additional currency that needs to be transferred if migrating from out of range to in range or out of range to opposite side out of range | ||
*/ | ||
dianakocsis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
recipient: string | ||
|
||
neededCurrency?: Currency | ||
/** | ||
dianakocsis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* The id of the token being sent. | ||
* The amount of additional currency that needs to be transferred if migrating from out of range to in range or out of range to opposite side out of range | ||
*/ | ||
dianakocsis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
tokenId: BigintIsh | ||
neededAmount?: BigintIsh | ||
} | ||
dianakocsis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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. not too bad it seems on the Trading API side to send in; curious how to derive the amount, seems it wouldnt bee to tedious to compute from earlier convos |
||
export interface PermitDetails { | ||
|
@@ -284,9 +282,11 @@ export abstract class V4PositionManager { | |
|
||
let value: string = toHex(0) | ||
|
||
let needToSendEth = isMint(options) && options.migrateOptions?.neededCurrency?.isNative | ||
|
||
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. questions...
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. i was thinking the trading api would calculate how much is needed of the currency in the new position 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. but i guess the sdk could handle all scenarios?
dianakocsis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// If migrating, we need to settle and sweep both currencies individually | ||
if (isMint(options) && options.migrate) { | ||
if (options.useNative) { | ||
if (isMint(options) && options.migrateOptions?.migrate) { | ||
if (options.useNative && !needToSendEth) { | ||
// unwrap the exact amount needed to send to the pool manager | ||
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 the case where neededCurrency is ETH and the position is in WETH this path should still be taken i think? 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. I think the opposite right? it would need to wrap not unwrap |
||
planner.addUnwrap(OPEN_DELTA) | ||
// payer is v4 position manager | ||
|
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.
would love a test for non-eth too, and for ETH-WETH wrapping combos if they are to be supported too