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

fix(swaps): validate Raiden's resolve request #1172

Merged
2 commits merged into from
Aug 26, 2019

Conversation

ghost
Copy link

@ghost ghost commented Aug 22, 2019

In this commit we validate Raiden's resolve request to make sure the
payment we're receiving has the previously agreed upon deal.makerCltvDelta as the
minimum duration of the lock.

In addition we change the error handling to return a status code 400
(Bad Request) to prevent Raiden from crashing. Previously we returned a
status code of 200 with the response of { secret: 'error message' }.

Closes: #1149

@ghost ghost requested review from sangaman and offerm August 22, 2019 15:26
@ghost ghost self-assigned this Aug 22, 2019
@kilrau
Copy link
Contributor

kilrau commented Aug 22, 2019

Please open an issue on raiden side for the crash @erkarl

@kilrau kilrau requested a review from moshababo August 22, 2019 18:58
@ghost ghost force-pushed the fix/raiden-resolve-request-validation branch from b18dcaa to bd57361 Compare August 23, 2019 08:23
@ghost
Copy link
Author

ghost commented Aug 23, 2019

Please open an issue on raiden side for the crash @erkarl

There already is: ExchangeUnion/raiden#1

@kilrau
Copy link
Contributor

kilrau commented Aug 23, 2019

In addition we change the error handling to return a status code 400 (Bad Request) to prevent Raiden from crashing. Previously we returned a status code of 200 with the response of { secret: 'error message' }.

I successfully tested this branch on native simnet, which since #1080 changed the behaviour of the XUD resolver when it gets an unfamiliar rHash and caused raiden to crash. With this branch, this is handled gracefully and raiden doesn't crash.

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.

Looks nice, the 400/404/500 HTTP error codes here are much more sensible than returning plain error message strings in place of the preimage. Just a few questions/suggestions below.

@@ -68,6 +68,7 @@ export type RaidenResolveRequest = {
reveal_timeout: number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be removing reveal and settle timeout as part of this PR? Or are you planning to do this separately? I'm thinking if it's done together this can be marked as closing #1149.

If so, you can also remove the commented out "unused" fields below.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, absolutely - good idea.

if (err.code === errorCodes.INVALID_ARGUMENT) {
if (
err.code === errorCodes.INVALID_ARGUMENT ||
err.code === swapErrorCodes.INVALID_RESOLVE_REQUEST
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 I should've made this a switch statement when I wrote this originally, but now I think it's even more clearly the nicer choice. Would you mind rewriting these if/elses as a switch?

Copy link
Author

Choose a reason for hiding this comment

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

Rewrote this as switch - looks much better now, thanks.

@ghost ghost force-pushed the fix/raiden-resolve-request-validation branch from bd57361 to 801a4dc Compare August 23, 2019 18:02
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, one last (or maybe two) change re: the new switch statement then this should be good to merge.

@ghost ghost requested a review from sangaman August 24, 2019 09:02
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.

Looks good! Do you think you could put the tslint rule in a separate commit tho?

@ghost ghost force-pushed the fix/raiden-resolve-request-validation branch from c904006 to e98de8f Compare August 25, 2019 09:35
@ghost ghost requested a review from sangaman August 25, 2019 09:36
@michael1011
Copy link
Contributor

@erkarl @sangaman could you please revisit this PR and merge it asap? The raiden in my local simnet instance is crashing a lot which causes some issues with the balancing of the initial deposit.

Karl Ranna added 2 commits August 26, 2019 11:54
In this commit we validate Raiden's resolve request to make sure the
payment we're receiving has the previously agreed upon `deal.makerCltvDelta` as the
minimum duration of the lock.

In addition we change the error handling to return a status code 400
(Bad Request) to prevent Raiden from crashing. Previously we returned a
status code of 200 with the response `{ secret: 'error message' }`.
@ghost ghost force-pushed the fix/raiden-resolve-request-validation branch from 85bf023 to 6d525a7 Compare August 26, 2019 11:55
@ghost ghost merged commit 561e396 into master Aug 26, 2019
@ghost ghost deleted the fix/raiden-resolve-request-validation branch August 26, 2019 13:47
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error handling Handling errors gracefully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raiden lock time follow-ups
4 participants