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

Conversation

kincaidoneil
Copy link
Member

@kincaidoneil kincaidoneil commented Feb 2, 2020

Big things

  • Minimum exchange rate enforcement to fix Enforce minimum exchange rates #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)

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

  • 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
crates/interledger-stream/src/congestion.rs 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
Comment on lines 319 to 323
assert!(
match result {
Err(Error::SendMoneyError(_)) => true,
_ => false,
},
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enforce minimum exchange rates
2 participants