Split MetaDEx RPC commands and payload creation

[dexX7]

"sendtrade" with action value is replaced by:

• "sendtrade": fromaddress propertyidforsale amountforsale propertyiddesired amountdesired
• "sendcanceltradesbyprice": fromaddress propertyidforsale amountforsale propertyiddesired amountdesired
• "sendcanceltradesbypair": fromaddress propertyidforsale propertyiddesired
• "sendcancelalltrades": fromaddress ecosystem

Until the test tools are updated, the old command "trade_MP" is still available and it forwards the commands. The commands are suffixed with _OMNI.

Suggestions for other names are very welcomed. :)

---

CreatePayload_MetaDExTrade() is split into:

• CreatePayload_MetaDExTrade()
• CreatePayload_MetaDExCancelPrice()
• CreatePayload_MetaDExCancelPair()
• CreatePayload_MetaDExCancelEcosystem()

This change has no functional effect, and does not change the transaction format, but solely wraps and seperates the actions into different logic units.

Related code is updated, including the reference-payload-creation unit tests.

A few notes to CANCEL_EVERYTHING:

It's behavior is as follows: if property for sale and property desired are within the same ecosystem, then all offers in that ecosystem are cancelled, and if they are in different ecosystems, or zero, then all offers of both ecosystems are cancelled.

That's not new, but now much more relevant, because the updated RPC command, and the payload creation function, directly comsume an ecosystem value.

It should be discussed, whether we want to support cancelling of any order, or restrict it to either one ecosystem (in UI and RPC interface).

I intend to continue the seperation, however, some feedback would be great, especially in regards to the pending transaction objects and UI, because I'm not sure, what other plans there are in that context.

---

This PR resolves #48.

[zathras-crypto]

Suggestions for other names are very welcomed. :)

Not to be pedantic, but for sendnewtrade I think the 'new' is redundant because you cannot send anything but a new trade. All the rest of our commands don't utilize "new" in the name (eg we don't have "sendnewgrant" "sendnewrevoke" "sendnewissuance" and so on).

In a nutshell I think "new" is implied and we should maintain continuity (perhaps replace "sendnewtrade" with "sendtrade" when test tools are updated?)

Also, should we pluralize here given the action could affect multiple trades? (eg sendcanceltradebypair vs sendcanceltradesbypair)

It should be discussed, whether we want to support cancelling of any order, or restrict it to either one ecosystem (in UI and RPC interface).

I'd go with providing the user the most capability - it seems relatively simple now you're separating this out. Cancel all should directly consume an ecosystem value (instead of trying to interpret from two property fields) - 1 for main eco, 2 for test eco, and whilst not explicitly stated in the spec, 0 for both.

I intend to continue the separation, however, some feedback would be great, especially in regards to the pending transaction objects and UI, because I'm not sure, what other plans there are in that context.

Sure - how can I help here? FYI at the present time pending objects are used purely when balances change (to prevent sending or trading the same funds twice) - since cancellations don't affect balances until confirmation (parsing and processing) I hadn't seen much need for pending cancellations as yet. Though further down the track I'm sure it would be nice to reflect pending actions (trades, cancellations etc) within the respective UI elements, I think that will need to wait until we have inbound pending sorted too (which @CraigSellars has asked is top of the list for 0.0.11).

[dexX7]

Great input, and I agree, the new is sort of redundant.

Also, should we pluralize here given the action could affect multiple trades?

This is what I was wondering about, too. I think we should pluralize here.

Regarding "sendcanceltradebyecosystem": should we emphasize the ecosystem in the name? I think it's annoyingly long, and we might as well use "sendcancelalltrades"?

Sure - how can I help here?

I'm mostly unsure how to deal with:

• rpctx.cpp#L997-L1003
• metadexcanceldialog.cpp#L330-L344

It would be awesome, if we find an alternative to the hack of deriving the ecosystem based on property identifiers, and I'm also not a huge fan of passing dummy arguments, so my questions basically:

