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/markets-buttons #5441

Merged
merged 6 commits into from
Feb 11, 2025
Merged

Jon/fix/markets-buttons #5441

merged 6 commits into from
Feb 11, 2025

Conversation

Jon-edge
Copy link
Collaborator

@Jon-edge Jon-edge commented Feb 4, 2025

Various fixes

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)

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.

Nice cleanup, but found a few small things.

currencyConfig: wallet.currencyConfig,
tokenId: balanceTokenId,
nativeAmount: wallet.balanceMap.get(balanceTokenId) ?? '0'
}).displayDollarValue(exchangeRates, DECIMAL_PRECISION)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can't do math with a display value. Display values are only for showing to the user, because they might do weird foreign things like using commas for decimal points.

Please add two methods to CryptoAmount called just dollarValue and fiatValue, and have them return plain numbers. You can then implement displayDollarValue and displayFiatValue by calling these two new methods, and then doing the return parseFloat(convertedAmount).toFixed(precision ?? 2) at the end, to get the display formatting. In the future, we might replace this formatting logic with something from our intl tools, in which case we would get the weird foreign stuff.

@@ -103,6 +106,7 @@ const CoinRankingDetailsSceneComponent = (props: Props) => {
const { route, navigation } = props
const { assetId, fiatCurrencyCode, coinRankingData: initCoinRankingData } = route.params

const currencyConfigMap = useSelector(state => state.core.account.currencyConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong. You already have account on the line below, so you can either do const currencyConfigMap = account.currencyConfig, which is fine because happen to know that this particular field doesn't change, or you can do const currencyConfigMap = useWatch(account, 'currencyConfig'), which is how we generally access core properties. The useSelector hook is never supposed to reach inside core objects.

Comment on lines 207 to 210
uninitializedStakingWallets.forEach(wallet => {
if (walletStakingStateMap[wallet.id] != null && Object.keys(walletStakingStateMap[wallet.id].stakePolicies).length > 0) return
dispatch(updateStakingState(currencyCode, wallet)).catch(err => showError(err))
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Please swap forEach for for (const wallet of uninitializedStakingWallets) { ..., and of course the return becomes a continue. We hardly ever use forEach in our codebase, and it's slower anyhow.

@Jon-edge Jon-edge force-pushed the jon/fix/markets-buttons branch 2 times, most recently from d83b540 to 5f2c008 Compare February 7, 2025 02:58
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.

Wonderful

@Jon-edge Jon-edge force-pushed the jon/fix/markets-buttons branch from 5f2c008 to 1da1c51 Compare February 11, 2025 00:53
@Jon-edge Jon-edge enabled auto-merge February 11, 2025 00:53
@Jon-edge Jon-edge force-pushed the jon/fix/markets-buttons branch from 1da1c51 to e93e569 Compare February 11, 2025 01:13
Also fix the Earn button loading UI and disabled state
- Remove dependency on existing wallets to gather stake information
- Properly distinguish stakeable assets/wallets vs looser coinrank assets/wallets.
- Fix incorrect wallet picker options from currency code overlaps between mainnet and token codes
- Fix staking logic, state, effects, order of operations
- Fix going to the stake scene before fully initializing stake state causes a crash
- Fix `isStakingLoading` condition
- Add APY text to Earn button
"Trade" is defined as bringing up a modal with Buy/Sell/Swap in `TransactionListTop` that brings you to the Exchange scene.

Since the button on `CoinRankingDetails` brings you directly to the Exchange scene, this button should be `Swap`
"Select Asset to Purchase/Sell" to match the Buy/Sell scenes' modals

For the rest, generic "Select Wallet" since Swap can go either direction once you are on the scene, and Earn can be both the stake and/or reward asset.
@Jon-edge Jon-edge force-pushed the jon/fix/markets-buttons branch from e93e569 to e4ad258 Compare February 11, 2025 03:01
@Jon-edge Jon-edge disabled auto-merge February 11, 2025 03:02
@Jon-edge Jon-edge merged commit b4e2393 into develop Feb 11, 2025
1 of 2 checks passed
@Jon-edge Jon-edge deleted the jon/fix/markets-buttons branch February 11, 2025 03:02
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