feat(bot-strategy-runner): add generalised bot runner #2851

chrismaree · 2021-04-12T13:49:14Z

Motivation

As the number of bots within the ecosystem grows so we need more scalable techniques for running bots. This PR adds a generalised bot runner that can pull addresses from a whitelist (such as the dev mining white list) and use this to execute a series of bots.

Summary

The design implemented in this PR is meant to accommodate community members running bots at home or on a VPS as well as UMA's serverless bot running infrastructure.

Details

This implementation contains a few key components:

ConfigBuilder.ts constructs configs to execute different kinds of bots (liquidator, disputer and monitor currently supported). Contains some helper functions for building white lists and fetching remote config files.
LogCaptureTransport.ts a custom Winston transport to capture logs produced by the strategy runner.
BotEntryWrapper.ts but runner that wraps the bots entry point, enabling generalized bot execution.
index.ts file containing all the logic to pull all the parts together and all concurrency logic.

For details on how to run the strategy runner (and other descriptions of the configs) please see the README.md file contained in this PR.

See this gif for what the progress bar looks like 👯

Testing

Ran end-to-end test, running the code as in production
New unit tests created
Existing tests adequate, no new tests required
All existing tests pass
Untested

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

…col/protocol into chrismaree/bot-stratergy-runner

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

chrismaree · 2021-04-13T12:55:30Z

packages/bot-strategy-runner/README.md

@@ -0,0 +1,96 @@
+# @uma/bot-strategy-runner


this looks really nasty like this in preview. take a look at the file here for something a bit easier to read.

chrismaree · 2021-04-13T13:07:09Z

packages/bot-strategy-runner/ExampleConfig.json

