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

Honoring the original unit price in updated offers #22

Closed
dexX7 opened this issue Apr 22, 2015 · 7 comments
Closed

Honoring the original unit price in updated offers #22

dexX7 opened this issue Apr 22, 2015 · 7 comments

Comments

@dexX7
Copy link
Member

dexX7 commented Apr 22, 2015

When calculating the amounts to trade on the meta DEx, the applied unit price of an order may be rounded to the next tradable unit, constrained by the amounts available, however, the original unit price should be honored in subsequent trades.

Example trade:

A1 offers 100.00000000 SPX for 10.00000000 MSC
A2 offers 0.80000000 MSC for 7.27272727 SPX

A1 receives 0.80000000 MSC, and has 9.272727273 SPX left for sale (A1's offer is updated)
A2 receives 7.27272727, and has 0 SPX left for sale (A2's offer is erased completely)

A1's offer is updated to: 92.72727273 SPX for 9.27272727 MSC

From the debug log:

PRICE CHECK TRADED_MOREINBUYER: buyer = 0.10000000000000000000000000000000000000000000000000 , inserted = 0.09999999996764705882448096885810350091593813232600 : PROBLEM!

This slightly off unit price is due to the ratio of 9.27272727/92.72727273.

As consequence, the unit price starts to drift, and may be significantly different after one or more trades.


A straight forward fix requires two additional fields for CMPMetaDEx objects, original_amount_forsale and original_amount_desired, which shall be used to calculate the unit price for (subsequent) offers.

This likely introduces the requirement of further adjustments to CMPMetaDEx::effectivePrice(), maybe to the core meta DEx logic, and certainly to the persistence of data, in particular CMPMetaDEx::saveOffer().

As far as I can see, no change of CMPTradeList::recordTrade() is necessary, but this hidden, or original unit price should be clearly reflected in all user faced channels (i.e. gettrade_MP, getorderbook_MP, ... ?).

@dexX7
Copy link
Member Author

dexX7 commented Apr 22, 2015

Alternatively, to save a field, there could be original_amount_forsale, original_amount_desired, amount_left_for_sale and amount_desired() => amount_left_for_sale * original_amount_desired / original_amount_forsale.

@CraigSellars
Copy link
Member

Good catch. This is something that Zathras mentioned during the all-hands earlier this week. Thanks!

@zathras-crypto
Copy link

Alternatively, to save a field, there could be original_amount_forsale, original_amount_desired, amount_left_for_sale and amount_desired() => amount_left_for_sale * original_amount_desired / original_amount_forsale.

Well, actually I know I originally suggested adding two new fields (for original values) to the CMPMetaDEx class but your comment above must have jigged a brain cell or two hehe - don't we already have everything we need without expanding the class?

private:
  int block;
  uint256 txid;
  unsigned int idx; // index within the block
  unsigned int property;
  uint64_t amount_forsale; // the amount for sale specified when the offer was placed
  unsigned int desired_property;
  int64_t amount_desired;
  uint64_t still_left_forsale;
  unsigned char subaction;
  std::string addr;

I think @m21's original intentions when building that class were to store the original values in amount_forsale and amount_desired and use still_left_forsale to maintain the running tally on how much is left in the trade (if partially matched). At some point the code has obviously deviated from this - I would posit somewhere around the decision to use a replacement methodology for partial fills - meaning amount_forsale & amount_desired lose their original values.

Thus, correct me if I'm wrong, but couldn't we try to steer things back in that original direction, and save ourselves the trouble of expanding the CMPMetaDEx class?

TL:DR; make amount_forsale and amount_desired immutable and use the still_left_forsale field combined with your suggestion

amount_desired() => amount_left_for_sale * original_amount_desired / original_amount_forsale

So it would become amount_desired() => still_left_forsale * amount_desired / amount_forsale

Thoughts?

@dexX7
Copy link
Member Author

dexX7 commented Apr 24, 2015

Thoughts?

Yeah, I was looking at my Python based trading engine, I build some time ago to quickly test trades (the logic isn't the one, which got specified IIRC), and after reading it, it sort of got me: "wait.. what if we calculate the desired amount on the fly? oh wait! isn't there even still_left_for_sale?" ... hehe.

This was very likely the original intend, which got twisted somehow. Some of the RPC fields also still show _original (the interface is very inconsistent, but that's probably worth another issue).

So I agree, and think this could be the way to go.

Since you also have a meta DEx branch, this is relevant: I started to split _dex into _dex and _mdex, and used the IDE for formatting:

I'm still not sure, how to structure it, e.g. there is safeOffer(), effectivePrice(), but also Set() and other upper case method names. Further, some blocks could be moved up or down ... but I think it would be useful, if we both use a common base. What do you think?

@zathras-crypto
Copy link

common base

Agreed, if you split them out and PR it, as long as it's just splitting and no functional changes we should be able to merge right away, and then I can work from the split files

@dexX7
Copy link
Member Author

dexX7 commented Apr 26, 2015

if you split them out and PR it, as long as it's just splitting and no functional changes

See #26. Please let me know, if this is fine.

@dexX7
Copy link
Member Author

dexX7 commented May 3, 2015

This issue is going to be resolved by the currently pending/close-to-be-ready changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants