ExchangeUnion / xud

Exchange Union Daemon 🔁 ⚡️
https://exchangeunion.com
GNU Affero General Public License v3.0
115 stars 44 forks source link

Rest endpoint for Raiden resolve request #931

Closed ghost closed 5 years ago

ghost commented 5 years ago

In order for Raiden to query for external hash from xud we need a rest endpoint.

@sangaman proposed to not go through the gRPC API since once we move to hold invoices we can deprecate that call.

ghost commented 5 years ago

Need expected JSON input for the POST request @offerm

sangaman commented 5 years ago

Yes, currently we do have an http endpoint for resolve requests via the web proxy, however it's outdated as it doesn't include the token address field that Raiden is specifying. We either need to:

1) Update the resolver proto definition to include the token address/identifier field, which will in turn update the http web proxy.

2) Create a separate HTTP service that will listen on a single endpoint for hash resolver requests, and put the proto definition on the sideline.

I'm leaning towards option 2, it feels a little bit cleaner (we don't have to rely on the web proxy which we haven't tested thoroughly to my knowledge) and is probably more performant. I also don't think it would be too much harder to implement since it's a single call with a single endpoint.

offerm commented 5 years ago

Here is the code:

 request = {
        'token': to_hex(token),
        'secrethash': to_hex(secret_request_event.secrethash),
        'amount': secret_request_event.amount,
        'payment_identifier': secret_request_event.payment_identifier,
        'payment_sender': to_hex(secret_request_event.recipient),
        'expiration': secret_request_event.expiration,
        'payment_recipient': to_hex(raiden.address),
        'reveal_timeout': raiden.config['reveal_timeout'],
        'settle_timeout': raiden.config['settle_timeout'],
    }

    try:
        response = requests.post(raiden.config['resolver_endpoint'], json=request)
sangaman commented 5 years ago

Thanks. That's pretty different from our current resolver message format, so all the more reason to go with option 2 above.

I'm thinking the only fields we really need to concern ourselves with for now are token, secrethash, and amount right? If you think one of the other fields is needed for xud purposes currently, let me know how they should be used.

kilrau commented 5 years ago

That's pretty different from our current resolver message format, so all the more reason to go with option 2 above.

Well, the raiden format will be the only format for the resolver after https://github.com/ExchangeUnion/xud/pull/923 is merged so..

any other arguments against https://github.com/improbable-eng/grpc-web ?

sangaman commented 5 years ago

Well, the raiden format will be the only format for the resolver after #923 is merged so..

any other arguments against https://github.com/improbable-eng/grpc-web ?

It just means that we'd have to put in work anyway to change the format, so might as well do it the cleaner/more performant way.

I don't have any other arguments other than what I've said here, but fyi we're not actually using grpc-web. I can't remember the details exactly now but we couldn't get it to work, but maybe support has improved for Node.js since.

I'll probably take a look at implementing an http endpoint for this tonight or tomorrow.

ghost commented 5 years ago

Option 2 feels like the right way to go.

sangaman commented 5 years ago

@offerm Just to confirm, the amount will always be the smallest unit available for that token, correct? gwei in the case of WETH?

ghost commented 5 years ago

the amount will always be the smallest unit available for that token

Pretty sure it's the smallest unit in integers (based on my experience with playing around with payment API).

sangaman commented 5 years ago

Also to clarify, it looks like Raiden is currently making unencrypted http calls. I take it that will be the approach for the time being, and I'll by default only listen to localhost for now.

sangaman commented 5 years ago

Another question @offerm, in your post here you had 'secrethash': to_hex(secret_request_event.secrethash), but in https://github.com/raiden-network/raiden/blob/master/raiden/network/resolver/client.py#L24 it looks like secrethash has an underscore like secret_hash. I'm guessing what's in their code with the underscore is correct, but just wanted to make sure I'm not missing anything.

kilrau commented 5 years ago

Option 2 feels like the right way to go.

Alright, since you both agree on that - vamos! @erkarl

I'll by default only listen to localhost for now.

Sounds like the right thing to do! @sangaman

kilrau commented 5 years ago

Btw it looks like we need two more issues:

correct? @erkarl @sangaman

ghost commented 5 years ago

If I'm not mistaken #940 will cover resolver <-> hodl invoice integration.

Simnet upgrade could probably be manual to speed up the process since we're not trying to be backwards compatible, yet.

offerm commented 5 years ago

@offerm Just to confirm, the amount will always be the smallest unit available for that token, correct? gwei in the case of WETH?

Yes.

Also to clarify, it looks like Raiden is currently making unencrypted http calls. I take it that will be the approach for the time being, and I'll by default only listen to localhost for now.

Resolver request made by raiden can be http or https. up to us to decide. localhost is good.

Another question @offerm, in your post here you had 'secrethash': to_hex(secret_request_event.secrethash), but in https://github.com/raiden-network/raiden/blob/master/raiden/network/resolver/client.py#L24 it looks like secrethash has an underscore like secret_hash. I'm guessing what's in their code with the underscore is correct, but just wanted to make sure I'm not missing anything.

Good catch - it shouldbe secrethash without the underscore. Looks like they merge the PR without the last commit. I recreated a new PR for this. For now, continue to use my branch (we will need it for the sha256 anyway)

Simnet upgrade could probably be manual to speed up the process since we're not trying to be backwards compatible, yet.

Upgrade can be done manually if we have a step by step instructions. Scripts needs to change so we need a PR anyway. xud-simnet-start would need changes anyway.