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

Added limit as a configurable variable to DeployablePredictionAgent #532

Merged
merged 8 commits into from
Oct 29, 2024

Conversation

gabrielfior
Copy link
Contributor

No description provided.

Copy link

coderabbitai bot commented Oct 28, 2024

Caution

Review failed

The pull request is closed.

Walkthrough

The changes in this pull request modify the DeployablePredictionAgent and DeployableTraderAgent classes in the prediction_market_agent_tooling/deploy/agent.py file. A new variable, n_markets_to_fetch, is introduced to enhance market fetching flexibility. The get_markets method is updated to accept a None limit, defaulting to this new variable. Additionally, the process_market method in DeployableTraderAgent is enhanced to include trade execution logic, logging, and conditional trade storage. Changes are also made to the APIKeys class in config.py and the import structure in various files, reflecting a transition to the safe_eth library.

Changes

File Path Change Summary
prediction_market_agent_tooling/deploy/agent.py - Added variable n_markets_to_fetch: int = MAX_AVAILABLE_MARKETS in DeployablePredictionAgent.
- Updated get_markets method to allow limit to be None, defaulting to self.n_markets_to_fetch.
- Modified process_market method in DeployableTraderAgent to include trade execution logic and logging.
- Updated after_process_market method to conditionally store trades based on store_trades.
prediction_market_agent_tooling/config.py - Updated check_if_is_safe_owner method to use SafeV141 instead of Safe for instantiation.
- Updated import paths for EthereumClient and SafeV141.
prediction_market_agent_tooling/tools/safe.py - Updated imports from gnosis to safe_eth for various Ethereum client interactions.
- Modified create_safe function to use SafeV141 instead of Safe and added type annotation for expected_safe_address as ChecksumAddress.
prediction_market_agent_tooling/tools/web3_utils.py - Updated import for Safe to SafeV141 from safe_eth.safe.
tests_integration_with_local_chain/safe/conftest.py - Updated test_safe fixture to return SafeV141 instead of Safe.
tests_integration_with_local_chain/markets/omen/test_omen.py - Skipped multiple tests due to local chain functionality issues and added comments for required adjustments.
tests_integration_with_local_chain/safe/test_erc20_transfer_using_safe.py - Updated import for Safe to SafeV141 from safe_eth.safe.
tests_integration_with_local_chain/safe/test_safe.py - Updated parameter types in test functions to use SafeV141 instead of Safe.

Possibly related PRs

Suggested reviewers

  • kongzii

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
prediction_market_agent_tooling/deploy/agent.py (1)

374-374: LGTM! Consider relocating the comment.

The changes to get_markets improve flexibility by allowing runtime override of the fetch limit while maintaining good defaults. However, the comment "Fetch the soonest closing markets to choose from" would be more appropriate above the available_markets assignment since it describes the sorting behavior.

        cls = market_type.market_class
-        # Fetch the soonest closing markets to choose from
        limit = limit if limit else self.n_markets_to_fetch
+        # Fetch the soonest closing markets to choose from
        available_markets = cls.get_binary_markets(
            limit=limit, sort_by=sort_by, filter_by=filter_by
        )

Also applies to: 380-380

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e9bce61 and df2915d.

