Skip to content

feat(pricefeed) Migrate Price Feed IDs to documentation #493

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

feat(pricefeed) Migrate Price Feed IDs to documentation #493

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f2aa8e1
initial commit
aditya520 Nov 6, 2024
a037765
added copy button
aditya520 Nov 6, 2024
1a298ee
improvements
aditya520 Nov 7, 2024
17f9b75
resolved comments
aditya520 Nov 12, 2024
f899e02
update
aditya520 Nov 13, 2024
4c8af3c
update
aditya520 Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 123 additions & 0 deletions components/PriceFeedTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
import { useState, useEffect } from "react";
import {
Table,
TableHead,
TableBody,
TableRow,
TableCell,
TextField,
Select,
MenuItem,
Box,
Snackbar,
} from "@mui/material";
Copy link
Contributor

Choose a reason for hiding this comment

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

Mentioned this in slack but we should try to use built-in nextra components where possible. I don't really have an issue with using mui for stuff that isn't in nextra, but I also don't think you get much out of the stuff you're using here and can probably do away with mui entirely.

import { HermesClient } from "@pythnetwork/hermes-client";
import copy from "copy-to-clipboard";

interface PriceFeed {
id: string;
name: string;
assetType: string;
}

export function PriceFeedTable() {
const [priceFeeds, setPriceFeeds] = useState<PriceFeed[]>([]);
const [searchTerm, setSearchTerm] = useState("");
const [selectedAssetType, setSelectedAssetType] = useState("All");
const [openSnackbar, setOpenSnackbar] = useState(false);

useEffect(() => {
const fetchPriceFeeds = async () => {
const hermesClient = new HermesClient("https://hermes.pyth.network");
const feeds = await hermesClient.getPriceFeeds();
const transformedFeeds = feeds.map((feed) => ({
id: feed.id,
name: feed.attributes.display_symbol || "",
assetType: feed.attributes.asset_type || "",
}));
setPriceFeeds(transformedFeeds);
};

fetchPriceFeeds();
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a few issues with this effect and the state it creates:

  1. It's not accurate to initialize priceFeeds to an empty array on line 24, you should properly handle loading states and represent that state differently from a loaded array.
  2. You aren't actually using priceFeeds anywhere except to get the list of asset types. You should extract the asset types and dedupe them when you load the data and store that in the state, rather than deduping in the render path on line 76 below.
  3. The transformation on lines 33 - 37 is IMO not useful; you're basically just changing snake case to camel case. IMO there isn't any serious benefit to doing that, and there are costs (added cognitive overhead, makes the code harder to search, etc). I know that camel case is the convention, but sticking to convention is not worth added complexity and making the code harder to search.
  4. You should always use a catch any time you call a promise without using await, e.g. on line 41. Omitting the catch means uncaught exceptions could potentially put the app into an unstable state; additionally here you should make the UI indicate errors properly.
  5. I always strongly recommend extracting functions like fetchPriceFeeds outside the closure scope. For one, it makes the code simpler and easier to follow; for two it's easy to introduce performance issues by creating closures in a hot path by accident.
  6. You should always prefer ?? over || (on lines 35 and 36), see https://typescript-eslint.io/rules/prefer-nullish-coalescing/.
  7. Minor nit but in es6 you can do [...new Set(...)] instead of Array.from(new Set(...)) to dedupe, the former is considered more idiomatic.
  8. You can use the "fake sum types" pattern I shared to encapsulate the loading/error state management here if you like.

Putting all this together you can change the component to something like this:

export const PriceFeedTable = () => {
  const [selectedAssetType, setSelectedAssetType] = useState("All");

  // ...
  <AssetTypesSelect
    selectedAssetType={selectedAssetType}
    setSelectedAssetType={setSelectedAssetType}
  />
}

type AssetTypesSelectProps = {
  selectedAssetType: string;
  setSelectedAssetType: (value: string) => void;
}

const AssetTypesSelect = ({ selectedAssetType, setSelectedAssetType }: AssetTypesSelectProps) => {
  const assetTypes = useAssetTypes();

  switch (state.type) {
    case AssetTypesStateType.NotLoaded:
    case AssetTypesStateType.Loading: {
      // use any spinner we're already using in the app anywhere else...
      return <Spinner /> 
    }
    case AssetTypesStateType.Error: {
      return <p>Error loading asset types!</p>;
    }
    case AssetTypesStateType.Loaded: {
      return (
        <Select
          value={selectedAssetType}
          onChange={(e) => setSelectedAssetType(e.target.value)}
          size="small"
          sx={{ width: 200 }}
        >
          <MenuItem value="All">All Asset Types</MenuItem>
          {state.assetTypes.map(assetType => (
            <MenuItem key={assetType} value={assetType}>
              {assetType ?? "Uncategorized"}
            </MenuItem>
          ))}
        </Select>
      );
    }
  }
};

const useAssetTypes = (): AssetTypesState => {
  const [assetTypes, setAssetTypes] = useState<AssetTypesState>(AssetTypesState.NotLoaded());

  useEffect(() => {
    setAssetTypes(AssetTypesState.Loading());
    fetchAssetTypes()
      .then(assetTypes => setAssetTypes(AssetTypesState.Loaded(assetTypes)))
      .catch(error => setAssetTypes(AssetTypesState.Error(error)));
  }, []);

  return assetTypes;
}

const fetchAssetTypes = async () => {
  const hermesClient = new HermesClient("https://hermes.pyth.network");
  const feeds = await hermesClient.getPriceFeeds();
  return [...new Set(feeds.map(feed => feed.attributes.asset_type ?? ""))];
};

enum AssetTypesStateType {
  NotLoaded,
  Loading,
  Loaded,
  Error
}

const AssetTypesState = {
  NotLoaded: () => ({ type: AssetTypesStateType.NotLoaded as const }),
  Loading: () => ({ type: AssetTypesStateType.Loading as const }),
  Loaded: (assetTypes: string[]) => ({ type: AssetTypesStateType.Loaded as const, assetTypes }),
  Error: (error: unknown) => ({ type: AssetTypesStateType.Error as const, error }),
}

type AssetTypesState = typeof AssetTypesState[keyof typeof AssetTypesState];

Copy link
Member Author

Choose a reason for hiding this comment

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

Replying to point 3.
I am using PriceFeeds variable to filter here

    const matchesSearch =
      feed.id.toLowerCase().includes(searchTerm.toLowerCase())

and then to display the data. What should be the best way around?

Copy link
Contributor

@cprussin cprussin Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, apologies, I was so focused on the asset types that I didn't think about the other usage of the feeds. In my suggested code, just replace fetchAssetTypes with:

const fetchPriceFeeds = async () => {
  const priceFeeds = await new HermesClient("https://hermes.pyth.network").getPriceFeeds();
  const assetTypes = [...new Set(priceFeeds.map(feed => feed.attributes.asset_type ?? ""))];
  return { priceFeeds, assetTypes };
};

Then update variable names etc as appropriate to reflect that the state is storing the price feeds and not just asset types (e.g. rename useAssetTypes to usePriceFeeds). Then update the component to e.g.:

export const PriceFeedTable = () => {
  const [selectedAssetType, setSelectedAssetType] = useState("All");
  const priceFeedsState = usePriceFeeds();

  // ...
  <AssetTypesSelect
    priceFeedsState={priceFeedsState}
    selectedAssetType={selectedAssetType}
    setSelectedAssetType={setSelectedAssetType}
  />
}

type AssetTypesSelectProps = {
  priceFeedsState: PriceFeedsState;
  selectedAssetType: string;
  setSelectedAssetType: (value: string) => void;
}

const AssetTypesSelect = ({ priceFeedsState, selectedAssetType, setSelectedAssetType }: AssetTypesSelectProps) => {
  switch (priceFeedsState.type) {
    // ...
  }
}

Let me know if this makes sense.


const filteredData = priceFeeds.filter((feed) => {
const matchesSearch =
feed.id.toLowerCase().includes(searchTerm.toLowerCase()) ||
feed.name.toLowerCase().includes(searchTerm.toLowerCase());
const matchesAssetType =
selectedAssetType === "All" || feed.assetType === selectedAssetType;
return matchesSearch && matchesAssetType;
});

const handleCopy = (id: string) => {
copy(id);
setOpenSnackbar(true);
};

return (
<Box>
<Box sx={{ mb: 3, display: "flex", gap: 2 }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

The Box component isn't useful with tailwind; these styles can be directly added to a div by using the correct utility classes, e.g. something like <div className="mb-1 flex gap-2">

<TextField
label="Search price feeds"
variant="outlined"
size="small"
value={searchTerm}
onChange={(e) => setSearchTerm(e.target.value)}
sx={{ width: 300 }}
/>
<Select
value={selectedAssetType}
onChange={(e) => setSelectedAssetType(e.target.value)}
size="small"
sx={{ width: 200 }}
>
<MenuItem value="All">All Asset Types</MenuItem>
{Array.from(new Set(priceFeeds.map((feed) => feed.assetType))).map(
(type) => (
<MenuItem key={type} value={type}>
{type || "Uncategorized"}
</MenuItem>
)
)}
</Select>
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 a great reason to use MUI for text fields or selects; we already have other components in the codebase for both cases, see e.g. https://docs.pyth.network/price-feeds/api-reference/cosmwasm/query-price-feed and the components on that page (Selector.tsx for the select and Input.tsx for the text input)

</Box>

<Table>
<TableHead>
<TableRow>
<TableCell>Ticker</TableCell>
<TableCell>Asset Type</TableCell>
<TableCell>Feed ID</TableCell>
</TableRow>
</TableHead>
<TableBody>
{filteredData.map((feed) => (
<TableRow key={feed.id}>
<TableCell>{feed.name}</TableCell>
<TableCell>{feed.assetType}</TableCell>
<TableCell
onClick={() => handleCopy(feed.id)}
sx={{
cursor: "pointer",
"&:hover": {
backgroundColor: "#BB86FC",
},
}}
>
{feed.id}
</TableCell>
</TableRow>
))}
</TableBody>
</Table>

<Snackbar
open={openSnackbar}
autoHideDuration={2000}
onClose={() => setOpenSnackbar(false)}
message="Feed ID copied to clipboard"
/>
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 avoid having to use this component by using the CopyToClipboard nextra component which has feedback built in and is the same component that's used for code block copying

</Box>
);
}
Loading
Loading