Change RubyMoney bank to currencylayer #10520

danieljames-dj · 2025-01-03T13:43:11Z

The bank is changed so that all our currencies will be supported.

lib/dues_calculator.rb

gregorbg · 2025-01-04T10:33:51Z

config/initializers/ruby_money.rb

+mclb.currencylayer = true
+mclb.ttl_in_seconds = 86_400
+mclb.cache = 'rails_money_cache.txt' # To be replaced with proper cache path.
+mclb.update_rates


This would mean rates are only updated once per server boot. We sometimes go for days without redeploying, which in some cases may mean exchange rates are up to ~10% out of sync.

(See also my other comment about more dynamic updating below)

I couldn't find a proper documentation on these part, but my best guess is that if ttl_in_seconds is set, whenever we ask the library to do an exchange, if the previous exchange was fetched before x seconds, it will invalidate the cache and fetch the newer rates. I feel so because if this is not how it works, it doesn't make sense to have ttl_in_seconds. What do you think?

I just verified from the library's source code that my guess is correct.

gregorbg · 2025-01-04T10:34:05Z

config/initializers/ruby_money.rb

+mclb.access_key = ENV.fetch("CURRENCY_LAYER_API_KEY", "")
+mclb.currencylayer = true
+mclb.ttl_in_seconds = 86_400
+mclb.cache = 'rails_money_cache.txt' # To be replaced with proper cache path.


Does the gem only support file caching? Would love to use Redis for this instead

Okay I didn't knew we were having redis. I couldn't get to see anywhere officially how to use it.

I tried mclb.cache = :redis_cache_store, { url: EnvConfig.CACHE_REDIS_URL } and I am not sure how to test if it worked.

gregorbg · 2025-01-04T10:34:14Z

config/initializers/ruby_money.rb

+mclb = Money::Bank::CurrencylayerBank.new
+mclb.access_key = ENV.fetch("CURRENCY_LAYER_API_KEY", "")
+mclb.currencylayer = true
+mclb.ttl_in_seconds = 86_400


Where did this value come from?

This is number of seconds in 1 day, I just copy-pasted from the documentation. I thought 1 day will be a good ttl for this case.

gregorbg · 2025-01-04T10:35:33Z

config/initializers/ruby_money.rb

-Money.default_bank = eu_bank
+mclb = Money::Bank::CurrencylayerBank.new
+mclb.access_key = ENV.fetch("CURRENCY_LAYER_API_KEY", "")
+mclb.currencylayer = true


What does this flag do and why is it needed? From my perspective as an outside reviewer who hasn't done any research on CurrencyLayer, this line of code reads like "Oh dear CurrencyLayer gem, which gives me a class called CurrencylayerBank, please really really access CurrencyLayer by the way!".

It's almost correct. Here's the link to documentation: LINK.