⛔ Files ignored due to path filters (2)
  • poetry.lock is excluded by !**/*.lock, !**/*.lock
  • pyproject.toml is excluded by !**/*.toml
📒 Files selected for processing (1)
  • prediction_market_agent_tooling/deploy/agent.py (2 hunks)
🔇 Additional comments (1)
prediction_market_agent_tooling/deploy/agent.py (1)

282-282: LGTM! Well-placed configuration variable.

The addition of n_markets_to_fetch as a class variable improves configurability while maintaining backward compatibility with the default value of MAX_AVAILABLE_MARKETS.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
tests_integration_with_local_chain/safe/conftest.py (1)

Safe import inconsistency found across the codebase

The codebase currently has mixed imports for the Safe class:

  • Most files use from gnosis.safe import Safe
  • One file uses from gnosis.safe.safe import Safe
  • One file uses from safe_eth.safe import Safe

Files to update for consistency:

  • prediction_market_agent_tooling/tools/safe.py (using safe_eth.safe)
  • prediction_market_agent_tooling/tools/web3_utils.py (using gnosis.safe.safe)
  • All other files use gnosis.safe
🔗 Analysis chain

Line range hint 3-4: Verify Safe import source consistency.

The AI summary indicates that Safe imports were changed from 'gnosis.safe.safe' to 'safe_eth.safe' across the codebase, but this file still imports from 'gnosis.safe'. This inconsistency might lead to type checking issues.

Let's verify the Safe import usage across the codebase:

Also applies to: 35-35

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check Safe import patterns across the codebase
# Expected: Consistent use of either 'gnosis.safe' or 'safe_eth.safe'

echo "Checking Safe imports..."
rg -l "from .*safe import Safe" 

echo -e "\nChecking Safe usage patterns..."
ast-grep --pattern 'from $_ import Safe'

Length of output: 1046

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between df2915d and c209942.

📒 Files selected for processing (4)
  • prediction_market_agent_tooling/config.py (1 hunks)
  • prediction_market_agent_tooling/tools/safe.py (4 hunks)
  • prediction_market_agent_tooling/tools/web3_utils.py (1 hunks)
  • tests_integration_with_local_chain/safe/conftest.py (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • prediction_market_agent_tooling/tools/web3_utils.py
🔇 Additional comments (5)
tests_integration_with_local_chain/safe/conftest.py (1)

35-35: LGTM: Type ignore removal.

The removal of the type ignore comment suggests that the underlying type issue has been resolved, which improves code quality.

prediction_market_agent_tooling/tools/safe.py (3)

91-91: Clean type handling improvement.

Good improvement removing the type ignore comment. The code is now properly typed with the new safe_eth library.


116-118: Good addition of explicit type safety.

The explicit ChecksumAddress type annotation improves code safety and clarity.


131-135: Verify impact of return type change.

The new return statement is more robust by ensuring checksum address format and explicit None handling. However, this change might affect existing callers.

✅ Verification successful

Return type change is safe to proceed

The return type change is safe because:

  1. All callers already handle the None case correctly (e.g., create_safe_for_agent.py checks if not safe_address)
  2. The address is consistently converted to checksum format in all places where it's used (e.g., in bet_omen.py, sell_all_omen.py, etc.)
  3. The test in conftest.py explicitly asserts for non-None value before usage
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check usage of create_safe function to verify compatibility with new return type

# Find direct usage of create_safe function
echo "Checking direct create_safe usage..."
ast-grep --pattern 'create_safe($$$)'

# Check for potential address comparisons that might be affected by checksum conversion
echo "Checking address comparisons..."
rg -A 2 "create_safe\(.*\)" 

Length of output: 1335


Script:

#!/bin/bash
# Let's check how the return value of create_safe is used in the calling code

# Check the usage context in test files
echo "Checking usage in tests..."
rg -B 2 -A 4 "safe_address = create_safe" tests_integration_with_local_chain/

# Check the usage in scripts
echo "Checking usage in scripts..."
rg -B 2 -A 4 "safe_address = create_safe" scripts/

# Check if there are any address comparisons or validations done on the safe_address
echo "Checking address handling..."
rg -A 2 "safe_address" tests_integration_with_local_chain/ scripts/

Length of output: 7016

prediction_market_agent_tooling/config.py (1)

203-203: LGTM! Type safety improvements.

The changes improve type safety by:

  1. Removing the type ignore comment, indicating better type compatibility
  2. Adding explicit boolean conversion to ensure a strict boolean return type

Let's verify the type compatibility of the Safe instantiation:

Also applies to: 205-205

✅ Verification successful

Type safety improvements verified successfully

The verification confirms:

  • No remaining type-ignore comments related to Safe instantiation
  • Consistent Safe imports across the codebase (from gnosis.safe import Safe)
  • The import pattern is used correctly in both production code (prediction_market_agent_tooling/config.py) and test files
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining type ignore comments related to Safe instantiation
# and verify the import changes across the codebase

# Test 1: Check for any remaining type ignore comments related to Safe
rg -i "# type.*ignore.*safe"

# Test 2: Verify Safe import consistency
rg "from gnosis.safe import Safe"

Length of output: 403

prediction_market_agent_tooling/tools/safe.py Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (4)
tests_integration_with_local_chain/safe/conftest.py (1)

Line range hint 17-35: Consider documenting Safe version selection

The fixture is using a specific Safe contract version (V141) but lacks documentation explaining why this version was chosen.

Add a docstring explaining the version selection:

 @pytest.fixture(scope="module")
 def test_safe(local_web3: Web3, test_keys: APIKeys) -> SafeV141:
+    """Creates a test Safe instance using SafeV141.
+    
+    SafeV141 is used because [explain version selection rationale here].
+    """
     web3 = local_web3
scripts/create_safe_for_agent.py (1)

Line range hint 19-29: Consider enhancing security measures.

While the script is well-structured, consider these security improvements:

  1. Use environment variables or a secure config file for private keys instead of CLI arguments
  2. Validate the RPC URL to prevent potential security risks
  3. Add an upper limit validation for fund_amount_xdai to prevent accidental large transfers

Here's a suggested implementation:

def validate_rpc_url(url: str) -> bool:
    """Validate RPC URL against allowed patterns/domains."""
    allowed_domains = ['gnosis.io', 'infura.io']  # Add your allowed domains
    from urllib.parse import urlparse
    parsed = urlparse(url)
    return any(domain in parsed.netloc for domain in allowed_domains)

def create_safe_for_agent(
    from_private_key: str = typer.Option(None, help="Private key (prefer using AGENT_PRIVATE_KEY env var)"),
    rpc_url: str | None = None,
    salt_nonce: int | None = None,
    fund_safe: bool = True,
    fund_amount_xdai: int = typer.Option(1, max=10, help="Amount to fund (max 10 xDAI)"),
) -> None:
    """Create and optionally fund a Safe for an agent.
    
    Private key can be provided via AGENT_PRIVATE_KEY environment variable (recommended)
    or --from-private-key option (less secure).
    """
    from_private_key = from_private_key or os.getenv('AGENT_PRIVATE_KEY')
    if not from_private_key:
        raise ValueError("Private key must be provided via AGENT_PRIVATE_KEY env var or --from-private-key option")
    
    if rpc_url and not validate_rpc_url(rpc_url):
        raise ValueError("Invalid or disallowed RPC URL")
tests_integration_with_local_chain/safe/test_safe.py (1)

50-50: LGTM! Consider enhancing test documentation.

The type annotation change to SafeV141 is correct. The test provides comprehensive coverage of safe contract interactions.

Consider adding a docstring to better document the test's purpose and the complex interaction flow with the Omen market:

def test_send_function_on_contract_tx_using_safe(
    local_ethereum_client: EthereumClient,
    local_web3: Web3,
    test_keys: APIKeys,
    test_safe: SafeV141,
) -> None:
    """
    Tests the safe's ability to execute contract transactions with Omen market.
    
    Verifies:
    1. Safe funding with xDAI
    2. Market liquidity requirements
    3. Token balance changes after transaction
    """
prediction_market_agent_tooling/config.py (1)

203-203: LGTM! Consider documenting the SafeV141 version requirement.

The change to SafeV141 is correct and aligns with the migration to safe-eth-py. Consider adding a comment or documentation note about the minimum required version of safe-eth-py to help future maintainers.

Add a comment above the class to document the version requirement:

# Requires safe-eth-py >= 6.0.0b41 for SafeV141 support
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c209942 and 3d8cfab.

⛔ Files ignored due to path filters (2)
  • poetry.lock is excluded by !**/*.lock, !**/*.lock
  • pyproject.toml is excluded by !**/*.toml
