-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: rename TransactionSignedEcRecovered
to RecoveredTx
#13074
Conversation
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.
smol nit
this is better, and we definitely need this otherwise we'd need to introduce associated types for the recovered tx variant which is a bit stupid
pending @Rjected
pub const fn signer(&self) -> Address { | ||
self.signer | ||
} | ||
pub type PooledTransactionsElementEcRecovered<T = PooledTransactionsElement> = RecoveredTx<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.
could we also keep an alias for the TransactionSignedEcRecovered
but mark it as deprecated, because I assume some people are using this type via reth-primitives
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.
yeah we should probably do that
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.
also maybe we could rename PooledTransactionsElementEcRecovered
to RecoveredPooledTx
?
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.
also maybe we could rename
PooledTransactionsElementEcRecovered
toRecoveredPooledTx
?
I think ideally we just use RecoveredTx<PoolTx::Pooled>
? the current PooledTransactionsElementEcRecovered
is already not used much
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.
agree that we should add an alias because TransactionSignedEcRecovered
is likely to be used by primitives consumers, also have a naming suggestion
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.
lgtm, pending alias
This will mostly be used with transaction generic so long type name isn't useful.
I've also made
PooledTransactionsElementEcRecovered
to just be a type alias forRecoveredTx<PooledTransactionsElement>
:reth/crates/primitives/src/transaction/pooled.rs
Lines 690 to 691 in 4b6ee79