-
Notifications
You must be signed in to change notification settings - Fork 82
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
[WIP] More Advanced Transaction Filtering #389
Conversation
cb2d072
to
30ec90f
Compare
52860cd
to
282205f
Compare
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.
Some initial feedback, I didn't dive too deep into the specific filters.
WriteSetChangeFilter(WriteSetChangeFilter), | ||
} | ||
|
||
impl Filterable<Transaction> for APIFilter { |
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.
You could use enum_dispatch / ambassador to help eliminate some of the boilerplate here.
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.
enum dispatchhhhh
pub trait Filterable<T> { | ||
/// Whether this filter is correctly configured/initialized | ||
/// Any call to `is_valid` is responsible for recursively checking the validity of any nested filters | ||
fn is_valid(&self) -> Result<(), Error>; |
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 imagine the reason we have is_valid
is because we can't do it in constructors because these structs might get built directly from configuration, and we can't do it in the parsing logic because serde doesn't provide powerful enough attributes to let us do validity checking at parsing time, yeah?
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.
in the constructors of what?
My assumption for these filters is they will be around for a long time, and different refs will flow through them- there's only ever one copy of these
Injecting it into the serialization would be on the GRPC side, which would be inside of poem, and I wouldn't want to mess with that 😬
} | ||
} | ||
|
||
fn is_allowed(&self, txn: &Transaction) -> bool { |
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 don't wanna nerd snipe you but it'd be neat if this function at the top level told you which filter allowed it if this is true 😛
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.
what do you mean?
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.
Like if a transaction got filtered out, it'd be cool to see which filter exactly it failed on.
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.
sigh :-p
this is not technically hard, just a pain in the ass hahaha... Follow up PR?
282205f
to
1492324
Compare
1492324
to
1891c31
Compare
e3ede30
to
a74334b
Compare
an MVP version of the transaction filtering crate
Performance
On a random 3mb proto of 1000txns during taptos, running the "everything" filter (looped) gives:
Random 10mb proto from April 25th with "everything" filter:
Graffio 100mb proto ("everything" filter, but with
or
):