📒 Files selected for processing (10)
  • prediction_market_agent_tooling/config.py (2 hunks)
  • prediction_market_agent_tooling/deploy/agent.py (2 hunks)
  • prediction_market_agent_tooling/tools/safe.py (2 hunks)
  • prediction_market_agent_tooling/tools/web3_utils.py (2 hunks)
  • scripts/create_safe_for_agent.py (1 hunks)
  • tests_integration_with_local_chain/conftest.py (1 hunks)
  • tests_integration_with_local_chain/markets/omen/test_omen.py (0 hunks)
  • tests_integration_with_local_chain/safe/conftest.py (3 hunks)
  • tests_integration_with_local_chain/safe/test_erc20_transfer_using_safe.py (1 hunks)
  • tests_integration_with_local_chain/safe/test_safe.py (3 hunks)
💤 Files with no reviewable changes (1)
  • tests_integration_with_local_chain/markets/omen/test_omen.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • prediction_market_agent_tooling/tools/web3_utils.py
🔇 Additional comments (14)
tests_integration_with_local_chain/safe/conftest.py (3)

17-17: Type safety improvement with explicit version

The return type change from Safe to SafeV141 improves type safety by being explicit about the Safe contract version being used.


35-35: Verify SafeV141 compatibility

The instantiation has been updated to use SafeV141. While this is correct, we should ensure this version is compatible with your deployment environment.

