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

issue #867 2. added usd value using coingecko api #948

Merged
merged 4 commits into from
Dec 11, 2024

Conversation

sahil7886
Copy link
Contributor

Description

Implements 2. in Issue #867
Added functionality to display the USD value of the connected wallet's APT balance. Created a new hook, useGetAPTPrice.ts that uses the coingecko api to fetch the current price of APT in USD. The BalanceCard.tsx then uses that hook to fetch the current price, do a few calculations on it and displays it. The USD Value only shows if the user has selected the mainnet.

Related Links

#867

Checklist

Comment on lines 3 to 7
export async function getAPTPrice(): Promise<number> {
const query = {
ids: "aptos", // CoinGecko ID for Aptos
vs_currencies: "usd", // Target currency
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, we should make this getPrice and take in any coingecko Id.

Then we can use it for anything that has a coingecko ID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes a lot of sense. Changed that and set the default value of the argument to the aptos id, so as to not change existing implementations of the function.

return Number(data.aptos.usd);
} catch (error) {
console.error("Error fetching APT price from CoinGecko:", error);
return 0; // Default fallback price
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make the fallback null, so then we can handle elsewhere to hide the dollar amount if it doesn't apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@gregnazario
Copy link
Contributor

Let me know if don't feel like changing it, and I will follow up later about it

@@ -0,0 +1,30 @@
const COINGECKO_API_ENDPOINT = "https://api.coingecko.com/api/v3/simple/price";

export async function getAPTPrice(): Promise<number> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for contributing! Some food for thought!

This lives in the hooks folder, however this as implemented is not a hook. I like the idea you were going with of having a separate fetch function and hook (ie: getPrice and useGetPrice). Perhaps export both functions, and the caller can choose which function to use.

Bellow is a simple example of how you might approach this. Note, this repo uses react-query which helps manage caching and state. Your code in BalanceCard.tsx can be simplified o remove the use of useState if you use react-query

import { useQuery } from "@tanstack/react-query";

export function useGetPrice(id: string) {
  return useQuery({
    queryKey: ["price", id],
    queryFn: () => getAPTPrice(id),
  });
}

// No need for state!
const {data: price} = useGetPrice('coin_id')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. I've implemented the useGetPrice() hook using react query. Let me know if I've done it right and if any more changes need to made. I've also kept the getPrice() function there as it is. Would it make sense to add the getPrice() function to api/index.ts and keep just the hook in the useGetPrice.ts file?

Copy link
Contributor

@gregnazario gregnazario left a comment

Choose a reason for hiding this comment

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

Love it! Thank you. @kaw2k give it a second look

Copy link
Contributor

@kaw2k kaw2k left a comment

Choose a reason for hiding this comment

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

Nice work!

@gregnazario gregnazario merged commit fc0b65b into aptos-labs:main Dec 11, 2024
2 checks passed
@sahil7886 sahil7886 deleted the usd-val-change branch December 15, 2024 23:10
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