mastercoin-MSC / mastercore

mastercore info
mastercoin.org
MIT License
24 stars 11 forks source link

Update to new data structures, getorderbook_MP #185

Closed ghost closed 9 years ago

ghost commented 9 years ago

Once delete is in, there will be some further discussion on the fate of the other RPCs, and how to handle that, I think

dexX7 commented 9 years ago

Can you please update L1127 to getorderbook_MP currency_id1 ( currency_id2 )? Doing so includes the parameters in the help overview. :)

I'm not sure if it should, but there could be a check, if both properties are in the same ecosystem.

Just some brainstorming: are there other places where a filtered view of orders could be useful or is already used (maybe in the order matching logic?) and if so, is there a way to access this directly? I'm sort of asking, because I believe there might be other actions in the future, e.g. "order the view by price" and I guess two implementations would probably start to differ quickly.

ghost commented 9 years ago

Right now the main discussion is how to do that Dexx, because there is no support for historical storage of these orders.

The current idea is to let gettransaction_MP be the home for those wishing to integrate mastercore into their own solution so they can create the custom queries, since Bitcoin core doesn't offer too much in terms of flexible querying on the blockchain in any form.

The other is some usage of a database or persisted storage using flat files, but that is cumbersome and difficult to manage. We'll have to see how things unfold, but for now, the data returned will be effectively for all time (unless you're filtering by timestamp or currency) because there isn't any internal deletion of Metadex orders.

I added those params to L1127, but it doesn't make much sense to me, since they are listed further in the string.

dexX7 commented 9 years ago

I added those params to L1127, but it doesn't make much sense to me, since they are listed further in the string.

When you enter "help" in the debug console or "./mastercore-cli help" then the old output was:

getnetworkhashps ( blocks height )
getnetworkinfo
getnewaddress ( "account" )
getorderbook_MP

And with this addition it's:

getnetworkhashps ( blocks height )
getnetworkinfo
getnewaddress ( "account" )
getorderbook_MP currency_id1 ( currency_id2 )

This sounds contradicting to me: "there is no support for historical storage of these orders" vs. "there isn't any internal deletion of Metadex orders".

My point was: if "getorderbook_MP" creates the impression of "this or that order will be filled next", but in reality the order matching and execution engine does something different, then I would consider it as something that seems broken.

ghost commented 9 years ago

the order matching algorithm is fairly deterministic, if you look at the output of getorderbook_MP and then the unit price of a new order, you will be able to tell if will be filled or not.

The issue at hand is this: Right now the data structures used on the RPC layer should hold only the active set of orders on the metadex. After metadex_delete is implemented this will be true, for now, getorderbook_MP shows all orders, historical and current (due to unimplemented functions).

Historical, or pruned data (filled orders) should still be retreivable using gettradessince_MP or gettradehistory_MP, but currently there isn't a good way to do this. That is what is meant by 'there is no support for historical storage of these orders' vs deletion.

dexX7 commented 9 years ago

Small other note: "getbalance_MP", "send_MP", "sendtoowners_MP" and a few more use "propertyid". For consistency I suggest to rename "currency_id" to "propertyid". Probably not the best and some "overall unification" needs to be done anyway (e.g. #163), but the more consistent, the better - imho.


That is what is meant ...

Ah I see, build finished, but didn't run it yet. :)

ghost commented 9 years ago

Fixed - but irrelevant i think, there's a pending RPC rename so it could all change in the next release or so... @m21 I think this is ready for review by you now

dexX7 commented 9 years ago

Tested - works fine.


Future notes:

Would be nice to see a price in addition to the ratio, but imho precision isn't specified yet and offers such "for sale: 10.0, desired: 0.00000001" don't fit well into the current "show 8 digits" format.

Instead of getopenorders_MP there could be an additional parameter for getorderbook_MP such as "include-inactive".

And somehow related: getorderbook_MP does what it should while gettradehistory_MP and gettradessince_MP confuse "trades" (matched and executed orders) with "orders/offers" (the intend to sell/buy).