-
Notifications
You must be signed in to change notification settings - Fork 34
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
Marketplace of tools (1/N) #237
Marketplace of tools (1/N) #237
Conversation
aea_version: '>=1.0.0, <2.0.0' | ||
fingerprint: | ||
__init__.py: bafybeibbn67pnrrm4qm3n3kbelvbs3v7fjlrjniywmw2vbizarippidtvi | ||
prediction_sum_url_content.py: bafybeieywowx265yycgf5735bw4zyabfy6ivwnntl6smxa2hicktipgeby | ||
fingerprint_ignore_patterns: [] |
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.
This wasn't updated - should I generate new values? If so, how?
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.
It's not necessary
""" | ||
This module implements a tool which prepares a transaction for the transaction settlement skill. | ||
Please note that the gnosis safe parameters are missing from the payload, e.g., `safe_tx_hash`, `safe_tx_gas`, 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.
This file contains logic for buying and selling tokens on Omen.
return error_response("No api key has been given.") | ||
|
||
with OpenAIClientManager(api_key): | ||
return transaction_builder(prompt) |
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.
This file is rather long, but I was unsure if splitting is encouraged, since other packages under customs
also contain large files.
If desired, happy to split this into multiple files for better readability.
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.
All good, its expected to be long, and cannot be split into multiple files atm. This is because these packages are loaded dynamically at runtime from IPFS.
@@ -74,6 +74,7 @@ replicate = "0.15.7" | |||
lxml = {extras = ["html-clean"], version = "^5.1.0"} | |||
langgraph = "==0.0.55" | |||
langchain-community = "==0.2.1" | |||
prediction-market-agent-tooling = {git = "https://github.com/gnosis/prediction-market-agent-tooling.git", rev = "gabriel/olas-deps"} |
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.
I wasn't sure this is the proper place for adding a new dependency required by the new tool - please advise.
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.
We might also need to add it to the tool's component.yaml dependencies. I'll let @0xArdi chime in.
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.
Check the dependencies section here
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.
tests/test_tools.py
Outdated
class TestOmenTransactionBuilder(BaseToolTest): | ||
"""Test Prediction COT.""" | ||
|
||
os.environ["GNOSIS_RPC_URL"] = "https://gnosis-rpc.publicnode.com" |
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.
ENV var needed for tool to be executed - will also be required during regular (non-test) execution.
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.
cc @0xArdi
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.
All good.
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.
Thanks @gabrielfior changes look great!
Just a small comment on the dependencies part. LMK if you have any questions.
aea_version: '>=1.0.0, <2.0.0' | ||
fingerprint: | ||
__init__.py: bafybeibbn67pnrrm4qm3n3kbelvbs3v7fjlrjniywmw2vbizarippidtvi | ||
prediction_sum_url_content.py: bafybeieywowx265yycgf5735bw4zyabfy6ivwnntl6smxa2hicktipgeby | ||
fingerprint_ignore_patterns: [] |
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.
It's not necessary
return error_response("No api key has been given.") | ||
|
||
with OpenAIClientManager(api_key): | ||
return transaction_builder(prompt) |
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.
All good, its expected to be long, and cannot be split into multiple files atm. This is because these packages are loaded dynamically at runtime from IPFS.
@@ -74,6 +74,7 @@ replicate = "0.15.7" | |||
lxml = {extras = ["html-clean"], version = "^5.1.0"} | |||
langgraph = "==0.0.55" | |||
langchain-community = "==0.2.1" | |||
prediction-market-agent-tooling = {git = "https://github.com/gnosis/prediction-market-agent-tooling.git", rev = "gabriel/olas-deps"} |
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.
Check the dependencies section here
tests/test_tools.py
Outdated
class TestOmenTransactionBuilder(BaseToolTest): | ||
"""Test Prediction COT.""" | ||
|
||
os.environ["GNOSIS_RPC_URL"] = "https://gnosis-rpc.publicnode.com" |
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.
All good.
@0xArdi As can be seen in the checks above, the test that I wrote () is still not passing during CI execution (please see relevant error message here). |
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.
@gabrielfior all the API keys are provided to the tool via the kwargs. In case you need any of them as enviroment variables, you will need to add them to the env in the tool itself. The mech does not provide any API keys as env vars.
if api_key is None: | ||
return error_response("No api key has been given.") | ||
|
||
gnosis_rpc_url: str | None = kwargs.get("api_keys", {}).get("gnosis_rpc_url", None) |
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.
Here is where I fetch gnosis_rpc_url from the api_keys
Thanks @0xArdi - refactored the logic to not expect |
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.
Thanks @gabrielfior , merging this now. Will address the RPC in a separate PR.
Proposed changes
Added tool for building buyTokens/sellTokens txParams.
Types of changes
What types of changes does your code introduce? (A breaking change is a fix or feature that would cause existing functionality and APIs to not work as expected.)
Put an
x
in the box that appliesChecklist
Put an
x
in the boxes that apply.main
branch (left side). Also you should start your branch off ourmain
.