Let's check for version-specific dependencies or configurations:

#!/bin/bash
# Look for SafeV141-specific configurations or version requirements
rg "SafeV141" -A 5 "requirements*.txt"
rg "SafeV141" "setup.py"
rg "safe.*version|SAFE.*VERSION" --type yaml

3-4: LGTM! Migration from gnosis.eth to safe_eth

The import changes reflect a clean migration to the new safe_eth library.

Let's verify this migration is consistent across the codebase:

✅ Verification successful

Migration from gnosis.eth to safe_eth is complete and consistent

The verification shows that:

  • All relevant files consistently use safe_eth imports
  • No remaining gnosis.eth imports were found
  • SafeV141 is consistently used across the codebase in:
    • Test configurations and tests
    • Core tooling and configuration files
    • Web3 utilities
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any remaining gnosis.eth imports that might need updating
rg "from gnosis\.eth" 

# Verify the new imports are used consistently
rg "from safe_eth\.eth import EthereumClient"
rg "from safe_eth\.safe\.safe import SafeV141"

Length of output: 309


Script:

#!/bin/bash
# Let's try a more comprehensive search for both old and new patterns

# Check for any remaining gnosis.eth imports
rg -l "gnosis\.eth"

# Check for safe_eth imports
rg -l "safe_eth"

# Look specifically for Safe/SafeV141 usage
rg -l "SafeV141"
rg -l "from.*Safe import Safe"

# Check Python files in the test directory for context
fd -e py -p "**/test*/**"

Length of output: 1063


Script:

#!/bin/bash
# Let's examine the content of files that use SafeV141 to verify consistent migration

# Check the content of files using SafeV141
rg "from.*safe_eth" -A 2 tests_integration_with_local_chain/safe/conftest.py
rg "from.*safe_eth" -A 2 tests_integration_with_local_chain/safe/test_safe.py
rg "from.*safe_eth" -A 2 prediction_market_agent_tooling/config.py
rg "from.*safe_eth" -A 2 prediction_market_agent_tooling/tools/safe.py
rg "from.*safe_eth" -A 2 prediction_market_agent_tooling/tools/web3_utils.py

