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

refactor: add from bytes trait to content key #1455

Merged
merged 2 commits into from
Sep 18, 2024

Conversation

njgheorghita
Copy link
Collaborator

@njgheorghita njgheorghita commented Sep 16, 2024

What was wrong?

Minor refactor to content keys

How was it fixed?

Added From trait to simplify construction

To-Do

@njgheorghita njgheorghita marked this pull request as ready for review September 16, 2024 17:46
@KolbyML
Copy link
Member

KolbyML commented Sep 16, 2024

This refactor or at least the idea of it from my perception came from reviewing my PR #1436 . Well I don't think this PR is that related directly to #1436 other then touching a similar area of code, I think #1436 should just be reviewed, I think offshoot PR's shouldn't be made until the original PR being reviewed is reviewed. This kind of seems like leap frogging in a sense, when I think the priority should be getting #1436 reviewed and merged in the first place.

@njgheorghita
Copy link
Collaborator Author

Yup, i'm in the process of reviewing #1436, it's just a pretty large pr, don't mind waiting to merge this until that's through

Copy link
Collaborator

@morph-dev morph-dev left a comment

Choose a reason for hiding this comment

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

While I'm not against this, I personally like more explicit approach, something like this:

impl HistoryContentKey {
    pub fn new_block_header_with_proof(block_hash: impl Into<[u8; 32]>) -> Self {
        Self::BlockHeaderWithProof(BlockHeaderKey {
            block_hash: block_hash.into(),
        })
    }

    pub fn new_block_body(block_hash: impl Into<[u8; 32]>) -> Self {
        Self::BlockBody(BlockBodyKey {
            block_hash: block_hash.into(),
        })
    }

    pub fn new_block_receipts(block_hash: impl Into<[u8; 32]>) -> Self {
        Self::BlockReceipts(BlockReceiptsKey {
            block_hash: block_hash.into(),
        })
    }

    ... // already existing functions
}

I'm fine with your approach as well (or both), so pick whatever you prefer.

🚢

Copy link
Member

@KolbyML KolbyML left a comment

Choose a reason for hiding this comment

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

:shipit:

@njgheorghita njgheorghita merged commit 0ff1b83 into ethereum:master Sep 18, 2024
9 checks passed
@njgheorghita njgheorghita deleted the refactor-content-key branch September 18, 2024 18:28
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