Xud: remove local_id from `SwapResult` in nomatching mode

[kilrau]

Currently nomatching mode (here a SwapResult) still maintains a local_id which doesn't make sense to me. There should be only one (global) order_id.

order_id: "631145c0-f901-11e8-b518-5fd626b71b77"
local_id: "e6fe35e0-f911-11e8-b9ca-27ecace2dafc"
pair_id: "LTC/BTC"
quantity: 0.001
r_hash: "aad8fae44138922fc52b1fca949c7f75294fa5f8345418634d23ba4d961b5b9f"
amount_received: 100000
amount_sent: 110000
peer_pub_key: "03fd337659e99e628d0487e4f87acf93e353db06f754dccc402f2de1b857a319d0"
currency_received: "LTC"
currency_sent: "BTC"

[kilrau]

Related: https://github.com/ExchangeUnion/xud/issues/223

[sangaman]

Sorry I'd missed this earlier, conceptually I don't follow though. Why wouldn't we want a local id for nomatching mode? What if an order has an id on the local exchange? Wouldn't we want a way to track that id in xud and associate it with the global order id?

[kilrau]

Wouldn't we want a way to track that id in xud and associate it with the global order id?

Yep, that's exactly what we want. But only on makers side. Sorry bad job from my side, you had no chance to understand what I meant @ImmanuelSegol @sangaman

Here the full story: on maker's side the order gets issued by the exchange, a local_id assigned by the exchange and then placed in xud. local_id is useful here since the exchange can process the returned SwapResult directly based on the local_id. On taker's side however, xud never knows about the internal id the exchange is using because xud receives the order first and only then tells the exchange about it. At the moment the local_id is there, with another, different uuid. That's not useful and even confusing -> that's why we want to remove it. Sorry for the missing explanation, please revise your PR when you have the chance @ImmanuelSegol

[sangaman]

In that taker image on the right, the local_id is the randomly generate global order id - since no local id was specified the local and global ids are the same.

I can see how that may be a little confusing, but what if the exchange does want to associate a taker order with an actual order on their exchange? Imagine I'm an exchange in no matching mode, I get order 123 and match it with an order in xud, now I call ExecuteSwap and I want to tell xud that the local id of the order is 123, so it gets saved in the database and can be searched/retrieved later.

Maybe the confusing part is that the order_id in the swap result refers to the global id of the maker order, whereas local_id refers to the local id of the local order, whether it's the maker or the taker. It's definitely not clear based on the comments in the proto file, so I'm thinking at a minimum we should clarify the comments (I can handle that) and maybe rename fields to be more explicit.

[kilrau]

In that taker image on the right, the local_id is the randomly generate global order id - since no local id was specified the local and global ids are the same.

Yes, exactly, but the taker should not generate another global order id (a4ed...). It should just take the existing one (8768...) from the maker and set it as local_id. I can't come up with a reason why not, is there? @sangaman

Maybe the confusing part is that the order_id in the swap result refers to the global id of the maker order, whereas local_id refers to the local id of the local order, whether it's the maker or the taker.

They should be the same, see above.

In short, xud never knows about the exchange's internal order_id when acting as a taker hence the exchange needs to do the mapping itself. Once we have a good idea how, we can improve this.

Other opinions? @sangaman @moshababo @offerm ?

[sangaman]

Yes, exactly, but the taker should not generate another global order id (a4ed...). It should just take the existing one (8768...) from the maker and set it as local_id. I can't come up with a reason why not, is there?

Because they are separate orders. And the 8768 order is a peer's order, not a local order, so it wouldn't make sense to use it as local id. The local id is used to track the exchange's internal id for an order, and if the taker order has one then we'd want to preserve that data.

In short, xud never knows about the exchange's internal order_id when acting as a taker hence the exchange needs to do the mapping itself.

Why? You can set a local id via PlaceOrder while acting as a taker order. I'm actually thinking we might want to add an optional local id parameter to the ExecuteSwap call, that way the exchange doesn't need to track this mapping itself, which seems more convenient to me.

[kilrau]

decision from the call: simply remove local_id from SwapResult response (both matching and nomatching mode) @ImmanuelSegol

[sangaman]

We may need to discuss this again real quick. Not including the local id for swap results when we are the taker still makes sense. However, I think we would still want it for swap results of our maker orders, no? #741 is currently removing it altogether.

[kilrau]

However, I think we would still want it for swap results of our maker orders, no?

Yes

[kilrau]

Closed as per https://github.com/ExchangeUnion/xud/pull/847#issuecomment-532698593