-
Notifications
You must be signed in to change notification settings - Fork 45
feat: new bands chart #1541
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: main
Are you sure you want to change the base?
feat: new bands chart #1541
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
d4203d3 to
107ea7e
Compare
5d6f4c8 to
675ff9d
Compare
DanielSchiavini
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.
I didn't manage to get through the PR yet, but I'm leaving a few ideas already
apps/main/src/llamalend/features/bands-chart/hooks/useProcessedBandsData.ts
Outdated
Show resolved
Hide resolved
apps/main/src/llamalend/features/bands-chart/hooks/useProcessedBandsData.ts
Outdated
Show resolved
Hide resolved
| market && collateralTokenAddress | ||
| ? { | ||
| symbol: market.collateral_token.symbol, | ||
| address: collateralTokenAddress, | ||
| chain: networks[rChainId].id, | ||
| } | ||
| : undefined | ||
| const borrowToken = | ||
| market && borrowedTokenAddress | ||
| ? { | ||
| symbol: market.borrowed_token.symbol, | ||
| address: borrowedTokenAddress, | ||
| chain: networks[rChainId].id, | ||
| } | ||
| : 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.
| market && collateralTokenAddress | |
| ? { | |
| symbol: market.collateral_token.symbol, | |
| address: collateralTokenAddress, | |
| chain: networks[rChainId].id, | |
| } | |
| : undefined | |
| const borrowToken = | |
| market && borrowedTokenAddress | |
| ? { | |
| symbol: market.borrowed_token.symbol, | |
| address: borrowedTokenAddress, | |
| chain: networks[rChainId].id, | |
| } | |
| : undefined | |
| market && collateralTokenAddress && { | |
| symbol: market.collateral_token.symbol, | |
| address: collateralTokenAddress, | |
| chain: networks[rChainId].id, | |
| } | |
| const borrowToken = | |
| market && borrowedTokenAddress && { | |
| symbol: market.borrowed_token.symbol, | |
| address: borrowedTokenAddress, | |
| chain: networks[rChainId].id, | |
| } |
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.
they will return this type if I make that change
const collateralToken: "" | {
symbol: string;
address: string;
chain: INetworkName;
} | 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.
argh this is ugly 😢 but I we actually don't need to check collateralTokenAddress, it will never be null, only market needs to be checked. And it could just be
| market && collateralTokenAddress | |
| ? { | |
| symbol: market.collateral_token.symbol, | |
| address: collateralTokenAddress, | |
| chain: networks[rChainId].id, | |
| } | |
| : undefined | |
| const borrowToken = | |
| market && borrowedTokenAddress | |
| ? { | |
| symbol: market.borrowed_token.symbol, | |
| address: borrowedTokenAddress, | |
| chain: networks[rChainId].id, | |
| } | |
| : undefined | |
| const chain = rChainId | |
| const collateralToken = market && { ...market.collateral_token, chain } | |
| const borrowToken = market && {...market.borrowed_token.symbol, chain } |
DanielSchiavini
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.
Sorry, it's a huge PR so there is a lot to review. If you prefer comments as a PR please let me know
apps/main/src/llamalend/queries/market-liquidation-band.query.ts
Outdated
Show resolved
Hide resolved
| // Parsed bands balance type used in chart data | ||
| export type ParsedBandsBalances = { | ||
| borrowed: string | ||
| collateral: 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.
Should we use Decimal type here?
apps/main/src/llamalend/features/bands-chart/hooks/useProcessedBandsData.ts
Outdated
Show resolved
Hide resolved
apps/main/src/llamalend/features/bands-chart/hooks/useUserBandsPriceRange.ts
Outdated
Show resolved
Hide resolved
apps/main/src/llamalend/features/bands-chart/TooltipContent.tsx
Outdated
Show resolved
Hide resolved
| <TooltipItem title={t`Your share of band`}>{userBandShare}</TooltipItem> | ||
| <TooltipItem variant="subItem" title={collateralToken?.symbol}> | ||
| {formatAbbreviatedNumber(data.userBandCollateralAmount)} | ||
| {`${formatUsd(data.userBandCollateralValueUsd ?? 0)}`} |
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.
Showing $0 just because we don't have the token price seems incorrect.
Also, the template string is not needed here
| {`${formatUsd(data.userBandCollateralValueUsd ?? 0)}`} | |
| {formatUsd(data.userBandCollateralValueUsd ?? 0)} |
| const price = Number(oraclePrice) | ||
| if (!Number.isFinite(price)) return [] | ||
|
|
||
| return [createMarkLine(price, `${formatUsd(+oraclePrice)}`, 'end', palette.oraclePriceLineColor)] |
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.
| return [createMarkLine(price, `${formatUsd(+oraclePrice)}`, 'end', palette.oraclePriceLineColor)] | |
| return [createMarkLine(price, formatUsd(+oraclePrice), 'end', palette.oraclePriceLineColor)] |
| collateralTokenAddress, | ||
| borrowedTokenAddress, | ||
| }) | ||
| const collateralToken = |
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 conversion is repeated, it should be a helper somewhere
| ? { | ||
| symbol: llamma.collateralSymbol, | ||
| address: collateralTokenAddress, | ||
| chain: networks[chainId].id, |
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 the networks mapping for this?
| chain: networks[chainId].id, | |
| chain: chainId, |
The new bands chart is available on market pages with BETA mode enabled.
Changes:
Still to do:
Add "bundle band"(will add at a later stage)(currently has placeholder)Add loading state (currently has placeholder)(will update at the same time as the candle chart)