Skip to content

Instant payments, confirmations threshold, webworks and Bitcoin Cash support. Version 0.8.0#30

Closed
maesitos wants to merge 22 commits intoSailias:masterfrom
maesitos:release/0.8.0/rails5
Closed

Instant payments, confirmations threshold, webworks and Bitcoin Cash support. Version 0.8.0#30
maesitos wants to merge 22 commits intoSailias:masterfrom
maesitos:release/0.8.0/rails5

Conversation

@maesitos
Copy link
Contributor

@maesitos maesitos commented Feb 28, 2018

I've been working in several features for this gem. I'm going to explain it a bit here but I've updated the README so everybody can understand how to use it.

Support for multi currencies

A simple way to create Bitcoin payments based on different currencies. If no currency is specified, the default will be used. You can do:
payable.bitcoin_payments.create!(reason: 'sale', price: amount_in_cents, currency: :usd)
or
payable.bitcoin_payments.create!(reason: 'sale', price: amount_in_cents, currency: :cad)

This method will now create the conversion before creating the payment if the conversion doesn't exist in the database.

New rake task

rake bitcoin_payable:update_rates_for_all_pairs Will update the rates of all pairs available in the payments and in the previous rates history.

Confirmations

A payment won't be considered paid if a confirmation doesn't reach the desired confirmations in BitcoinPayable.config.confirmations.

rake bitcoin_payable:process_payments will be monitoring the confirmations for a given transaction and set the payment as paid when reached. Otherwise if the confirmations doesn't increase with the chain, the transaction will be 'set aside' (actually the confirmation value will be set to -1).

Zero confirmations transactions

In order to improve the user experience the gem will allow you to accept transactions with zero confirmations. So if a transaction enters the mempool it'll set your 'payable' as paid. Simply done by setting BitcoinPayable.config.zero_tx = true.

rake bitcoin_payable:process_payments will monitor the confirmations and if a transaction doesn't reach BitcoinPayable.config.confirmations the BitcoinPayment will be set as :pending and the payable will be notified so he can do his stuff to roll back the payment.

In order to implement the roll back notification (all covered in the README)

def Product < ActiveRecord::Base
  def bitcoin_payment_rollback
    self.cancel_payment!
  end
end

Webhooks

Enable them by setting BitcoinPayable.config.allowwebhooks = true. The gem will set to routes:

bitcoin/lastblock

Will be called every block and will trigger rake bitcoin_payable:process_payments to see if the pending payments have been paid or to monitor the transaction that are still not secure.

bitcoin/notifytransaction

This route will receive and store transactions that have zero confirmations, that is, that are still in the mempool. This route only will be enabled if BitcoinPayable.config.zero_tx is set to true. This will give the user the experience of sending an instant payment.

Bitcoin Cash

It's possible now to use the Bitcoin Cash network by settingBitcoinPayableconfig.crypto = :bch, set BitcoinPayableconfig.crypto = :btc for Bitcoin.

Miscellaneous.

There are some small changes I did to clean the code. Since this version has changed a lot and it's based in a new API I commented the rest of the adapters, I didn't see the point in implementing them.

Updating from the vesting 0.7 should be easy as the interface hasn't changed. You'd only need to set the environment variables for the API and set the new configuration in your Rails API.

I hope you like it! I'm looking forward for the merge so that I can implement it in my store.

PS: I'll also push a port for rails 3.2 (my store still uses 3.2)

@maesitos maesitos force-pushed the release/0.8.0/rails5 branch from 6e28b9b to 666a134 Compare March 2, 2018 16:27
@maesitos maesitos force-pushed the release/0.8.0/rails5 branch from 666a134 to 670877b Compare March 2, 2018 16:34
@Sailias
Copy link
Owner

Sailias commented Mar 2, 2018

@maesitos thanks for the pull request.

There is a lot to comment on, as this pull request takes bitcoin_payable in a new direction.

Webhooks

My understanding is that you don't want to run the processor on an interval, but leverage Blocktrail's subscription API to receive only the events that we are monitoring. I looked through their documentation and didn't see any limits on the number of subscriptions you are allowed to create. While this seems like an instant win, it makes me think they will impose limitations and this solution won't scale.

Perhaps we should reach out to Blocktrail to get more clarity on this before we make this the default implementation.

I have been thinking about limiting API requests for a while and just never got around to it, so I think this would work quite nicely.

Bitcoin Cash

As you are aware there is another version of the gem https://github.com/Sailias/cryptocoin_payable that supports other cryptos. I am not opposed to only integrating BCH into this gem as it can leverage the same blockchain APIs, but I would draw the line close to that. Support of any other cryptocurrencies should be part of that other gem.

Accept multiple fiats at runtime

I like this functionality. Charging of taxes isn't built into this repo and should be charged prior to issuing a payment. I will have to look through this to make sure all the conversions are being pulled properly.

Instant payments

Confirmations were added fairly recently and instant payments were the default. I will say that I am unsure about your implementation regarding this feature. Accepting a payment, marking it as paid_in_full and then reverting it after a period of time seems weird to me.

If the number of confirmations is set to 0, it will immediately be "confirmed". The possible states are:

  • paid_in_full (not confirmed)
  • partially_paid (does not take confirmations into account)
  • confirmed

I think we just need to implement a better notification system between the gem and the parent app. If someone sees "paid_in_full" and wants to take an action on it, they can. If they want to ship the order only when it's "confirmed" they can.