# Check for any remaining old Safe imports or usage
rg "from.*gnosis\.eth.*Safe" -A 2
rg "Safe\(" -A 1

Length of output: 1146

scripts/create_safe_for_agent.py (1)

7-7: LGTM! Verify API compatibility with safe_eth.

The import change aligns with the codebase-wide migration from gnosis to safe_eth library.

Let's verify that the EthereumClient maintains the same interface:

✅ Verification successful

Import change is safe and consistent with codebase-wide migration

The verification confirms:

  • The EthereumClient API is consistently used across the codebase with the same interface patterns (get_balance, is_contract, get_network, etc.)
  • The migration from gnosis to safe_eth is thorough, with multiple files already using the new imports
  • No remaining gnosis.eth imports found, only references to Gnosis documentation and RPC URLs
  • The safe_eth library is well-integrated across test and production code with consistent usage patterns
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify EthereumClient usage and interface compatibility

# Test 1: Check for any additional EthereumClient imports that might need updating
rg -l 'from gnosis\.eth import.*EthereumClient'

# Test 2: Compare method usage patterns between old and new implementations
rg -A 2 'EthereumClient\(' 

Length of output: 1438


Script:

#!/bin/bash
# Let's check the actual usage patterns of EthereumClient to verify API compatibility

# Check EthereumClient method calls in both old and new implementations
rg -A 3 'ethereum_client\.' 

# Check imports to see if we missed any files still using gnosis
rg 'from.*gnosis' 

# Look for any Safe-related imports to understand the migration context
rg 'from.*safe_eth.*import'

Length of output: 8973

tests_integration_with_local_chain/safe/test_erc20_transfer_using_safe.py (2)

Line range hint 15-69: Test implementation looks robust and comprehensive.

The test function thoroughly validates the ERC20 token operations with Safe contract, including:

  • Token deposits
  • Transfers to/from Safe
  • Approval mechanisms
  • Balance verifications
  • Withdrawal functionality

The unchanged test implementation suggests that the safe_eth.Safe class maintains API compatibility with the previous gnosis.safe.Safe class.


3-3: Verify Safe class compatibility across the codebase.

The change from gnosis.safe to safe_eth.safe appears to be part of a broader migration. Let's verify that this change is consistent across the codebase and that the new Safe class maintains API compatibility.

✅ Verification successful

Let me gather more information about the Safe class usage to better understand the migration and API compatibility.


Let me check the dependencies and any other Safe-related implementations to ensure we have a complete picture.


Import change from gnosis.safe to safe_eth.safe is correct and consistent

The migration from gnosis.safe to safe_eth.safe is verified to be correct based on the following evidence:

  1. The project explicitly depends on safe-eth-py = "^6.0.0b41" in pyproject.toml
  2. No remaining references to gnosis.safe were found in the codebase
  3. The Safe implementation in tools/safe.py consistently uses the new safe_eth imports
  4. The test file's usage of Safe class matches the API provided by safe-eth-py
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent usage of safe_eth.safe across the codebase
# and check for any remaining gnosis.safe imports that might have been missed.

echo "Checking for any remaining gnosis.safe imports..."
rg "from gnosis.safe import"

echo "Verifying consistent safe_eth.safe usage..."
rg "from safe_eth.safe import"

echo "Looking for potential Safe class usage patterns to verify API compatibility..."
ast-grep --pattern 'Safe($$$)'

Length of output: 574


Script:

#!/bin/bash
# Let's examine the actual usage of Safe class in the test file
echo "Checking Safe class usage in the test file:"
cat tests_integration_with_local_chain/safe/test_erc20_transfer_using_safe.py

# Let's also check for any other files that might be using Safe
echo -e "\nChecking for other Safe class references:"
rg "Safe\." -A 2

# Look for potential configuration or setup files
echo -e "\nChecking for potential requirements or dependency files:"
fd "requirements|setup.py|pyproject.toml"

Length of output: 3791


