Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Deref implementation from CandidateHash and update affected co… #6763

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sylvaincormier
Copy link

This pull request removes the Deref implementation from CandidateHash and updates all affected code to reflect this change. This resolves a specific issue where the use of Deref for CandidateHash was deemed inappropriate and could lead to potential misuse or unclear code semantics.

Issue Linked: Fixes #5224
Integration
Downstream projects relying on CandidateHash should note that the Deref implementation is no longer available. Instead, direct methods or traits should be used for accessing the inner hash value. No additional changes are required apart from adopting this updated interface.
Example:

Before:

let hash_value = candidate_hash.deref();

After:

let hash_value = candidate_hash.value();

Ensure that all usages of Deref for CandidateHash in downstream projects are updated to avoid compilation errors.
Review Notes

This PR includes the following changes:

  1. Removed the Deref implementation for CandidateHash.
    
  2. Updated all internal usages of CandidateHash to use explicit methods or appropriate patterns instead of relying on Deref.
    
  3. Verified all tests to ensure the updated code remains functional and passes all cases.
    

Detailed implementation notes:

  • Removed the Deref implementation from CandidateHash.

  • Refactored internal usages to align with the updated interface.

  • Updated tests across all affected modules to ensure compatibility.

My PR includes a detailed description as outlined above.

  1. I have ensured that downstream integration notes are included and actionable.
  2. I have reviewed and labeled my PR appropriately.
  3. All relevant tests pass successfully, verifying the fix.

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Dec 4, 2024

User @sylvaincormier, please sign the CLA here.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team December 4, 2024 21:33
@sylvaincormier sylvaincormier marked this pull request as draft December 4, 2024 22:36
@sylvaincormier
Copy link
Author

sylvaincormier commented Dec 4, 2024

This is my first pull request to this repo so I have converted as a draft as a precaution, please advise

@sylvaincormier
Copy link
Author

Closing to resubmit with cleaned up branch

@sylvaincormier sylvaincormier reopened this Dec 5, 2024
@sylvaincormier sylvaincormier force-pushed the fix-5224-remove-candidatehash-deref branch from d4d1c60 to a283182 Compare December 5, 2024 21:04
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.

Fix CandidateHash newtype
1 participant