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

Settle immediately if account with negative settle_to is created #622

Merged
merged 11 commits into from Feb 6, 2020

Conversation

@gakonst
Copy link
Member

gakonst commented Feb 5, 2020

Closes #591

This PR does some housekeeping & refactoring before tackling the issue.

Our settlement client logic was broken in 2 pieces, account creation and execution of the settlement. The first lived in the API crate while the second one lived in the Settlement crate.

This PR moves the account creation logic to the Settlement crate, and takes advantage of its retry-mechanism to add retry features to the send_settlement and send_message calls. This also allows us to finally remove the tokio-retry crate and with that the dependency on futures 0.1 compat.

The client also made sense to exist in the settlement::core module, as we want it to be consumed by other packages as well without them having to turn on the settlement_api feature.

Once that house keeping was done, the API Client was switched to use the one from interledger_settlement, and the action taken is: if negative settle_to is provided on account creation (or modification by the admin) -> execute settlement

gakonst added 4 commits Feb 5, 2020
… on SettlementClient
…de indefinitely
@gakonst gakonst requested a review from emschwartz as a code owner Feb 5, 2020
@gakonst gakonst requested a review from kincaidoneil Feb 5, 2020
…move futures 0.1 compat
@gakonst gakonst force-pushed the gakonst/settlement-account-creation branch from 5c49cd2 to 4dc569d Feb 5, 2020

// On successful account creation, if the settle_to was negative, we should send a
// settlement request to them
if let Some(settle_to) = settle_to {

This comment has been minimized.

Copy link
@kincaidoneil

kincaidoneil Feb 6, 2020

Collaborator

We need to update the balance before we settle: suppose we prefund, then send a Prepare which gets fulfilled, now we're going to have settled that whole prefund amount twice, which is bad.

One super quick and dirty way to do this... call update_balances_for_fulfill on the store with a 0 amount, which will return the amount to settle (you'll need to remove the check if outgoing_amount == 0 from that method, but otherwise everything should still work?). Then use that amount instead of settle_to for the amount you pass to the settlement client

This comment has been minimized.

Copy link
@gakonst

gakonst Feb 6, 2020

Author Member

Aha, good point. I'm fine with getting the settle_to value via the update_balances_for_fulfill call. It might be worth also doing the same in the Err(reject) branch of the call, if the reject was a T04 then it should trigger settlement after calling update_balances_for_fulfill(account, 0).

crates/interledger-api/src/http_retry.rs Show resolved Hide resolved
gakonst added 3 commits Feb 6, 2020
we will abuse process_fulfill with 0 balance packets to trigger settlements
we removed the 0 packet check inside the store implementation to abuse it during account creation
@gakonst gakonst force-pushed the gakonst/settlement-account-creation branch from 4dc569d to 66c8988 Feb 6, 2020
gakonst added 3 commits Feb 6, 2020
@gakonst gakonst merged commit 2e42258 into master Feb 6, 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
@gakonst gakonst deleted the gakonst/settlement-account-creation branch Feb 6, 2020
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.