balanced / balanced-api

Balanced API specification.
220 stars 72 forks source link

Bulk credits sweep account #707

Closed remear closed 9 years ago

remear commented 9 years ago

Missing scenarios

https://github.com/balanced/balanced-api/pull/707#issuecomment-62575260

mjallday commented 9 years ago

i think the reason we went with transfers as opposed to credit/debits for these transactions is this:

you created a credit to the sweep account from order /orders/123123. now when you look at /orders/123123/credits it would not exist because it was not a credit to the order. when i look at /orders/123123/debits would i see a debit? that doesn't make sense if so, we just created a credit.

with a transfer all you see is a transfer, it's omnidirectional it had a source of the order balance and a destination of the sweep account.

mjallday commented 9 years ago

looking :+1: @remear.

mjallday commented 9 years ago

@remear is customer_deposit_account the correct terminology for this? we've informally called it a sweep account internally which i'm not too tied to but from reading the official definition

A deposit account is a savings account, current account, or other type of bank account, at a banking institution that allows money to be deposited and withdrawn by the account holder.

it sounds very generic and the same term could apply to several of the funding instruments within the Balanced API.

remear commented 9 years ago

Let's refer to it as sweep_account.

mjallday commented 9 years ago

I had a bit of a deeper think about this spec, here's what I think we need to flesh out:

@remear can you please implement the reversal and debit settlement scenarios in this pull-request? thanks! :)

matin commented 9 years ago

In this case I would create a reversal of the credit associated to a previously succeeded settlement and then create a new settlement. This settlement would have a negative balance which would result in a debit against the merchant's bank account

@mjallday We may be using different words to describe the same thing ... I suggest reversal should still happen against the account even if it means that the balance of the account becomes negative. That allows the marketplace to net the reversal against future credits or perform a settlement of a negative amount.

on retrying ... keep it explicit—no automatic retry. Why does a settlement have to be treated like a transaction to work?

remear commented 9 years ago

I think what's described in the reversals section @mjallday shared is not what settlements are for. If they need to get funds back into orders faster for refunding, that's what https://github.com/balanced/balanced-api-private/issues/13 is aiming to solve. I get what's being said, but that scenario seems like a side effect of how the sweep account reversing could work and not the scenario that we're actually trying to solve.

How are we going to handle reversals from settlements? I think this negative balance settlement is confusing since it sounds like the system also creates settlements and I assume those would show up to Customers. I'd suspect many would get confused at how it was created and how it all works.

I'm more +1 to make these one-way only, at least for now, to alleviate this barrier for implementing Orders and revisit reversals later.

I'll think more on the retrying of failed settlements.

mjallday commented 9 years ago

We may be using different words to describe the same thing

Yes I believe so. They settlement is only created when they choose to do so, the sweep account can maintain a balance, negative or positive, until explicitly settled.

re: retrying - there is no auto retry. What I meant is do we create an entirely new settlement, or we do create a new transfer and update the state on the settlement. To put another way, when a settlement transitions to the FAILED state, is it possible to transition it to PENDING again after updating the funding instrument or is FAILED terminal and I must create a brand new settlement?

mjallday commented 9 years ago

@remear when I was talking to customers that was one of their primary questions. The recurring blockers for moving to Orders were:

IMO we can achieve all of these pretty easily with the sweep.

since it sounds like the system also creates settlements and I assume those would show up to Customers

There is no settlement resource currently exposed to customers that I'm aware of. The legacy settlement object that we have internally should not be considered in this implementation.

mjallday commented 9 years ago

@remear re: retries

I would do

PUT /settlements/ST123123123 { destination: /bank_accounts/BA123123 }

and have that create a new transfer, and update the state to PENDING again. I would only allow this for settlements whose state is FAILED.

this will generate an audit event so it will be possible to see the history of attempts.

reversals should be dead easy. just reverse the txn associated to the sweet account and then create the settlement as per-normal. lmk if i missed anything.

this should be testable on integration marketplaces by using any of the test bank account numbers that create failed transactions i think so the spec can take that into account.

remear commented 9 years ago

the ability to cover a refund in one order with funds from another

My understanding was that we don't want this.

There is no settlement resource currently exposed to customers that I'm aware of

We're exposing one. What I meant is the system is creating settlement resources as well as the API user. My concern was exposing those system settlements seems like it would be confusing to people, especially with seeing negative balances.

Brain dump commencing... maybe I'm misunderstanding how you're suggesting we want to represent these reversals. To me, a settlement means settling funds from an Account out to a funding instrument, not the other way around. A settlement for a reversal seems confusing. The reversal would also be associated to the new settlement instead of the one that has the transfer you're "reversing". Why exactly does reversing a settlement (partial, or whole?) require another settlement to be created? What if you reversed the settlement and it just had another transfer associated to the same account settlement?

The suggested retry approach seems fine to me. I need to give more thought on how to handle canceled settlements.

matin commented 9 years ago

@remear a "settlement" is a settlement of accounts/balances. If there's a positive balance, that gets settled. If there's a negative balance, that gets settled.

@mjallday can you clarify what you mean by this?

the ability to cover a refund in one order with funds from another

Do you mean net out the reversal from past/future credits of other orders?

matin commented 9 years ago

