Bonfida / agnostic-orderbook

Apache License 2.0
79 stars 33 forks source link

Bug in Fixed point modulo maths #47

Closed Henry-E closed 2 years ago

Henry-E commented 2 years ago

This operation does not output the correct result.

https://github.com/Bonfida/agnostic-orderbook/blob/0acdcea2ec4a4760be68a076c5a883ce9f7daa20/program/src/processor/new_order.rs#L123-L125

Example:

Tick price: 0.1 As FP32: 429496729 Limit price: 0.9 As FP32: 3865470566

3865470566 % 429496729 = 5 This is presumably because of the floor operation in the FP32 calculation. Pre floor values are3865470566.4 and 429496729.6 So these value probably need to be converted back to floats before doing the modulo operation?

ellttBen commented 2 years ago

Hi @Henry-E! The underlying ticksize is defined in FP32 terms. This means that your limit price must be a multiple of the tick size in FP32. The easiest way to compute this would be with something like : floor(0.9 / 0.1) * 429496729.

We think it wouldn't be a good idea to add some float logic to the AOB, because it's too computationally expensive for little benefit. This problem should be solved on the client side. This might be a usability issue so thanks for reporting it, we'll think about adding a helper util to perform this kind of computation.

Henry-E commented 2 years ago

Yes, it might be useful to add some typescript tests demoing the different basic functionality of placing orders, reading the event queue and the order book. I couldn't find any examples in the dex v4 repo but I might have been looking in the wrong place.