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

feat: warn user of stale exchange rates #1284

Merged
merged 9 commits into from
May 8, 2024
Merged

Conversation

HashEngineering
Copy link
Collaborator

@HashEngineering HashEngineering commented May 1, 2024

Issue being fixed or feature implemented

Related PR's and Dependencies

Screenshots / Videos

How Has This Been Tested?

  • QA (Mobile Team)

Checklist:

  • I have performed a self-review of my own code and added comments where necessary
  • I have added or updated relevant unit/integration/functional/e2e tests

@HashEngineering HashEngineering self-assigned this May 1, 2024
@HashEngineering HashEngineering marked this pull request as ready for review May 8, 2024 05:12
Comment on lines +9 to +25
class ExchangeRatesConfig
@Inject
constructor(
context: Context,
walletDataProvider: WalletDataProvider
) : BaseConfig(
context,
PREFERENCES_NAME,
walletDataProvider
) {
companion object {
private const val PREFERENCES_NAME = "exchange_rates_config"
val EXCHANGE_RATES_RETRIEVAL_TIME = longPreferencesKey("exchange_rates_retrieval_time")
val EXCHANGE_RATES_RETRIEVAL_FAILURE = booleanPreferencesKey("exchange_rates_retrieval_error")
val EXCHANGE_RATES_PREVIOUS_RETRIEVAL_TIME = longPreferencesKey("exchange_rates_previous_retrieval_time")
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At first I was going to put these in WalletUIConfig, but that would have created a circular reference.

Copy link
Member

Choose a reason for hiding this comment

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

WalletUIConfig might be better to leave for UI settings that can be changed by the user, or closely related preferences.

constructor(currencyCode: String, rate: String?, retrievalTime: Long) {
this.currencyCode = currencyCode
this.rate = rate
this.retrievalTime = retrievalTime
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CTX does return a field that contains the time their servers last retrieved this particular rate.

It is possible for CTX to return rates, but they are stale.

Comment on lines +91 to 100
currencyName =
if (currencyCode.length == 3) {
try {
getCurrency().displayName
} catch (x: IllegalArgumentException) {
currencyCode
}
} else {
currencyCode
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here, I am sure that the ktlint plugin on automatically formatted this file upon save. I have turned off this feature.

Comment on lines +29 to +38
object MyTheme {
val ToastBackground = Color(0xff191c1f).copy(alpha = 0.9f)

val Body2Regular = TextStyle(
fontSize = 14.sp,
lineHeight = 20.sp,
fontFamily = FontFamily.Default, // FontFamily(Font(R.font.inter)) // crashes,
fontWeight = FontWeight(400),
color = Color.White
)
Copy link
Collaborator Author

@HashEngineering HashEngineering May 8, 2024

Choose a reason for hiding this comment

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

This is a start for a "stylesheet" file. There probably is a better way to do this, perhaps a standard way that it is done with Compose.

Copy link
Member

Choose a reason for hiding this comment

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

Usually in JC you would set a MaterialTheme with a typography field, however, the names of standard text styles aren't going to match with our typography sheet in Figma, so it's better to have named styles as you did here.

Comment on lines +90 to +91
previousRates.clear()
previousRates.addAll(exchangeRatesDao.getAll())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

here we save the previous rates in memory to compare with the new rates.

Comment on lines +202 to +209
val storageManagerIntent =
Intent(
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
StorageManager.ACTION_MANAGE_STORAGE
} else {
Settings.ACTION_MANAGE_APPLICATIONS_SETTINGS
}
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a way to get the ktlint plugin for Android Studio to use our rules in this project?

This was another case where I was using the new ktlint plugin to reformat a file on save, which would actually change a lot in wallet and in other modules may apply rules that we don't use.

Copy link
Member

Choose a reason for hiding this comment

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

I think if you set .editorconfig as the baseline file in ktlint plugin settings, it should pickup our rules.
I removed ktlint plugin from the studio because after some update it broke and started to complain about everything.

Copy link
Member

@Syn-McJ Syn-McJ left a comment

Choose a reason for hiding this comment

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

Looks good

@HashEngineering HashEngineering merged commit 4ef1b81 into master May 8, 2024
2 checks passed
@HashEngineering HashEngineering deleted the feature-stale-rates branch August 29, 2024 04:35
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