@mjallday thanks for the clarification. +1 for creating a new settlement. FAILED should be a terminal state.

mjallday commented 9 years ago

FAILED should be a terminal state.

I disagree :)

In my mind, for both invoices and settlements (but can focus just on the settlement) they are grouping constructs the same as an order. Therefore the transaction associated with the transfer can fail but the settlement can be retried (which creates a new transaction).

This gives us debit.settlement when using a client library to find the settlement associated to the transaction rather than [settlement for settlement in debit.settlements if settlement.state != 'SUCCEEDED'][0].

If you view a settlement as a transaction then that would be inconsistent with other transactions but in my mind a settlement is not a transaction.

matin commented 9 years ago

@mjallday where is the balance represented for a settlement (positive or negative) when it has the status FAILED and the destination hasn't been updated?

mjallday commented 9 years ago

the balance would be the sum of the transactions associated to the settlement.

matin commented 9 years ago

@mjallday the account balance. not the settlement balance.

  1. Account balance of $100
  2. Settlement of $100
  3. credit $50 to account. Account has a balance of $50
  4. Settlement of $100 fails

What is the account balance? $50 or $150?

mjallday commented 9 years ago

the account with the type of sweep has a scalar balance the same as an escrow account or any other type. it's common across all accounts. is that what you mean?

matin commented 9 years ago

@mjallday submitted too early. see my updated comment.

mjallday commented 9 years ago

Depends if settlement is a transaction or not. If it's a grouping construct then those funds are still associated to the settlement which can be retried. If it's a transaction then it goes to the FAILED state and the funds are moved back into the account.

That's the crux of the issue, is a settlement a transaction or a grouping construct. I may have confused the issue by likening it to an invoice, it doesn't have to be one way or the other unless we can think of a good reason.

Invoices are this way because the invoice is a collection of fees. When you try to settle or re-settle an invoice the fees associated to it regardless of the failure or success of the funds movement. It's a snapshot of fees for a specific period in time.

I initially viewed a settlement through this lens. It's a group of transactions that are being settled. Would it be difficult for a customer if they decided to settle funds from 3 credits and then had it fail and the next time they settled it included 4 orders?

mjallday commented 9 years ago

If I were the marketplace in that scenario I would want to create settlements in the same manner that we create invoices. E.g. I'd like to have some control over the contents of each settlement. For example, I want to pay out weekly via a batch, that would mean I'd like transactions from 00:00 Monday to 23:59:59 the following Sunday. I can get close to having that control by creating my settlements via a cron job that runs at 00:00 Monday.

If my settlement fails and I create a new one that includes the next weeks settlements then I now have to explain to my customer that the next settlement is different from the usual settlements. Having control of what transactions go into the settlement that I create could alleviate this, as could controlling when it's created.

This is the strongest argument for having a settlement be a grouping construct. If Balanced wants to give control of the contents at some point in the future, it cannot be a transaction because I cannot alter the contents of it at a later date. If it's a grouping construct I can create it, add the contents and then move the funds when I'm ready. I would have more control. I do not see any additional complexity from doing this either (assuming we do not expose the ability to control settlement contents later but just have it as an option) so that's my preference.

matin commented 9 years ago

@mjallday should you be able to cancel a settlement?

matin commented 9 years ago

@remear please don't use the word "sweep" anywhere. I've seen it pop up in a couple places (though not currently in the specs). It should just be "account". If you need to define a type, make it customer and marketplace based on the owner of the account.

remear commented 9 years ago

@matin After thinking about this for the last few weeks, I disagree. Since a Customer has a collection of Accounts and it's very likely in the future they won't all be the same kind of account, Accounts naturally need a type. I'd rather just set it up now than have to redo things later to accommodate account types.

customer and marketplace types are too vague. A Customer has an account but its type is customer? I sense a disturbance in support. Discussed options were deposit and sweep. The preference seemed to be sweep. Unfortunately there are a finite number of words in the English language so some words and terms are going to have to be reused. People will have to learn the terminology we use just like they have to learn the terminology everyone else uses. Let's just pick something that clearly conveys what the account represents and/or does.

Happy to continue this discussion over at https://github.com/balanced/balanced-api-private/issues/15

remear commented 9 years ago

@mjallday Comments addressed

mjallday commented 9 years ago

credit.json needs to accept a funding instrument with prefix AT - @rserna2010

mjallday commented 9 years ago

needs the following on the marketplaces.

diff --git a/fixtures/marketplaces.json b/fixtures/marketplaces.json
index 79cc9cf..8ced423 100644
--- a/fixtures/marketplaces.json
+++ b/fixtures/marketplaces.json
@@ -70,6 +70,16 @@
                     "type": "string",
                     "format": "uri",
                     "pattern": "/disputes"
+                },
+                "marketplaces.settlements": {
+                    "type": "string",
+                    "format": "uri",
+                    "pattern": "/settlements"
+                },
+                "marketplaces.accounts": {
+                    "type": "string",
+                    "format": "uri",
+                    "pattern": "/accounts"
                 }
             },
             "required": [
@@ -84,6 +94,8 @@
                 "marketplaces.callbacks",
                 "marketplaces.card_holds",
                 "marketplaces.orders",
+                "marketplaces.settlements",
+                "marketplaces.accounts",
                 "marketplaces.disputes"
             ]
         },