Skip to content

refactor: Reducing code redundancy - #3778

Merged
SimonRastikian merged 2 commits into
mainfrom
chore/reduce-e2e-test-redundancy
Jul 13, 2026
Merged

refactor: Reducing code redundancy#3778
SimonRastikian merged 2 commits into
mainfrom
chore/reduce-e2e-test-redundancy

Conversation

@SimonRastikian

Copy link
Copy Markdown
Contributor

Tiny cleanup. Creating a helper function and reducing code redundancy.

@SimonRastikian SimonRastikian changed the title chore: Reducing code redundancy refactor: Reducing code redundancy Jul 10, 2026
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

PR title type suggestion: This PR changes only test files, so the type prefix should probably be test: instead of refactor:.

Suggested title: test: reduce code redundancy in request_during_resharing

@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Pull request overview

Consolidates four repeated domain-lookup blocks in test_request_during_resharing into a single helper (create_domain) that finds a domain by its Protocol. Behavior is preserved because each Protocol variant appears at most once in this test's domain configuration (CaitSith, DamgardEtAl, Frost, ConfidentialKeyDerivation), so the previously additional DomainPurpose / Curve filters were redundant.

Changes:

  • Introduced create_domain(&RunningContractState, Protocol) -> DomainConfig helper.
  • Replaced four inline .iter().find(...) blocks with helper calls.
  • Removed now-unused Curve import; added RunningContractState import.

Reviewed changes

Per-file summary
File Description
crates/e2e-tests/tests/request_during_resharing.rs Extracted domain lookup helper and replaced repeated find blocks. Adjusted imports.

Findings

Blocking: none.

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

  • crates/e2e-tests/tests/request_during_resharing.rs:14 — The name create_domain is misleading: the function does not construct or register a domain, it looks one up in the existing contract state. Consider renaming to find_domain or domain_for_protocol so future readers aren't left wondering whether this mutates state.
  • crates/e2e-tests/tests/request_during_resharing.rs:10-13 — The doc comment leans toward paraphrasing the code (per engineering-standards.md §Write helpful code comments). The second sentence carries the load-bearing information (why filtering by protocol alone is sufficient); the first sentence is redundant with a well-named function. Consider collapsing to a single line stating the uniqueness invariant, e.g. /// Panics if no domain matches; each protocol appears at most once in this test's domain set.
  • crates/e2e-tests/tests/request_during_resharing.rs:64-67 — Minor: since the helper now returns an owned DomainConfig and only .id is used at each call site, you could return DomainId directly (or take &contract_state.domains.domains as input) to avoid the .clone(). Not worth blocking on in a test, but tidier.

✅ Approved

@SimonRastikian SimonRastikian self-assigned this Jul 13, 2026
@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

PR title type suggestion: This PR changes only test files, so the type prefix should probably be test: instead of refactor:.

Suggested title: test: reduce code redundancy in request_during_resharing test

Comment on lines +16 to +17
.find(|d| d.protocol == protocol_type)
.unwrap_or_else(|| panic!("no domain with protocol {protocol_type:?}"))

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 (non-blocking): .find() returns the first match and this only panics on zero matches — not on multiple. The doc says "each protocol appears at most once in this test's set," but nothing enforces it, and production actually has two CaitSith domains (Sign + ForeignTx). If default_for_test ever grows a second same-protocol domain, this would silently pick whichever comes first (the old purpose == Sign filter guarded against that). Cheap to make it fail loudly instead:

let mut matches = contract_state.domains.domains.iter().filter(|d| d.protocol == protocol_type);
let domain = matches.next().unwrap_or_else(|| panic!("no domain with protocol {protocol_type:?}"));
assert!(matches.next().is_none(), "multiple domains with protocol {protocol_type:?}");
domain.id

@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, approved — pure test refactor, no logic change. One non-blocking nit on the helper's uniqueness assumption.

@SimonRastikian
SimonRastikian added this pull request to the merge queue Jul 13, 2026
Merged via the queue into main with commit 59c1a45 Jul 13, 2026
16 of 17 checks passed
@SimonRastikian
SimonRastikian deleted the chore/reduce-e2e-test-redundancy branch July 13, 2026 10:27
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