-
Notifications
You must be signed in to change notification settings - Fork 71
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
Boolean Logic Transaction Filtering #392
base: main
Are you sure you want to change the base?
Conversation
2a41972
to
c73f38f
Compare
c73f38f
to
236b94d
Compare
} | ||
} | ||
|
||
impl Filterable<i32> for Option<i32> { |
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.
We use i64
in most places in the processors, should we use i64
here too?
|
||
#[inline] | ||
fn is_allowed(&self, struct_tag: &MoveStructTag) -> bool { | ||
self.address.is_allowed(&struct_tag.address) |
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.
Does it expect that the address is 64 length? Should we support both truncated and standardized addresses?
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.
it does expect standardized addresses, yeah... I was hoping this could be a convention, but we could also put it in custom serde?
/// This is a wrapper around ChangeItemFilter, which differs because: | ||
/// While `ChangeItemFilter` will return false if the Event does not match the filter, | ||
/// `ChangeItemFilter` will return true- i.e `WriteSetChangeFilter` *only* tries to match if the | ||
/// change type matches its internal change type |
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.
Do we need both the ChangeItemFilter
and WriteSetChangeFilter
? If it's possible, I prefer having a single one to keep things simpler.
Also how will we check a transaction if a transaction is allowed? Is it if any of its wsc is allowed?
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.
currently this simple
version- yeah
In the follow up PR, there is a AND
, OR
, and NOT
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
):