There are two endpoints - new (https://apilayer.com/marketplace/currency_data-api) and old (https://currencylayer.com/product). I created the access key in old, and to use old, I need to make it true. If I don't set this, it will point to new by default.

I'm fine with switching to new if needed, I don't have any personal preference. The only reason why I chose old is that it directly talks with currencylayer, while the new will first go to apilayer and then it goes to currencylayer and hence increases the latency a bit.

gregorbg · 2025-01-04T10:37:36Z

config/initializers/ruby_money.rb

-eu_bank = EuCentralBank.new
-Money.default_bank = eu_bank
+mclb = Money::Bank::CurrencylayerBank.new
+mclb.access_key = ENV.fetch("CURRENCY_LAYER_API_KEY", "")


This should be AppSecrets.CURRENCYLAYER_API_KEY instead. We have a top-level file called app_secrets.rb where you can see how we configure secrets access.

In dev and test, the values for these secrets are loaded from environment variables, which means placing it in .env.development and .env.test is correct. For production (and staging), you tell me the real access key through a (semi-)secure channel, and I insert it into a Vault runtime that feeds our production servers

Got it, thanks. I've changed it.

gregorbg · 2025-01-04T10:38:07Z

.env.test

@@ -35,3 +35,4 @@ STAGING_OAUTH_SECRET=example-secret
 AVATARS_PUBLIC_STORAGE=local
 AVATARS_PRIVATE_STORAGE=local_private
 DUMP_HOST=https://assets.worldcubeassociation.org
+CURRENCY_LAYER_API_KEY=9d254ec37e1591eec964dac947fafa09


Are these keys (here in test and same in dev) random dummy values or are they actual access keys?

They are actual access keys from my test account. I understand that exposing actual access keys is risky, but I went ahead since my test account is a free account and I don't mind even if the account gets deactivated.

The reason why I added actual access key is that if I give a dummy access key, it will throw error in CI/CD.

I tried if there is any way to fix this with a dummy access key, but couldn't find any way. Can you please let me know whether you faced this problem while initializing env variables for paypal or stripe or anything similar?

We actually use access keys from testing or dummy accounts for Stripe and PayPal. Look into .env.test, you will see that a lot of PAYPAL_* variables exist. These are from "real" accounts, which means we have actual username/password to log in to PayPal dashboard. But if you do actually log in, you will see that these accounts are marked as test accounts by PayPal, and you cannot send actual, real money with them. Only sandbox money. So that's why the accounts are not real, they are only "real".

Okay, I think currencylayer doesn't have a test account as it doesn't do any sensitive things. So will it make sense to create two currencylayer accounts for WCA - one for test and one for production?

Hm, now that I think about it, I have another idea...
What would you think about using the if Rails.env.test? check to use the EuCentralBank in local environments and switch on the CurrencyLayer in production only?

You can introduce a flag in EnvConfig (file env_config.rb) where developers can switch on CurrencyLayer manually if they want, for local testing and stuff

That is a great idea, I've now implemented it. :-)

gregorbg · 2025-01-05T08:31:25Z

config/initializers/ruby_money.rb

+  mclb.access_key = AppSecrets.CURRENCY_LAYER_API_KEY
+  mclb.currencylayer = true
+  mclb.ttl_in_seconds = 86_400
+  mclb.cache = proc.new do |payload|


Shouldn't this be Proc.new (note the capitalization)?

Actually rubocop asked to replace Proc.new with proc. After re-reading again, I realized it's proc and not proc.new. I cannot test it right now as my access key has expired. Once the budget is approved, I'll test again using a new test key created with WCA's account.

gregorbg · 2025-01-05T08:32:11Z

config/initializers/ruby_money.rb

-eu_bank = EuCentralBank.new
-Money.default_bank = eu_bank
+if Rails.env.test? || Rails.env.development?
+  eu_bank = EuCentralBank.new


You should also update_rates the same way you're doing for the new CurrencyLayer bank below

Oh yes, done.

gregorbg · 2025-01-05T08:32:58Z

config/initializers/ruby_money.rb

 Money.locale_backend = :i18n

-eu_bank = EuCentralBank.new
-Money.default_bank = eu_bank
+if Rails.env.test? || Rails.env.development?


There is Rails.env.local? which is exactly an equivalent of "test or dev"

Oh that's nice, updated.

danieljames-dj

Thanks @gregorbg for your review. I hope this is now fine codewise, let me know if not. I'll ping you once the budget is approved for the same so that I can test it in staging.

danieljames-dj · 2025-01-07T05:22:21Z

config/initializers/ruby_money.rb

-eu_bank = EuCentralBank.new
-Money.default_bank = eu_bank
+if Rails.env.test? || Rails.env.development?
+  eu_bank = EuCentralBank.new


Oh yes, done.

danieljames-dj · 2025-01-07T05:22:53Z

config/initializers/ruby_money.rb

 Money.locale_backend = :i18n

-eu_bank = EuCentralBank.new
-Money.default_bank = eu_bank
+if Rails.env.test? || Rails.env.development?


Oh that's nice, updated.

danieljames-dj · 2025-01-07T05:27:09Z

config/initializers/ruby_money.rb

+  mclb.access_key = AppSecrets.CURRENCY_LAYER_API_KEY
+  mclb.currencylayer = true
+  mclb.ttl_in_seconds = 86_400
+  mclb.cache = proc.new do |payload|


Actually rubocop asked to replace Proc.new with proc. After re-reading again, I realized it's proc and not proc.new. I cannot test it right now as my access key has expired. Once the budget is approved, I'll test again using a new test key created with WCA's account.

gregorbg

Code fine, pending valid API keys

github-actions bot assigned danieljames-dj Jan 3, 2025

danieljames-dj force-pushed the currencylayer branch 3 times, most recently from 32be5f1 to cca9d8e Compare January 3, 2025 17:17

gregorbg requested changes Jan 4, 2025

View reviewed changes

danieljames-dj force-pushed the currencylayer branch 3 times, most recently from 32309df to d9cffea Compare January 5, 2025 07:12

Change RubyMoney bank to currencylayer

a17813f

danieljames-dj force-pushed the currencylayer branch from d9cffea to a17813f Compare January 5, 2025 08:24

gregorbg requested changes Jan 5, 2025

View reviewed changes

Review changes

79ce66d

danieljames-dj commented Jan 7, 2025

View reviewed changes

gregorbg reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change RubyMoney bank to currencylayer #10520

Change RubyMoney bank to currencylayer #10520

danieljames-dj commented Jan 3, 2025

gregorbg Jan 4, 2025

danieljames-dj Jan 4, 2025

danieljames-dj Jan 4, 2025

gregorbg Jan 4, 2025

danieljames-dj Jan 4, 2025

gregorbg Jan 4, 2025

danieljames-dj Jan 4, 2025

gregorbg Jan 4, 2025

danieljames-dj Jan 4, 2025

gregorbg Jan 4, 2025

danieljames-dj Jan 4, 2025

gregorbg Jan 4, 2025

danieljames-dj Jan 4, 2025

gregorbg Jan 5, 2025

danieljames-dj Jan 5, 2025

gregorbg Jan 5, 2025

gregorbg Jan 5, 2025

danieljames-dj Jan 5, 2025

gregorbg Jan 5, 2025

danieljames-dj Jan 7, 2025

gregorbg Jan 5, 2025

danieljames-dj Jan 7, 2025

gregorbg Jan 5, 2025

danieljames-dj Jan 7, 2025

danieljames-dj left a comment

danieljames-dj Jan 7, 2025

danieljames-dj Jan 7, 2025

danieljames-dj Jan 7, 2025

gregorbg left a comment

Change RubyMoney bank to currencylayer #10520

Are you sure you want to change the base?

Change RubyMoney bank to currencylayer #10520

Conversation

danieljames-dj commented Jan 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieljames-dj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregorbg left a comment

Choose a reason for hiding this comment