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

Jon/fix/uk-compliance #5319

Merged
merged 6 commits into from
Oct 23, 2024
Merged

Jon/fix/uk-compliance #5319

merged 6 commits into from
Oct 23, 2024

Conversation

Jon-edge
Copy link
Collaborator

@Jon-edge Jon-edge commented Oct 23, 2024

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Requirements

If you have made any visual changes to the GUI. Make sure you have:

  • Tested on iOS device
  • Tested on Android device
  • Tested on small-screen device (iPod Touch)
  • Tested on large-screen device (tablet)

…rings

- We need a distinction now for a past buy/sell that already occurred, vs the verb to buy/sell.
- Would incur a small cost to re-translate, but worth the organizational benefits to remove duplicates at the same time. Most of the localizations for the `TX_ACTION_LABEL_MAP` renamed keys are not translated, anyway.
- Only the `buy_crypto_modal_buy_action` had translations, which were ported over for the interim while we wait for translations.
@Jon-edge Jon-edge force-pushed the jon/fix/uk-compliance branch 2 times, most recently from 90b8058 to 2d40ad6 Compare October 23, 2024 01:29
Copy link
Contributor

@swansontec swansontec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes seem OK, but we will need a small tweak to get performance back up.


useAsyncEffect(
async () => {
setCountryCode(await getCountryCodeByIp())
Copy link
Contributor

@swansontec swansontec Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getCountryCodeByIp function does a network call. We don't want to do a bunch of network calls as we navigate the app - it would be better to just do one call at boot-up, and then use the cached value everywhere.

We already do this with getFirstOpenInfo - if we've successfully loaded the file, it just returns the cached value instead of loading it again. We also do something similar in getDeviceSettings, but this one is actually sync rather than async, because we initialize it at boot-up.

Whether we go sync or async, I believe getCountryCodeByIp needs the same treatment to avoid a major performance regression. Also, what happens if we are in airplane mode? We might even want to cache the value between runs, maybe saving it in device settings, so we always know where we are.

Copy link
Collaborator Author

@Jon-edge Jon-edge Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was trying to come up with a new method to do it once somewhere and pass it around somehow without success, those are good ideas

Copy link
Contributor

@swansontec swansontec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will work


if (firstOpenInfo.countryCode == null) {
// Not critical if we can't get the country code
firstOpenInfo.countryCode = await getCountryCodeByIp().catch(() => undefined)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to re-save here, so it's available on the next run?

Copy link
Collaborator Author

@Jon-edge Jon-edge Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, then they're stuck as a UK-designated device if they happened to open the app in the UK when this change goes live. Could see issues with people visiting the UK or VPN-ing.

@Jon-edge Jon-edge merged commit c63fe78 into develop Oct 23, 2024
2 checks passed
@Jon-edge Jon-edge deleted the jon/fix/uk-compliance branch October 23, 2024 21:44
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