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

feat: make block struct generic over header type #1179

Merged
merged 3 commits into from
Aug 23, 2024

Conversation

mvares
Copy link
Contributor

@mvares mvares commented Aug 22, 2024

This PR allows Header to assume a type based on generic.

Closes #1177

@mvares
Copy link
Contributor Author

mvares commented Aug 22, 2024

hey @emhane, do you know why the CI is failing?

Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

need to make sure that all the implementations for Block will also work for the non-default type. may advice is to add default generics last, after you got everything compiling.

@mvares
Copy link
Contributor Author

mvares commented Aug 22, 2024

need to make sure that all the implementations for Block will also work for the non-default type. may advice is to add default generics last, after you got everything compiling.

@emhane , I ran all tests and I awaited by compiling. I don't got any error.

image

@emhane
Copy link
Member

emhane commented Aug 22, 2024

sorry wasn't clear enough:

remove the default for the generic.
compile.
fix bugs.
add default generic.

that's how I usually do.

Block::into_full_block is only impl for default header generic now

@mvares
Copy link
Contributor Author

mvares commented Aug 23, 2024

sorry wasn't clear enough:

remove the default for the generic. compile. fix bugs. add default generic.

that's how I usually do.

Block::into_full_block is only impl for default header generic now

Hmm. Looks a good start.

@mvares mvares requested a review from emhane August 23, 2024 00:27
@mvares
Copy link
Contributor Author

mvares commented Aug 23, 2024

@emhane, maybe BlockResponse should have a Header generic. What do you think?

@emhane
Copy link
Member

emhane commented Aug 23, 2024

@emhane, maybe BlockResponse should have a Header generic. What do you think?

block response does have generic header + transaction in form of associated types

/// Header type
type Header;
/// Transaction type
type Transaction;

crates/rpc-types-eth/src/block.rs Outdated Show resolved Hide resolved
crates/rpc-types-eth/src/block.rs Outdated Show resolved Hide resolved
@mvares mvares requested a review from emhane August 23, 2024 15:45
@emhane emhane merged commit 8a06509 into alloy-rs:main Aug 23, 2024
26 checks passed
@mvares mvares deleted the header-generic branch August 23, 2024 16:57
@klkvr
Copy link
Member

klkvr commented Aug 23, 2024

@emhane wdyt about switching generics order? it would turn this update into a non-breaking change and also seems more convenient as imo users are most likely to change the tx generic

@klkvr klkvr mentioned this pull request Aug 26, 2024
3 tasks
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.

[Feature] Make alloy_rpc_types_eth::Block generic over header
3 participants