Conversation
|
Reminder: Please update the DB Readme and comment whether migrations are reversible (include rollback scripts if applicable). Caused by: |
|
Some clarifying questions to make sure I understand correclty:
|
The most important part is that we now add the interactions from the
Yes, the settlement contracts takes an
Currently the autopilot loads the interactions associated with an order from that table instead of re-parsing the appdata. In the long term it would be more correct IMO to do that instead of using the table. Although that would require ethflow orders to pass the
Yes. Technically the ethflow specific stuff can modify any table. So anything the generic onchain order handler does afterwards needs to be "compatible" with the changes the specific handler did before. In this specific case that means we need to make sure that we don't overwrite any interactions (i.e. |
mrnaveira
left a comment
There was a problem hiding this comment.
Based on the answers before, this LGTM. Although it would be nice to add a test scenario of a failed hook execution.
Fixes: #3388
Description
Currently ethflow orders don't have their hooks executed. The reason is that ethflow orders don't get added to the order book via a REST API call which would execute the usual code paths. Instead the autopilot indexes events emitted from certain contracts and adds it to the orderbook that way. This executes a bunch of different code paths that actually didn't check the hooks at all.
Changes
--hooks-contract-addressinto shared args because theautopilotnow needs it toointeractionstable to include theexecutionflag. The readme was updated accordingly.How to test
I updated an existing ethflow e2e test to pass 1 pre and post hook in the appdata. The hooks set an allowance for the trader address. Because we use approval interactions we can easily see in which context the user passed interaction got executed.
If we didn't sandbox the interaction correctly the approval would be set by the settlement contract. But in our test we can see that the approval was given by the trampoline contract. This shows that we didn't introduce a security risk by accident with this fix.