Skip to content

[WIP] Porter update based on post-migration deprecation of duration when sampling#107

Draft
derekpierre wants to merge 3 commits intonucypher:developmentfrom
derekpierre:sampling-update
Draft

[WIP] Porter update based on post-migration deprecation of duration when sampling#107
derekpierre wants to merge 3 commits intonucypher:developmentfrom
derekpierre:sampling-update

Conversation

@derekpierre
Copy link
Copy Markdown
Member

@derekpierre derekpierre commented Mar 25, 2026

>>> Before merging we need to pin to nucypher:v7.7.x instead of cynusv's fork <<<

Type of PR:

  • Bugfix
  • Feature
  • Documentation
  • Other

Required reviews:

  • 1
  • 2
  • 3

What this does:

Updates Porter based on incompatible changes made to TACoApplication contract. duration is no longer a parameter when obtaining TACo active stakers.

Related to:

Issues fixed/closed:

  • Fixes #...

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network.
E.g., if this PR addresses a nucypher/productdev issue, let reviewers know!

Notes for reviewers:

What should reviewers focus on?
Is there a particular commit/function/section of your PR that requires more attention from reviewers?

@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.88%. Comparing base (bc1aee7) to head (c5996f7).

Additional details and impacted files
@@               Coverage Diff               @@
##           development     #107      +/-   ##
===============================================
- Coverage        90.91%   90.88%   -0.04%     
===============================================
  Files               18       18              
  Lines              969      965       -4     
===============================================
- Hits               881      877       -4     
  Misses              88       88              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates Porter to align with the post-migration TACoApplication contract change where duration is no longer used when sampling/obtaining active stakers.

Changes:

  • Remove duration from the GetUrsulas and BucketSampling schemas and from Porter’s python/web interface methods.
  • Update test fixtures and test suites to stop using duration.
  • Pin nucypher dependency to a different git commit/repo to pick up the upstream contract/API changes.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
porter/schema.py Removes duration from request schemas for get_ursulas and bucket_sampling.
porter/main.py Removes duration from Porter’s get_ursulas/bucket_sampling implementations and stops passing it to reservoir/agent calls.
porter/interfaces.py Removes duration from the control interface method signatures and implementer calls.
tests/conftest.py Updates mocks/fixtures to match the new agent method signatures (no duration).
tests/test_get_ursulas.py Removes schema/interface tests that exercised duration.
tests/test_bucket_sampling.py Removes schema/interface/web tests that exercised duration.
requirements.txt Changes nucypher dependency source/commit.
dev-requirements.txt Changes nucypher dependency source/commit for dev installs.
Comments suppressed due to low confidence (4)

porter/schema.py:366

  • Same as /get_ursulas: because BaseSchema includes unknown keys, an incoming request that still includes duration will be passed through and then forwarded into the interface/implementer call chain, but bucket_sampling(...) no longer accepts it. Add explicit handling (reject with InvalidInputData or drop the key) to prevent TypeError at runtime.
    min_version = VersionString(
        required=False,
        load_only=True,
        click=click.option(
            "--min-version",

tests/test_get_ursulas.py:52

  • With duration removed from the API surface, it would be good to keep a regression test asserting what happens when clients still send duration (e.g., explicit InvalidInputData with a helpful message, or that the param is ignored). Right now the duration-specific schema tests were removed, but there’s no coverage for the new behavior toward legacy inputs.
    # optional components

    # only exclude
    updated_data = dict(required_data)
    exclude_ursulas = []
    for i in range(2):
        exclude_ursulas.append(get_random_checksum_address())
    updated_data["exclude_ursulas"] = exclude_ursulas
    GetUrsulas().load(updated_data)

tests/test_bucket_sampling.py:66

  • Similar to test_get_ursulas_schema: duration-related tests were removed, but there’s no replacement coverage that asserts how legacy duration inputs are handled now (reject with a clear validation error vs silently ignored). Adding a targeted test here would prevent regressions into server-side TypeErrors when old clients still send duration.
    # optional components

    # only exclude
    updated_data = dict(required_data)
    exclude_ursulas = []
    for i in range(2):
        exclude_ursulas.append(get_random_checksum_address())
    updated_data["exclude_ursulas"] = exclude_ursulas
    BucketSampling().load(updated_data)

porter/schema.py:123

  • BaseSchema uses unknown = INCLUDE, so requests that still send the now-removed duration param will pass validation and then be forwarded to PorterInterface.get_ursulas(...), which no longer accepts duration, causing a runtime TypeError (likely a 500). Consider explicitly rejecting duration with a clear InvalidInputData message (or excluding unknown keys for this schema / popping duration during load) to avoid breaking older clients with an internal error.
    min_version = VersionString(
        required=False,
        load_only=True,
        click=click.option(
            "--min-version",
            "-mv",

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

multidict==6.7.0 ; python_version >= "3.10" and python_version < "4" and (implementation_name == "cpython" or implementation_name == "pypy")
nodeenv==1.10.0 ; python_version >= "3.10" and python_version < "4" and (implementation_name == "cpython" or implementation_name == "pypy")
nucypher @ git+https://github.com/nucypher/nucypher.git@b0f610cde06a9dfc79750d85f64bd78c5aa74f2a ; python_version >= "3.10" and python_version < "4" and (implementation_name == "cpython" or implementation_name == "pypy")
nucypher @ git+https://github.com/cygnusv/nucypher.git@361976c2cfef73b62c07fb78d4d915c7e8fef6b9 ; python_version >= "3.10" and python_version < "4" and (implementation_name == "cpython" or implementation_name == "pypy")
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

Same concern as requirements.txt: dev-requirements.txt now pins nucypher to cygnusv/nucypher. Please pin to the upstream repo (or a released version) to keep developer environments consistent and avoid relying on a fork for day-to-day development.

Suggested change
nucypher @ git+https://github.com/cygnusv/nucypher.git@361976c2cfef73b62c07fb78d4d915c7e8fef6b9 ; python_version >= "3.10" and python_version < "4" and (implementation_name == "cpython" or implementation_name == "pypy")
nucypher @ git+https://github.com/nucypher/nucypher.git ; python_version >= "3.10" and python_version < "4" and (implementation_name == "cpython" or implementation_name == "pypy")

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This will be fixed before merging this PR. We have to wait for nucypher/nucypher#3726 to be merged into v7.7.x before we can properly pin to the upstream repos.

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