-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Check XRP endpoints for circular paths (RIPD-1781): #3209
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3209 +/- ##
===========================================
- Coverage 70.61% 70.61% -0.01%
===========================================
Files 676 676
Lines 54339 54346 +7
===========================================
+ Hits 38374 38377 +3
- Misses 15965 15969 +4
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM. CI has manual test failures that look related, so probably worth investigating.
I think should support circular payment not disable it |
Me too, but currently this is not supported, so until they are properly enabled I guess it is fine to make sure they are strictly not happening and not just most of the time unless there's some special edge case. |
All the forward and backward problems and then it seems that rippled can already handle that, so why not just enabling circular payments. It's a feature more than a drawback IMO. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good. The change does what is intended. I left a nit suggesting that the unit test coverage might be improved. But I'll leave that decision to you.
JLOG(j_.debug()) | ||
<< "XRPEndpointStep: loop detected: Index: " << ctx.strandSize | ||
<< ' ' << *this; | ||
return temBAD_PATH_LOOP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run code_cov over the new code, this line returning temBAD_PATH_LOOP
isn't hit. Is there a (not too painful) way to hit this line with a unit test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I can make a test that will hit this. The reason is the book steps also check for a path, and the book step will detect the loop before the endpoint will.
I'd still like to leave this code in, but in practice I don't think it can be hit.
f8a67e9
to
2b6293b
Compare
Rebased onto 1.5.0-b5 and added two more tests. Since this has already been approved in it's current version, I'm still going to consider this approved (but of course feel free to review the new tests). I'm going to aim to get this into 1.6 rather than 1.5, since we're so late into the 1.5 cycle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
c5b605e
to
85d21d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Code changes still look good.
I recommend patching up the commit message so the lines in the body are no more than 72 characters long. Right now, using git log
, the commit message looks like this in my terminal:
Check XRP endpoints for circular paths (RIPD-1781):
The payment engine restricts payment paths so two steps do not input the sam
e
Currency/Issuer or output the same Currency/Issuer. This check was skipped w
hen
the path started or ended with XRP. An example of a path that was incorrectl
y
accepted was: XRP -> //USD -> //XRP -> EUR
This patch enables the path loop check for paths that start or end with XRP.
@scottschurr I don't mind wrapping at 72 characters. Will do. I know you've requested changes like this in the past. The reason I keep posting 80 character commit messages is my editor wraps git commit messages at 80 characters by default and I haven't taken the time to figure out what I need to do to change that. |
The payment engine restricts payment paths so two steps do not input the same Currency/Issuer or output the same Currency/Issuer. This check was skipped when the path started or ended with XRP. An example of a path that was incorrectly accepted was: XRP -> //USD -> //XRP -> EUR This patch enables the path loop check for paths that start or end with XRP.
85d21d9
to
a9333bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks.
This introduces a new amendment called |
The payment engine restricts payment paths so two steps do not input the same
Currency/Issuer or output the same Currency/Issuer. This check was skipped when
the path started or ended with XRP. An example of a path that was incorrectly
accepted was: XRP -> //USD -> //XRP -> EUR
This patch enables the path loop check for paths that start or end with XRP.