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: change RawContentKey to alloy_primites::Bytes #1431

Merged

Conversation

etherhood
Copy link
Contributor

@etherhood etherhood commented Sep 9, 2024

What was wrong?

Vec<u8> was being used to represent Raw Content Key.

See #1404 for details

How was it #fixed?

Replaced it with alloy_primites::Bytes

To-Do

@etherhood etherhood marked this pull request as ready for review September 9, 2024 09:30
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.

This is a good start, but I think that this PR should also make some changes in OverlayContentKey.

Here are the changes that we should do:

  • remove Into<Vec<u8>> from supertraits set (we should use to_bytes directly)
  • change TryFrom<Vec<u8>, Error = ContentKeyError> to TryFrom<RawContentKey, Error = ContentKeyError> in supertraits set
  • to_bytes function should return RawContentKey

I think some of these are trivial, but some might require refactoring (I don't know until I try). If refactoring is not huge, I would put it into this PR, otherwise, it would be fine to split it into multiple PRs.

ethportal-api/src/types/portal_wire.rs Outdated Show resolved Hide resolved
@morph-dev
Copy link
Collaborator

Also, see https://www.conventionalcommits.org/en/v1.0.0/ for failing check.

@etherhood etherhood force-pushed the feature/update-rawcontentkey-type branch from a5d5ff6 to feaf9d5 Compare September 9, 2024 17:41
@etherhood etherhood force-pushed the feature/update-rawcontentkey-type branch from feaf9d5 to 046a4e1 Compare September 9, 2024 18:15
@etherhood
Copy link
Contributor Author

Implemented RawContentKey in OverlayContentKey and in downstream traits as well

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.

Few comments, repeating in many places:

  • we should use RawContentKey::_ instead of Bytes::_ (example: Bytes::new())
    • this allows it easier to later replace RawContentKey with something else (instead of being alias) if we want to
    • it also makes code more explicit
  • we should avoid unnecessary creating new objects, examples:
    • use RawContentKey::from_str directly (instead of using hex_decode to get Vec<u8> and then convert that into RawContentKey)
    • remove .clone() when not needed).

ethportal-api/src/types/content_key/beacon.rs Outdated Show resolved Hide resolved
ethportal-api/src/types/content_key/beacon.rs Outdated Show resolved Hide resolved
ethportal-api/src/types/content_key/history.rs Outdated Show resolved Hide resolved
ethportal-api/src/types/content_key/history.rs Outdated Show resolved Hide resolved
ethportal-api/src/types/content_key/overlay.rs Outdated Show resolved Hide resolved
trin-beacon/src/storage.rs Outdated Show resolved Hide resolved
trin-beacon/src/storage.rs Outdated Show resolved Hide resolved
trin-validation/src/header_validator.rs Outdated Show resolved Hide resolved
ethportal-api/src/types/content_key/state.rs Outdated Show resolved Hide resolved
portalnet/src/find/query_info.rs Outdated Show resolved Hide resolved
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.

Looks good! If you can address last 2 comments, and I will merge the PR.

}
}

impl From<&StateContentKey> for Vec<u8> {
impl From<&StateContentKey> for RawContentKey {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be removed (similar to other ones)

ethportal-api/src/types/content_key/state.rs Outdated Show resolved Hide resolved
@morph-dev morph-dev added the refactoring Code refactoring label Sep 11, 2024
@morph-dev
Copy link
Collaborator

You can ignore failing commitlint check, as I will squash and merge changes anyway.

But for the future, problem is the commit message: "fix: CI fixes and resolved conflicts with master", because it starts with capital letter (maybe something that we should change in our configuration).

@etherhood etherhood force-pushed the feature/update-rawcontentkey-type branch from 585bbc8 to 88f0e87 Compare September 11, 2024 07:09
@etherhood
Copy link
Contributor Author

Fixed the commit message and remove From impl for StateContentKey

@morph-dev morph-dev changed the title Chore: Update rawcontentkey type refactor: change RawContentKey to alloy_primites::Bytes Sep 11, 2024
@morph-dev morph-dev merged commit eeef99f into ethereum:master Sep 11, 2024
9 checks passed
@etherhood etherhood deleted the feature/update-rawcontentkey-type branch September 11, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants