Sailias / bitcoin_payable

A rails bitcoin payment processing gem
MIT License
121 stars 31 forks source link

Testnet not supported + other bugs #23

Closed mhluska closed 6 years ago

mhluska commented 6 years ago

When I run rake bitcoin_payable:process_payments on localhost/testnet I get Testnet not supported. How can I fully test this code on my own machine without real money if testnet is not fully supported?

Edit: From looking at the code, it looks like I have to do BitcoinPayable.config.adapter = 'blockcypher' to get testnet support. This should be added to the readme.

Edit2: It looks like the blockcypher adapter crashes for unconfirmed transactions because transaction["confirmed"] is nil:

$ rake bitcoin_payable:process_payments
rake aborted!
ArgumentError: invalid date
/Users/mhluska/.rvm/gems/ruby-2.4.2/bundler/gems/bitcoin_payable-22696803b40d/lib/bitcoin_payable/adapters/blockcypher_adapter.rb:24:in `iso8601'

A side question I have: how often should my app be running rake bitcoin_payable:process_prices and rake bitcoin_payable:process_payments? Seems like once every 30 mins for the former is reasonable and once every 1 minute for the latter?


Another question I have is: is there any way to associate payments with a user without modifying the migration files + monkey patching? Currently I'm doing this:

module BitcoinPayable
  class BitcoinPayment
    belongs_to :user
  end
end

class User < ApplicationRecord
  has_many :bitcoin_payments, class_name: 'BitcoinPayable::BitcoinPayment'
end

Another question I have is: why does this gem not consider confirmations at all? Isn't that important for a payment processor?

Sailias commented 6 years ago

@mhluska the number of times you run the payment processor will be limited by the API throttling you. Right now the approach is very naive, check all outstanding payments, even those created 2 years ago without any activity.

In order to reduce the number of API calls, this approach can be optimized in various ways including allowing it custom via a custom AR scope or something.

I can image the requests are properly rate limited and sent constantly according to this rate limit as opposed to all at once, as fast as possible every 10 mins.

At the moment there isn't a user association as a payment can be attached to a polymorphic parent. I've always thought that a user wouldn't have a payment, but would pay for something attached to a user.

For example:

Those connecting models would be associated to the user.

mhluska commented 6 years ago

At the moment there isn't a user association as a payment can be attached to a polymorphic parent. I've always thought that a user wouldn't have a payment, but would pay for something attached to a user.

That makes sense. The issue I found with this approach was that it's difficult to track who the sender is and who the receiver is. In my app, one user pays for a video to buy/obtain it from another user so I need to know who the sender of the payment is in order to attach it to his data record after.

Sailias commented 6 years ago

@mhluska I understand your use case. This could be solved with an order or sale table that links seller, buyer and video.

It could be solved by assigning the payment to a user as you suggested, but imagine a payment splitting app. Now we need to split a payment to multiple users. This solution doesn't work anymore.

I could however make a sale with a total amount, then create sale_payment_splits and assign a user to each of the sale_payment_splits and create a new payment for each sale_payment_split

While I don't see any harm in adding a user reference to the table, I don't think it would work in every scenario and it might be more consistent to leave the implementation to the parent application.

mhluska commented 6 years ago

I see. That makes sense to me. I'll have to remove the use of user_id from my active PR then.