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

refetch short data on new block #1121

Merged
merged 3 commits into from
Jun 3, 2024
Merged

Conversation

jackburrus
Copy link
Contributor

@jackburrus jackburrus commented May 31, 2024

This PR refetches the short preview data on new blocks. Below is a screenshot of what the 'fetching' state looks like on the preview.
Screenshot 2024-05-31 at 3 24 18 PM

Copy link

vercel bot commented May 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hyperdrive-monorepo-hyperdrive-trading ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 3, 2024 3:07pm
hyperdrive-sepolia-production ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 3, 2024 3:07pm
hyperdrive-sepolia-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 3, 2024 3:07pm
testnet-v1 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 3, 2024 3:07pm
2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
hyperdrive-js ⬜️ Ignored (Inspect) Visit Preview Jun 3, 2024 3:07pm
trading-competition ⬜️ Ignored (Inspect) Visit Preview Jun 3, 2024 3:07pm

Copy link

changeset-bot bot commented May 31, 2024

⚠️ No Changeset found

Latest commit: de8cb40

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

/>
}
disclaimer={(() => {
if (traderDeposit && !!amountOfBondsToShortAsBigInt) {
if (!amountOfBondsToShortAsBigInt) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to reorganize this disclaimer, so i'm going to write out this ternary logic here just to double check my work.

  • If the user has not input a short amount (!amountOfBondsToShortAsBigInt) -> don't show the disclaimer.
  • If the user has input an amount, but that amount makes the hasEnoughLiquidity become falsy -> only show the pool limit exceeded note.
  • In all other cases where the user has input an amount -> show the disclaimer, but ensure a skeleton is shown only on the stats that are being refetched on new blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was super helpful. I think we should capture this in the code either by pulling the conditions out into nicely named variables and/or adding code comments.

@@ -68,7 +67,7 @@ export function OpenLongPreview({
<LabelValue
label="You receive"
value={
openLongPreviewStatus === "loading" ? (
openLongPreviewStatus === "loading" && long.bondAmount ? (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized that if the user has not input an amount for longs, we want to show the default value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, i'm curious why the preview status would be loading if there was no input amount. Are we not turning off the preview hook in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check it out because I was confused too. I console logged it and without any input the status was still 'loading'

if (traderDeposit && !!amountOfBondsToShortAsBigInt) {
if (!amountOfBondsToShortAsBigInt) {
return null;
} else if (!!amountOfBondsToShortAsBigInt && !hasEnoughLiquidity) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no need for the else-if

/>
}
disclaimer={(() => {
if (traderDeposit && !!amountOfBondsToShortAsBigInt) {
if (!amountOfBondsToShortAsBigInt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This was super helpful. I think we should capture this in the code either by pulling the conditions out into nicely named variables and/or adding code comments.

Pool limit exceeded. Max short size is{" "}
{formatBalance({
balance: maxBondsOut || 0n,
decimals: hyperdrive.decimals,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I see it was like this before, but while we're here can we add the missing places and includeCommas args to formatBalance?

hy{baseToken.symbol}
</p>
);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no need for the else

@@ -68,7 +67,7 @@ export function OpenLongPreview({
<LabelValue
label="You receive"
value={
openLongPreviewStatus === "loading" ? (
openLongPreviewStatus === "loading" && long.bondAmount ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, i'm curious why the preview status would be loading if there was no input amount. Are we not turning off the preview hook in this case?

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