Script:

#!/bin/bash
# Check pyproject.toml for dependencies
echo "Checking dependencies in pyproject.toml:"
cat pyproject.toml

# Check Safe implementation in the tools
echo -e "\nChecking Safe implementation in tools:"
cat prediction_market_agent_tooling/tools/safe.py

# Look for test fixtures that might be using Safe
echo -e "\nChecking for test fixtures:"
rg "def test_safe" -A 3

Length of output: 7970

tests_integration_with_local_chain/conftest.py (2)

Line range hint 47-48: Review EthereumClient initialization.

The local_ethereum_client fixture creates an empty EthereumClient() instance without passing the web3 instance from the dependency. This might cause issues if the client requires initialization parameters.

#!/bin/bash
# Description: Check the EthereumClient usage pattern in tests and implementation

# Check EthereumClient initialization patterns
ast-grep --pattern 'EthereumClient($$$)'

# Check if there are any test files using this fixture
rg -A 3 "local_ethereum_client"

Consider updating the initialization to:

 @pytest.fixture(scope="module")
 def local_ethereum_client(local_web3: Web3) -> EthereumClient:
-    return EthereumClient()
+    return EthereumClient(local_web3)

10-10: Verify the safe-eth library migration across the codebase.

The import change from gnosis.eth to safe_eth.eth looks good, but let's ensure all related imports are consistently updated.

✅ Verification successful

Migration from gnosis.eth to safe_eth.eth is complete and consistent

The verification shows that:

  • No remaining gnosis.eth imports exist in the codebase
  • All safe_eth.eth imports are consistently used across the test files and main codebase
  • The EthereumClient is properly initialized with URI parameters in production code, while test fixtures use empty constructors which is a valid testing pattern
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining gnosis.eth imports that might have been missed
# and verify the new safe_eth.eth imports are used consistently.

echo "Checking for any remaining gnosis.eth imports..."
rg "from gnosis.eth import" 

echo "Verifying safe_eth.eth usage..."
rg "from safe_eth.eth import"

Length of output: 4076


Script:

#!/bin/bash
# Let's check for both import patterns and actual usage to get a complete picture

# Check for any remaining gnosis.eth imports across the codebase
echo "=== Checking gnosis.eth imports ==="
rg "from gnosis\." -A 1

# Check for safe_eth imports across the codebase
echo -e "\n=== Checking safe_eth imports ==="
rg "from safe_eth\." -A 1

# Check EthereumClient initialization patterns
echo -e "\n=== Checking EthereumClient initialization patterns ==="
ast-grep --pattern 'EthereumClient($$$)'

Length of output: 2856

tests_integration_with_local_chain/safe/test_safe.py (2)

34-34: LGTM! Type annotation correctly updated.

The type annotation change to SafeV141 aligns well with the version assertion in the test.


3-4: Verify safe_eth version requirements.

The migration from gnosis.eth to safe_eth library is a significant change. Please ensure that the project's dependency requirements are updated accordingly.

✅ Verification successful

Dependencies are properly specified with safe_eth_py version 6.0.0b41

The migration from gnosis.eth to safe_eth is properly handled in the project. The poetry.lock file shows that safe_eth_py version 6.0.0b41 is specified as a dependency, and the imports are consistently used across multiple files in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if requirements.txt or setup.py exists and contains safe_eth
fd -e txt -e py -x grep -l "safe_eth" {} \;

Length of output: 461


Script:

#!/bin/bash
# Check for dependency specifications in setup.py or requirements files
fd "setup.py|requirements" -x cat {} \;

# Check for safe_eth imports and their usage patterns
rg "safe_eth" -A 2 -B 2

Length of output: 4217

prediction_market_agent_tooling/tools/safe.py (1)

9-13: Verify safe_eth migration completeness.

The imports have been correctly updated to use safe_eth. However, as noted in previous reviews, several other files still need to be updated for consistency.