@@ -0,0 +1,14 @@
+{


sample config that will pull directly from the global whitelist.

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

chrismaree · 2021-04-13T15:14:33Z

@UMAprotocol/eng this package does not yet have any unit tests. Thus far I've been manually testing this against all main net bots.

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

mrice32 · 2021-04-14T20:16:23Z

.eslintrc.js

+        "@typescript-eslint/no-var-requires": 0,
+        "@typescript-eslint/no-empty-function": 0


Do we need to remove these rules globally or is it possible to just add an inline comment to ignore the rule where we violate it?

I tried adding the inline rule for a few hours but could not get it to work as expected unfortunately. I agree with you that this is not ideal to have within the global rules. I will try again with the inline stuff and report back.

packages/bot-strategy-runner/README.md

nicholaspai · 2021-04-14T21:00:48Z

packages/bot-strategy-runner/ExampleConfig.json

+    "https://raw.githubusercontent.com/UMAprotocol/protocol/master/packages/affiliates/payouts/devmining-status.json"
+  ],
+  "globalAddressBlacklist": [
+    "0x516f595978D87B67401DaB7AfD8555c3d28a3af4",


What are these addresses? Would be helpful as an example to explain whether they are EMP's or something else

These ones are either expired or we dont have price feeds for them. There are some contracts we run dev mining on that we dont run feeds on. that's what these addresses are for.

How do you recommend I add comments? json does not support comments.

nicholaspai · 2021-04-14T21:07:41Z

packages/bot-strategy-runner/src/index.ts

+import { runBot } from "./BotEntryWrapper";
+
+// Defaults for bot execution. If the user does not define these then these settings will be used.
+const defaultPollingDelay = 120;


Why not 5 mins?

can change it to that to be more consistant with the hub! I set it to this as the default polling delay within the bots is 2 mins.

nicholaspai · 2021-04-14T21:08:21Z

packages/bot-strategy-runner/src/index.ts

+
+// Defaults for bot execution. If the user does not define these then these settings will be used.
+const defaultPollingDelay = 120;
+const defaultStrategyTimeout = 60;


Should this timeout be 2 mins? I feel like 60 mins is too short, while anything over 2 mins is definitely an error. Empirically speaking.

ye, this was mainly to match to the bots implementation. I'm hoary to make this 2 miss and then set the polling delay in to 5 mins. will be easier to reason about I think

I've updated to use 5 mins for the polling delay and 2 mins for timeout to be more conservative and consistant with the hub.

packages/bot-strategy-runner/src/index.ts

nicholaspai

Since these scripts are going to run bots in prod, similar to those in serverless-orchestration, should we have unit tests? I can see us wanting to test any of the scripts in bot-strategy-runner/src for example.

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>

chrismaree · 2021-04-15T08:45:17Z

Since these scripts are going to run bots in prod, similar to those in serverless-orchestration, should we have unit tests? I can see us wanting to test any of the scripts in bot-strategy-runner/src for example.

sure. I can add unit tests for it but perhaps we can leave it for a separate PR as this is already pretty bloated and a lot of code. I dont think we will roll this into production just yet so it'll be ok to have untested code in master for a few days.

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

daywiss

this is really cool, most of the complexity here is just wrangling the configs and logging, actually running the bots is really simple, which is great.

this is not a blocker, but i guess my main concern here is error handling. It seems like there's a pattern of avoiding real errors. There is probably a good reason for why you do this though, maybe its just carry over from previous design or for the sake of clean logging, but I think things would be easier to debug if we embraced throwing often. Have a top level error handler which handles formatting the error into a log, rather than doing this deep in the code or requiring logger to understand context of the log.

daywiss · 2021-04-15T13:01:40Z

packages/bot-strategy-runner/src/ConfigBuilder.ts

+}
+
+// Takes in two objects of any depth and merges them. In the case of a collision in keys the second object will take priority.
+export function mergeConfig(...args: any) {


fyi lodash has several utilities for this, these look most closely to what you want:

merge

defaultsDeep

this is tricky code and probably should have a unit test

ye, this is a better solution! thanks for pointing me towards them. I'll replace this custom method with the lodash one.

daywiss · 2021-04-15T13:06:30Z

packages/bot-strategy-runner/src/ConfigBuilder.ts

+}
+
+// Takes in an object of any structure and returns the exact same object with all strings converted to check sum format.
+function replaceAddressCase(object: any) {


this is cool

daywiss · 2021-04-15T13:14:30Z

packages/bot-strategy-runner/src/index.ts

+      allBotsConfigs,
+      (botConfig: any) =>
+        Promise.all([
+          _updateProgressBar(botConfig), // As the promise progresses through bots update the progress bar.


daywiss · 2021-04-15T13:30:12Z

packages/bot-strategy-runner/src/ConfigBuilder.ts

+    const responseJson = await Promise.all(responses.map((response: any) => response.json()));
+    responseJson.forEach((contractAddressesResponse: any) => {
+      if (contractAddressesResponse.empWhitelist) whitelist = [...whitelist, ...contractAddressesResponse.empWhitelist];
+      else console.log("Global Whitelist file does not have the `empWhitelist` key or is malformed");


why not throw error?

ye, you right. will throw an error here.

well actually there was a reason for this. it is possible that the global whitelist is malformed but everything else is still fine. in that case the way it is structure here the bot will continue to work as expected, minus the ones from the whitelist.

However, this is pretty difficult to reason about so I think we should ideally throw an error like you prescribed. have updated accordingly.

daywiss · 2021-04-15T13:34:18Z

packages/bot-strategy-runner/src/LogCaptureTransport.ts

+    try {
+      // eslint-disable-next-line prefer-const
+      let { timestamp, level, error, ...args } = info;
+      if (error) {


is there a way the logger could not care about the data its logging?

so the reason it needs to do this is that the error object is of type Error and this does not log nicely at all. If you want to store this within a json object you cant easily read it or consume it within a nested structure (such as within a GCP log). This code here basically strips out all nasty punctuation and enables this to be treated as a simple string.

I'm curious how you'd prefer this to be consumed. very open to any alternative pattern!

daywiss · 2021-04-15T13:35:05Z

packages/bot-strategy-runner/src/index.ts

+// Returns a promise that is resolved after `seconds` delay. Used to limit how long a spoke can run for.
+const _rejectAfterDelay = (seconds: number | undefined, executionConfig: any) =>
+  new Promise((resolve, _) => {
+    setTimeout(resolve, (seconds ? seconds : defaultStrategyTimeout) * 1000, {


this should be rejecting based on the function name, but i understand why you would rather resolve. we are seeing the logging system leaking into completely unrelated parts of the code.

Agree, this is a bit confusing.

ye, this is a great point and something I spent a few hours trying to find a better pattern on but ended up settling on this.

The main issue is if the promise rejects (or an error is thrown within the promise) then you cant use bluebird promises. I tried out using node async and p-limit which all worked pretty well but their error handling was really poor. Node async did not have any pattern that was workable with this (that I could find). p-limit was better as you could use promise.allSettled which can handle errors via a rejected status but the code was more convoluted and even harder to read. We do something like this in the serverless hub. you can check it out here

protocol/packages/serverless-orchestration/src/ServerlessHub.js

Line 119 in 48c290f

if (result.status == "rejected") {

The pattern I ended up going for was to ensure that promises never error or reject. For example, the bot-wrapper has a try-catch block wrapping any code that could error. Then, append a standard error prop to the response from the method and check for this after the fact. This standardizes things over all promises within this context.

My question is: how would you recommend dealing with rejected promises? what pattern would you follow if you want to handle concurrency like with a bluebird promise?

Yeah, I hear you. Promise.allSettled not being available in bluebird is annoying and makes this inconvenient. I think your current implementation is reasonable!

mrice32 · 2021-04-15T14:11:13Z

packages/bot-strategy-runner/src/ConfigBuilder.ts

+  const target: any = {}; // create a new object
+
+  // deep merge the object into the target object
+  const merger = (obj: any) => {


nit: why not call this method deepMerge?

I've removed this method and am using lodash after Davids comment :)

mrice32 · 2021-04-15T14:17:33Z

packages/bot-strategy-runner/src/ConfigBuilder.ts

+// Takes in an object of any structure and returns the exact same object with all strings converted to check sum format.
+function replaceAddressCase(object: any) {
+  const stringifiedObject = JSON.stringify(object);
+  const replacedStringifiedObject = stringifiedObject.replace(/0x[a-fA-F0-9]{40}/g, toChecksumAddress);


I don't think this will cause any problems, but is it possible that this will mess with byte strings? I'm assuming that even if it does, it won't break anything. Just wanted to mention it.

ye, it could but I think that this will only modify strings of length 40 starting with 0x. bytestrings tend to be longer at 64 chars (bytes32) with a 0x at the front.

I think it will touch any strings of length greater than 40 hex characters (since the first 40 hex characters would satisfy the regex). Regardless, I'm not too worried about checksumming random byte strings > 40 characters -- just might look a little weird in the logs.

My only point was to raise the issue in case you thought this could break something (like toChecksum erroring on non-addresses or common interpreters breaking if we paste in bytes with varied casings).

packages/bot-strategy-runner/src/index.ts

mrice32 · 2021-04-15T14:20:38Z

packages/bot-strategy-runner/src/index.ts

+    // Execute all bots in a bluebird map with limited concurrency. Note that none of the `runBot` calls ever throw errors.
+    // Rather, they indicate an error via an `error` key within the response. The promise.race between the `runBot` and
+    // _rejectAfterDelay bounds how long a given strategy can run for.
+    const executionResults = await (bluebird as any).map(


Why do you have to do this cast? Do the bluebird types not contain the map function?

otherwise I get this error:

I have installed "@types/bluebird" so I'm not sure why this is the case. will do some more playing

ok I was able to address this by changing the import type. now imported as import bluebird from "bluebird"; and it works as expected without the typecast :)

mrice32 · 2021-04-15T14:28:22Z

packages/bot-strategy-runner/src/index.ts

+    // _rejectAfterDelay bounds how long a given strategy can run for.
+    const executionResults = await (bluebird as any).map(
+      allBotsConfigs,
+      (botConfig: any) =>


Don't we have well-defined types for the bot configs?

yup! good point. I have imported them and used them in this mapping.

mrice32 · 2021-04-15T14:49:11Z

packages/bot-strategy-runner/src/index.ts

+  // Construct bot configs for all enabled bots. This includes all bot specific overrides, whitelists, blacklists and any
+  // extra settings defined in the config. See the readme for full details on what is possible with this config.
+  const allBotsConfigs = await buildBotConfigs(globalWhiteList, strategyRunnerConfig);
+  console.log("allBotsConfigs", allBotsConfigs);


Is this console.log supposed to be here?

nope. have remove it!

mrice32 · 2021-04-15T14:50:06Z

packages/bot-strategy-runner/src/index.ts

+// Returns a promise that is resolved after `seconds` delay. Used to limit how long a spoke can run for.
+const _rejectAfterDelay = (seconds: number | undefined, executionConfig: any) =>
+  new Promise((resolve, _) => {
+    setTimeout(resolve, (seconds ? seconds : defaultStrategyTimeout) * 1000, {


Agree, this is a bit confusing.

mrice32

Looks great! Left a few minor comments.

One high-level question: what do you think we could do to bring down the number of any declarations in the code? Just convert all the other packages to ts or are they inevitable?

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

…col/protocol into chrismaree/bot-stratergy-runner

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

chrismaree · 2021-04-16T08:32:51Z

Looks great! Left a few minor comments.

One high-level question: what do you think we could do to bring down the number of any declarations in the code? Just convert all the other packages to ts or are they inevitable?

Thanks for your review! I have addressed all your comments/questions and nits.

regarding your any comment you right that the main thing is just converted things in ts and declaring more interfaces.

@mrice32 what's your opinion on testing in this package?

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

mrice32 · 2021-04-20T04:34:44Z

Looks great! Left a few minor comments.
One high-level question: what do you think we could do to bring down the number of any declarations in the code? Just convert all the other packages to ts or are they inevitable?

Thanks for your review! I have addressed all your comments/questions and nits.

regarding your any comment you right that the main thing is just converted things in ts and declaring more interfaces.

@mrice32 what's your opinion on testing in this package?

@chrismaree is the question whether it's okay to not have tests for it? Or how we should structure tests?

As far as how you would structure a test, I think we could do limited testing with a sample config to prove:

That the runner is reading the config correctly.
That the runner acts as expected on error.

However, I'm not super opinionated. What are your thoughts?

chrismaree · 2021-04-21T06:51:58Z

Looks great! Left a few minor comments.
One high-level question: what do you think we could do to bring down the number of any declarations in the code? Just convert all the other packages to ts or are they inevitable?

Thanks for your review! I have addressed all your comments/questions and nits.
regarding your any comment you right that the main thing is just converted things in ts and declaring more interfaces.
@mrice32 what's your opinion on testing in this package?

@chrismaree is the question whether it's okay to not have tests for it? Or how we should structure tests?

As far as how you would structure a test, I think we could do limited testing with a sample config to prove:

That the runner is reading the config correctly.

That the runner acts as expected on error.

However, I'm not super opinionated. What are your thoughts?

It was mainly about the structure of the tests. I agree with your breakdown. I will leave this for a separate PR though as there is a fair amount of code already in this PR! I also think we should have a test that validates configs are built correctly in the right structure.

chrismaree added 8 commits March 30, 2021 22:49

init

4d38910

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

fixed lint

7025af7

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

Delete yarn-error.log

de89d33

fix buiild again

fb0c7aa

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

Merge branch 'chrismaree/bot-stratergy-runner' of github.com:UMAproto…

eef3423

…col/protocol into chrismaree/bot-stratergy-runner

fixed build issues

15c8ded

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

Added readme and more comments

e594f21

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

removed test directory

51884a2

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

chrismaree commented Apr 13, 2021

View reviewed changes

Restore FindContractVersion.js

077a459

chrismaree commented Apr 13, 2021

View reviewed changes

fixed build

e59b561

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

chrismaree marked this pull request as ready for review April 13, 2021 15:13

chrismaree requested review from mrice32 and nicholaspai and removed request for mrice32 April 13, 2021 15:13

chrismaree requested review from mrice32 and daywiss April 14, 2021 08:36

chrismaree added 2 commits April 14, 2021 10:42

Merge branch 'master' into chrismaree/bot-stratergy-runner

bc4adf0

Update for find contract version

60cde9a

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

mrice32 reviewed Apr 14, 2021

View reviewed changes

nicholaspai reviewed Apr 14, 2021

View reviewed changes

packages/bot-strategy-runner/src/index.ts Outdated Show resolved Hide resolved

nicholaspai reviewed Apr 14, 2021

View reviewed changes

chrismaree and others added 2 commits April 15, 2021 10:15

Update packages/bot-strategy-runner/README.md

7fd0c8c

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

Update packages/bot-strategy-runner/src/index.ts

842d44b

Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>

lint and review nits

0561d0c

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

daywiss approved these changes Apr 15, 2021

View reviewed changes

mrice32 reviewed Apr 15, 2021

View reviewed changes

chrismaree and others added 5 commits April 16, 2021 10:10

Update packages/bot-strategy-runner/src/index.ts

96656ed

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

Update

a0a35e4

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

Merge branch 'chrismaree/bot-stratergy-runner' of github.com:UMAproto…

1fc99c4

…col/protocol into chrismaree/bot-stratergy-runner

Fixed linter

4299920

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

updated type cast

d7352eb

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

more review nits

2c16209

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

chrismaree requested review from mrice32 and nicholaspai April 19, 2021 06:01

mrice32 approved these changes Apr 20, 2021

View reviewed changes

chrismaree merged commit a748107 into master Apr 21, 2021

		"@typescript-eslint/no-var-requires": 0,
		"@typescript-eslint/no-empty-function": 0

feat(bot-strategy-runner): add generalised bot runner #2851

feat(bot-strategy-runner): add generalised bot runner #2851

Conversation

chrismaree commented Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismaree commented Apr 13, 2021

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

nicholaspai left a comment

Choose a reason for hiding this comment

chrismaree commented Apr 15, 2021

daywiss 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismaree Apr 16, 2021 • edited Loading

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

mrice32 left a comment

Choose a reason for hiding this comment

chrismaree commented Apr 16, 2021

mrice32 commented Apr 20, 2021

chrismaree commented Apr 21, 2021

chrismaree commented Apr 12, 2021 •

edited

Loading

chrismaree Apr 16, 2021 •

edited

Loading