blockonomics / whmcs-bitcoin-plugin

Accept bitcoins on your WHMCS, payments go directly into your wallet
8 stars 12 forks source link

Fix convert to for processing #103

Closed blockonomics closed 3 years ago

blockonomics commented 3 years ago

This PR will address #80 More discussion can be found on https://github.com/blockonomics/whmcs-bitcoin-plugin/pull/93

blockonomics commented 3 years ago

Looks like my comments were confusing. I wasn't suggesting that we ourselves roundup. I was asking how WHMCS was rounding and giving us params[amount] on this line. We shouldn't be doing any rounding and should use percentage to calculate the correct amount.

Here is an example: Convert To For Processing: USD Base currency: USD Client currency: VES RATE: 800000 Invoice value: 99200 VES

USD value on payment page : 0.12 USD (should actually be 0.124USD but WHMCS rounds it) BTC value on payment page: 0.000006561 BTC

Now when we receive callback, do the following:

This percentage method should work for both convert to for processing and normal case. In convert to for processing we should be careful to credit the client currency invoice value

agarzon commented 3 years ago

Since we need to increase the precision (that's was the culprit the problems with credits and underpayments), and there is not possible way to make a dynamic change in the field value because library dba is not installed and neither part of WHMCS and also because the usual is to name extra tables in WHMCS with the prefix mod_ I propose to create a new table which will already contain the needed changes, including order_currency, so no changes in the table, just have a new one... (notice this happened before, when the table used to be called: blockonomics_bitcoin_orders, this is open to discussion.

WHMCS uses decimal(10,5) for conversions. Consult resources/sql/tblcurrencies.schema.sql

getGatewayVariables was constantely repetead in the code, this way we avoid repetitions. In fact, this can be even better removing non-dynamic getters, this change will come in a different PR.

DarrenWestwood commented 3 years ago

Hi @agarzon, currently changing the precision of the calculation fixes the amount during checkout. However, WHMCS still saves the amount of the invoice in tblinvoices to 2 decimals. This means we are still required to round the amount to 2 decimal places during callbacks for the invoice to be correctly marked as paid, since adding credits to the invoice happens during addInvoicePayment https://github.com/blockonomics/whmcs-bitcoin-plugin/blob/master/modules/gateways/callback/blockonomics.php#L149.

Credit same percentage of client currency value. So if 100%BTC value is paid, we credit 100% client currency which is 99200VES

Instead of trying to adjust the value WHMCS gives during the checkout, we can instead compare the expected BTC amount and the received BTC amount, during callbacks as a percentage. With this percentage, we should be able to credit the correct amount to the invoice In the above example, If the user pays 0.000006561 BTC (0.12 USD), they should be credited the total for the invoice (99200 VES) as they paid 100% of the BTC total.

blockonomics commented 3 years ago

The PR looks very good. However, I found an issue. Additional Partial payments to invoices already partially paid are not registering. In think this maybe due to the total we are getting from tblinvoices. What happens to total in case of partial payments ?

blockonomics commented 3 years ago

With partial payments it looks like the total amount is taken from tblinvoice.total_amount instead of the remaining balance amount which is leading to wrong calculations

Here is the steps to reproduce

  1. Partially paid invoice - Looking good Screenshot_2020-11-27 Company Name - Invoice #10
  2. Attempt to pay - Looking good Screenshot_2020-11-27 Bitcoin Payment - Company Name
  3. Now I paid 0.0008BTC which is around half the expected BTC. It should credit around 992INR. However, it has actually credited 1994 INR