Skip to content

refactor(primitives): simplify FromRecoveredTx by delegating to from_recovered_tx#349

Merged
samlaf merged 1 commit intoseismicfrom
refactor--simplify-from-recovered-tx-by-delegating
Mar 25, 2026
Merged

refactor(primitives): simplify FromRecoveredTx by delegating to from_recovered_tx#349
samlaf merged 1 commit intoseismicfrom
refactor--simplify-from-recovered-tx-by-delegating

Conversation

@samlaf
Copy link
Copy Markdown
Contributor

@samlaf samlaf commented Mar 25, 2026

Same fix as that from SeismicSystems/seismic-evm#42

Not sure if there was an issue here, but its just unnecessary duplicate code that is likely to contain a bug like it did in evm.

@samlaf samlaf requested a review from cdrappi as a code owner March 25, 2026 21:20
Comment on lines +203 to +206
// TODO: delegate to TxEnv::from_recovered_tx(tx, sender) once seismic-evm
// rev is bumped to include the FromRecoveredTx<TxSeismic> impl.
// See https://github.com/SeismicSystems/seismic-evm/pull/42
SeismicTypedTransaction::Seismic(tx) => TxEnv {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

flagging this TODO in case someone reads this from the future

@github-actions
Copy link
Copy Markdown
Contributor

Refactors FromRecoveredTx implementation to delegate to TxEnv for standard transaction types, reducing code duplication.

Phase 1
crates/seismic/primitives/src/transaction/signed.rs:220tracing::debug!("from_recovered_tx: tx: {:?}", tx) logs the full SeismicTransaction<TxEnv> with Debug formatting. For confidential transactions (type 0x4a), the TxEnv's data field contains encrypted input that should be treated as sensitive. Consider logging only the tx_hash or using a custom formatter that excludes the data field.

LGTM — The refactoring properly delegates standard transaction types to TxEnv::from_recovered_tx(), eliminating manual field mapping. The Seismic transaction handling remains manual with a clear TODO explaining the dependency on seismic-evm PR #42. The authorization_list change for EIP-7702 appears correct since the Either import was removed and compilation succeeds.

@samlaf samlaf merged commit 86b6dfd into seismic Mar 25, 2026
8 checks passed
@samlaf samlaf deleted the refactor--simplify-from-recovered-tx-by-delegating branch March 25, 2026 21:34
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.

2 participants