✅ Verification successful

Migration to safe_eth is complete and consistent

The verification shows that:

  • No remaining gnosis imports were found in the key files
  • safe_eth is consistently used across the codebase including:
    • Configuration (config.py)
    • Tools (web3_utils.py)
    • Scripts (create_safe_for_agent.py)
    • Tests (tests_integration_with_local_chain/safe/*)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for remaining gnosis imports and verify safe_eth adoption
# Expected: No gnosis imports in updated files, consistent safe_eth usage

# Check for any remaining gnosis imports
echo "Checking for remaining gnosis imports in key files..."
rg "from gnosis\.|import gnosis" \
   "prediction_market_agent_tooling/config.py" \
   "prediction_market_agent_tooling/tools/web3_utils.py" \
   "scripts/create_safe_for_agent.py" \
   "tests_integration_with_local_chain/safe/"

# Verify safe_eth adoption
echo "Verifying safe_eth adoption..."
rg "from safe_eth\.|import safe_eth" \
   "prediction_market_agent_tooling/config.py" \
   "prediction_market_agent_tooling/tools/web3_utils.py" \
   "scripts/create_safe_for_agent.py" \
   "tests_integration_with_local_chain/safe/"

Length of output: 1401

prediction_market_agent_tooling/config.py (1)

6-7: Verify consistent usage of safe_eth imports across the codebase.

The imports have been correctly updated from gnosis to safe_eth. Let's verify that this change has been applied consistently across all files.

✅ Verification successful

All gnosis imports have been successfully replaced with safe_eth

The verification shows:

  • No direct imports from gnosis package remain in the Python files
  • The only remaining references to "gnosis" are in test comments pointing to documentation
  • safe_eth imports are consistently used across the codebase in relevant files
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining gnosis imports that need to be updated

echo "Checking for remaining gnosis imports..."
rg "from gnosis\." --type py

echo -e "\nChecking for gnosis package usage..."
rg "gnosis\." --type py

echo -e "\nVerifying safe_eth imports are used..."
rg "from safe_eth\." --type py

Length of output: 2041

prediction_market_agent_tooling/deploy/agent.py (2)

282-282: LGTM! Good addition of the configurable limit.

The new class variable n_markets_to_fetch with a default value of MAX_AVAILABLE_MARKETS is well-placed and properly typed.


380-380: 🛠️ Refactor suggestion

Consider removing the limit parameter from the method signature.

As suggested in the previous review, it would be better to remove the limit parameter from the method signature and always use self.n_markets_to_fetch. This would make the configuration more consistent and prevent potential confusion from having two ways to set the limit.

Apply this change to the method signature and its usage:

    def get_markets(
        self,
        market_type: MarketType,
        sort_by: SortBy = SortBy.CLOSING_SOONEST,
        filter_by: FilterBy = FilterBy.OPEN,
-        limit: int | None = None,
    ) -> t.Sequence[AgentMarket]:
        cls = market_type.market_class
        # Fetch the soonest closing markets to choose from
        available_markets = cls.get_binary_markets(
            limit=self.n_markets_to_fetch, sort_by=sort_by, filter_by=filter_by
        )
        return available_markets

prediction_market_agent_tooling/tools/safe.py Show resolved Hide resolved
@@ -200,6 +200,6 @@ def check_if_is_safe_owner(self, ethereum_client: EthereumClient) -> bool:
if not self.SAFE_ADDRESS:
raise ValueError("Cannot check ownership if safe_address is not defined.")

s = Safe(self.SAFE_ADDRESS, ethereum_client) # type: ignore[abstract]
s = SafeV141(self.SAFE_ADDRESS, ethereum_client)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for using SafeV141 instead of just Safe? Is it safe ( 😁 )? Safe in a sense that what if the agent is using different version of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mypy complains because Safe is an abstract class.
SafeV141 is the latest version , and the one we create for all our agents (

)

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