feat(raiden): check for direct channels #1065

sangaman · 2019-06-25T10:17:35Z

Closes #1027.

This adds logic to check that we have a direct channel with a peer before enabling trading for a given Raiden token. Currently multi-hop Raiden payments are not well supported, and this will prevent any attempts to swap Raiden tokens without a direct channel.

Note, this branch is based off of #1055 (which has useful refactoring for determining whether to disable specific currencies trading) and #1064 (which updates raiden calls used in this PR) so it must be rebased and merged only after those two PRs are merged. The key commit to review for the purposes of this PR is the last one.

sangaman · 2019-06-25T16:35:38Z

To be clear, this disables a trading pair entirely with a given peer if the pair involves a raiden currency for which we don't have a direct channel. I can certainly make this configurable so it can easily be turned off, and also make the logging more clear.

Making the check happen only when an order is placed doesn't really work since we may have channels with some peers but not with others, and may connect to other peers later on for whom we don't have direct channels.

Another option is to check right before we attempt or accept a swap, but this involves more code and potentially a bad user experience for users to be shown orders that are guaranteed not to work.

ghost · 2019-06-26T09:44:37Z

this disables a trading pair entirely with a given peer if the pair involves a raiden currency for which we don't have a direct channel

I feel this would provide much better user experience than displaying a warning upon placing an order or executing a swap. Also, you have already done the hard work and implemented it.

The concern that Offer had was that we want to be able to turn it off it starts to cause problems. I'd make it configurable under debug section. debugRaidenDirectChannelChecks=true?

kilrau · 2019-06-26T12:52:57Z

The concern that Offer had was that we want to be able to turn it off it starts to cause problems. I'd make it configurable under debug section. debugRaidenDirectChannelChecks=true?

Thanks @erkarl . After sleeping on this, I also think we should go ahead with it. Make it configurable, default to true. I'd rather call it raidenDirectChannelChecks=true in a [debug] section in the config, but that's not important.

Reason: everything else I could come up with is not better or quite some coding work. Also, simnet can support direct channels in an automated manner. And I'll put a clear warning on this limitation in the wiki install guides.

michael1011

The concept looks good to me and I have just some minor questions/comments but I didn't test this branch yet. I will do that once it is rebased

lib/orderbook/OrderBook.ts

lib/p2p/Peer.ts

michael1011 · 2019-07-04T18:15:06Z

lib/p2p/Peer.ts

+  /** The version of xud this peer is using. */
+  private _version?: string;
+  /** The node pub key of this peer. */
+  private _nodePubKey?: string;


Why do these new variables have an underscore as prefix?

I made getter functions for these two variables and needed to use different naming for the variables themselves, this was the cleanest way I could think to do it. It was done as part of #1055. I can change it in a separate PR if you have a better approach in mind.

lib/p2p/Pool.ts

ghost · 2019-07-05T10:15:30Z

Will review and test this manually once it is rebased.

sangaman · 2019-07-16T22:05:11Z

I updated the commit to check for a direct channel with sufficient balance before each swap attempt. I put the logic in the getRoutes function for the raiden client, if we don't have a channel with enough balance the method will now return an empty array and the swap will fail due to NoRouteFound before we ever attempt to make payments for the swap.

kilrau · 2019-07-17T14:29:16Z

lib/raidenclient/RaidenClient.ts

+          }
+        }
+      }
+      return []; // no direct channels, return no routes


I definitely would print a NoDirectChannels error here, not generic NoRouteFound. Maybe sth along the lines. Swap failed due to no direct raiden channel, please make sure to have a direct raiden channel with this peers and try swapping again.

I can log messages like that here explaining that either there is no direct channel with this peer or that we have one but it doesn't have enough balance, but note that either way this will go in the log output and not in a grpc response where it's front and center for a user who has just issued an xucli command can see.

I added log statements - I'm hesitant though to introduce a new swap failure reason tho since it would require some non-trivial refactoring and since this is a temporary safeguard that we may remove later. NoRouteFound I think is descriptive enough, it tells the user that there's no suitable route to make a payment and the standard corrective action for that in any case is to open a channel to the peer we want to swap with.

Alright. We'll make it clear in the documentation.

ghost · 2019-07-17T16:35:45Z

lib/raidenclient/RaidenClient.ts

-    // stub placeholder, query routes not currently implemented in raiden
-    // assume a fixed lock time of 100 Raiden's blocks
-    return [{
+  public getRoutes =  async (amount: number, destination: string, currency: string) => {


Normally I'd advocate an integration test here, but since this is temporary logic... 😆

lib/raidenclient/RaidenClient.ts

This adds logic to check that we have a direct channel with a peer with sufficient balance for raiden currencies before accepting or proposing a swap. Currently multi-hop Raiden payments are not well supported, and failed payments are not handled well in Raiden. This will prevent any attempts to swap Raiden tokens without a direct channel. Closes #1027.

sangaman added raiden swaps labels Jun 25, 2019

sangaman requested a review from a user June 25, 2019 10:17

sangaman self-assigned this Jun 25, 2019

sangaman changed the title ~~Raiden/direct channel check~~ feat(raiden): check for direct channels Jun 25, 2019

This was referenced Jun 26, 2019

Raiden todo's ExchangeUnion/xud-simnet#28

Closed

Enable multiple routing attempts = multi-hop raiden routes #1068

Closed

kilrau requested a review from michael1011 July 2, 2019 16:14

michael1011 reviewed Jul 4, 2019

View reviewed changes

sangaman force-pushed the raiden/direct-channel-check branch from 2650000 to 338fb55 Compare July 16, 2019 22:03

sangaman requested a review from michael1011 July 16, 2019 22:03

kilrau reviewed Jul 17, 2019

View reviewed changes

sangaman force-pushed the raiden/direct-channel-check branch 3 times, most recently from fc7802b to 3bab0a7 Compare July 17, 2019 16:33

ghost approved these changes Jul 17, 2019

View reviewed changes

sangaman force-pushed the raiden/direct-channel-check branch from 3bab0a7 to a87e903 Compare July 17, 2019 18:45

sangaman merged commit ab78f26 into master Jul 17, 2019

sangaman deleted the raiden/direct-channel-check branch July 17, 2019 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(raiden): check for direct channels #1065

feat(raiden): check for direct channels #1065

sangaman commented Jun 25, 2019

sangaman commented Jun 25, 2019

ghost commented Jun 26, 2019

kilrau commented Jun 26, 2019

michael1011 left a comment

michael1011 Jul 4, 2019

sangaman Jul 16, 2019

ghost commented Jul 5, 2019

sangaman commented Jul 16, 2019

kilrau Jul 17, 2019

sangaman Jul 17, 2019

sangaman Jul 17, 2019

kilrau Jul 18, 2019

ghost Jul 17, 2019

feat(raiden): check for direct channels #1065

feat(raiden): check for direct channels #1065

Conversation

sangaman commented Jun 25, 2019

sangaman commented Jun 25, 2019

ghost commented Jun 26, 2019

kilrau commented Jun 26, 2019

michael1011 left a comment

Choose a reason for hiding this comment

michael1011 Jul 4, 2019

Choose a reason for hiding this comment

sangaman Jul 16, 2019

Choose a reason for hiding this comment

ghost commented Jul 5, 2019

sangaman commented Jul 16, 2019

kilrau Jul 17, 2019

Choose a reason for hiding this comment

sangaman Jul 17, 2019

Choose a reason for hiding this comment

sangaman Jul 17, 2019

Choose a reason for hiding this comment

kilrau Jul 18, 2019

Choose a reason for hiding this comment

ghost Jul 17, 2019

Choose a reason for hiding this comment