btcpayserver / btcpayserver-php-client

PHP implementation for the BTCPayServer cryptographically secure RESTful API
MIT License
29 stars 33 forks source link

Allow more currencies to be used for products/store pricing #45

Closed ndeet closed 3 years ago

ndeet commented 3 years ago

Use case: I want to be able to set the Item (Shop) denominated currency to anything that is supported by BTCPay Server. E.g. I want to price my products denominated in USDT, L-BTC, LTC, etc.

Specific use case covered This is needed for the upcoming support for Liquid tokens to be able to set the pricing to a specific token that is not one of the hardcoded ones.

Problem: In this old implementation the supported currencies are hardcoded in the Currency class (see here).

This means that only those predefined mostly Fiat and BTC can be set as the store/invoice currency.

Proposed solution To have maximum flexibility I think it makes sense to remove the hardcoded property alltogether and remove the setCode() method's check for allowed values. I think this was only done by Bitpay because they had a limited set of FIAT and crypto they supported but with BTCPay Server this is not the case anymore and everything can be supported.

@Kukks @dennisreimann what do you guys think about that? Wanted to discuss before doing the PR.

Kukks commented 3 years ago

Use case: I want to be able to set the Item (Shop) denominated currency to anything that is supported by BTCPay Server. E.g. I want to price my products denominated in USDT, L-BTC, LTC, etc.

Specific use case covered This is needed for the upcoming support for Liquid tokens to be able to set the pricing to a specific token that is not one of the hardcoded ones.

Problem: In this old implementation the supported currencies are hardcoded in the Currency class (see here).

This means that only those predefined mostly Fiat and BTC can be set as the store/invoice currency.

Proposed solution To have maximum flexibility I think it makes sense to remove the hardcoded property alltogether and remove the setCode() method's check for allowed values. I think this was only done by Bitpay because they had a limited set of FIAT and crypto they supported but with BTCPay Server this is not the case anymore and everything can be supported.

@Kukks @dennisreimann what do you guys think about that? Wanted to discuss before doing the PR.

ACK from my end. If possible, try to do it as an overload so that we don't do a breaking change for existing users

ndeet commented 3 years ago

ACK from my end. If possible, try to do it as an overload so that we don't do a breaking change for existing users

Ok, so to be sure to be on the same page: I should create a BTCPayCurrency class that extends Currency and overload the setCode() method so we can then instantiate new BTCPayCurrency() instead of new Currency() right?

Kukks commented 3 years ago

ACK from my end. If possible, try to do it as an overload so that we don't do a breaking change for existing users

Ok, so to be sure to be on the same page: I should create a BTCPayCurrency class that extends Currency and overload the setCode() method so we can then instantiate new BTCPayCurrency() instead of new Currency() right?

You can even change the code in Currency to make it less restrictive if you want. As long as those that update can have the code still working instead of a runtime error ;)

ndeet commented 3 years ago

There should be no runtime error when the check for allowed currency symbols is removed. :)