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

Stop sending packets if F-family error encountered #440

Closed
wants to merge 6 commits into from

Conversation

@nkramer44
Copy link
Contributor

nkramer44 commented Mar 17, 2020

Fixes #430

@nkramer44 nkramer44 force-pushed the nk/430-short-circuit branch from ebb6c6e to be2a301 Mar 17, 2020
Signed-off-by: nkramer44 <noah.ph.kramer@gmail.com>
@nkramer44 nkramer44 force-pushed the nk/430-short-circuit branch from be2a301 to f1555f9 Mar 17, 2020
@nkramer44 nkramer44 requested review from sappenin, theotherian and nhartner Mar 17, 2020
nkramer44 added 2 commits Mar 17, 2020
Signed-off-by: nkramer44 <noah.ph.kramer@gmail.com>
…/quilt into nk/430-short-circuit
response.handle(
(fulfill) -> {},
(reject) -> {
if (!reject.getCode().equals(F08_AMOUNT_TOO_LARGE) &&

This comment has been minimized.

Copy link
@kincaidoneil

kincaidoneil Mar 17, 2020

Shouldn't this also keep retrying on F99 errors?

This comment has been minimized.

Copy link
@nkramer44

nkramer44 Mar 17, 2020

Author Contributor

@sappenin would know better...

The Java receiver only returns an F99 if the prepare amount < stream amount, but the Java sender doesn't have any retry behavior to deal with F99s. Maybe it should?

This comment has been minimized.

Copy link
@kincaidoneil

kincaidoneil Mar 19, 2020

There are other cases where the JavaScript STREAM receiver returns F99 errors, too, which is something to consider.

The Java receiver only returns an F99 if the prepare amount < stream amount

But should the payment fail if only a single packet doesn't meet the minimum exchange rate? Or should there be multiple attempts?

Rust and JS retry like normal on F99s, but I think we probably need more sophisticated behavior for the exchange rate issues.

This comment has been minimized.

Copy link
@nkramer44

nkramer44 Mar 30, 2020

Author Contributor

Agreed. For now, our main motivation for short circuiting is to reduce our log output/error alerting, which is mainly happening on F02s. I will narrow this condition to only short circuit on F02s, and we can discuss what we want to do on F99s.

theotherian and others added 2 commits Mar 27, 2020
Signed-off-by: nkramer44 <noah.ph.kramer@gmail.com>
sappenin added a commit that referenced this pull request Mar 30, 2020
Fixes #430
* Supersedes PR #440 (now closed).
* Fixes #430: Clarify and enhance the InterledgerErrorCodes that the SimpleStreamSender will fail-fast on and stop the stream. These include any F or R-family error, except F08 and F99. Any T-family error will NOT abort the sendMoney operation.
* Fixes Invalid Denomination: If a receiver doesn't send back `ConnectionAssetDetails` frames to the sender during preflight, then the denomination will be absent. In this case, the FixedReceiverAmountPaymentTracker doesn't handle this condition properly.
* Enhance unit & integration tests to cover these scenarios.
* Add Javadoc in various places and cleanup formatting

Signed-off-by: David Fuelling <sappenin@gmail.com>
@sappenin sappenin mentioned this pull request Mar 30, 2020
@sappenin

This comment has been minimized.

Copy link
Contributor

sappenin commented Mar 30, 2020

This PR has been superseded by #443

@sappenin sappenin closed this Mar 30, 2020
sappenin added a commit that referenced this pull request Apr 1, 2020
* Fixes #430

* Supersedes PR #440 (now closed).
* Fixes #430: Clarify and enhance the InterledgerErrorCodes that the SimpleStreamSender will fail-fast on and stop the stream. These include any F or R-family error, except F08 and F99. Any T-family error will NOT abort the sendMoney operation.
* Fixes Invalid Denomination: If a receiver doesn't send back `ConnectionAssetDetails` frames to the sender during preflight, then the denomination will be absent. In this case, the FixedReceiverAmountPaymentTracker doesn't handle this condition properly.
* Enhance unit & integration tests to cover these scenarios.
* Add Javadoc in various places and cleanup formatting

Signed-off-by: David Fuelling <sappenin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.