-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add coingecko #53
base: master
Are you sure you want to change the base?
Add coingecko #53
Conversation
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.
just some early comments
} | ||
} | ||
""" | ||
def getGlobalDefiData(): |
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.
would be good to use the pythonic style for functions here, just for consistency
'symbol': 'Puuvilla'}] | ||
""" | ||
def get_top_nft_by_24h_native_token(): | ||
url = f'https://api.coingecko.com/api/v3/nfts/list?order=h24_volume_native_desc' |
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.
maybe good to extract the base api url out into a constant?
@@ -141,6 +141,50 @@ Required parameters: | |||
Return value description: | |||
-JSON object with trait names and values of the NFT asset | |||
--- | |||
Widget magic command: <|fetch-nft-list-order-by-trading-volume-native-token()|> | |||
Description of widget: List all supported NFT, ordered by 24h trading volume of native token | |||
Connected wallet required: no |
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 i experimented with removing this in another diff to simplify wallet stuff, so we can drop this. but we should add a "Return value description" to any fetch-* commands, so the AI knows what to expect.
Also, this diff adds a lot more commands, I think the description of the widget needs to be very clear about what it is doing, so the tool knows when to pull up a given widget and use it. E.g. instead of market cap, it could state market capitalization. also trading volume of what tokens, etc.
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.
also, this might cause performance of other demo flows to drop, we will need to test it rigorously, or collapse into fewer, more specific use cases. not sure
response = requests.get(url) | ||
response.raise_for_status() | ||
result = response.json()['companies'] | ||
return ','.join(["{name} with total holding of {total_holdings}".format(name=item['name'],total_holdings=item['total_holdings']) for item in result]) |
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 these return comma-delimited text lists? or should we try to use table/list containers?
it would be helpful to have the return describe something like "An NFT collection with name {name} and address {address}", instead of just "{name} with address {address}". If you see the center.py integration, there could be some useful examples to follow for containers.
No description provided.