-
Notifications
You must be signed in to change notification settings - Fork 154
Implement sell=buy support for Sell orders #3894
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
Conversation
de09f52 to
858ac73
Compare
|
I think this PR needs to be converted to a draft, since more changes are expected. @m-sz , is that correct? |
|
Yep, had a bunch of 1:1s and will be adding feature flag to the whole change to control it in orderbook. |
|
I have updated the PR with CLI argument to orderbook to allow for same sell and buy tokens. |
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
405acc6 to
6fe22d1
Compare
Co-authored-by: José Duarte <duarte.gmj@gmail.com>
Co-authored-by: José Duarte <duarte.gmj@gmail.com>
Co-authored-by: José Duarte <duarte.gmj@gmail.com>
Co-authored-by: José Duarte <duarte.gmj@gmail.com>
Co-authored-by: José Duarte <duarte.gmj@gmail.com>
| .context("summary buy token is missing")?; | ||
|
|
||
| if *sell_token_lost >= sell_token_lost_limit || *buy_token_lost >= buy_token_lost_limit { | ||
| if (!sell_token_lost.is_zero() && *sell_token_lost >= sell_token_lost_limit) |
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.
So in other words "something that used to work now fails". I'd like a more thorough investigation than that. If adding this check is indeed the correct thing to do here there should be a convincing explanation.
| // Check for orders selling wrapped native token for native token. | ||
| if &order.sell_token == native_token && order.buy_token == BUY_ETH_ADDRESS { | ||
| return Err(PartialValidationError::SameBuyAndSellToken); | ||
| } |
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 is this always forbidden? With the new feature this should be allowed when the flag is enabled, no?
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 will test it out
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.
Is that a case we want to support? As I understand, right now if a user places such order we are simply unwrapping their WETH for ETH, same goes the other way around.
I tried if it works, and it seems that in order to support that, we would need to plug in native token everywhere where I check for sell=buy (so into the solver for example), and then compare the tokens to native or ETH buy address
| input, | ||
| output, | ||
| interactions: Vec::default(), | ||
| gas: eth::Gas(U256::ZERO) + self.solution_gas_offset, |
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.
Did you check that there is logic somewhere that accounts for the order hooks? I'm a bit worried that the baseline solver will start bleeding money when it doesn't consider the gas costs correctly.
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 routing (which is present in the non sell=buy case) takes a Request and max amount of hops.
Request contains wrappers (which I understand are the hooks), and they are present in the solution in both cases.
Routing itself cares about the sell and buy token, computing a path to solve for them, not taking into account wrappers. So I think we should be fine, but I will check it on staging with hooks.
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 will introduce an E2E test case verifying the hooks gas usage in sell=buy orders in a follow-up PR.
Co-authored-by: Martin Magnus <martin.beckmann@protonmail.com>
Co-authored-by: Martin Magnus <martin.beckmann@protonmail.com>
|
I have revisited how we calcualate the before and after balance. This now allows us to avoid the |
Introduce special case to handle sell=buy when calculating the out amounts.
The out_amount had to be extended to I512, to account for negative outcomes of the previous calculation
|
With @MartinquaXD's help, We have also came up with a correct way to account for sell and buy token's |
| // It looks like the out_amount is 0 but only because the sell and buy tokens | ||
| // are the same. |
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 this comment can now be dropped in favor of the more specific one below.
Sell=buy token initiative
The ability to swap the same token pairs unlocks the potential for CoW to generally become a transaction relay service. This feature will allow users to execute pre- and post- interaction hooks as gasless transactions. The overall implementation has certain caveats:
Lift restrictions on sell=buy token
Based on input from @fhenneke: