Skip to content

chore: force usage of constructor for PermanentKeyshareData#2437

Merged
gilcu3 merged 3 commits intomainfrom
1217-force-usage-of-constructor-for-permanentkeysharedata
Mar 25, 2026
Merged

chore: force usage of constructor for PermanentKeyshareData#2437
gilcu3 merged 3 commits intomainfrom
1217-force-usage-of-constructor-for-permanentkeysharedata

Conversation

@gilcu3
Copy link
Copy Markdown
Contributor

@gilcu3 gilcu3 commented Mar 13, 2026

Closes #1217

The issue was expanded to cope with ( see the comments below):

Unfortunately, this PR isn't helping in decoupling the node and the contract behavior.
Ideally, we want to get rid of the assumption that keyshares stored on the node follow the same order with which they are stored in the contract, and that the resharing of keys happen in the exact same order in which the keys are stored in the KeyshareStorage.

@gilcu3 gilcu3 marked this pull request as ready for review March 13, 2026 07:56
@claude
Copy link
Copy Markdown

claude bot commented Mar 13, 2026

Code Review

Clean encapsulation PR that makes PermanentKeyshareData fields private and routes all access through getters and the validated new() constructor. All call sites are mechanically updated.

No critical issues found.

Minor note (non-blocking): from_legacy and Deserialize both bypass the new() constructor's consistency validation. from_legacy is fine since a single keyshare trivially satisfies ordering invariants. The Deserialize bypass is pre-existing and necessary for loading persisted data, but worth being aware of if the invariants ever become more complex.

✅ Approved

Copy link
Copy Markdown
Contributor

@kevindeforth kevindeforth left a comment

Choose a reason for hiding this comment

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

Unfortunately, this PR isn't helping in decoupling the node and the contract behavior.

Ideally, we want to get rid of the assumption that keyshares stored on the node follow the same order with which they are stored in the contract, and that the resharing of keys happen in the exact same order in which the keys are stored in the KeyshareStorage.

Comment thread crates/node/src/keyshare/permanent.rs Outdated
Comment thread crates/node/src/keyshare/permanent.rs
@gilcu3 gilcu3 marked this pull request as draft March 13, 2026 12:10
@gilcu3 gilcu3 force-pushed the 1217-force-usage-of-constructor-for-permanentkeysharedata branch 5 times, most recently from 9df87ff to 8b0a45a Compare March 23, 2026 14:53
@gilcu3 gilcu3 force-pushed the 1217-force-usage-of-constructor-for-permanentkeysharedata branch from 8b0a45a to e971248 Compare March 23, 2026 15:27
@gilcu3 gilcu3 marked this pull request as ready for review March 23, 2026 15:34
@claude
Copy link
Copy Markdown

claude bot commented Mar 23, 2026

PR title type suggestion: This PR restructures how PermanentKeyshareData is accessed (making fields private, enforcing constructor usage), which is refactoring—not just routine maintenance. Since source code files are changed, the type prefix should probably be refactor: instead of chore:.

Suggested title: refactor: force usage of constructor for PermanentKeyshareData

@claude
Copy link
Copy Markdown

claude bot commented Mar 23, 2026

Code Review

Clean PR that removes the implicit ordering assumption between permanent keyshares and contract keysets, replacing prefix-based matching with order-independent domain_id-based matching. All keyshares are now sorted by domain_id as the canonical order.

No critical issues found.

✅ Approved

kevindeforth
kevindeforth previously approved these changes Mar 24, 2026
Copy link
Copy Markdown
Contributor

@kevindeforth kevindeforth left a comment

Choose a reason for hiding this comment

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

Thank you, only nits!

Comment thread crates/node/src/keyshare.rs Outdated
Comment thread crates/node/src/keyshare.rs
Comment thread crates/node/src/keyshare.rs
Comment thread crates/node/src/keyshare.rs Outdated
@claude
Copy link
Copy Markdown

claude bot commented Mar 25, 2026

PR title type suggestion: This PR refactors code to enforce constructor usage for PermanentKeyshareData, which is a structural change. Consider using refactor: instead of chore: to be more precise.

Suggested title: refactor: force usage of constructor for PermanentKeyshareData

@claude
Copy link
Copy Markdown

claude bot commented Mar 25, 2026

PR title type suggestion: This PR restructures code to enforce constructor usage, which is a refactoring effort. The type prefix should probably be refactor: instead of chore:.

Suggested title: refactor: force usage of constructor for PermanentKeyshareData

Copy link
Copy Markdown
Contributor

@kevindeforth kevindeforth left a comment

Choose a reason for hiding this comment

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

Thank you!

@claude
Copy link
Copy Markdown

claude bot commented Mar 25, 2026

PR title type suggestion: This PR restructures code to enforce constructor usage for PermanentKeyshareData. The type prefix should probably be refactor: instead of chore:

Suggested title: refactor: enforce constructor usage for PermanentKeyshareData

Copy link
Copy Markdown
Collaborator

@netrome netrome left a comment

Choose a reason for hiding this comment

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

No blockers spotted, but it feels like a lot of the logic would end up much cleaner if we used e.g. a BTreeSet for the keyshares (along with an appropriate Ord implementation)

Comment thread crates/node/src/keyshare/permanent.rs
@gilcu3 gilcu3 added this pull request to the merge queue Mar 25, 2026
@gilcu3 gilcu3 removed this pull request from the merge queue due to a manual request Mar 25, 2026
@gilcu3 gilcu3 added this pull request to the merge queue Mar 25, 2026
@gilcu3 gilcu3 removed this pull request from the merge queue due to a manual request Mar 25, 2026
@gilcu3 gilcu3 added this pull request to the merge queue Mar 25, 2026
Merged via the queue into main with commit 0c47e62 Mar 25, 2026
26 checks passed
@gilcu3 gilcu3 deleted the 1217-force-usage-of-constructor-for-permanentkeysharedata branch March 25, 2026 09:36
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.

force usage of constructor for PermanentKeyshareData

3 participants