Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Routing #58
Routing #58
Changes from all commits
b6f8d3b
d5dc31a
2ca710a
664d2c2
8b687b8
1717039
2549c3a
e1c46fd
8aae01b
5823d2f
7e25390
607ea3b
cf82527
9ebc9d5
54a48e5
01a5db0
31cbfe5
d50f18d
4b010ff
7c6675b
aa148b3
1586c0d
06961bf
528f15d
0ac638a
1844d70
86039e4
90f3f35
50372c7
64f4645
e92b023
8283fb6
7aacd94
f2a3dd8
4792d1c
903d5fa
f1c4746
4b7d019
f555329
9c11de4
b2fdb51
963c360
eb801c7
86fd91a
22c0879
3a4c36d
e86036b
2236812
456d0c8
9e5e418
5ceaadf
20224d7
d02049b
ff206ec
09b2e23
b51f0ea
b625d74
d1d66ec
e2ae6d3
7094a75
9ee5768
f14c6f0
7545fa3
a92da59
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
my thought is that slippage protection should be with the swap commands? why do we also need slippage protection with settle commands?
i think we can make settle just take an amount, and if you're ok with the full settle amount then you pass in a constant/sentinel
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.
Its to support split route trades. Lets say youre trading 1000 DAI for USDC, where you want at least 998 USDC
Then you would do a command thats like "take all USDC, and check we're receiving at least 998".
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 implement the same style of logic in UR using the sweep command e.g. 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.
For v3 split routes it trades both routes, with the recipient of the UR, and then calls
SWEEP(USDC, 998)
which will revert the trade if 998 weren't receivedThere 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 do it with the unwrapETH command to do the same check for ETH output trades e.g. 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.
but surely you're also checking slippage from USDT to USDC and from DAI to USDC? or are you ignoring those checks until the end? My concern is that we're doing lots of slippage checks and the ones in the middle then dont even matter?
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.
My other thought process then also is just if posm & routers take/settle commands looks different now bc I was not planning on doing extra slippage checks on take/settle. (IK this is a future TODO but just adding my thoughts in this thread. Not blocking on getting this PR merged)
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 the ones in the middle dont matter. the user doesnt care if one half of the trade gets hit with more slippage than the other - theres just an output amount that they want from the trade in total and thats it
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.
for split route trades we pass in 0 as minAmountOut at each stage - and then do aggregated slippage checks on total output
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 could do it as a different cmmand or something depending how the other commands end up
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.
#199 just tagging that 199 should also put these in a lib so we can use in both contracts
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 make this type(uint256).max so its super explicit? Also matches other patterns like approving with type(uint256).max to mean my entire balance
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 weird.. on codespaces the code I was reviewing had the changes where you kept the flag for checking to use the contract balance.. I guess we can do that in a different PR!
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.
why not just import SafeCast from core?
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 function doesnt exist in core rip