Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Bitcoin payin #3317

Closed
wants to merge 31 commits into from
Closed

Bitcoin payin #3317

wants to merge 31 commits into from

Conversation

rohitpaulk
Copy link
Contributor

#1963

  • UI Work
    • Add bitcoin to settings page
    • Create bitcoin-payin page
    • Create coinbase button on request, embed iframe
    • Handle messages from iframe on success/failure
    • Fix how bitcoin payins are shown on the 'history' page
    • Modify receipts to show bitcoin payins too
  • Handle coinbase callback
    • Update DB, balance
    • Handle mispayments - overpaid, underpaid. Skip for now, handle via support.
    • Send emails
    • Check for a secret_key
  • Handle fees properly
  • Handle both sandbox and non-sandbox mode
  • Think about security
    • Do we need identity verification to allow a bitcoin payin?

@rohitpaulk rohitpaulk self-assigned this Apr 5, 2015
@rohitpaulk
Copy link
Contributor Author

A mispayment is an order payment that was received under abnormal circumstances. Two triggers will cause a mispayment to occur:

The customer’s payment was received after the 10-minute order timeout expired.
The customer’s payment was greater than or less than the actual order amount.

Mispayments are kinda tricky. We have three options -

# 1: accept all mispayments and add them to the user's balance

We do receive a callback everytime a mispayment is made.

screenshot from 2015-04-06 00 24 44

Coinbase's UI will always show something like ^^ though, so it might be weird if we just silently ignored the problem and credited the user's account.

Maybe send an email with details?

"We received an amount different that you intended to pay, we've added it to your balance. Email support if you'd like a refund".

Under the hood, this will make stuff a bit complicated though. We link every exchange to a route that contains the order ID as the address. All mispayments belong to the same order (route, in our case), so when we do create new exchanges for each mispayment, where would we store the mispayment_id? In the exchange's note column?

# 2: refund all mispayments as soon as we receive them

We'd have to collect a refund address from the user for this. This isn't required for someone with a coinbase account, but that's unlikely - people who have a coinbase account would pay using coinbase, which would never cause a mispayment to happen in the first place. Should we ask the user to add a bitcoin address to his profile before he can make a bitcoin payin?

# 3: Don't do anything, handle through support.

Even though these are edge cases, that'd be pretty bad UX that's not the best UX.

EDIT: I've proceeded with a simple error message that says "contact support" for now. The coinbase iframe also shows a similar message.

@rohitpaulk rohitpaulk changed the title Bitcoin payin [WIP] Bitcoin payin Apr 10, 2015
@rohitpaulk
Copy link
Contributor Author

A few screenshots:
screenshot from 2015-04-10 14 47 53

screenshot from 2015-04-10 14 47 22

screenshot from 2015-04-10 14 48 55

screenshot from 2015-04-10 14 34 00

@rohitpaulk
Copy link
Contributor Author

Notes on deployment:

  • Generate a random string and add it as COINBASE_SECRET_KEY in heroku config
  • In Coinbase, set the default callback URL to https://gratipay.com/callbacks/coinbase?secret_key=the_secret_key_generated_above
  • Set COINBASE_SANDBOX_MODE to false in heroku config
  • Fill our merchant profile on coinbase, add logos etc.
  • Fill all redirect URLs in merchant settings page to /about/me or something similar.

@rohitpaulk rohitpaulk removed their assignment Apr 10, 2015

status = order['status']

# We're ignoring mispayments/expired orders here.
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem very wise to completely ignore errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expired orders don't matter to us at all, but we could do more with mispayments.

We do show client side error messages like shown in the last screenshot in #3317 (comment). Does sending an email sound good to you? The other options are listed in #3317 (comment), I think they're unnecessary and too much of work for now.

@Changaco
Copy link
Contributor

I don't like the changes you've made to exchanges.py and test_billing_exchanges.py. record_exchange should not do things differently depending on the route, it's only supposed to consider the status and the amount.

@Changaco Changaco removed the Review label Apr 13, 2015
( SELECT r.*::exchange_routes
FROM exchange_routes r
WHERE r.id = e.route
) AS route
Copy link
Contributor

Choose a reason for hiding this comment

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

Not very important, but the indentation is inconsistent here.

@rohitpaulk
Copy link
Contributor Author

I don't like the changes you've made to exchanges.py and test_billing_exchanges.py. record_exchange should not do things differently depending on the route, it's only supposed to consider the status and the amount.

What if we want to record an exchange that we know is successful? Would we have to call record_exchange first, and then call record_exchange_result?

And record_exchange was already doing different things depending on the route, it was updating the balance for bank transfers but not for credit card charges. Just that the check was for amount < 0 and not the route.

This prevents mispayments (after an order is complete) from modifying
one's balance.
@rohitpaulk
Copy link
Contributor Author

@Changaco - Also, this change will allow us to reuse record_exchange in record-an-exchange.spt once #3295 is done.

@techtonik
Copy link
Contributor

Is there a diagram or an algorithm about what's going on (mispayment etc.)?

@rohitpaulk
Copy link
Contributor Author

@techtonik - No, there isn't. I can explain a bit though.

When a user clicks 'pay' on the routes/bitcoin-payin.html page, we send coinbase a request to create a button, and we use the response to insert an iframe into the page. Now, three things can happen.

  1. The user successfully completes his order.
  2. The order expires (10 min window) and the user doesn't pay anything.
  3. The user sends a wrong amount, or sends an amount after the 10 minute window. (Mispayment).

For each of these cases, we receive a callback at www/callbacks/coinbase.spt. The callback code only hits our db if the order is successful. It creates an exchange, updates the balance of the user, and sends them an email too.

Case 2 (expiry) isn't a problem for us, the user probably didn't intend to pay at all.

Case 3 is tricky, as mentioned in #3317 (comment). We just show the user an error message (coinbase does the same in the iframe) that asks them to contact support to retrieve the mispaid bitcoin. Refunding the mispaid bitoin is something that one of us (only @whit537 for now, see #2178) can do through Coinbase's UI.

screenshot from 2015-04-10 14 34 00

@Changaco
Copy link
Contributor

What if we want to record an exchange that we know is successful? Would we have to call record_exchange first, and then call record_exchange_result?

Either that or modify record_exchange properly.

And record_exchange was already doing different things depending on the route, it was updating the balance for bank transfers but not for credit card charges.

No, that's not what it's doing. Although it's not implemented it is possible to do payins via balanced-ba and payouts via balanced-cc. Your changes are breaking record_exchange's handling of these potential cases. The route is irrelevant, what matters is the exchange's amount (i.e. whether it's a payin or payout), and its status.

@rohitpaulk
Copy link
Contributor Author

@Changaco - efef589, b6e13e2


# If not, create one.
if not isinstance(route, ExchangeRoute):
route = ExchangeRoute.insert(participant, 'coinbase-payin', order['id'])
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a race condition. You should try the insert first, and do the from_address if you get an IntegrityError.

@rohitpaulk
Copy link
Contributor Author

We're dropping bitcoin, closing this.

@rohitpaulk rohitpaulk closed this May 19, 2015
@chadwhitacre chadwhitacre deleted the bitcoin-payin branch July 13, 2015 12:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants