Skip to content

Conversation

@Sneagan
Copy link
Collaborator

@Sneagan Sneagan commented Dec 5, 2025

The project has old and non-standard formatting. Fix this.

@Sneagan Sneagan requested a review from mrose17 December 5, 2025 03:57
@github-actions
Copy link

github-actions bot commented Dec 5, 2025

[puLL-Merge] - brave-intl/ofac-sanctioned-digital-currency-addresses@27

Description

This PR is a significant code quality and tooling improvement to the OFAC sanctioned digital currency addresses repository. The changes focus on:

  1. Code formatting standardization: Reformatting all Python files using modern linting tools (ruff, black-style formatting)
  2. Type hints and safety: Adding type annotations and improving type checking with mypy
  3. Development tooling: Adding a comprehensive Python development setup including:
    • Poetry for dependency management
    • Pre-commit hooks for automated code quality checks
    • Testing infrastructure (pytest, coverage)
    • Security scanning (bandit)
    • Dead code detection (vulture)
    • Type checking (mypy)
  4. Import organization: Standardizing import ordering and removing unused imports
  5. String formatting: Converting to f-strings and improving string concatenation
  6. Bug fixes: Fixing type checking issues (e.g., isinstance(args.format, str) instead of type(args.format) == str)

Possible Issues

  1. Large poetry.lock file: The 1698-line poetry.lock file adds significant repository bloat. Consider using .gitignore to exclude it if reproducible builds aren't critical, or document why it's committed.

  2. Removed webdriver_manager dependency: The import from webdriver_manager.chrome import ChromeDriverManager was removed from ofac_scraper.py, but it's unclear if this dependency is still needed elsewhere or if Chrome driver management changed.

  3. Breaking change in exception handling: In ofac_scraper.py, the exception handling changed from bare except TimeoutException: to except TimeoutException as e:, which should be fine, but the raised exception now chains with from e - this could affect error handling upstream.

  4. Percentage calculation in update_s3_objects.py: The line break in the percentage calculation could cause issues if the calculation spans multiple lines incorrectly:

f"{total_actions} ({(
    created_count + removed_count + error_count
) / total_actions * 100:.1f}%)"

Security Hotspots

  1. Selenium without explicit ChromeDriver management (Medium Risk): The removal of webdriver_manager imports and the Service import reordering in ofac_scraper.py suggests ChromeDriver management may have changed. If the driver binary isn't properly validated, this could introduce supply chain risks. Ensure ChromeDriver is obtained from trusted sources.

  2. Environment variable check for bypass logic (Medium Risk - update_s3_objects.py:341-351):

if os.getenv("GITHUB_ACTOR") not in ["mrose17", "Sneagan", "mschfh"]:

This hardcoded whitelist for bypassing the 15% deletion safety check relies on an environment variable that could be spoofed in some execution contexts. Consider using GitHub's OIDC tokens or signed commits for authentication instead.

  1. S3 upload without explicit encryption (Low-Medium Risk - update_s3_objects.py:154):
s3_client.upload_fileobj(Fileobj=empty_file, Bucket=bucket, Key=object_key)

No server-side encryption parameters are specified. While the objects are empty, consider adding ServerSideEncryption='AES256' for defense in depth.

Privacy Hotspots

None identified. The code handles publicly available sanctions data and doesn't process PII.

Changes

Changes

generate-address-list.py

  • Reformatted with black-style formatting (88 char line length)
  • Organized imports alphabetically
  • Converted string formatting to f-strings
  • Fixed type checking: isinstance() instead of type() ==
  • Added type hints implicitly through better code structure
  • Fixed bug on line 158: was checking isinstance(args.format, str) but variable should be args.assets

ofac_scraper.py

  • Reorganized imports (removed unused imports)
  • Changed variable naming from SCREAMING_CASE to snake_case for non-constants (MAX_RETRIESmax_retries)
  • Added exception chaining (raise ... from e)
  • Reformatted to black style
  • Improved string formatting with f-strings

update_s3_objects.py

  • Extensive reformatting to black style
  • Removed List type hint import in favor of built-in list
  • Improved string formatting and concatenation
  • Reorganized imports
  • Better type annotations
  • Clearer variable names and formatting

update_s3_objects_test.py

  • Added module docstring
  • Reorganized imports
  • Reformatted to black style
  • Improved type hints (List[str]list[str])
  • Better code organization and readability

poetry.lock (new file)

  • Complete dependency lock file with 1698 lines
  • Pins all direct and transitive dependencies
  • Includes dev dependencies for linting, testing, type checking

pyproject.toml (new file)

  • Poetry project configuration
  • Development tooling configuration (ruff, mypy, bandit, pytest, vulture)
  • Scripts for CLI entry points
  • Comprehensive dev dependency specifications
sequenceDiagram
    participant Dev as Developer
    participant PreCommit as Pre-commit Hooks
    participant Ruff as Ruff Linter
    participant Mypy as Type Checker
    participant Bandit as Security Scanner
    participant Tests as Pytest
    
    Dev->>PreCommit: git commit
    PreCommit->>Ruff: Check code style
    Ruff-->>PreCommit: ✓ Pass
    PreCommit->>Mypy: Type check
    Mypy-->>PreCommit: ✓ Pass
    PreCommit->>Bandit: Security scan
    Bandit-->>PreCommit: ✓ Pass
    PreCommit->>Tests: Run tests
    Tests-->>PreCommit: ✓ Pass
    PreCommit-->>Dev: Commit allowed
    
    Note over Dev,Tests: Main execution flow
    
    participant CLI as CLI Entry Point
    participant OFAC as OFAC Scraper
    participant S3 as S3 Sync
    
    CLI->>OFAC: Extract addresses from XML
    OFAC->>OFAC: Scrape SHA256 from website
    OFAC-->>CLI: Sanctioned addresses
    CLI->>S3: Sync to S3 bucket
    S3->>S3: Generate add/remove actions
    S3->>S3: Apply actions (with safety checks)
    S3-->>CLI: Sync complete
Loading

Copy link
Collaborator

@mrose17 mrose17 left a comment

Choose a reason for hiding this comment

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

Wow! Well, i've had to do this a few times as well... as long as the tests produce the same results before & after, it's good to go!

@Sneagan Sneagan merged commit e0ceffb into main Dec 5, 2025
13 checks passed
@Sneagan Sneagan deleted the fix/formatting branch December 5, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants