Skip to content

feat: Enhance filter expression parsing and evaluation #222

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

Merged
merged 87 commits into from
Jun 3, 2025

Conversation

isSerge
Copy link
Contributor

@isSerge isSerge commented May 14, 2025

Summary

This PR is an attempt to address issues discussed in #204

Key changes:

Introduction of Expression sub-module responsible for:

  • AST definitions
  • parsing (using winnow crate)
  • shared evaluation logic (ConditionEvaluator trait, evaluation errors, helpers)

EVM and Stellar both have implementations of ConditionEvaluator (evaluator.rs) responsible for:

  • conversion of parameter values (LHS) into chain-specific types
  • LHS vs RHS comparison

evaluate_expression method in chain filters now only delegates parsing and evaluation to dedicated modules

Breaking changes:

  • Expression parsing is more strict due to parsing and evaluation decoupling. For example in expression"input contains '567890'" RHS now has to be in single quotes to let parser now it's a string, otherwise it result in type mismatch error
  • Stellar syntax arguments[0][1] == 20 is no longer valid. LHS has to be an identifier (can introduce keyword if this is crucial)

Next steps:

  • Refactor evaluate_expression to return Result<bool, EvaluationError>
  • Parsing of implicit arrays for EVM (e.g. uint8[] "1,2,3")
  • Stellar events key access 0.2

If any of these is critical - let me know and I will include here. Otherwise I think there are already a lot of changes to review.

I haven't updated Antora docs yet, will add a commit once we discuss and confirm implementation.

Testing Process

Checklist

  • Add a reference to related issues in the PR description.
  • Add unit tests if applicable.
  • Add integration tests if applicable.
  • Add property-based tests if applicable.
  • Update documentation if applicable.

isSerge added 30 commits May 5, 2025 14:33
…ralValue for clarity, update parsing methods, move expression module to service folder
@shahnami
Copy link
Member

shahnami commented May 31, 2025

@isSerge I'm thinking would it make sense to start the work on the follow-up PRs and review them individually but merge them all into this one before we merge into main?

We're mainly just waiting for:

  • Refactor evaluate_expression to return Result<bool, EvaluationError>
  • Parsing of implicit arrays for EVM (e.g. uint8[] "1,2,3")
  • Stellar events key access 0.2

However, I think the latter you've already integrated, right?

Btw, we're just waiting for @KitHat's review some time next week normally.

@isSerge
Copy link
Contributor Author

isSerge commented May 31, 2025

@shahnami yes, Stellar key access is already there. Let me submit the rest

@isSerge
Copy link
Contributor Author

isSerge commented May 31, 2025

@shahnami the thing is these are all implemented in my forked repo branches. I can consolidate all changes within my fork and include here, or can submit separate stacked PRs to the main. Which do you prefer?

@shahnami
Copy link
Member

shahnami commented Jun 1, 2025

@shahnami the thing is these are all implemented in my forked repo branches. I can consolidate all changes within my fork and include here, or can submit separate stacked PRs to the main. Which do you prefer?

I see, okay, let's add them to this PR then

@isSerge
Copy link
Contributor Author

isSerge commented Jun 2, 2025

@shahnami I have included changes to handle array types, direct object (map) comparisons and return Result for evaluate_expression methods.

There are some integration tests failing occasionally:
integration::bootstrap::main::test_create_trigger_handler
integration::bootstrap::main::test_create_trigger_handler_empty_matches
integration::bootstrap::main::test_create_trigger_handler_with_conditions
integration::blockwatcher::service::test_max_past_blocks_limit
integration::blockwatcher::service::test_fresh_start_processing
but seems unrelated to my changes

@NicoMolinaOZ
Copy link
Contributor

NicoMolinaOZ commented Jun 2, 2025

Hi @isSerge , can you check if this combination of expressions works for you on Stellar mainnet? For some reason, I am not able to test this specific case.

event: supply_collateral(Address,Address,Vec<i128>)
expression: 0 contains 'CCW67TSZV3SSS2HXMBQ5JFGCKJNXKZM7UQUWUZPUTHXSTZLEO7SJMI75' AND (2 contains '1300000,1293965' OR 1 contains 'GDDRA7DZMXU35GVZBWXB63DNNITZVJRCIIITN5E37ACSKVR2VPBRWYOR')
transaction hash: 3f7087804a989520d0842e0a5569617f19debf00f65a49e63ef3e245c88d065
monitor: stellar_swap_dex

Thanks!

@isSerge
Copy link
Contributor Author

isSerge commented Jun 2, 2025

@NicoMolinaOZ I suspect the problem is this expression: 2 contains '1300000,1293965': contains operator only checks if a single item is present in LHS, in this case RHS is parsed as a string '1300000,1293965'

@NicoMolinaOZ
Copy link
Contributor

@NicoMolinaOZ I suspect the problem is this expression: 2 contains '1300000,1293965': contains operator only checks if a single item is present in LHS, in this case RHS is parsed as a string '1300000,1293965'

Thanks for the response. I saw that we are having some issues parsing complex types for EVM functions and events. When you have an object like this:
data { field: string[] another_field: number } solidity under the hood converts it to a tuple, and actually, we don't support parsing tuples. We want to merge this PR and implement these improvements in the future.

Thanks!

@shahnami shahnami merged commit 3cb0849 into OpenZeppelin:main Jun 3, 2025
15 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: allowlist cla: signed S-needs-review Awaiting review (code or design)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter expression parser fails on parentheses and standard boolean operator precedence
3 participants