wip: Host associate type, ChainSpec refactor #1528
Conversation
|
Closing it as it is merged in Nomic repo |
|
|
||
| /// Trait for retrieving transaction information required for execution. | ||
| pub trait Transaction { | ||
| pub trait Transaction: Clone + Debug + Default + PartialEq + Eq { |
There was a problem hiding this comment.
I was looking at this again and I'm not a fan a of adding these trait bounds on the Transaction trait itself.
This is saying that anything that implements Transaction should also have implemented Clone, Debug, Default, PartialEq, and Eq.
I think the trait should be further constrained to trait bounds at the usage site, not universally. This trait inherently doesn't have any of these requirements and thus is overly constrained, limiting reusability.
The same applies for the type TransactionValidationError. Maybe that should live on a secondary trait that's specifically used for the trait ChainSpec?
There was a problem hiding this comment.
There is an efficient way of doing this that I could create a proof-of-concept for, but it requires the latest version of Rust.
Is there a minimum supported version of Rust for revm?
There was a problem hiding this comment.
No, latest stable is fine
There was a problem hiding this comment.
This is what I had in mind: 4bb0f27
This adds ChainSpec as associate type inside Host and removed a need to modify instruction.
Small refactor on
ChainSpecwhere associate types are moved to standalone traits.