-
Notifications
You must be signed in to change notification settings - Fork 9
Investigate multiplication overflow issue #203
Comments
This is cool, but aren't there other places where unlimited orders are placed? |
See gnosis/dex-open-solver#64 for more details |
Putting this on hold until we have a conclusion for our investigation of gnosis/dex-open-solver#64 |
I think at least for the OWL liquidity we will need this anyways (maybe the max-amount can be configurable) |
We could easily lower this amount by a very large margin (10⁹) without compromising much on our selling point of not requiring any maintenance. This margin should be enough to avoid the issue we are having with unlimited orders, at least for the main tokens that are traded in the exchange. More concretely, decreasing the magic number for unlimited orders by 10⁹ would allow a user to swap back and forth at every batch for more than one year an amount of one million of a 18-decimal token without renewing the orders. To verify that with the node console:
An order is just the two amounts of tokens to be swapped against each other. The situation is worse for tokens with more decimals and tokens that are very cheap because a user would have to swap more token units to move the same amount of "value" compared to a more standard token. |
Looking at gnosis/dex-open-solver#64 it looks like a significant factor for the issue we were seeing was that we were trading with ourselves (and since the solver does not "exploit" trades with one-selves by maxing out the potential trading volume until all balance is eaten up by fees) we had a large disregarded utility. I'm no longer sure we actually need to do anything here. On the other hand making the amount configurable seems simple enough and could be useful to have in any case. |
|
Further discussion is needed before getting rid of unlimited orders, since it's not clear how frequently the issue occurs and whether this change would ameliorate the issue. For now we keep unlimited orders until the issue presents itself again. |
A good suggestion of @fedgiac, would be that perhaps we could get a good idea of where to go if we can force this scenario to happen on the testnet. |
@fedgiac How is this coming along? Do we have an open PR with some code related to the investigation? |
Because we can run into multiplication overflows during the utility-calculation, we should no longer use them
The text was updated successfully, but these errors were encountered: