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.
|
|
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. |
| JLOG(j_.debug()) | ||
| << "XRPEndpointStep: loop detected: Index: " << ctx.strandSize | ||
| << ' ' << *this; | ||
| return temBAD_PATH_LOOP; |
There was a problem hiding this comment.
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.
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.
seelabs
force-pushed
the
xrp-circular-paths
branch
from
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. |
seelabs
force-pushed
the
xrp-circular-paths
branch
from
c5b605e
to
85d21d9
Compare
There was a problem hiding this comment.
👍 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.
seelabs
force-pushed
the
xrp-circular-paths
branch
from
85d21d9
to
a9333bb
Compare
seelabs
added
the
Passed
|
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.