Skip to content

Feat(pricefeed/price feed id) add data #445

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

Merged
merged 10 commits into from
Oct 21, 2024

Conversation

aditya520
Copy link
Member

No description provided.

Copy link

vercel bot commented Oct 8, 2024

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

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 21, 2024 4:02pm
documentation ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 21, 2024 4:02pm

aditya520 and others added 8 commits October 18, 2024 12:54
* remove mention of pgas

* Revert "Merge branch 'main' of https://github.com/pyth-network/documentation"

This reverts commit b2cc20f, reversing
changes made to 20caa0e.

* chore(entropy) Protocol design edit (#446)

* chore(entropy) Protocol design ediy

* improvements

* improvements

* chore(pricefeed) Add unichain sepolia (#447)

* fix(pricefeed) Fix unichain address (#448)

* initial commit

---------

Co-authored-by: Aditya Arora <[email protected]>
Copy link
Contributor

@cprussin cprussin left a comment

Choose a reason for hiding this comment

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

Main suggestions are to see if you can use the built-in nextra components for some of this stuff and to improve the state management in the price-feed-ids network data loading block

Comment on lines 10 to 20
interface TableColumnWidths {
assetType: string;
name: string;
ids: any;
}

const columnWidths: TableColumnWidths = {
assetType: "w-3/10",
name: "w-1/5",
ids: "w-1/2",
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's any reason to extract all this out, just put the tailwind classes in at the call sites where they're being consumed (i.e. lines 70 - 72)


const PriceFeedTable = ({ priceFeeds }: { priceFeeds: PriceFeed[] }) => {
const [selectedAssetType, setSelectedAssetType] = useState<string>("All");
const [copiedId, setCopiedId] = useState<string | null>(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

I generally recommend using undefined instead of null, see sindresorhus/meta#7 for a good discussion on why

strokeWidth={2}
d="M8 16H6a2 2 0 01-2-2V6a2 2 0 012-2h8a2 2 0 012 2v2m-6 12h8a2 2 0 002-2v-8a2 2 0 00-2-2h-8a2 2 0 00-2 2v8a2 2 0 002 2z"
/>
</svg>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming that this is a copy icon right? Why not use the CopyIcon.tsx that's already in the codebase?

Copied to clipboard
</div>
)}
</div>
Copy link
Contributor

@cprussin cprussin Oct 19, 2024

Choose a reason for hiding this comment

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

A lot of this component is doing the same stuff that's already in Nextra in copy-to-clipboard.tsx, code.tsx, and pre.tsx. There's also built-in table components too. I'd advise trying to find a way to leverage the nextra built in components for this stuff -- e.g. I'm guessing you can replace this entire component with something very similar to this:

import { Code, Pre, Table, Td, Th, Tr } from 'nextra/components';

const PriceFeedTable = ({ priceFeeds }: { priceFeeds: PriceFeed[] }) => {
  // .... filtering stuff

  return (
    <Table>
      <thead>
        <Tr>
          <Th className="w-3/10">Asset Type</Th>
          <Th className="w-1/5">Name</Th>
          <Th className="w-1/2">Feed ID</Th>
        </Tr>
      </thead>
      <tbody>
        {filteredFeeds.map(({ assetType, name, ids }) => (
          <Tr key={name}>
            <Td>{assetType}</Td>
            <Td>{name}</Td>
            <Td>
              <Pre data-copy="">
                <Code>
                  {ids}
                </Code>
              </Pre>
            </Td>
          </Tr>
        ))}
      </thead>
    </Table>
  );
};

Note pre is short for Preformatted and is nearly always the right html tag to wrap a code block, see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/pre.

Not only will this drastically reduce the amount of custom code here, but you'll also make things much more consistent with the rest of the site.

</thead>
<tbody>
{filteredFeeds.map((feed, index) => (
<tr key={index}>
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use something that's unique and not index based here, e.g. feed.name, as the key. Using the index will cause issues when you change the filter because React will try to recycle HTML elements that were used for other feeds when indices change and you'll get hard to debug issues where you'll see stale data in the UI.

const fetchPriceFeeds = async () => {
try {
const response = await fetch('https://hermes.pyth.network/v2/price_feeds');
const data = await response.json();
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, you should validate the response type here. This is where zod can be helpful.

Alternatively if you use the Hermes js client, it already has validation and everything built in

}
};

fetchPriceFeeds();
Copy link
Contributor

@cprussin cprussin Oct 19, 2024

Choose a reason for hiding this comment

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

You should ideally pass a canceler here using AbortController. You should also attach a catch handler to the promise, all promises should be caught explicitly to avoid uncaught exceptions in the boundary between promise and async code (even though you have a catch in the async code there, the code could easily be refactored such that uncaught exceptions make their way through, and without having a catch handler on the promise the code is unsafe).

You can also extract out the fetchPriceFeeds function to clean things up a bit.

You should also add loading and error states.

Finally I'd suggest putting all the component code in an actual .tsx file and just importing it here, rather than keeping it inline. Your formatting isn't handled by prettier, you don't get linting, etc, when you have the code inline.

Putting this all together would look something like this:

import { useState, useEffect } from 'react';
import { z } from "zod";

import PriceFeedTable from "../../../components/PriceFeedTable";

export const PriceFeedData = () => {
  const priceFeedData = usePriceFeedData();

  switch (state.type) {
    case StateType.NotLoaded:
    case StateType.Loading: {
      return <Spinner /> // TODO implement a spinner / loader...
    }

    case StateType.Error: {
      return <p>Oh no!  An error occurred: {state.error}</p>
    }

    case StateType.Loaded: {
      return <PriceFeedTable priceFeeds={state.data} />;
    }
  }
};

const usePriceFeedData = (): State => {
  const [state, setState] = useState<State>(State.NotLoaded());

  useEffect(() => {
    setState(State.Loading());
    const controller = new AbortController();
    fetchPriceFeeds(controller)
      .then(data => setState(State.Loaded(data)))
      .catch((error: unknown) => {
        console.error('Error fetching price feeds:', error);
        setState(State.Error(error));
      });
    return () => controller.abort();
  }, []);

  return state;
}

const dataSchema = z.array(z.object({
  asset_type: z.string(),
  display_symbol: z.string(),
  ids: z.array() // TODO: I don't know what this shape should be but you should type this correctly!
}).transform(({ asset_type, display_symbol, id }) => ({
  assetType: asset_type,
  name: display_symbol,
  ids: id
})));

type Data = z.infer<typeof dataSchema>;

enum StateType = {
  NotLoaded,
  Loading,
  Error,
  Loaded
}

const State = {
  NotLoaded: () => ({ type: StateType.NotLoaded as const }),
  Loading: () => ({ type: StateType.Loading as const }),
  Error: (error: unknown) => ({ type: StateType.Error as const, error }),
  Loaded: (data: Data) => ({ type: StateType.Loaded as const, data }),
}
type State = ReturnType<typeof State[keyof typeof State]>;

const fetchPriceFeeds = async (signal: AbortSignal): Promise<Data> => {
  const response = await fetch('https://hermes.pyth.network/v2/price_feeds', { signal });
  return dataSchema.parse(await response.json());
};

Note this is untested and still needs a few TODO items filled in so don't copy-paste blindly!

@aditya520 aditya520 merged commit 37e68d2 into main Oct 21, 2024
5 checks passed
@aditya520 aditya520 deleted the feat(pricefeed/price-feed-id)-add-data branch October 21, 2024 17:44
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.

3 participants