Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Transaction.php to fix transaction amount error #29

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

git-seb
Copy link

@git-seb git-seb commented Jun 6, 2024

Fix: wrong amount was being sent to API causing the following error showing up: 'Something went wrong: Transaction amount must be greater than 0'

git-seb added 2 commits June 6, 2024 15:22
Fix: wrong amount was being sent to API causing the following error showing up: 'Something went wrong: Transaction amount must be greater than 0'
Cleaning up code a bit
@Crypto2
Copy link

Crypto2 commented Jun 6, 2024

Without conversion this looks like it could result in a mix of currencies. ie. if the user picked Japanese Yen but the store's internal currency is USD.

@git-seb
Copy link
Author

git-seb commented Jun 6, 2024

Without conversion this looks like it could result in a mix of currencies. ie. if the user picked Japanese Yen but the store's internal currency is USD.

Without this change, it sent the wrong amount.

See here log before change:
[amount] => 0.00101081
[currency1] => GBP
[currency2] => BTC
[buyer_email] => xxxx
[buyer_name] => Seb
[invoice] => ABSDEV_202400114
[ipn_url] => https://dev.xxx.co.uk/en/coinpayments/ipn/handle/

Log after change:
[amount] => 8.0000
[currency1] => GBP
[currency2] => BTC
[buyer_email] => xxxx
[buyer_name] => Seb
[invoice] => ABSDEV_202400123
[ipn_url] => https://dev.xxx.co.uk/en/coinpayments/ipn/handle/

@Crypto2
Copy link

Crypto2 commented Jun 6, 2024

I mainly just mean that it needs to be sure that these are both referring to the same currency, no matter what currency the user has selected:

'amount' => $order->getBaseGrandTotal(),
'currency1' => $this->getPaymentConfig('receive_currency'),

So they can't just pick JPY and get a massive discount or anything like that. So I think if you are using getBaseGrandTotal() then currency1 should probably be getBaseCurrencyCode() if these are the right docs https://www.magentoextensions.org/documentation/interface_magento_1_1_quote_1_1_api_1_1_data_1_1_totals_interface.html

@git-seb
Copy link
Author

git-seb commented Jun 7, 2024

I mainly just mean that it needs to be sure that these are both referring to the same currency, no matter what currency the user has selected:

'amount' => $order->getBaseGrandTotal(),
'currency1' => $this->getPaymentConfig('receive_currency'),

So they can't just pick JPY and get a massive discount or anything like that. So I think if you are using getBaseGrandTotal() then currency1 should probably be getBaseCurrencyCode() if these are the right docs https://www.magentoextensions.org/documentation/interface_magento_1_1_quote_1_1_api_1_1_data_1_1_totals_interface.html

I tested your suggestion but it thrown an error (Transaction is not found). Actually when that error happened, the error function actually contains an error as well (error is undefined). I've fixed that as well which I will add later as well. (The error function is spelled as 'err', renamed it to 'error'.)

So I reverted to before your suggestion to the fix I provided in this pull request, changed the webshop to EUR and tested placing an order. The EUR amount in cart was €9,39. After placing the order I see in CoinPaymentDebug.log currency1 is still GBP instead of EUR so that is perfect. It also took the GBP amount of 8,00 so it did not resulted in mixed currencies.

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.

2 participants