search

PayButton / paybutton

Accept eCash by adding a donation or buy button to your website.
https://paybutton.org
MIT License
38 stars 19 forks source link

Fix/supported currencies #406

Closed lissavxo closed 6 months ago

lissavxo commented 6 months ago

Related to #405

Description

Test plan

Run project with yarn build, make sure the systems validates correctly the currency

lissavxo commented 6 months ago

I am wondering if we need the disctinction. isValidCurrency, that will use the restricted list, is only called at one place, in the whole code base — on WidgetContainer.

isFiat, that checks if it belongs to the bigger list, is called many more times.

In the end, this change only affects that one place in WidgetContainer where isValidCurrency is called. I am not sure if that's the desired behavior, but as far as I can tell we could just change the fiatCurrencies list to include only CAD and USD. Is there any aspect in which we would want to support e.g. AUD, but not "fully support it"? If no, I suggest we just delete the new list (supportedFiatCurrencies) and modify the old one (fiatCurrencies) instead.

In this case, we could add another function, isSupportedFiat, that checks if the fiat currency is supported. I believe having both isFiat and isSupportedFiat, and also fiatCurrencies and supportedFiatCurrencies, will help us provide more information to guide the user through error handling. If the user types a fiat currency that exists but is not supported, we can be more precise in our response, saying that we do not support this fiat currency or that this fiat currency is not valid.

But I get that in the codebase we use isFiat as a check for supported fiat currencies, so I am fine with doing what you suggested.

chedieck commented 6 months ago

In this case, we could add another function, isSupportedFiat, that checks if the fiat currency is supported. I believe having both isFiat and isSupportedFiat, and also fiatCurrencies and supportedFiatCurrencies, will help us provide more information to guide the user through error handling. If the user types a fiat currency that exists but is not supported, we can be more precise in our response, saying that we do not support this fiat currency or that this fiat currency is not valid.

Yeah, but then this will require manually going through each one of those isFiat calls, understanding what is happening and checking if it should remain isFiat or if it should be changed to isSupportedFiat, which I guess would not be worth the effort of doing it, considering the scope we're now on. Also, that would be something that would be best done when the functions that make those calls are covered by the tests.