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

refactor: Make TransactionValidationOutcome generic over error type #14046

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PanGan21
Copy link
Contributor

@PanGan21 PanGan21 commented Jan 28, 2025

Closes: #14037

Makes TransactionValidationOutcome generic over error type.

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.

yes, we want to generalise the impl body too

impl<T: PoolTransaction, E> TransactionValidationOutcome<T, E> { .. }

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

hmm,

this error already supports arbitrary errors via

/// Any other error that occurred while inserting/validating that is transaction specific
#[error(transparent)]
Other(Box<dyn PoolTransactionError>),

skeptical that it's worth integrating an error type

which I think would need to be added to

pub struct PoolInner<V, T, S>

@emhane
Copy link
Member

emhane commented Jan 30, 2025

rollups want to match on their error variants @mattsse . eventually all components that are customisable at sdk level should have an error associated type for good devx.

@mattsse
Copy link
Collaborator

mattsse commented Jan 30, 2025

rollups want to match on their error variants

what's a use case for this?

@emhane
Copy link
Member

emhane commented Jan 30, 2025

rollups want to match on their error variants

what's a use case for this?

do smthg based on the error variant

@mattsse
Copy link
Collaborator

mattsse commented Jan 30, 2025

can we add a helper that typechecking the dyn PollError instead, don't think a dedicated type for this is super useful

@emhane
Copy link
Member

emhane commented Jan 30, 2025

can we add a helper that typechecking the dyn PollError instead, don't think a dedicated type for this is super useful

sure rollups can work with down casting. though semantically it's nicer for the rollups to wrap l1 errors in a variant, so they can extend it. another place I'm having the same problem rn is PayloadError from alloy. in upcoming op fork there is a need for adding other error variants for failing payload validation. instead of moving this problem to alloy to add an Other variant, it would be nicer if the trait PayloadValidator had an associated type for error.

@PanGan21
Copy link
Contributor Author

yes, we want to generalise the impl body too

impl<T: PoolTransaction, E> TransactionValidationOutcome<T, E> { .. }

I added this for now.
In case the approach needs to change and add the type checking helper function please let me know :)

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

@PanGan21

can we instead make this

pub trait PoolTransactionError: core::error::Error + Send + Sync {

`: Any``

and add

    #[inline]
    fn as_any(&self) -> &dyn std::any::Any;

to the trait?

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.

lgtm

@mattsse
Copy link
Collaborator

mattsse commented Feb 3, 2025

actually @PanGan21 we probably don't need the additional Any fn if we require that this trait is 'static

and add these helper functions

#14180

to

pub enum InvalidPoolTransactionError {

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.

Make TransactionValidationOutcome generic over error type
3 participants