Configuration file #131

mfw78 · 2024-01-17T10:38:51Z

Description

Configuration methodology (via CLI options) has been outgrown with the number of options. This PR removes all chain / network related configuration and moves to a configuration file.

Changes

Move all configuration for chains to a dedicated file
Remove unused features (replay tx / replay block)
Eliminates dynamic download of filter policies (now within configuration file)
Update README.md

How to test

Adjust the config.json.example for suitable configuration
Run and verify sync process completes warmup

Related Issues

Fixes #129

socket-security · 2024-01-17T10:40:30Z

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

Packages	Version	New capabilities	Transitives	Size	Publisher
@types/ajv	1.0.0	None	`+0`	514 B	types
ajv-formats	2.1.1	None	`+0`	52.2 kB	esp
json-schema-to-typescript	13.1.2	network, environment	`+14`	3.82 MB	bcherny

alfetopito

Nice refactor!
Some nitpicks.

src/index.ts

src/types/types.d.ts

src/utils/filterPolicy.ts

src/types/config.schema.json

Co-authored-by: Leandro <alfetopito@users.noreply.github.com>

anxolin

Half way in my review, i need to try it more. I send a few comments.

In general I feel that given the size of the change:

you could have waited a bit for the review, is good that for such a change we have more eyes
it looks like this PR does a bunch of things that would be better to keep in separate PRs

I like the config. Feels much better

README.md

anxolin · 2024-01-18T13:38:01Z

README.md


 ## Overview

