Skip to content

refactor: deduplicate package_hashes by reusing grey_state version#808

Open
wangzishuai1987 wants to merge 1 commit into
jarchain:masterfrom
wangzishuai1987:refactor/dedup-package-hashes
Open

refactor: deduplicate package_hashes by reusing grey_state version#808
wangzishuai1987 wants to merge 1 commit into
jarchain:masterfrom
wangzishuai1987:refactor/dedup-package-hashes

Conversation

@wangzishuai1987
Copy link
Copy Markdown

Summary

Contributes to #186 — eliminate code duplication across grey crates.

Two functions with identical core logic existed in separate crates:

Crate Function Difference
grey-state package_hashes(reports) Extract all hashes
grey-services accumulated_package_hashes(reports, count) Extract hashes from first count reports

Changes

  • Make grey_state::accumulate::package_hashes public
  • grey_services::accumulation::accumulated_package_hashes now delegates to grey_state::accumulate::package_hashes with a truncated slice

Testing

All existing tests pass. No behavior changes — pure refactoring.

@github-actions
Copy link
Copy Markdown
Contributor

Genesis Review

Comparison targets:

How to review

Post a comment with the following format (rank from best to worst):

/review
difficulty: <commit1>, <commit2>, ..., <commitN>, currentPR
novelty: <commit1>, <commit2>, ..., <commitN>, currentPR
design: <commit1>, <commit2>, ..., <commitN>, currentPR
verdict: merge

Use the short commit hashes above and currentPR for this PR.
Each line ranks all comparison targets + this PR from best to worst.

To meta-review another reviewer's comment, react with 👍 or 👎.

@sorpaas
Copy link
Copy Markdown
Contributor

sorpaas commented Apr 28, 2026

/review
difficulty: 313ee6b, 4b127bb, 606640e, 3c52fee, 0658d85, currentPR, 9d4407d, c1d7fdf
novelty: 313ee6b, 4b127bb, 606640e, 3c52fee, 0658d85, currentPR, 9d4407d, c1d7fdf
design: 313ee6b, 3c52fee, 0658d85, currentPR, 4b127bb, 606640e, 9d4407d, c1d7fdf
verdict: notMerge

Deduplicates accumulated_package_hashes by reusing the existing grey_state::accumulate::package_hashes (newly exposed pub) after truncating to count. Verified semantic equivalence: original iter().take(count) and new &reports[..count.min(reports.len())] both behave identically when count > reports.len() (both yield the full slice), and min rules out a panic on out-of-bounds indexing. Modest design win — replaces a 5-line closure with a one-line delegation. Above pure cleanup (9d4407d, c1d7fdf), well below the carry-flag fix (313ee6b) or the state-root dedup (3c52fee). Holding off on merge: Security audit failure is environmental (yanked deps), but combined with first-time contributor status, applying conservative scrutiny per /jar-review auto guidance.

@github-actions
Copy link
Copy Markdown
Contributor

JAR Bot: Review recorded from @sorpaas (1 reviews, 0 meta-reviews).
Merge weight: 0/37665 (need >50%).

@sorpaas
Copy link
Copy Markdown
Contributor

sorpaas commented Apr 28, 2026

/review
difficulty: 313ee6b, 4b127bb, 606640e, 3c52fee, 0658d85, currentPR, 9d4407d, c1d7fdf
novelty: 313ee6b, 4b127bb, 606640e, 3c52fee, 0658d85, currentPR, 9d4407d, c1d7fdf
design: 313ee6b, 3c52fee, 0658d85, currentPR, 4b127bb, 606640e, 9d4407d, c1d7fdf
verdict: merge

Updating to merge. Deduplicates accumulated_package_hashes by reusing the (now public) grey_state::accumulate::package_hashes after truncating to count. Verified semantic equivalence: original iter().take(count) and new &reports[..count.min(reports.len())] both yield the full slice when count > reports.len(); min rules out an OOB panic. Modest design win. No security implications.

@github-actions
Copy link
Copy Markdown
Contributor

JAR Bot: Quorum reached — triggering merge.
Reviews: 1, meta-reviews: 0.
Merge weight: 34219/37665 (>50%).

`grey_state::accumulate::package_hashes` and
`grey_services::accumulation::accumulated_package_hashes` had identical
core logic (extract package_hash from WorkReport slice). The only
difference was an optional `.take(count)` in the grey-services version.

Make `package_hashes` pub in grey-state and have grey-services delegate
to it with a truncated slice, eliminating the duplication.

Contributes to jarchain#186.
@wangzishuai1987 wangzishuai1987 force-pushed the refactor/dedup-package-hashes branch from fefea5e to 3985d51 Compare April 29, 2026 12:58
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.

2 participants