Skip to content
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

feat: apply filtering policy #116

Merged
merged 9 commits into from
Nov 22, 2023
Merged

feat: apply filtering policy #116

merged 9 commits into from
Nov 22, 2023

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Nov 19, 2023

Description

This PR implements a basic filtering policy for Programmable Orders.

Configuration

You can define your own policy by defining your own file with the rules.

The format allows you to SKIP or DROP orders who match some criteria:

{
  "owners": {
    "0x0000000000000000000000000000000000000000": "SKIP"
  },
  "handlers": {
    "0x467751B9224f7828b37e2c242F7c37F1f8b91A0D": "DROP"
  }
}

Initial config

The initial config lives now in:
https://github.com/cowprotocol/watch-tower/blob/config

There there's one file per network.

This branch is just temporal. The plan is to have the config done in https://github.com/cowprotocol/watch-tower-config which is a private repository, but it was remove from the scope of this PR.

Filter Policy

In charge of the filtering. For now, I added only pre-filters but i could have postfilters in the future.
Also the rules are pretty simplistic, but future versions of the config could really have more advance filtering.

It can filter by owner and handler.

Workflow

  1. In every block, the policy is loaded before processing the conditional orders (TODO: maybe skip if there's none!)
  2. For every conditional order processed, we apply the filters and depending on the result
  • ACCEPT: Will let the order be processed (normal flow)
  • SKIP: Will skip the order. Next block, we will decide again based on the configuration by the time is processed, if it's still SKIP will keep not processing the order.
  • DROP: Will skip the order for this run and any future blocks.

Params for the CLI

The filtering is OPTIONAL, so by default no filtering will be done.

EDIT: Removed the config param, now it uses the --chain-config (new way after the merge of #112)
You can pass optionally the URL of the configuration you want to use, for example:
--filterPolicyConfig https://raw.githubusercontent.com/cowprotocol/watch-tower/config/filter-policy-100.json

You can specify the config as the 5th argument in --chain-config argument

It also works for the multi-network run.

Test

  • Run without filtering
  • Run with the initial config in config branch: LOG_LEVEL=DEBUG,checkForAndPlaceOrder=DEBUG yarn ts-node ./src/index.ts run --rpc <rpc> --deployment-block 29389123 --page-size 5000 --filterPolicyConfig https://raw.githubusercontent.com/cowprotocol/watch-tower/config/filter-policy-100.json
  • Run with your own config, experiment with it
LOG_LEVEL=DEBUG,checkForAndPlaceOrder=DEBUG \
  yarn ts-node ./src/index.ts run \
  --chain-config <gc-rpc>,,,https://raw.githubusercontent.com/cowprotocol/watch-tower/config/filter-policy-100.json \
  --page-size 5000

Out of scope

Add auth for the HTTPS request of the config file

Copy link

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@types/node-fetch 2.6.9 None +1 54.8 kB types

@anxolin anxolin force-pushed the accept-policy branch 2 times, most recently from c0e5881 to e1ae9ce Compare November 20, 2023 20:31
@anxolin anxolin marked this pull request as ready for review November 21, 2023 12:08
@shoom3301
Copy link
Contributor

Just approved, can't reason about these changes

@anxolin anxolin changed the title feat: apply accept policy feat: apply filtering policy Nov 21, 2023
@anxolin anxolin requested a review from mfw78 November 21, 2023 14:18
Copy link
Contributor

@mfw78 mfw78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason why this is limited to being applied to run-multi, and hasn't been implemented for the run subcommand case?

Comment on lines +44 to +45
const action =
owners.get(owner) || handlers.get(conditionalOrderParams.handler);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a suggestion, are we sure we want to consider the individual owner case first? This may open up attacks if there's a public list with an owner explicitly whitelisted, in which case, one can create a bad handler and target that owner address with fake events being emitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now don't make sense, as the default is to ACCEPT. It makes sense if we allow to change the default to DROP or SKIP (by the way, good feature and easy to add)

However, if we change it in the future, we might not remember this, so changing it just in case.

@anxolin
Copy link
Contributor Author

anxolin commented Nov 21, 2023

Is there any reason why this is limited to being applied to run-multi, and hasn't been implemented for the run subcommand case?

The run don't require changes. It has already the param. If you try the command above should work

@mfw78 mfw78 added the enhancement New feature or request label Nov 22, 2023
@anxolin anxolin merged commit 8f1db0e into main Nov 22, 2023
4 checks passed
@anxolin anxolin deleted the accept-policy branch November 22, 2023 11:01
@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants