Meta DEx network/transaction message fields about omnicore

[zathras-crypto]

change the order of transaction fields

Seems (arguably) change with quite limited value proposition.

Please discuss, whether transaction type 21 can be restructured, such that the ecosystem field is no longer at the end of the data package, and the non-validated fields are removed

What do we gain here in tangible terms - I think smaller payloads is a worthwhile sentiment but when all current MetaDEx functionality fits into Class C already, what tangible benefits do we realize other than shaving a few bytes (maybe 3-5%?) from an average 300 byte transaction? We slimmed down so much with Class C already that I wonder how much we gain from this.

or ideally, if the transaction type can be replaced and split into 4 seperated ones (for each action) without action byte.

I actually kind of support this because I think it makes the whole thing a little cleaner and less ambiguous but I'm not sure - would need to spend a little more time looking over your suggested changes (from a brief look I saw you were still using the action byte in the RPC calls, which doesn't make sense to me if we're removing it?).

TL:DR; I guess my view comes down to that of:
"How much delay and how much risk does this introduce, and for what gains?"
If the answer to delay and risk is minimal/low and we get good gains from it I'm supportive - but I also know leadership won't allow us to push back release for long.

Also just want to say excellent stuff & don't take my feedback as negative :)

[dexX7]

Thanks @zathras-crypto!

In my opinion it should never had been specified this way, and it already caused trouble, because empty/zero fields were initially rejected.

The actual gain is probably small, but since it's not yet live, and the scope of the changes isn't huge, I think it's better to optimize now, rather than adding some v1 in the future.

I saw you were still using the action byte in the RPC calls, which doesn't make sense to me if we're removing it?

The interface of trade_MP was unchanged for the sake of compability, because all RPC tests I wanted to run also use trade_MP with action byte, but it creates the different payloads/transactions tx 25: new offer, tx 26: cancel at price, tx 27: cancel pair, tx 28: cancel everything as expected.

The changes very roughly:

• MSC_TYPE_METADEX was removed and replaced by MSC_TYPE_MDEX_NEW, MSC_TYPE_MDEX_PRICE, MSC_TYPE_MDEX_PAIR, MSC_TYPE_MDEX_ECOSYSTEM
• any positive check (RPC, UI) for MSC_TYPE_METADEX was replaced by "is either MSC_TYPE_MDEX_NEW, _PRICE, _PAIR, _ECOSYSTEM?"*
• any negative negative check for MSC_TYPE_METADEX was replaced by "is none of MSC_TYPE_MDEX_NEW, _PRICE, _PAIR, _ECOSYSTEM?"*
• logicMath_MetaDEx() was split into logicMath_MetaDEx_New(), logicMath_MetaDEx_CancelPrice(), logicMath_MetaDEx_CancelPair(), logicMath_MetaDEx_CancelEcosystem()
• the minimum accepted payload size was reduced from 8 to 5 byte
• CreatePayload_MetaDExTrade() was removed, and CreatePayload_MetaDExTrade(), CreatePayload_MetaDExCancelPrice(), CreatePayload_MetaDExCancelPair(), CreatePayload_MetaDExCancelEcosystem() were added to create the different payloads
• trade_MP/trade_OMNI was adjusted such that the correct payload is created
• a similar adjustment was used for the UI (create trade dialog, cancel trades dialog)

*Example (sort of lame, because there are no pending objects for cancel orders at this point, but the intention was to provide exactly the same behavior as before):

-  if (p_pending->type == 21) { htxo.txType = "MetaDEx Trade"; htxo.fundsMoved = false; } // send and metadex trades are the only supported outbound txs (thus only possible pending) for now
+  if (MSC_TYPE_MDEX_NEW == p_pending->type || MSC_TYPE_MDEX_CANCEL_PRICE == p_pending->type || MSC_TYPE_MDEX_CANCEL_PAIR == p_pending->type || MSC_TYPE_MDEX_CANCEL_ECOSYSTEM == p_pending->type) { htxo.txType = "MetaDEx Trade"; htxo.fundsMoved = false; } // send and metadex trades are the only supported outbound txs (thus only possible pending) for now

The actual logic, the GUI etc. already differentiates between new or cancel orders for obvious reasons, and unless I missed something, this it mostly all that's to do. :)

[zathras-crypto]

The actual gain is probably small, but since it's not yet live, and the scope of the changes isn't huge, I think it's better to optimize now, rather than adding some v1 in the future.

Understood - and that's kind of my feeling about the RPC layer too - why bother trying to maintain backwards compatibility with something that's never been released when instead we can just start fresh with dedicated RPC calls (sendtrade_OMNI, sendcanceltradebypair_OMNI sendcanceltradebyprice_OMNI, sendcancelalltrades_OMNI) or something to that effect - this way if we are saying we want to remove ambiguity let's do it across the board.

I realize it's a PITA as you'd have to redo the tests (and anyone who's playing with this stuff in test integrations would have to adapt but I don't think there are more than a couple of projects in that basket).

[dexX7]

sendtrade_OMNI, sendcanceltradebypair_OMNI sendcanceltradebyprice_OMNI, sendcancelalltrades_OMNI

Imho this is something we should adopt, even without a new transaction format.

I realize it's a PITA as you'd have to redo the tests

Haha, this is really not an issue. ;) Most of the meta DEx tests are currently only available via the old Python tests, and still need to be added to OmniJ anyway.

[achamely]

Omniwallet/Omniengine had limited support for original metadex development and that was only on testnet. There was going to need to be a major update/rewrite of that portion of the code with the new metadex launch anyway. I'm all for removing ambiguity and getting very specific/defined actions calls so the spit definitions of tx 25/26/27/28 is good in my book and should save some headache down the line when others start looking into integration

[dexX7]

Thanks for the feedback, @achamely! Ideally Omniwallet should use native Omni Core commands, and if something is missing, please feel free to open a ticket.

To tackle the topic, I suggest to move forward with Seperate RPC commands for DEx actions #48, and pin down all areas where action bytes are involved. Almost all parts already have a seperate logic, where the action byte usually acts as switch point. As last step there could be a seperation on a transaction level, if we decide this is the way to go.

[achamely]

@dexX7 once native commands for creating tx's from unknown address is available we'll use it. But at the moment OmniCore needs to have the address added/scanned before it will create tx's. Unfortunately for Omniwallet, this isn't possible. 1: too many addresses, 2: we don't have users private keys to add.

So we'd need support for the addrindex functionality to create tx's or get btc balance info

[zathras-crypto]

Interesting to note, even after a --startclean old trades still show up (albeit 'cancelled'). I'm looking into why now...

[zathras-crypto]

Scratch above comment, looks like I made a mess of the compile :( d'oh