Skip to content

refactor: Enforcing strong typing in governance.rs - #4238

Merged
SimonRastikian merged 2 commits into
mainfrom
renaming-threshold
Aug 27, 2026
Merged

refactor: Enforcing strong typing in governance.rs#4238
SimonRastikian merged 2 commits into
mainfrom
renaming-threshold

Conversation

@SimonRastikian

Copy link
Copy Markdown
Contributor

Enforces strong typing of threshold in test function.
Part of #3903

@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown

Pull request overview

Test-only refactor in crates/contract/src/api/governance.rs that replaces the bare u64 threshold parameters of the setup_running_contract_with_domain test helper with the domain newtypes GovernanceThreshold and ReconstructionThreshold, and renames the threshold parameter of setup_voting_context_and_vote to governance_threshold. Part of #3903. No production code paths are touched.

Changes:

  • setup_running_contract_with_domain now takes GovernanceThreshold / ReconstructionThreshold instead of two positional u64s; the wrapping (GovernanceThreshold::new, ReconstructionThreshold::new) moves out to the five call sites.
  • setup_voting_context_and_vote's threshold parameter renamed to governance_threshold.
  • Doc comment on setup_running_contract_with_domain reworded to drop the now-removed parameter names.

Reviewed changes

Per-file summary
File Description
crates/contract/src/api/governance.rs Strongly types the two threshold parameters of the setup_running_contract_with_domain test helper and updates its five call sites; renames one parameter in setup_voting_context_and_vote; rewords one doc comment.

Findings

No blocking issues. Verified that GovernanceThreshold (crates/primitives/src/threshold.rs:29) and ReconstructionThreshold (crates/primitives/src/threshold.rs:62) are distinct newtypes with no From<ReconstructionThreshold> for GovernanceThreshold, so the two arguments can no longer be transposed silently — which is exactly the win this refactor buys. All five call sites preserve the original numeric values in the correct positional order (3/2/2, 4/3/3, 3/2/2, 5/4/2, 5/4/4), so no test semantics change.

Non-blocking (nits, follow-ups, suggestions):

  • crates/contract/src/api/governance.rs:256 — after the rewording, the doc comment no longer says anything the signature does not: num_participants -> "num_participants participants", governance_threshold -> "the given governance threshold", reconstruction_threshold -> "with the given reconstruction threshold". Per CLAUDE.md §Code Comments ("if a reader can reconstruct it from the names and types on the same screen, delete it"), the strong typing has made this comment redundant. The only non-obvious facts it carries are the ones the signature cannot express — that the contract lands in Running with exactly one CaitSith Sign domain — so consider trimming to just that, e.g. /// Builds a Running contract with a single CaitSith [`DomainPurpose::Sign`] domain., or dropping it entirely.
  • crates/contract/src/api/governance.rs:257 — the [`Sign`] intra-doc link does not resolve to any item in scope (the variant is DomainPurpose::Sign). Test code is outside rustdoc's view so CI will not reject it, but per CLAUDE.md §Code Comments it should be either [`DomainPurpose::Sign`] or a plain code span. Pre-existing, and moot if the comment is trimmed per the point above.
  • crates/contract/src/api/governance.rs:168, :190, :217 — three call sites in this same file still pass an untyped u64 threshold to setup_tee_test_contract(num_participants, threshold_value: u64) (crates/contract/src/api/test_utils.rs:186), which then wraps it internally at test_utils.rs:198. That is the same pattern this PR removes, so "strong typing in governance.rs" is not quite complete. The helper lives in a different file and is shared, so it is reasonable to leave for a follow-up under Get completely rid of pure "threshold" terminology #3903 — just flagging so it is not lost.

✅ Approved

gilcu3
gilcu3 previously approved these changes Aug 25, 2026

@gilcu3 gilcu3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you!

Comment on lines 259 to +262
fn setup_running_contract_with_domain(
num_participants: usize,
threshold: u64,
reconstruction_threshold: u64,
governance_threshold: GovernanceThreshold,
reconstruction_threshold: ReconstructionThreshold,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: it is a bit arguable if we should use the strong type here, in the sense that this is a test helper, so receiving u64 and converting internally makes life easier for the caller.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I know, but I seriously wanted to get rid of the loose types. It's ok if strong typing is enforced here

@SimonRastikian SimonRastikian self-assigned this Aug 25, 2026
first_participant_id: &AccountId,
participants: Participants,
threshold: GovernanceThreshold,
governance_threshold: GovernanceThreshold,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: there are a few more places you can update the name as well if you want to be consistent - let threshold = GovernanceThreshold::new(...) to let governance_threshold = GovernanceThreshold::new(...) at 169, 191, 218 and 462.

barakeinav1
barakeinav1 previously approved these changes Aug 26, 2026

@barakeinav1 barakeinav1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks, one nit

@SimonRastikian
SimonRastikian dismissed stale reviews from barakeinav1 and gilcu3 via 6a8590d August 26, 2026 14:54
@SimonRastikian

Copy link
Copy Markdown
Contributor Author

Thanks @barakeinav1 for the note

@SimonRastikian
SimonRastikian added this pull request to the merge queue Aug 27, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 27, 2026
@SimonRastikian
SimonRastikian added this pull request to the merge queue Aug 27, 2026
Merged via the queue into main with commit a93394c Aug 27, 2026
35 of 41 checks passed
@SimonRastikian
SimonRastikian deleted the renaming-threshold branch August 27, 2026 09:38
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