-
Notifications
You must be signed in to change notification settings - Fork 7
feat(mobile): implement token details, ref LEA-3015 #1465
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
Conversation
Leather Web Build b0490d6 → https://pr-1465-leather-web.wallet-6d1.workers.dev |
This PR's build fingerprint differs from the latest production build's fingerprint. Merging this PR would require us to submit the build to the stores. (Note: Make sure your PR is up to date) |
092d688
to
cc4e73a
Compare
@@ -43,7 +42,7 @@ export default function AccountActivityScreen() { | |||
})} | |||
</Screen.Title> | |||
} | |||
data={activity.value as OnChainActivity[]} // TODO: Unclear why was this cast. Needs clearing up. | |||
data={activity.value.filter(activity => activity && 'asset' in activity)} |
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.
Filtering for only OnChainActivity
to avoid casting. I could probably do this in the Query itself
|
||
function onOpenToken({ asset, availableBalance, quoteBalance }: OnOpenTokenProps) { | ||
setSheetData({ asset, availableBalance, quoteBalance }); | ||
// analytics.track('token_sheet_opened', { source: 'action_bar' }); |
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.
Keeping these as a placeholder. I'll add the analytics keys soon
tokenSheetRef.current?.present(); | ||
} | ||
|
||
const onPressToken = tokenDetailsFlag ? onOpenToken : undefined; |
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 have enabled tokenDetailsFlag
in dev
but off in production in launch darkly.
There is no way to trigger the TokenDetails
sheet without entering here first
const availableBalance = value?.btc.availableBalance; | ||
const quoteBalance = value?.quote.availableBalance; | ||
|
||
if (!availableBalance || !quoteBalance) { |
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 need to clean this up
@@ -83,6 +87,14 @@ function Sip10BalanceWrapper({ data, mode = 'full' }: Sip10BalanceWrapperProps) | |||
contractId={item.asset.contractId} | |||
quoteBalance={item.quote.totalBalance} | |||
imageCanonicalUri={item.asset.imageCanonicalUri} | |||
onPress={() => { | |||
// pass balance and quote balance to the sheet from here |
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 am passing the balances to the sheet via a callback as we already have that information at this point.
Without doing this we would need to call the balance hooks again in the sheet and then have 6 different setups for each scenario (excluding Runes) - BTC
, STX
and SIP-10
both all balances and account balance.
accountName={account.name} | ||
walletName={ | ||
<Text variant="caption01" lineHeight={16}> | ||
{/* Should perhaps refactor account to have a wallet name? Avoids using the wallet store here */} |
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.
We need to use WalletLoader
to get the wallet
name. We could probably add that to the account object
</Text> | ||
} | ||
// FIXME LEA-3015: refactor address prop to not be address - leftPrimary or something like that | ||
address={ |
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'm re-using AccountListItemComponent
but it needs to be refactored to be more generic
/> | ||
} | ||
balance={ | ||
// FIXME LEA-3015: isQuoteCurrency is crashing for non BTC tokens |
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.
<TokenDetailsCard | ||
title={ | ||
<Box flexDirection="row" alignItems="center" gap="2"> | ||
{/* FIXME - this icon needs to be 16x16 but require refactors to make it work */} |
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.
@@ -0,0 +1,77 @@ | |||
import { AccountBalance, useAccountBalance } from '@/queries/balance/account-balance.query'; |
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'll be removing this file completely. I replaced this with passing balances as props as I think that's cleaner than these custom hooks
const availableBalance = tokenBalance?.availableBalance; | ||
const quoteBalance = tokenBalance?.quoteBalance; | ||
|
||
if (tokenId === 'BTC') { |
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.
<Screen> | ||
<Screen.Header | ||
blurOverlay={false} | ||
// topElement={isErrorTotalBalance && <FetchErrorCallout />} |
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 need to add error handling in case of incorrect balances / timeout etc.
.filter(activity => activity && 'asset' in activity) | ||
.filter(activity => filterByTicker(activity, ticker)) | ||
.filter(activity => { | ||
if (isDefined(accountIndex) && isDefined(fingerprint)) { |
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.
When an account is selected, this re-renders and filters activity using that account ID.
We have another query for account specific activity by asset but this data is already here
|
||
<TokenDetailsTable | ||
name={`${capitalize(asset.chain)} (${asset.symbol})`} | ||
// for Layer design has 'Layer 1 (Bitcoin)' but no other examples. Only showing 'Layer 1' for now based on previous use of getChainLayerFromAssetProtocol |
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.
ticker={asset.symbol} | ||
accountIndex={accountIndex} | ||
fingerprint={fingerprint} | ||
// TokenDetails is Activity ListHeader to avoid nested scrolling errors |
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 page is structured to be centered around the ActivityList
FlatList
.
I pass all the non-activity content as the ListHeader
so that scrolling works smoothly on both and it doesn't give errors.
cc4e73a
to
fed0a13
Compare
b9df0ec
to
4f67d72
Compare
4f67d72
to
15dae8e
Compare
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.
Thank you @pete-watters ! I think this is good to merge but not to release for now (feature flag is helping tremendously here!). Let's merge this and then open smaller PRs to fix some issues
}: TokenActivityProps) { | ||
const scrollViewAdjustmentOffset = 56; | ||
const { refreshing, onRefresh } = useRefreshHandler(); | ||
const activity = useTotalActivity(); |
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 is going to be a big issue for performance on bigger wallets. To my knowledge we have a separate service for BTC/STX/SIP10 activities, so we'd rather use that
data={activity.value | ||
.filter(activity => activity && 'asset' in activity) | ||
.filter(activity => filterByTicker(activity, ticker)) | ||
.filter(activity => { |
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'd throw this filters into a useMemo as it might be a performance issue to filter these all out
This PR adds the first draft of token details. It's close but some more work is needed regarding:
I have feature flagged the whole thing so it could be merged safely or not.
Here's a demo:
token-details.mp4