-
Notifications
You must be signed in to change notification settings - Fork 237
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: add generics to filter, transaction, and pub_sub. #573
feat: add generics to filter, transaction, and pub_sub. #573
Conversation
In favor of making the code reusable by other networks. At what point is this PR done? do we have a list of types we need to update? |
this process will take a few iterations because when adding new network specific types we'll discover more things that could be reused. I think we will run into a few issues with types that have multiple network specific fields, like Transactions,Logs,Block,Header, for which we either need to add a bunch of generics or also do some kind of network trait with associated types... perhaps, at some point it's just easier to simply duplicate the types but not sure if we want to |
The block struct also needs to take a generic for example but that means many other dependent structs will need the same thing later. Making Transaction a trait here would solve such issue for example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are reasonable,
I think we could also restrict this to : Transaction
crates/rpc-types/src/eth/pubsub.rs
Outdated
pub enum SubscriptionResult<T = Transaction> { | ||
/// New block header. | ||
Header(Box<RichHeader>), | ||
/// Log | ||
Log(Box<Log>), | ||
/// Transaction hash | ||
TransactionHash(B256), | ||
/// Full Transaction | ||
FullTransaction(Box<Transaction>), | ||
FullTransaction(Box<T>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume eventually there'll be the case where this also needs a log type.
Just updated that. We can add the log generic later when needed probably |
Can we merge this? @mattsse @prestwich |
* feat: add generics to filter, block, transaction, and pub_sub. * refactor: change blockhash structs to not take a generic. * refactor: remove the block generic transaction * refactor: make BlockTransactions generic * refactor: make subscription result use the fixed transaction type
Motivation
In order to be able to use several structs and types from this repository for the Optimism port, they should take generics that can be replaced/used with the optimism types.
Solution
Add generics to the necessary structs and types.
PR Checklist