1. Could we add more specific AddPendingXXX() functions, such as AddPendingMetaDexCancelAll(), AddPendingMetaDexTrade()?
2. Could we split MetaDExCancelDialog::SendCancelTransaction() into more specific methods to _CancelByPrice(), _CancelByPair(), ...?

... whilst not explicitly stated in the spec, 0 for both.

Hehe, so this is what I was referring to, when I mentioned "it caused quite some trouble already" in the thread about transaction formats.

Initially CANCEL_EVERYTHING worked such that it cancels all trades of both ecosystems, and at that time valid property identifiers and amounts were checked and required on the RPC layer, and zero values were rejected completely. This was clearly a violation against the "ecosystems are strictly seperated" approach, and checking the values didn't make much sense, so I started to deal with it, allowed zero values on the RPC layer, changed the spec to what it is now, and implemented the additional logic.

Because there is no explicit ecosystem field, it was required to derive the ecosystem based on the provided values, namely property identifier for sale and "property identifier desired`. While it is somewhat reasonable in my opinion to have a rule such as: "if the identifiers are in the main ecosystem, then cancel trades in the main ecosystem, and if they are in the test ecosystem, then cancel trades in the test ecosystem". But still, there are also the cases where the identifiers are in different ecosystems, or where they are zero. I didn't want to invalidate those transactions, because there is no strong relation between property identifiers and the ecosystem, but it's rather a hack, and "cancel trades in both ecosystems, if the values are zero" seemed like the lesser evil.

Depending on the outcome of #47 we may reject ecosystem = 0 on a protocol level, but if it remains as it is, then I suggest to:

1. Only provide a way to cancel trades in either one ecosystem via UI, but not both
2. Allow ecosystem = 0 via RPC, since it's the interface for advanced use, and it's a legit protocol action

[dexX7]

Updated to "sendtrade_OMNI", "sendcanceltradesbyprice_OMNI", "sendcanceltradesbypair_OMNI", "sendcancelalltrades_OMNI".

The old command "trade_MP" acts as forwarder, until OmniJ is updated.

[zathras-crypto]

Regarding "sendcanceltradebyecosystem": should we emphasize the ecosystem in the name? I think it's annoyingly long, and we might as well use "sendcancelalltrades"?

+1

Could we add more specific AddPendingXXX() functions, such as AddPendingMetaDexCancelAll(), AddPendingMetaDexTrade()?

Absolutely

Could we split MetaDExCancelDialog::SendCancelTransaction() into more specific methods to _CancelByPrice(), _CancelByPair(), ...?

Sure, we'd just have a handler function triggered by the "Send Cancel Request" button that branched out into separate functions based on the selected radio button.

In terms of the UI in general - I don't want you to worry too much about that dude I'll adapt it to follow whatever structure we need to - I think the right thing to do is get the fundamentals right (which incidentally I think includes breaking out MetaDEx transactions away from an action byte and into independent transaction types)

1. Only provide a way to cancel trades in either one ecosystem via UI, but not both
2. Allow ecosystem = 0 via RPC, since it's the interface for advanced use, and it's a legit protocol action

I'm not sure here mate, why disable functionality via the UI that would be available via RPC? If we're going to allow cancelling trades in both ecosystems via ecosystem=0 I see no reason to force a UI user to send two transactions (once for each ecosystem).

[dexX7]

I've been thinking about the pending transactions: ideally they are not added on the UI/RPC layer, but by the wallet (or in theory: when the unconfirmed transaction enters the system [which is currently not handled]). We're probably not yet there, but I imagine with the new populate and interpret functions, it should become doable.

I'm not sure here mate, why disable functionality via the UI that would be available via RPC?

Yeah, you're right. If ecosystem = 0 is allowed, then it should be supported.

Do you agree this PR is good to go? If so, I'd like to get it in, and then update OmniJ, and finally remove trade_MP.

[zathras-crypto]

Do you agree this PR is good to go?

I do :) Again I wanted to just note my support for splitting out the transactions in their entirety to make separate MetaDEx transactions for each action. Just because we used an action byte in DEx v1 in no way means we have to make the same mistake (imo) for the MetaDEx. I see this PR as tying in very closely with that topic.