Skip to content
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

Remove solvable orders zero input balance check #1388

Closed
wants to merge 1 commit into from

Conversation

vkgnosis
Copy link
Contributor

Reverts #137 which was used to fix #136 . While working on partially fillable orders I noticed I did not understand the purpose of this code.

The mentioned PR adds a check for orders that require no input balance. It justifies that this could happen for partially fillable orders. I don't understand that because:

  • We don't allow orders to be created with zero amounts.
  • When fetching open orders from the database, we filter out orders that are completely filled.

Thus it is impossible that an order will end up needing 0 input balance at this point. But then why did we think this was the correct fix before? I don't know.

Test Plan

CI

@vkgnosis vkgnosis requested a review from a team as a code owner March 29, 2023 08:34
@nlordell
Copy link
Contributor

Thus it is impossible that an order will end up needing 0 input balance at this point.

It can happen if you have, for example, a buy order selling 1Xwei for 2Ywei after the order is half filled (as 1Xwei / 2 = 0Xwei in integer math).

@MartinquaXD
Copy link
Contributor

I took a look at the order that initially caused the problem (copied from this instance file):

        "22": {
            "sell_token": "0xddafbb505ad214d7b80b1f830fccc89b60fb7a83",
            "buy_token": "0xe91d153e0b41518a2ce8dd3d7944fa863463a97d",
            "sell_amount": "0",
            "buy_amount": "1890",
            "allow_partial_fill": true,
            "is_sell_order": false,
            "fee": {
                "amount": "0",
                "token": "0xddafbb505ad214d7b80b1f830fccc89b60fb7a83"
            },
            "cost": {
                "amount": "99472603385085",
                "token": "0xe91d153e0b41518a2ce8dd3d7944fa863463a97d"
            },
            "is_liquidity_order": true,
            "mandatory": false,
            "has_atomic_execution": false
        },

This is a partially fillable liquidity buy order. The important part is sell_amount == 0 but buy_amount: != 0 which is very odd as it should never happen that a buy order is not fully executed but already has no sell_amount left to give.
I guess since we trust our smart contract to 100% verify the limit prices this could indicate some bug which causes our bookkeeping in the DB to be a bit off.
The order was returned from the DB because our query only checks that buy orders still have some unexecuted buy_amount and sell orders still have some unexecuted sell_amount left which was the case for this order.
So I think what the original patch accomplished was not to fix the underlying issue (bookkeeping bug) and instead only masked it.
All of this assumes that we didn't adjust the order's amounts further on it's way from DB to instance.json.

@vkgnosis vkgnosis closed this May 8, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 8, 2023
@vkgnosis vkgnosis deleted the zero-amoutn-check branch July 7, 2023 07:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter out or remove filled partially fillable orders
3 participants