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

Marketplace of tools (1/N) #237

Merged
merged 8 commits into from
Jun 7, 2024

Conversation

gabrielfior
Copy link
Contributor

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 applies

  • Non-breaking fix (non-breaking change which fixes an issue)
  • Breaking fix (breaking change which fixes an issue)
  • [x ] Non-breaking feature (non-breaking change which adds functionality)
  • Breaking feature (breaking change which adds functionality)
  • Refactor (non-breaking change which changes implementation)
  • Messy (mixture of the above - requires explanation!)

Checklist

Put an x in the boxes that apply.

  • [ x] I have read the CONTRIBUTING doc
  • [ x] I am making a pull request against the main branch (left side). Also you should start your branch off our main.
  • Lint and unit tests pass locally with my changes -> Lint throws error on file unrelated to this PR
  • [ x] I have added tests that prove my fix is effective or that my feature works

Comment on lines +7 to +11
aea_version: '>=1.0.0, <2.0.0'
fingerprint:
__init__.py: bafybeibbn67pnrrm4qm3n3kbelvbs3v7fjlrjniywmw2vbizarippidtvi
prediction_sum_url_content.py: bafybeieywowx265yycgf5735bw4zyabfy6ivwnntl6smxa2hicktipgeby
fingerprint_ignore_patterns: []
Copy link
Contributor Author

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not necessary

Comment on lines +20 to +23
"""
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.
"""
Copy link
Contributor Author

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.

Comment on lines 389 to 392
return error_response("No api key has been given.")

with OpenAIClientManager(api_key):
return transaction_builder(prompt)
Copy link
Contributor Author

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.

Copy link
Collaborator

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"}
Copy link
Contributor Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @0xArdi , added dependencies to more files (please see commit)

class TestOmenTransactionBuilder(BaseToolTest):
"""Test Prediction COT."""

os.environ["GNOSIS_RPC_URL"] = "https://gnosis-rpc.publicnode.com"
Copy link
Contributor Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

cc @0xArdi

Copy link
Collaborator

Choose a reason for hiding this comment

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

All good.

Copy link
Collaborator

@0xArdi 0xArdi left a 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.

Comment on lines +7 to +11
aea_version: '>=1.0.0, <2.0.0'
fingerprint:
__init__.py: bafybeibbn67pnrrm4qm3n3kbelvbs3v7fjlrjniywmw2vbizarippidtvi
prediction_sum_url_content.py: bafybeieywowx265yycgf5735bw4zyabfy6ivwnntl6smxa2hicktipgeby
fingerprint_ignore_patterns: []
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not necessary

Comment on lines 389 to 392
return error_response("No api key has been given.")

with OpenAIClientManager(api_key):
return transaction_builder(prompt)
Copy link
Collaborator

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"}
Copy link
Collaborator

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

class TestOmenTransactionBuilder(BaseToolTest):
"""Test Prediction COT."""

os.environ["GNOSIS_RPC_URL"] = "https://gnosis-rpc.publicnode.com"
Copy link
Collaborator

Choose a reason for hiding this comment

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

All good.

@gabrielfior
Copy link
Contributor Author

@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).
Could it be the case that the ENV OPENAI_API_KEY is not defined during CI test execution? The tox definition is here .
Also, the test that is failing passes locally.
Could you please provide guidance? Thanks.

Copy link
Collaborator

@0xArdi 0xArdi left a 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)
Copy link
Contributor Author

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

@gabrielfior
Copy link
Contributor Author

@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.

Thanks @0xArdi - refactored the logic to not expect GNOSIS_RPC_URL as environment variable, rather to fetch it from the api_keys (please see link).
Now I assume that a new secret (GNOSIS_RPC_URL) should be added to the Github Test runner (see change) for the unit tests to pass.

Copy link
Collaborator

@0xArdi 0xArdi left a 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.

@0xArdi 0xArdi merged commit 84b7148 into valory-xyz:main Jun 7, 2024
3 of 7 checks passed
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