balanced / balanced-api

Balanced API specification.
220 stars 72 forks source link

Forex #564

Closed steveklabnik closed 8 years ago

steveklabnik commented 10 years ago

This pull request will eventually fulfill the "Forex" milestone: https://github.com/balanced/balanced-api/issues?milestone=9&state=open

This is the very first step: debiting a card in €.

There are two things left to do before this is 'ready':

steveklabnik commented 10 years ago

Green! :shipit: . ha!

... I guess this means we aren't actually validating the currency field server side. :sweat_smile:

matthewfl commented 10 years ago

validate? fields? what? madness

did you assert that the field comes back with the same currency that you set?

steveklabnik commented 10 years ago

@mjallday point taken. we don't test the invoicing stuff here at all, right?

mahmoudimus commented 10 years ago

@steveklabnik @mjallday completely agree. Fees should in my opinion be a separate concern of the API.

steveklabnik commented 10 years ago

Reverted that commit.

steveklabnik commented 10 years ago

Bam. 157 currencies.

steveklabnik commented 10 years ago

There needs to be information on the response for the Debit that specifies by how much the escrow increased (in the escrow's or order escrow's currency). This is required for a marketplace to be able to audit the increase in their escrow balance.

steveklabnik commented 10 years ago

(above comment by @matin )

mjallday commented 10 years ago

Would a currency pair quote solve that? e.g. EUR/USD=1

If there was a forex quote resource you could query that ahead of time to get the exchange rate, submit to with the transaction (before the quote expires) to guarantee the transaction gets that rate (omitting the quote would give you a rate of whatever is current), and then link to the quote on the transaction to provide the auditing you are asking for.

mahmoudimus commented 10 years ago

@mjallday seeing what the converted number is and a convenient ratio on the debit's response is extremely useful.

@steveklabnik and @matin are correct, they'd like to audit their escrow balance, but I personally think no customer will run analytics by querying all the debits from our API. I think they'll request for a bulk export and or a way to view their invoices in a deep dive to see what their starting balance at the start of the day and all the invoice events generated for that day of business.

mjallday commented 10 years ago

no customer will run analytics by querying all the debits

they do build state machines that listen to webhooks and do their own reconciliation from that. (based on personal experience in irc helping implement such)

mahmoudimus commented 10 years ago

Let's get some customers to plz testify to this. I think they'd rather fetch a different resource (like invoices) 2 do their computations from that rather than querying all debits. That seems like a fools errand 2 me.

mjallday commented 10 years ago

TESTIFY BROTHER! CAN I GET AN AMEN?

image

https://botbot.me/freenode/balanced/2014-03-24/?tz=America/Los_Angeles

Read the conversation between jondkinney and myself.

Invoices are hard to use for reconciliation. The issue that I've heard with using invoices is that they have to trust us to get the math correct. If they want to verify this themselves then they need to build a state machine and adjust the escrow as txns go through various states.

This is digressing from the forex conversation, either way I believe we need to provide them with the exchange rate that the transaction was assigned.

steveklabnik commented 10 years ago

I'm going to have a discussion with Cath and Tim today about some of this, as well.

steveklabnik commented 10 years ago

Right, so there's two things here: first, that having the information also be in the debit rather than in an invoice would be useful, and that will probably be the same for forex.

So the real question is naming, I guess. Sometimes, a debit goes to an order, and sometimes, it doesn't, so escrow_amount seems bad. Thoughts?

matin commented 10 years ago

@steveklabnik can you explain what you mean by "the information"?

I assume you're trying to figure out how best to return the amount (in the currency of the escrow).

In the case where Balanced only supports a Order/escrow in USD, you can have the field be amount_in_usd.

What's stopping you from using that field name to keep it simple for now?

steveklabnik commented 10 years ago

Yes, your assumption was correct.

I guess I'm wary of creating yet another USD-specific name baked in. What happens when we have multiple currency escrows?

matin commented 10 years ago

What happens when we have multiple currency escrows?

We'll have learned a lot more than we know now about customer behavior and what people want. At which time, we can add a new revision and solve the problem based on everything that we've learned.

If you do accept that amount_in_usd works if we assume usd only, then the next step would presumably be amount_in_order_currency + order_currency. Not sure "order" is the right word but something following that format.

mahmoudimus commented 10 years ago

@matin why not just return the currency back w/ every amount?

matin commented 10 years ago

Assume the marketplace escrow or order is denominated in USD. A debit should return something like:

{
    "amount": 10000,
    "currency": "EUR",
    "amount_in_order_currency": 13187,
    "order_currency": "USD"
}

There's two currencies: the currency of the buyer and the currency of the marketplace/order

steveklabnik commented 10 years ago

Seems good.

steveklabnik commented 10 years ago

I'm not happy with *_order_currency as a name, but generally, this is right. And once I get a final list of currencies, I will extract a specific file as @matthewfl suggests. But this should be good for actually implementing as of now.

mjallday commented 10 years ago

should _order_currency be _escrowed_currency? an order holds its funds in escrow. it also means that doing a GET on something that does not use an order can use the same fields so you do not need to parse something differently depending on the destination of funds.

that does not hold true for transfers however...

steveklabnik commented 10 years ago

Right, the problem with escrow is that with accounts & orders, it's not the right word...

matthewfl commented 10 years ago

how about having an object of the relevant currencies, something like

"amounts": {
   "usd": 12345,
   "btc": 9001
}

that seems somewhat awkward now that I have written it, but I think it might be nice to limit the number of top level fields on any resource

mjallday commented 10 years ago

i was going down the same path as you matthew, but agreed it feels odd.

@steveklabnik is this considered a backwards compatible change? i ask because tho the fields you're proposing are additional so far and there are no existing changes, you can no longer say that that escrow = sum(debits.amount) - sum(refunds.amount) - sum(credits.amount) + sum(reversals.amount).

matthewfl commented 10 years ago

my understanding is that this change is suppose to be compatible with rev1.1. That said, even if it was setup as not backwards compatible, I don't think people operating in a single currency should have to deal with the awkwardness of multi currency

steveklabnik commented 10 years ago

Orders already made that not true, right, @mjallday ?

steveklabnik commented 10 years ago

So, after discussing this with @mjallday , we should just make sure that this is clear to those who implement forex. It's still going to be backwards compatible, because until you do forex stuff, that's the same, and it's not a property we've ever explicitly guaranteed, but we should be careful about casually mentioning it.

steveklabnik commented 10 years ago

Updated this to use a currency reference so things don't get out of hand, as well as the full list of currencies, and renamed the two new fields to refer to 'captured' currency, since 'orders' was kinda confusing. Still not 100% happy with that name, but it's good enough.