Skip to content

refactor: dedupe the 1_000_000 issue-ID bound in cli/issue_commands/helpers.py#873

Open
ebios-star wants to merge 1 commit intoentrius:testfrom
ebios-star:refactor/dedupe-issue-id-bound-in-cli-helpers
Open

refactor: dedupe the 1_000_000 issue-ID bound in cli/issue_commands/helpers.py#873
ebios-star wants to merge 1 commit intoentrius:testfrom
ebios-star:refactor/dedupe-issue-id-bound-in-cli-helpers

Conversation

@ebios-star
Copy link
Copy Markdown
Contributor

Summary

gittensor/cli/issue_commands/helpers.py currently declares the same bound twice inside the same file:

```python

Line 41 — module level, used by validate_issue_id

MAX_ISSUE_ID = 1_000_000
```

```python

Line 787 — function-local, inside _read_issues_from_child_storage

MAX_REASONABLE_ISSUE_ID = 1_000_000
if next_issue_id > MAX_REASONABLE_ISSUE_ID:
console.print(...'unreasonably large'...)
```

Both checks gate on the same value (`1_000_000`); they share intent ("how many on-chain issues do we ever expect?") but live as separate copies in the same module. A future bound change that touches one but not the other silently drifts: `validate_issue_id` would still cap user input at the old value while the storage sanity check warned at the new value, or vice versa.

This PR replaces the function-local constant with the existing module-level `MAX_ISSUE_ID`. Comparison operators are kept verbatim — `validate_issue_id` continues to reject `>= 1_000_000` (max valid input `999_999`) while the storage sanity check continues to warn on `> 1_000_000` — so the existing one-off difference between the two cap semantics is preserved on purpose.

Net: -1 line of source, single source of truth for the bound.

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Other

Testing

  • New `tests/cli/test_read_issues_sanity_bound.py` pins the sanity boundary at `MAX_ISSUE_ID`:
    • `next_issue_id == MAX_ISSUE_ID + 1` → triggers the sanity gate, returns `[]`, does not issue any per-issue `childstate_getStorage` RPC.
    • `next_issue_id == MAX_ISSUE_ID` → proceeds past the gate (verified by `rpc_request` being called for the per-issue lookup loop).
  • Existing `validate_issue_id` tests in `tests/cli/test_cli_helpers.py` (which import `MAX_ISSUE_ID` directly and assert `MAX_ISSUE_ID == 1_000_000`) are unchanged and continue to pass.

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • Changes are documented (if applicable)

…elpers.py

`gittensor/cli/issue_commands/helpers.py` declared the same bound twice
inside the same file:

- Module-level `MAX_ISSUE_ID = 1_000_000` (used by `validate_issue_id`)
- Function-local `MAX_REASONABLE_ISSUE_ID = 1_000_000` inside
  `_read_issues_from_child_storage` (used as a sanity ceiling on
  `next_issue_id` reported by the contract)

Both checks fire on `> 1_000_000` (the `>=` in `validate_issue_id` and
the `>` in the sanity check are intentionally one-off — the user-input
cap rejects 1_000_000 itself, the storage cap accepts it). They share
the same bound value, so a future change to one cap that forgets the
other would silently drift.

Drop the local re-declaration and reference the existing module-level
`MAX_ISSUE_ID` instead. Comparison operators stay verbatim, so the
warn-and-return-empty boundary is preserved.

Add `tests/cli/test_read_issues_sanity_bound.py` pinning the boundary:
- `next_issue_id == MAX_ISSUE_ID + 1` triggers the sanity gate, returns
  `[]`, and skips the per-issue childstate RPC loop entirely.
- `next_issue_id == MAX_ISSUE_ID` proceeds past the gate (proven by the
  per-issue rpc_request actually being called).
@ebios-star
Copy link
Copy Markdown
Contributor Author

Hi @anderdc @LandynDev — friendly review request whenever you have a moment.

Two same-value declarations of the issue-ID upper bound currently live in the same file (MAX_ISSUE_ID at module level + MAX_REASONABLE_ISSUE_ID re-declared inside _read_issues_from_child_storage). This PR drops the local re-declaration and reuses the module-level constant directly — the comparison operators stay verbatim so the existing one-off difference between the user-input cap (>= MAX_ISSUE_ID rejects 1,000,000) and the storage sanity cap (> MAX_ISSUE_ID accepts 1,000,000) is preserved on purpose.

Net: -1 line of source. New tests/cli/test_read_issues_sanity_bound.py pins the sanity boundary at the inclusive (==) and exclusive (+1) edges, so a future bound change updates both caps together.

Happy to address feedback or rebase if needed.

@xiao-xiao-mao xiao-xiao-mao Bot added the refactor Code restructuring without behavior change label Apr 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor Code restructuring without behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant