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

feat(stream): sender refactor with rate enforcement #616

Merged
merged 2 commits into from Feb 7, 2020
Merged

Conversation

@kincaidoneil
Copy link
Collaborator

kincaidoneil commented Feb 2, 2020

Big things

  • Minimum exchange rate enforcement to fix #273
    • Uses exchange rate store to calculate rate to recipient, minus configurable slippage margin
    • All packets are unfulfillable while waiting for destination asset details from recipient
  • Refactor of STREAM sender using async-await and tokio for scheduling to improve performance
  • Moved all rate APIs to new interledger-rates crate

Little things

  • Added checking of STREAM sequence number to prevent replays
  • Added safe reporting of the minimum delivered amount
  • Cross-currency integration tests, and testing failure if spread is too large/poor exchange rate
  • Added behavior to fail fast if the rate of rejected packets is too high (> 99%, and at least 200 attempts)
  • Added additional metadata to the STREAM delivery receipt (minor changes to receipt returned from API, updated Swagger docs to reflect this)
  • Added slippage configuration to /payments endpoint (percentage of calculated exchange rate which is acceptable to deliver)
@kincaidoneil kincaidoneil requested review from aanchal4 and gakonst Feb 2, 2020
@kincaidoneil kincaidoneil requested review from dora-gt and emschwartz as code owners Feb 2, 2020
@kincaidoneil kincaidoneil removed request for dora-gt and emschwartz Feb 6, 2020
@kincaidoneil kincaidoneil force-pushed the ko-stream-rates branch 4 times, most recently from c740d7c to 958c80d Feb 6, 2020
@gakonst
gakonst approved these changes Feb 7, 2020
Copy link
Member

gakonst left a comment

  • Rates crate looks good
  • Service-util refactor also looks good
  • great work on the new stream client, finally it will not infinitely loop if we send a payment above min_amount

Here's a PR with some more suggestions: #629

crates/interledger-rates/src/lib.rs Outdated Show resolved Hide resolved
docs/api.yml Show resolved Hide resolved
examples/eth-xrp-three-nodes/README.md Outdated Show resolved Hide resolved
examples/eth-xrp-three-nodes/README.md Outdated Show resolved Hide resolved
assert!(
match result {
Err(Error::SendMoneyError(_)) => true,
_ => false,
},
Comment on lines 319 to 323

This comment has been minimized.

Copy link
@gakonst

gakonst Feb 7, 2020

Member

nit: You could just do let err = send_money(...).await.unwrap_err(), and then check that the error is the expected one

This comment has been minimized.

Copy link
@kincaidoneil

kincaidoneil Feb 7, 2020

Author Collaborator

I couldn't find an idiomatic way to compare enum variants without a match? And then it's kinda awkward to translate that into an assertion

crates/interledger-stream/src/client.rs Show resolved Hide resolved
crates/interledger-stream/src/client.rs Outdated Show resolved Hide resolved
crates/interledger-stream/src/client.rs Outdated Show resolved Hide resolved
PaymentEvent::MaxInFlight => {
// Wait for 100ms for any request to complete, otherwise try running loop again
// to see if we reached the timeout since last fulfill
let fut = timeout(

This comment has been minimized.

Copy link
@gakonst

gakonst Feb 7, 2020

Member

technically since you called await on this, it's no longer a future but the inner result. It might be useful to make this timeout be an exponential backoff or grow in some way? Maybe add a TODO if you think it'd be useful, otherwise leave as is

This comment has been minimized.

Copy link
@kincaidoneil

kincaidoneil Feb 7, 2020

Author Collaborator

Updated this so it calculates the deadline (last fulfill timestamp + max duration since last fulfill), then waits for any request to complete or reach the deadline before running the loop again, which should timeout the payment.

1c5ffed#diff-3c8f182d5b0fd6fb02fa8ce8402491e4R263-R306

kincaidoneil and others added 2 commits Jan 31, 2020
Co-Authored-By: Georgios Konstantopoulos <me@gakonst.com>
@kincaidoneil kincaidoneil force-pushed the ko-stream-rates branch from f17616f to 70fcb21 Feb 7, 2020
@kincaidoneil kincaidoneil merged commit ed83801 into master Feb 7, 2020
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test-md Your tests passed on CircleCI!
Details
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.

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