-The [`ComposableCoW`](https://github.com/cowprotocol/composable-cow) conditional order framework requires a watch-tower to monitor the blockchain for new orders, and to post them to the [CoW Protocol `OrderBook` API](https://api.cow.fi/docs/#/). The watch-tower is a standalone application that can be run locally as a script for development, or deployed as a docker container to a server, or dappnode.
+The [programmatic order](https://docs.cow.fi/cow-protocol/concepts/order-types/programmatic-orders) framework requires a watch-tower to monitor the blockchain for new orders, and to post them to the [CoW Protocol `OrderBook` API](https://docs.cow.fi/cow-protocol/reference/apis/orderbook). The watch-tower is a standalone application that can be run locally as a script for development, or deployed as a docker container to a server, or dappnode.


not from this PR, I would start saying with what it is and not what it requires.

What about something in these lines:

The programatic orders allow you to automatically place unlimited order, following your arbitrary trading strategy defined using the programmatic order frameworks.

Because smart contracts can't place orders in the Orderbook API, you will need a Watch-Tower that monitors these contracts and places an order on your behalf.

This project contains a general-purpose watch tower that will monitor all or some specific accounts, or orders depending on configuration.

While nice, the idea is mostly that there is a hyperlink there that explains it, and reduces duplication of typing / explanation.

README.md

anxolin · 2024-01-18T13:57:20Z

config.json.example

@@ -0,0 +1,12 @@
+{
+    "networks": [


this is nicer indeed. But now we have an hybrid of envs and config.

Shouldn't we add all the config in the config file if we just made it mandatory to add one?

We could, I don't have a strong opinion either way on this, and only did this way so as to minimise work, and minimise type duplication between the watch-tower and the Pulumi configuration for infrastructure (not a strong argument though).

anxolin · 2024-01-18T13:58:44Z

src/commands/index.ts

@@ -1,5 +1,2 @@
 export * from "./run";
-export * from "./runMulti";
-export * from "./replayBlock";
-export * from "./replayTx";


was this necesary in this PR? now is ok, but feels like could be done in a different PR

Why do we want to remove these features?

runMulti has been superceded (actually, run was removed and runMulti was repurposed to run).

replayBlock and replayTx weren't actually implemented (so removed dead stubs).

anxolin

Worked great!

soft approve

anxolin · 2024-01-19T16:48:56Z

src/index.ts

@@ -343,6 +162,30 @@ function parseAddressOption(option: string, previous: string[] = []): string[] {
  return [...previous, option];
 }

+function validateJSON(filePath: string): Config {


could we reflect in the name this is the JSON of the config file?
validate json sounds too generic.

anxolin · 2024-01-19T16:50:11Z

src/index.ts

+    // Validate the data against the schema
+    if (!validate(data)) {
+      console.error("Configuration file validation failed:", validate.errors);
+      process.exit(1);


not a good practice to terminate the program just like that

normally you throw and whomever needs to handle, handles

anxolin · 2024-01-19T16:51:38Z

src/types/config.schema.json

+            "type": "array",
+            "items": {
+                "type": "object",
+                "properties": {


nit, you can add descriptions to the fields and/or examples

anxolin · 2024-01-19T16:54:56Z

src/types/config.schema.json

+{
+    "$schema": "http://json-schema.org/draft-07/schema#",
+    "type": "object",
+    "properties": {


what are required? also default

Not sure what you mean by this?

anxolin · 2024-01-19T16:56:55Z

src/utils/filterPolicy.ts

  protected config: PolicyConfig | undefined;

-  constructor({ configBaseUrl }: FilterPolicyParams) {
-    this.configUrl = configBaseUrl;
+  constructor(config: Config["networks"][number]["filterPolicy"]) {


can you give a name to this type? feels hard to read

anxolin · 2024-01-19T17:05:08Z

src/types/config.schema.json

+                            "defaultAction": {
+                                "$ref": "#/$defs/filterAction"
+                            },
+                            "owners": {


owners and handlers have the same type. would it be worth it to reference it?

Actually would be probably be nice to define the FilterPolicy too. This way you will get a Typescript definition for it you can reuse, so you don't need to do things like:
constructor(config: Config["networks"][number]["filterPolicy"]) {

mfw78 added 5 commits January 17, 2024 10:12

feat: config schema

39884ee

chore: types and remove unused commands

6637318

chore: adjust runner options for config files

c0b8f79

chore: example configuration file

9267ede

docs: update readme

ba79855

mfw78 added the enhancement New feature or request label Jan 17, 2024

mfw78 requested review from anxolin and a team January 17, 2024 10:38

mfw78 self-assigned this Jan 17, 2024

mfw78 changed the title ~~File configuration~~ Configuration file Jan 17, 2024

fix: typo

3263da1

alfetopito approved these changes Jan 17, 2024

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

src/types/types.d.ts Outdated Show resolved Hide resolved

src/utils/filterPolicy.ts Outdated Show resolved Hide resolved

src/types/config.schema.json Outdated Show resolved Hide resolved

shoom3301 approved these changes Jan 17, 2024

View reviewed changes

mfw78 and others added 3 commits January 17, 2024 12:25

chore: dry schema

c064848

Update src/index.ts

7730d96

Co-authored-by: Leandro <alfetopito@users.noreply.github.com>

chore: dry convertToMap

e7bda59

mfw78 merged commit 597e30b into main Jan 18, 2024
4 checks passed

mfw78 deleted the file-configuration branch January 18, 2024 09:29

github-actions bot locked and limited conversation to collaborators Jan 18, 2024

anxolin reviewed Jan 18, 2024

View reviewed changes

anxolin reviewed Jan 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configuration file #131

Configuration file #131

mfw78 commented Jan 17, 2024

socket-security bot commented Jan 17, 2024

alfetopito left a comment

anxolin left a comment

anxolin Jan 18, 2024

anxolin Jan 18, 2024

mfw78 Jan 19, 2024

anxolin Jan 18, 2024

mfw78 Jan 19, 2024

anxolin Jan 18, 2024

anxolin Jan 18, 2024

mfw78 Jan 19, 2024

anxolin left a comment

anxolin Jan 19, 2024

anxolin Jan 19, 2024

anxolin Jan 19, 2024

anxolin Jan 19, 2024

mfw78 Jan 22, 2024

anxolin Jan 19, 2024

anxolin Jan 19, 2024

Configuration file #131

Configuration file #131

Conversation

mfw78 commented Jan 17, 2024

Description

Changes

How to test

Related Issues

socket-security bot commented Jan 17, 2024

alfetopito left a comment

Choose a reason for hiding this comment

anxolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anxolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment