Added hook tests #9

tamir-cow · 2025-10-27T07:21:27Z

Hook Validation Logic & Data Structure Refactoring

Summary

This PR implements the validation logic for hooks through the check_hooks function

Related PR: link - Contains the data fetching layer and extraction logic

Step 1: Fill Status Determination
offchain_trade = _find_offchain_trade(offchain_data, order_uid)
is_first_fill = offchain_trade.already_executed_amount == 0

Step 2: Hook Candidate Presence Check
has_no_hook_candidates = (
not onchain_data.hook_candidates.pre_hooks and
not onchain_data.hook_candidates.post_hooks
)
if has_no_hook_candidates and is_first_fill:
return False # Expected hooks but none executed
Rationale: If the order has hooks defined and it's the first
fill, onchain data must contain hook candidates.

Step 3: Pre-Hook Validation (Rule 3a)

Case A: First Fill
if is_first_fill:
for pre_hook in hooks.pre_hooks:
if not _check_hook_execution(onchain_data, order_uid,
pre_hook, "pre"):
return False

Requirement: All declared pre-hooks must be executed
Validation: Delegates to _check_hook_execution() for each
pre-hook

Case B: Subsequent Fill
else: # Not first fill
if hooks.pre_hooks and
onchain_data.hook_candidates.pre_hooks:
for pre_hook in hooks.pre_hooks:
for executed_hook in
onchain_data.hook_candidates.pre_hooks:
if (executed_hook.target == pre_hook.target and
executed_hook.calldata ==
pre_hook.calldata):
logger.error("Pre-hook incorrectly executed
for subsequent fill")
return False

Requirement: Pre-hooks must NOT be executed on subsequent
fills
Validation: Checks for presence of any declared pre-hooks in
executed hooks

Step 4: Post-Hook Validation (Rule 3b)
for post_hook in hooks.post_hooks:
if not _check_hook_execution(onchain_data, order_uid,
post_hook, "post"):
return False

Requirement: All declared post-hooks must be executed on
EVERY fill
No conditional logic: Always validated regardless of fill
status

Rule Mapping

Rule 1: Pre-Hook Timing

Requirement: Pre-hooks must execute before the settlement
contract pulls user funds. The rule is enforced in the other pr when creating the hook candidates.

Rule 2: Post-Hook Timing

Requirement: Post-hooks must execute after the settlement
contract pushes proceeds to users.

Parallel to Rule 1: Same guarantee mechanism, applied to
post-hooks.

Rule 3a: Pre-Hooks on First Fill Only

Requirement: Partially fillable orders must execute pre-hooks
only on the first fill.

Dual Enforcement:

Positive check: First fill must have all pre-hooks
Negative check: Subsequent fills must not have any pre-hooks

Fill status determined by already_executed_amount == 0

Rule 3b: Post-Hooks on Every Fill

Requirement: Partially fillable orders must execute post-hooks
on every fill (first and subsequent).

Rule 4a: Matching Triplet (target, calldata, gas_limit)

Requirement: Executed hook must match expected hook on target
address, calldata, and gas limit.

Target Address and Calldata - regular equality check

Gas Limit:

Comparison: actual >= expected (inequality, not equality)
Special Case: gas_limit == 0 means unlimited (skip check)

Rule 4b: Hook Attempt Verification

Requirement: Hook must be attempted even if it reverts (revert
does not violate rules).

Not implemented yet

Rule 4c: No Upstream Reverts

Requirement: Intermediate calls between settle() and hook
execution must not revert.

Not implemented yet

Rule 4d: Gas Sufficiency

Requirement: Available gas forwarded to hook must be ≥
specified gas limit. Handled as part of rule 4a.

if a native price is missing, a trade will not contribute to the score computation

fhenneke

Made one pass over the code. The general approach of adding hook attributes to onchain and offchain data seems good.

I had some issues matching the rules as described in the docstring of check_hooks() to the actual code. Maybe one changes to comments are required here. Will have a look at the fetching logic next.

There should be dedicated unit tests for check_hooks().

circuit_breaker_validator/check_tx.py

circuit_breaker_validator/models.py

circuit_breaker_validator/check_tx.py

tamir-cow · 2025-11-19T13:05:45Z

Made one pass over the code. The general approach of adding hook attributes to onchain and offchain data seems good.

I had some issues matching the rules as described in the docstring of check_hooks() to the actual code. Maybe one changes to comments are required here. Will have a look at the fetching logic next.

There should be dedicated unit tests for check_hooks().

You need to look at both prs, this pr only has second phase rules applied and the other has the first phase

fhenneke · 2025-11-19T19:34:56Z

I had a look at both this PR and at the PR related to data fetching and populating the objects used here.

I still think that every rule should either be checked explicitly or it should be explained (in docstrings of the function and model and in the PR description) how the data models must already make sure a test is not required.

Going through the different rules:

This is guaranteed by OnchainSettlementData.hook_candidates being populated in a certain way -> docstring of check_hooks and OnchainSettlementData.
Same as 1.
This is currently not checked. One could put that into the data fetching (and document that) or one would need to change some models here and handle that explicitly here.
a) I think one should check for matching (target, calldata, gas_limi) here even if the data fetching already makes sure it is satisfied. It is easy to check that as well and clearer how the code relates to the rule.
b, c, d) This is currently not implemented. I think it is reasonable to extend the Hook data structure (e.g. an attribute enough_gas_available and no_upstream_revert) and test it explicitly once we check this. Until then, the docstring should mention that this is not yet implemented.

3. Partially fillable orders: a. Should execute the pre-hooks on the first fill only b. Should execute the post-hooks on every fill by checking the executed field to determine if first fill or not

fhenneke

I added a few comment.

Note that the comment here as well as some of the points raise in the last review are largely unaddressed.

circuit_breaker_validator/check_tx.py

circuit_breaker_validator/models.py

circuit_breaker_validator/check_tx.py

fhenneke

I left a few comment.

The most important ones are about checking for gas and checking for not executing hooks.

The design of the hook test also used some strong assumptions on data fetching. These should be removed or documented.

circuit_breaker_validator/check_tx.py

circuit_breaker_validator/models.py

tests/test_check_tx.py

fhenneke · 2025-11-28T11:01:45Z

tests/test_check_tx.py

+        }
+        offchain_data.trades = [offchain_trade]
+
+    elif scenario == "insufficient_gas":


(This is probably wrong, but lets wait for comments by Federico.)

With the new changes I would rename that to "wrong gas limit" or something like that. A test on insufficient gas will only be implemented later.

I agree on renaming this to something like "incorrect gas limit," but overall the test looks reasonable. Maybe:

- gas_limit=1000, # Less than required 1030000 + gas_limit=1000, # Different from the required 1030000

tests/test_check_tx.py

fhenneke · 2025-11-28T11:06:37Z

tests/test_check_tx.py

+        pre_hook_2 = Hook(
+            target=HexBytes("0xC02De8aB58f29E4d9fa9e274eC7c05a04e397313"),
+            calldata=HexBytes("0xc3985579"),
+            gas_limit=1040000,
+        )
+        post_hook_2 = Hook(
+            target=HexBytes("0xD03De8aB58f29E4d9fa9e274eC7c05a04e397314"),
+            calldata=HexBytes("0xd4985580"),
+            gas_limit=2050000,
+        )


I would use examples which are immediately identifiable as different from pre_hook and post_hook.

fhenneke

I still think the logic of checking hooks would benefit from a few changes. If we stick to the current implementation, which I do not recommend, there needs to be more explicit docs and also explicit checks for data consisentency.

fhenneke · 2025-12-02T09:43:19Z

circuit_breaker_validator/models.py

+              ensuring Rule 1 (pre-hooks execute before pulling user funds)
+            - Post-hooks appear after the corresponding trade execution in the transaction trace,
+              ensuring Rule 2 (post-hooks execute after pushing proceeds)
+            - The ordering in each list reflects the actual execution order in the transaction


Is this relevant? For now, we do not use it. In the future it is probably not enough to implement other checks.

circuit_breaker_validator/models.py

fhenneke · 2025-12-02T09:54:43Z

tests/test_check_tx.py

+        }
+        offchain_data.trades = [offchain_trade]
+
+    elif scenario == "insufficient_gas":


With the new changes I would rename that to "wrong gas limit" or something like that. A test on insufficient gas will only be implemented later.

fhenneke · 2025-12-02T09:56:26Z

tests/test_check_tx.py

+        offchain_data.trades = [offchain_trade]
+
+    elif scenario == "subsequent_fill_with_pre_hooks":
+        # Subsequent fill - pre-hooks should NOT be executed, but they are


That comment should probably more descriptive since the expected behavior is that the test succeeds.

circuit_breaker_validator/check_tx.py

circuit_breaker_validator/models.py

circuit_breaker_validator/check_tx.py

fedgiac

The overall logic and the code looks good. I only have small comments and suggestions.

fedgiac · 2025-12-04T09:53:50Z

circuit_breaker_validator/check_tx.py

+    # If there are hooks in the offchain data and it's the first fill,
+    # but there aren't hook candidates in onchain data, return False
+    has_no_hook_candidates = (
+        not hook_candidates.pre_hooks and not hook_candidates.post_hooks
+    )
+    if has_no_hook_candidates and is_first_fill:
+        logger.error(
+            f"Transaction hash {tx_hash!r}: "
+            f"Hooks defined for order {trade.order_uid!r} "
+            f"but no hook candidates found in transaction"
+        )
+        return False
+


Is this code really needed? The following code checks if the pre-hooks are in the candidate pre-hooks (if first fill) and if post-hooks are in the candidate post-hooks. This then seems redundant?

I don't believe so, it is added after encountering this case: 4814655aacfd3db9ebbaf16c428f23372aaf66cd39456c0c5fc15c1759d35588 where we have a mismatch in hooks because of subsequent fills, and based on logic we should skip them.

The transaction you link to has order 0xce7aa6aa17e2e5f0e6cf94ff437de07add4b2852d9098c66237210fabd93dfb0fb632268c37856c6efd48738ef60a74680c4fb626b04786c in it, which has a hook, so it shouldn't match this block because has_no_hook_candidates is false.

But my point here is similar to this, with the extra note that it's particularly strange that if is_first_fill is false, not hook_candidates.pre_hooks is true, and not hook_candidates.post_hooks is true, then it skips this early exit condition and continues by using empty loop in the following checks.

circuit_breaker_validator/check_tx.py

fedgiac · 2025-12-04T10:58:38Z

circuit_breaker_validator/check_tx.py

+    # If there are no trades, the rule for hooks is automatically satisfied
+    if not offchain_data.trades:
+        return True
+


This isn't needed, right? The loop should be empty and then the function returns true.

You are right, this is to make this check explicit and clear

I don't mind much, so feel free to keep as it is, but I was a bit confused here because I thought I was missing something. My vote always goes for less code when possible!

Those early exits tend to confuse me more than they help as the case excluded is quite naturally treated with an empty for loop.

All other early exits in this PR seem to also not have an effect on the outcome of running the code but only serve the purpose to reduce mental load and, in the future, make bugs less likely.

For the example

if not offchain_data.trades: return True

an can still see how it can help and it does not add too much noise (though more noise than I would like).

For the example

# If there are hooks in the offchain data and it's the first fill, # but there aren't hook candidates in onchain data, return False has_no_hook_candidates = ( not hook_candidates.pre_hooks and not hook_candidates.post_hooks ) if has_no_hook_candidates and is_first_fill: logger.error( f"Transaction hash {tx_hash!r}: " f"Hooks defined for order {trade.order_uid!r} " f"but no hook candidates found in transaction" ) return False

I would argue that the combination of three conditionals and including negations does not decrease mental load for me and adds quite a bit of noise. I can see this increasing the risk of bugs in the future.

circuit_breaker_validator/check_tx.py

tests/test_check_tx.py

fedgiac · 2025-12-04T11:28:39Z

tests/test_check_tx.py

+        }
+        offchain_data.trades = [offchain_trade]
+
+    elif scenario == "insufficient_gas":


I agree on renaming this to something like "incorrect gas limit," but overall the test looks reasonable. Maybe:

- gas_limit=1000, # Less than required 1030000 + gas_limit=1000, # Different from the required 1030000

tests/test_check_tx.py

circuit_breaker_validator/check_tx.py

circuit_breaker_validator/models.py

fhenneke · 2025-12-05T09:42:26Z

Apart from some nitpicks the code looks good to me.

