-
-
Notifications
You must be signed in to change notification settings - Fork 218
feat: allow selecting multiple fiat currencies #3384 #3595
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
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @aryanraj45, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement by allowing users to select and display multiple fiat currencies concurrently. This change eliminates the need for users to constantly switch between currencies to view their balances, providing a more convenient and comprehensive financial overview, especially for those managing funds across different regional currencies or stablecoins. The underlying architecture has been updated to support this multi-selection, from persistent storage of selected currencies to dynamic fetching and display of their respective exchange rates. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request primarily introduces support for displaying multiple fiat currencies simultaneously within the application. Key changes include updating the Conversion.tsx component to iterate and display rates for a list of fiat currencies stored in SettingsStore.settings.fiats. The FiatStore.ts was modified to fetch rates for multiple fiats using Promise.all and its getSymbol and getRate methods now accept an optional currencyCode argument. The SettingsStore.ts was updated to include a fiats array in its settings, ensuring backward compatibility by initializing it from the single fiat setting if missing. Related utility functions in AmountUtils.ts and UnitsUtils.ts were adjusted to handle dynamic currency codes. The SelectCurrency.tsx view was refactored to allow users to select multiple fiat currencies, adding or removing them from the fiats array, and the display in Currency.tsx now shows all selected fiats. Review comments suggest improving type safety in FiatStore.ts by using a type guard instead of as any when filtering fiat rates, and refactoring the currency selection logic in SelectCurrency.tsx to use a single isSelected variable for clarity and to reduce code duplication.
stores/FiatStore.ts
Outdated
| } else if (rate) { | ||
| this.fiatRates = [rate]; | ||
| } | ||
| this.fiatRates = rates.filter((r) => r != null) as any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using as any bypasses TypeScript's type safety. A type guard would be a better approach to ensure the filtered array is correctly typed.
| this.fiatRates = rates.filter((r) => r != null) as any; | |
| this.fiatRates = rates.filter((r): r is { name?: string; cryptoCode: string; currencyPair: string; code: string; rate: number; } => r != null); |
| (!currencyConverter && | ||
| selectedCurrency === | ||
| item.value) || | ||
| (!selectedCurrency && | ||
| item.value === DEFAULT_FIAT) | ||
| SettingsStore.settings.fiats.includes( | ||
| item.value | ||
| )) || | ||
| (!SettingsStore.settings | ||
| .fiats && | ||
| item.value === DEFAULT_FIAT) | ||
| ? themeColor('highlight') | ||
| : themeColor('text'), | ||
| fontFamily: 'PPNeueMontreal-Book' | ||
| }} | ||
| > | ||
| {`${item.flag ? item.flag : ''} ${ | ||
| item.key | ||
| } ${ | ||
| item.value ? `(${item.value})` : '' | ||
| }`} | ||
| {`${item.flag ? item.flag : ''} ${item.key | ||
| } ${item.value ? `(${item.value})` : '' | ||
| }`} | ||
| </ListItem.Title> | ||
| </ListItem.Content> | ||
| {(selectedCurrency === item.value || | ||
| (!selectedCurrency && | ||
| {(SettingsStore.settings.fiats.includes( | ||
| item.value | ||
| ) || | ||
| (!SettingsStore.settings.fiats && | ||
| item.value === DEFAULT_FIAT)) && | ||
| !currencyConverter && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to determine if a currency is selected is duplicated for setting the color and showing the checkmark icon. This can be extracted into a single variable for clarity and to avoid repetition.
const isSelected = !currencyConverter && (
(SettingsStore.settings.fiats && SettingsStore.settings.fiats.includes(item.value)) ||
(!SettingsStore.settings.fiats && item.value === DEFAULT_FIAT)
);You can then use isSelected for both the color style and the conditional rendering of the checkmark icon. This would make the code cleaner and easier to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aryanraj45, you don’t need to change these files, so please remove this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review, @ajaysehwal!
I've removed the accidental change to gradle-wrapper.properties.
I also did a quick pass to clean up the code and improve type safety (removed unnecessary non-null assertions).
|
@aryanraj45, please run yarn verify locally before pushing your changes |
|
@ajaysehwal Done! I've fixed all 65+ linting and formatting errors identified by yarn verify. Cleaned up unused variables in |
ajaysehwal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aryanraj45, your commit structure doesn’t look great right now. You’ve used the same title for two commits, and the last two commits seem unnecessary. Please revert the latest three commits and force-push again with only two commits. lmk if anything is unclear.
37a3170 to
9aa3c5b
Compare
|
@ajaysehwal I've squashed the cleanup and refactor updates into a single commit as requested. The PR now contains exactly two commits: The original feature request. |
| distributionBase=GRADLE_USER_HOME | ||
| distributionPath=wrapper/dists | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-9.0.0-bin.zip | ||
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.13-bin.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this changed?
components/UnitToggle.tsx
Outdated
| const { changeUnits, units } = UnitsStore!; | ||
| const { settings } = SettingsStore!; | ||
|
|
||
| if (!UnitsStore || !SettingsStore) return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this change required?
utils/AmountUtils.test.ts
Outdated
| }); | ||
|
|
||
| it('returns error when rate for currency is not available', () => { | ||
| it('returns 0 when rate for currency is not available', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this behavior need to be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change prevents a visual flicker when switching currencies. When switching from one fiat to another, there's a brief moment where the new rate isn't loaded yet. Displaying 'Disabled' or 'N/A' causes the UI to flash, which is jarring. Returning '0' with the correct symbol ensures a smooth transition until the actual rate loads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach can cause further issues. You should find another approach.
This wasn't an issue before with single fiat functionality.
utils/UnitsUtils.ts
Outdated
| }; | ||
|
|
||
| type Units = 'sats' | 'BTC' | 'fiat'; | ||
| type Units = 'sats' | 'BTC' | 'fiat' | string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a fan of this approach. Do we maybe need a type for our fiat strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to make changes to this view? Shouldn't everything be covered by the Conversion and UnitToggle components?
9aa3c5b to
7b7d6e7
Compare
|
@kaloudis Thanks for the detailed review! I've addressed all the feedback: Reverted gradle-wrapper.properties (accidental change)
|
ajaysehwal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aryanraj45, please review all the changed files. There still seem to be a lot of unnecessary changes. I don’t think this feature requires this many modifications, so please remove the unnecessary ones and keep only the required changes
utils/AmountUtils.test.ts
Outdated
| }); | ||
|
|
||
| it('returns error when rate for currency is not available', () => { | ||
| it('returns 0 when rate for currency is not available', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach can cause further issues. You should find another approach.
This wasn't an issue before with single fiat functionality.
7b7d6e7 to
b30f53c
Compare
b30f53c to
3bd340e
Compare
| @@ -168,6 +168,8 @@ export interface Settings { | |||
| authenticationAttempts?: number; | |||
| fiatEnabled?: boolean; | |||
| fiat?: string; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to have both fiat and fiats - best to consolidate them. Probably best to keep fiat and make it backwards compatible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaloudis done Consolidated fiat and fiats - fiat is now automatically derived from fiats[preferredFiatIndex]
f43de3c to
166418e
Compare
Description
This PR addresses issue #3384 by allowing users to select multiple fiat currencies simultaneously.
Key Changes:
fiatsarray to persist multiple currency selections.Why this is useful?
Users no longer need to switch settings back and forth to see their balance in different currencies. This is particularly useful for users dealing with multiple regional currencies or stablecoins.
Visuals
| Currency Selection |
|---|---|
Screen.Recording.2026-01-25.at.08.mp4
Fixes
Fixes #3384