Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(raiden): add sendPayment support #874

Merged
merged 1 commit into from Apr 16, 2019
Merged

Conversation

erkarl
Copy link
Collaborator

@erkarl erkarl commented Apr 8, 2019

In this commit we're adding support for RaidenClient.sendPayment that
enables us to do a trusted swap.

closes #797

@ghost ghost assigned erkarl Apr 8, 2019
@ghost ghost added the in progress label Apr 8, 2019
Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, made some comments below.

lib/Xud.ts Outdated
@@ -74,25 +75,30 @@ class Xud extends EventEmitter {
this.db = new DB(loggers.db, this.config.dbpath);
await this.db.init(this.config.initdb);

const swapClients: { [currency: string]: BaseClient | undefined } = {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking it actually might have made sense to do this earlier, but I'm thinking it might be cleaner to make this a Map<string, BaseClient> type. We're mapping currency to other values elsewhere in the codebase. But this change could also happen in a separate PR.

@@ -75,19 +78,23 @@ class RaidenClient extends BaseClient {
public readonly type = SwapClient.Raiden;
public readonly cltvDelta: number = 0;
public address?: string;
public currencies: { [key: string]: string } = {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a Map type as well, and maybe it would make more sense to call this tokenAddresses since that's what the values are?

}
});
} catch (e) {
this.logger.error(`failed to set currencies for Raiden: ${e}`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you make this this.logger.error('failed to set currencies for Raiden', e); the error method will log the exception message and stack trace if applicable.

let tokenAddress = '';
if (deal.role === SwapRole.Maker) {
// we are the maker paying the taker
amount = deal.makerAmount;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be takerAmount as that's the amount the taker is expecting to receive.

tokenAddress = this.currencies[deal.takerCurrency];
} else {
// we are the taker paying the maker
amount = deal.takerAmount;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be makerAmount.

// TODO: Secret hash. Depending on sha256 <-> keccak256 problem:
// https://github.com/ExchangeUnion/xud/issues/870
const tokenPaymentResponse = await this.tokenPayment(tokenAddress, deal.destination!, amount);
console.log('tokenPaymentResponse is', tokenPaymentResponse);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace this with a debug statement? And JSON.stringify the response.

@erkarl erkarl force-pushed the feat/raiden-sendpayment branch 5 times, most recently from e45c6e0 to dd1ed2e Compare April 11, 2019 09:02
@erkarl erkarl changed the title [WIP] feat(raiden): add sendPayment support feat(raiden): add sendPayment support Apr 11, 2019
Copy link
Collaborator Author

@erkarl erkarl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sangaman feedback applied. I've got the PR to the closest as I can to a real swap, but without being able to provide a custom sha256 hash for Raiden payments the swaps will not succeed.

I chose not to apply temporary code to support a "trusted" swap as there were too many places that required changes. However, it's still possible to test Raiden payments via xud:

  1. Setup Raiden for xud A & B
  2. Fund both Raiden accounts with ETH & WETH
  3. Setup direct channels between A & B
  4. (optional) test payment without xud
curl -i -X POST http://localhost:5001/api/v1/payments/0xc778417E063141139Fce010982780140Aa0cD5Ab/<receiverAddress> -H 'Content-Type: application/json' --data-raw '{"amount": 1234, "identifier": <randomInteger>}'
  1. Add WETH currency to A & B - ./bin/xucli addcurrency WETH Raiden 18 0xc778417E063141139Fce010982780140Aa0cD5Ab
  2. Add WETH pair to A & B - ./bin/xucli addpair WETH BTC
  3. Execute buy WETH order on B
  4. Execute sell WETH order on A

/**
* A class representing a client to interact with raiden.
*/
class RaidenClient extends BaseClient {
public readonly type = SwapClient.Raiden;
public readonly cltvDelta: number = 0;
public readonly cltvDelta: number = 1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for this change is that the SwapRequestPacket will not validate otherwise:
https://github.com/ExchangeUnion/xud/blob/master/lib/p2p/packets/types/SwapRequestPacket.ts#L37

@@ -426,7 +433,7 @@ class Swaps extends EventEmitter {
}

const responseBody: packets.SwapAcceptedPacketBody = {
makerCltvDelta: deal.makerCltvDelta || 0,
makerCltvDelta: deal.makerCltvDelta || 1,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for this change is that the SwapAcceptedPacket will not validate otherwise:
https://github.com/ExchangeUnion/xud/blob/master/lib/p2p/packets/types/SwapAcceptedPacket.ts#L36

Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you. Looks like about as close as we can get it without the hashes matching up.

@erkarl erkarl merged commit ed0afb2 into master Apr 16, 2019
@ghost ghost removed the in progress label Apr 16, 2019
@erkarl erkarl deleted the feat/raiden-sendpayment branch April 16, 2019 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Overview] Prepare XUD for ERC20<>Lightning swap
2 participants