-
Notifications
You must be signed in to change notification settings - Fork 229
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
V3 liquidity commands #355
Changes from 45 commits
60c4fc8
c70c9ff
568eafa
c30a295
7b63210
ec7e6e8
dee9fd3
89a56e4
44c77ee
0ff8024
83567fa
aab39ee
519ca80
4d0d406
af22cfe
ab21582
e65d3c5
caad744
77e6879
0d685e3
70e7e8a
21b7cd0
4726a9c
4aea013
c2eaed0
1aca629
31b15bf
bb39a36
40712fc
8296d4a
fd63875
567558c
b90b7bd
eae1ffa
d1f76f0
3349feb
0d37076
762158d
30d9e6d
62e5ff8
af02244
19a0a42
fde31eb
ee2dfc5
f4c4cf3
3575bb3
47d93fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
// SPDX-License-Identifier: GPL-3.0-or-later | ||
pragma solidity ^0.8.24; | ||
|
||
import {INonfungiblePositionManager} from '@uniswap/v3-periphery/contracts/interfaces/INonfungiblePositionManager.sol'; | ||
|
||
struct MigratorParameters { | ||
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. any particular reason this is in a struct and is not just passed in as an address? when I read 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. because itll also include v4 position manager so this struct is just preemptively there 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. Could change it but at this point it would be a bigger renaming overhaul of UR 😅 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. yeah i just followed the pattern from the others! and we'll have v4posm in there too 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. sg |
||
address v3PositionManager; | ||
} | ||
|
||
contract MigratorImmutables { | ||
/// @dev v3PositionManager address | ||
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 should be
For example:
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. changed PaymentsImmutables too |
||
INonfungiblePositionManager internal immutable V3_POSITION_MANAGER; | ||
|
||
constructor(MigratorParameters memory params) { | ||
V3_POSITION_MANAGER = INonfungiblePositionManager(params.v3PositionManager); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// SPDX-License-Identifier: GPL-3.0-or-later | ||
pragma solidity ^0.8.24; | ||
|
||
import {MigratorImmutables} from '../modules/MigratorImmutables.sol'; | ||
import {INonfungiblePositionManager} from '@uniswap/v3-periphery/contracts/interfaces/INonfungiblePositionManager.sol'; | ||
|
||
abstract contract V3ToV4Migrator is MigratorImmutables { | ||
function isValidAction(bytes4 selector) internal pure returns (bool) { | ||
return selector == INonfungiblePositionManager.decreaseLiquidity.selector | ||
|| selector == INonfungiblePositionManager.collect.selector | ||
|| selector == INonfungiblePositionManager.burn.selector; | ||
} | ||
|
||
function isAuthorizedForToken(address caller, uint256 tokenId) internal view returns (bool) { | ||
address owner = V3_POSITION_MANAGER.ownerOf(tokenId); | ||
return caller == owner || V3_POSITION_MANAGER.getApproved(tokenId) == caller | ||
|| V3_POSITION_MANAGER.isApprovedForAll(owner, caller); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
solmate/=lib/solmate/ | ||
permit2/=lib/permit2/ | ||
forge-std/=lib/forge-std/src/ | ||
@openzeppelin/=node_modules/@openzeppelin | ||
@uniswap=node_modules/@uniswap | ||
@openzeppelin/contracts=node_modules/@openzeppelin/contracts | ||
@uniswap/v3-core/=node_modules/@uniswap/v3-core/ | ||
@uniswap/v2-core/=node_modules/@uniswap/v2-core/ | ||
@uniswap/v3-periphery/=lib/v3-periphery/ |
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 separate to this PR but I think this is odd.. to have SECOND_IF_BOUNDARY as a command? Cause its not really a command.. why not just make the logic command < Command. ERC721_PERMIT? or basically just whatever the command that exists on that boundary
@hensha256 ?
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 we just thought it was easier to read in the dispatcher to have boundaries... but i see that its confusing to have it in the commands file
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 guess you could make it a separate constant and just the only downside is we have two constants equal to the same value?