swapResultsList improvements

[offerm]

price is missing from the swapResultsList.

Consider also to add the preimage or remove the rhash.

Should we have role here? isn't it always zero?

xud admin$ xucli sell 0.004 LTC/BTC 0.875
{
  "internalMatchesList": [],
  "swapResultsList": [
    {
      "orderId": "d7fa8c31-e42e-11e8-8219-a328c72ea886",
      "localId": "871f9570-e42f-11e8-ba73-5559559634a7",
      "pairId": "LTC/BTC",
      "quantity": 0.0005,
      "rHash": "c3f490a0641aa4d8bd0b456e74d50650fa42182ba4e5ed162304ae1a35ade8c9",
      "amountReceived": 45000,
      "amountSent": 50000,
      "peerPubKey": "03fd337659e99e628d0487e4f87acf93e353db06f754dccc402f2de1b857a319d0",
      "role": 0
    },
    {
      "orderId": "d6a842a1-e42e-11e8-8219-a328c72ea886",
      "localId": "871f9570-e42f-11e8-ba73-5559559634a7",
      "pairId": "LTC/BTC",
      "quantity": 0.001,
      "rHash": "c95ecd85d640879ec1cca3eb447ad13d6bd1651ad2041d2682867af3fc812097",
      "amountReceived": 89000,
      "amountSent": 100000,
      "peerPubKey": "03fd337659e99e628d0487e4f87acf93e353db06f754dccc402f2de1b857a319d0",
      "role": 0
    },
    {
      "orderId": "87a31d51-e42f-11e8-8219-a328c72ea886",
      "localId": "871f9570-e42f-11e8-ba73-5559559634a7",
      "pairId": "LTC/BTC",
      "quantity": 0.001,
      "rHash": "128a1cc547a9b2d7d84cd961e50d6999e7305b3eaa5a8586c0d202a7c925c8cd",
      "amountReceived": 90000,
      "amountSent": 100000,
      "peerPubKey": "03fd337659e99e628d0487e4f87acf93e353db06f754dccc402f2de1b857a319d0",
      "role": 0
    },
    {
      "orderId": "d6a842a1-e42e-11e8-8219-a328c72ea886",
      "localId": "871f9570-e42f-11e8-ba73-5559559634a7",
      "pairId": "LTC/BTC",
      "quantity": 0.0005,
      "rHash": "269e0ffa650188925a781caa468f676a832c38b276da3ce2855cea8871ad5075",
      "amountReceived": 44500,
      "amountSent": 50000,
      "peerPubKey": "03fd337659e99e628d0487e4f87acf93e353db06f754dccc402f2de1b857a319d0",
      "role": 0
    },
    {
      "orderId": "89dd2ed1-e42f-11e8-8219-a328c72ea886",
      "localId": "871f9570-e42f-11e8-ba73-5559559634a7",
      "pairId": "LTC/BTC",
      "quantity": 0.001,
      "rHash": "136adedb482639135251cb3b3787bf61a697d72ff50f55ae142085ad75aca715",
      "amountReceived": 90000,
      "amountSent": 100000,
      "peerPubKey": "03fd337659e99e628d0487e4f87acf93e353db06f754dccc402f2de1b857a319d0",
      "role": 0
    }
  ]
}

[kilrau]

Price is missing from the swapResultsList.

Yes, wanted to open an issue for this too!

Consider also to add the preimage or remove the rhash.

Adding preimage: yes, but why remove r_hash

Should we have role here? isn't it always zero?

Yep, I think we can remove this (always taker).

[offerm]

@kilrau I wrote

Consider also to add the preimage or remove the rhash.

note the or. If we add the preimage, we should not remove the rhash.

[kilrau]

Summary @ImmanuelSegol (also let us know your opinion):

• add price
• add preimage
• remove role

[sangaman]

Adding the price makes sense, but what's the value in adding the preimage? It wouldn't hurt but I don't know how it would be useful.

The role indeed can always be determined based on whether it's a swap result from placeOrder (always taker) or from subscribeSwaps (always maker). So we can remove it but it'd be nice to add some comments explaining that clearly in the proto definition.

[offerm]

r_hash and preimage should be together. Both included or both excluded. SubscribeSwap is working only on the maker side? Here again we confuse the CLI level with with the RPC calls/objects. The point here is that there is no value to should the `role` as part of placeorder CLI command since it will always be 'taker`. IMHO This simple change should be done on the CLI level and not on the backend level.

…

On Mon, Nov 12, 2018 at 1:10 AM Daniel McNally ***@***.***> wrote: Adding the price makes sense, but what's the value in adding the preimage? It wouldn't hurt but I don't know how it would be useful. The role indeed can always be determined based on whether it's a swap result from placeOrder (always taker) or from subscribeSwaps (always maker). So we can remove it but it'd be nice to add some comments explaining that clearly in the proto definition. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#656 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJQ0csXyplOtvtMosUjJg2QoXpM63P2Cks5uuK5ugaJpZM4YWvCS> .

[sangaman]

SubscribeSwap is working only on the maker side?

Yes this is how we agreed to approach it (otherwise it would duplicate info that we get in the placeOrder response) and is stated in the comments for the rpc method.

I'm not confusing anything, I just think the same reasons why we might not want role in the cli output also apply to why we might not want it in the api response. Either way works tho, it's a minor/cosmetic change and I was just sharing my opinion.

[kilrau]

r_hash and preimage should be together. Both included or both excluded.

Agree and let's include it for now for testing purposes of outside testers.

Here again we confuse the CLI level with with the RPC calls/objects. The
point here is that there is no value to should the role as part of
placeorder CLI command since it will always be 'taker`.
IMHO This simple change should be done on the CLI level and not on the
backend level

That's correct, but it's fine - a minor change and Daniel already agreed on removing it.

[kilrau]

Regarding subscribeSwaps , we always try to discuss these things in a group, multiple reviews of PRs - it's never to put on one ;)

Let's re-open the discussion about subscribeSwaps in nomatching mode from what we know today. Main calls are:

• executeSwap executes an atomic swap in a matter of with a remote peer. You can continue using your internal orderIDs, xud takes care of mapping such with orderIDs in the network. The swap might take up to several seconds to complete.
• subscribeSwaps informs about successful/failed swaps with other peers in an event stream, use this call to determine when to remove an order from your internal order book.

I suggest we offer the same SYNC + ASYNC way to update about a swap result as we offer for placeOrder. Meaning executeSwap can be called SYNC and delivers final swap results once everything is done OR can be called ASYNC, just delivers an immediate ACK if in xud is all good and all results come in by the time they occure via subscribeSwaps eventstream. Since only the taker calls executeSwap, this includes the taker.

I understand this is not implemented yet, hence would open an issue. Agree/different opinions about my suggestion? @offerm @sangaman

[sangaman]

The rpc subscription methods were discussed a ton (see #123 and #500). If we want to discuss more or about executeSwap that's fine but I don't think it should be in this issue because it's unrelated and will make the discussion harder to follow. In short I don't see the need for the changes and I think it would add complexity.

[kilrau]

Taking the subscribeSwap discussion out, this issue is ready to go @ImmanuelSegol : #656 (comment)