In https://github.com/Sailias/cryptocoin_payable, @mhluska has a pull request that implements better notifications. Sailias/cryptocoin_payable#6

Here is an excerpt:

def notify_payable_event(event_name)
  method_name = :"coin_payment_#{event_name}"
  if self.payable.respond_to?(method_name)
    self.payable.send(method_name, self)
  end

  if self.payable.respond_to?(:coin_payment_event)
    self.payable.coin_payment_event(self, event_name)
  end
end

I will have more feedback soon.

@maesitos
Copy link
Contributor Author

maesitos commented Mar 4, 2018

Hello @Sailias

Web-hooks

Yes if it's going to be the default and only alternative for now it has to be a very reliable and 'unlimited' API. From their docs:

Rate Limits

Free developer accounts are limited to 432,000 API requests per 24 hours, at a rate of 300 request per minute.

When you reach the rate limit you will get an error response with the 429 status code.
We will send you a notification when you're getting close to the rate limit, so you can upgrade in time or contact us to request an extension.

And that's previous from the acquisition by Bitmain. They made in 2017 a whopping revenue of 4 billion USD so the API is backed by a solvent business. I did request a pull to the blocktrail dependency so we could get an exception in case the limit is reached aside from the email they say the'd send you yunixon/blocktrail#4

Bitcoin Cash

I'm aware of the other library and since my new proposal creates a more coupled code—at least with the API—I preferred to work on top of this this gem. More features in exchange for a more coupled code code I guess.

I think that more than about to crossing the border line, adding Bitcoin Cash is the only chance for this gem to have a use case in the mid term as Bitcoin have shifted towards transacting off chain witch is a complete new thing.

Instant payments

At least there isn't anything related to confirmations in the master branch.

I did improve the concept following your suggestion. Quoting from the new version of the Readme:

Notify your application when a payment is made

Use the bitcoin_payment_paid method

def Product < ActiveRecord::Base
  has_bitcoin_payments

  def create_payment(amount_in_cents)
    self.bitcoin_payments.create!(reason: 'sale', price: amount_in_cents)
  end

  def bitcoin_payment_paid
    self.alert_owner
  end

  def bitcoin_payment_paid_and_comfirmed
    self.ship!
  end
end

bitcoin_payment_paid

This method will be called whenever a payment is received without taking into consideration the number of confirmations from the transactions for this payment. If config.allowwebhooks is set to true, this method will be called almost instantly as transactions with zero confirmations will also set the payment as paid. If you need to absolutely know the payment is secure, use bitcoin_payment_paid_and_comfirmed. At the moment of calling this method, the payment will have the state paid_in_full.

bitcoin_payment_paid_and_comfirmed

This method will me called then all the transactions associated with a payment have reached the confirmations set in config.confirmations. At the moment of calling this method, the payment will have the state confirmed.

More generic notifications

    def method_missing(m, *args)
      method = m.to_s
      if method.start_with?('notify_payable_')
        attribute = method[15..-1]
        if payable.respond_to?("bitcoin_payment_#{attribute}")
          payable.send("bitcoin_payment_#{attribute}")
        end

      elsif method.end_with?('_tx_notifications')
        adapter = BitcoinPayable::Adapters::Base.fetch_adapter
        adapter.send(method, address)
      else
        super
      end
    end

Gym time. I'll comment a bit more on my changes a bit later

@mhluska
Copy link

mhluska commented Mar 4, 2018

Looks like there's a lot of great work here. Would it make sense to split it up into several PRs? E.g. one for web hooks, one for Bitcoin Cash etc.

I'm unsure of the decision to add Bitcoin Cash to this gem instead of cryptocoin_payable. I was planning to add BCH and LTC support to that gem in the near future. I suppose it's fine if we think of both gems as completely separate projects.

@Sailias
Copy link
Owner

Sailias commented Mar 5, 2018

I agree with @mhluska on both points. One thing about the webhook implementation is that all other adapters have been removed in your code. I find that as people contribute they have a preference as to which adapter they feel should be default. This is the reason I created adapters in the first place, so people could roll their own. I don't think that adapters should be deleted though as they will break other people's projects.

I think it would be best of this pull request was split into at least 3 pull requests.

  1. Web hooks with all adapters added back in
  2. BCH support
  3. Multiple Fiat support

I also agree as previously stated that I'm not against adding BCH here and in cryptocoin_payable if we treat them as separate projects (which we are).

@maesitos how do you feel about splitting this large PR into smaller ones that can be tested easier?

Edit: Also let me know if you would prefer a different division of the PR content.

@maesitos
Copy link
Contributor Author

maesitos commented Mar 5, 2018

I've been all evening dividing it into three PR, it's going to take me a while as this request has many house keeping and a lot of the logic is very interlinked. I'll post 3 PR chronologically so the next PR will include the changes of the previous, it's not going to be possible to do all the 3 PR from the master. Is that OK?

You guys are lucky that I'm a Bitcoin millionaire with nothing to do... just kidding I sold my 450BTC extremely early :( 2013 even previous to the MtGox pump.

@Sailias
Copy link
Owner

Sailias commented Mar 5, 2018

@maesitos that was an expensive pizza. @maesitos I appreciate splitting these up, it will allow us to implement some of the functionality sooner and not have it be tied up by the other functionality.

Hopefully getting it into your project earlier.

@maesitos maesitos mentioned this pull request Mar 6, 2018
@maesitos maesitos closed this Mar 8, 2018
@maesitos maesitos deleted the release/0.8.0/rails5 branch June 12, 2018 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants