search

coblox / nectar

GNU General Public License v3.0
0 stars 1 forks source link

Order matching #112

Closed rishflab closed 4 years ago

rishflab commented 4 years ago

Upgraded nectar to work with the new order matching engine and setup swap protocol in comit-rs. These changes are required for project tantalus.

bonomat commented 4 years ago

Hi Rishab, please add a few words on the status of this draft PR, e.g. what's missing in your opinion and what's the goal, anything in particular we should look at.

rishflab commented 4 years ago

Hi Rishab, please add a few words on the status of this draft PR, e.g. what's missing in your opinion and what's the goal, anything in particular we should look at.

Sorry about that. Added in the purpose to the description.

bonomat commented 4 years ago

Hey @rishflab . As you might have noticed, you've removed the rustfmt.toml but some formatting changes remain. Instead of removing the rustfmt.toml file with your commit in 379c8b6 (#112) you could do an interactive rebase back to 24e36e0 (#112) and not add it in the first place. That way your history remains clean and some formatting changes will never happen (such as the one @D4nte commented on: https://github.com/coblox/nectar/pull/112/files#r480484323).

da-kami commented 4 years ago

Sorry for not investigating further. When trying to run this for the demo I noticed some issues:

  1. The order that was published was incorrect by one decimal point (instead of 12000 it was 1200). I think this is either a precision or a general conversion error.

  2. Once Nectar "re-publishes" (i.e. cancel + new order) orders cnd never picks them up. Since the initial orders are picked up this is most likely a problem of cancelling orders.

Could not investigate further so far.

rishflab commented 4 years ago

Sorry for not investigating further. When trying to run this for the demo I noticed some issues:

  1. The order that was published was incorrect by one decimal point (instead of 12000 it was 1200). I think this is either a precision or a general conversion error.
  2. Once Nectar "re-publishes" (i.e. cancel + new order) orders cnd never picks them up. Since the initial orders are picked up this is most likely a problem of cancelling orders.

Could not investigate further so far.

hmm thanks for the feedback, ill see what i can do tommorow morning.

da-kami commented 4 years ago

Sorry for not investigating further. When trying to run this for the demo I noticed some issues:

  1. The order that was published was incorrect by one decimal point (instead of 12000 it was 1200). I think this is either a precision or a general conversion error.
  2. Once Nectar "re-publishes" (i.e. cancel + new order) orders cnd never picks them up. Since the initial orders are picked up this is most likely a problem of cancelling orders.

Could not investigate further so far.

hmm thanks for the feedback, ill see what i can do tommorow morning.

No worries, we got a demo with 2 UIs + 2 cnds working :) - can demo with Nectar in Cooldown :D

rishflab commented 4 years ago

Note you still remove the call to remove the active peer from the DB. Except for that, LGTM. Please note my functional review is limited as I am fully aware of the final design you are aiming for.

Yep you are right,