But I only want to approve it after checking how our code which depends on this repo handles the changes.

Co-authored-by: Felix Henneke <[email protected]>

fedgiac

The code looks good, the only unaddressed comments are about coding style and can be disregarded. Not sure if I should approve since I don't know how the process work in this codebase, but you can consider this an approval to the best of my knowledge.

fedgiac · 2025-12-05T15:54:40Z

circuit_breaker_validator/models.py

+    # 0 means it's the first fill, any other value means it's not
+    already_executed_amount: int


Suggestion, this came to mind while reviewing the other PR. It shouldn't be an issue by how we formulated the rules, but this may change in the future.

Suggested change

# 0 means it's the first fill, any other value means it's not

already_executed_amount: int

# This value represent how much the order was executed _before the settlement_.

# If the order is executed twice in the same settlement, this value will be the same for both orders.

# 0 means it's the first fill, any other value means it's not

already_executed_amount: int

tamir-cow added 3 commits November 18, 2025 18:53

Added hook tests

acdb11f

Fixed MyPy warnings

dec5eec

Finished hook tests implementation

2739758

tamir-cow force-pushed the feature/hook-tests branch from 7dbe595 to 2739758 Compare November 18, 2025 16:53

fhenneke added 5 commits November 18, 2025 19:03

fix bug with missing prices

3d5b81d

if a native price is missing, a trade will not contribute to the score computation

add test for missing native price case

c267cc5

fix black

e10f7f6

add log when score is skipped

8d28739

use pytest.parametrize for parameters

31f5aee

tamir-cow force-pushed the feature/hook-tests branch from a7886e2 to 73e7e91 Compare November 18, 2025 17:03

tamir-cow marked this pull request as ready for review November 18, 2025 17:13

Fixed UTs

15a09dc

tamir-cow force-pushed the feature/hook-tests branch from 5698353 to 15a09dc Compare November 19, 2025 08:06

fhenneke requested changes Nov 19, 2025

View reviewed changes

Addressed CR comments

8188649

tamir-cow force-pushed the feature/hook-tests branch from ff197ba to 8188649 Compare November 20, 2025 13:13

tamir-cow added 5 commits November 20, 2025 15:47

Merge branch 'main' into feature/hook-tests

7dd8107

Pylint fixes

e892764

Added the following rule:

0139688

3. Partially fillable orders: a. Should execute the pre-hooks on the first fill only b. Should execute the post-hooks on every fill by checking the executed field to determine if first fill or not

Lint

0411f3d

Fixed order of operations

f67c5ad

fhenneke requested changes Nov 26, 2025

View reviewed changes

Improved documentation and testing

74caeeb

fhenneke requested changes Nov 28, 2025

View reviewed changes

tamir-cow added 2 commits December 1, 2025 13:40

Addressed CR comments

e9ded00

Updated UTs

a4546f1

fhenneke requested changes Dec 2, 2025

View reviewed changes

Updated check hooks to iterate over trades rather than order hooks

61fd7f1

fedgiac reviewed Dec 4, 2025

View reviewed changes

Updated tests

715e676

tamir-cow force-pushed the feature/hook-tests branch from 3e98c74 to 715e676 Compare December 4, 2025 12:18

Updated docstring

921d1ef

fedgiac reviewed Dec 4, 2025

View reviewed changes

circuit_breaker_validator/check_tx.py Outdated Show resolved Hide resolved

Updated docstring

b2e9004

fhenneke reviewed Dec 5, 2025

View reviewed changes

circuit_breaker_validator/models.py Outdated Show resolved Hide resolved

fhenneke self-requested a review December 5, 2025 09:40

Update circuit_breaker_validator/models.py

41325f3

Co-authored-by: Felix Henneke <[email protected]>

fedgiac reviewed Dec 5, 2025

View reviewed changes

		# 0 means it's the first fill, any other value means it's not
		already_executed_amount: int

Added hook tests #9

Are you sure you want to change the base?

Added hook tests #9

Uh oh!

Conversation

tamir-cow commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fhenneke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tamir-cow commented Nov 19, 2025

Uh oh!

fhenneke commented Nov 19, 2025

Uh oh!

fhenneke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fhenneke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhenneke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fedgiac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

tamir-cow commented Oct 27, 2025 •

edited

Loading