improve(liquidator): Send log when WDF is purposefully not activated by nicholaspai · Pull Request #2432 · UMAprotocol/protocol

nicholaspai · 2021-01-19T20:05:46Z

Signed-off-by: Nick Pai npai.nyc@gmail.com

Motivation

Problem mentioned as part of #2419

Previously, this error log is being emitted even if the WDF is not being activated on purpose (i.e there is no error):

Summary

Liquidator queries LiquidatorStrategy to determine if it should emit a specialized log about the WDF not being activated yet.

…ated Signed-off-by: Nick Pai <npai.nyc@gmail.com>

coveralls · 2021-01-19T20:51:36Z

Coverage remained the same at 93.284% when pulling 39c1912 on npai/2419 into e6a54e6 on master.

chrismaree

Looks good! as discussed IRL we need to think about if we always want to be sending out these info messages. We really just need to know when the initial WDF liquidation is sent out and if the bot is running low on funds/cant extend a liquidation. The way this is set up is we'll see this info message every 5 mins, which I think is fine, but might be a bit noisy.

packages/liquidator/src/liquidator.js

chrismaree · 2021-01-20T13:02:55Z

packages/liquidator/src/liquidationStrategy.js

-      parseFloat(defenseActivationPercent)
-    )
-      return false;
+    if (!passedDefenseActivationPercent({ position, currentBlockTime })) return false;


can you not just return passedDefenseActivationPercent({ position, currentBlockTime }) at this point?

chrismaree · 2021-01-20T13:05:02Z

packages/liquidator/src/liquidator.js

+        ) {
+          this.logger.info({
+            at: "Liquidator",
+            message: "Withdrawal liveness has not passed WDF activation threshold, skipping✋",


shall we use a new emoji? this one is used in other places.

key comment, will do

daywiss

Looks great Nick, I like how you broke out the check into its own function. Approving assuming you will address @chrismaree concerns

daywiss · 2021-01-20T14:37:35Z

packages/liquidator/src/liquidator.js

+          this.defenseActivationPercent &&
+          !this.liquidationStrategy.utils.passedDefenseActivationPercent({ position, currentBlockTime })
+        ) {
+          this.logger.info({


will these logs cause pager duty to go off? I think id prefer not tripping PD on this since it could happen every time the bot runs

INFO level logs should not set of PagerDuty

nicholaspai · 2021-01-20T15:13:56Z

Looks good! as discussed IRL we need to think about if we always want to be sending out these info messages. We really just need to know when the initial WDF liquidation is sent out and if the bot is running low on funds/cant extend a liquidation. The way this is set up is we'll see this info message every 5 mins, which I think is fine, but might be a bit noisy.

Good point, given the constraint that this bot runs serverlessly this is a bit tricky. Thoughts on changing this to a debug log vs keeping it as an info? I'm not sure what we'd prefer as a team, but I kind of like the idea of having more information than not.

Co-authored-by: Chris Maree <christopher.maree@gmail.com>

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

chrismaree · 2021-01-21T09:13:29Z

packages/liquidator/index.js

  liquidatorConfig,
-  liquidatorOverridePrice
+  liquidatorOverridePrice,
+  startingBlock,


as discussed on slack, you could consider putting these within the liquidatorConfig.

I'm going to pack startingBlock and endingBlock into liquidatorConfig before creating the Liquidator.js object, but I still want to read those vars from the environment because the serverless hub sets them conveniently there

See this commit for the proposed changes in response to your comment.

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

daywiss

Looks good to me, i just have questions about how starting and ending block gets set. Is this something that gets persisted in gcp somehow and set dynamically in cloud run or what?

chrismaree

This looks great! thanks for refining this & adding all the nits.

chrismaree · 2021-01-21T20:25:20Z

packages/liquidator/index.js

    };

+    // Add block window into `liquidatorConfig`
+    let overriddenLiquidatorConfig = {


nit: does this need to be a new variable? could you not just re-asign liquidatorConfig?

liquidatorConfig = {...liquidatorConfig, startingBlock, endingBlock };

The only reason I bring this up is that it is more consistent with how I was trying to override the liquidator config in the perpetual generalization.

Good call, I originally had it set this way but ran into an error because I redeclared liquidatorConfig with a let statement.

nicholaspai · 2021-01-21T20:28:02Z

Looks good to me, i just have questions about how starting and ending block gets set. Is this something that gets persisted in gcp somehow and set dynamically in cloud run or what?

Yep look at how the Hub sets the environment here

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

chrismaree · 2021-01-22T12:12:06Z

packages/liquidator/src/liquidator.js

+              fromBlock: this.startingBlock,
+              toBlock: this.endingBlock
+            });
+            if (blockWindowHasRequestedWithdraw) {


tiny nit: This could be done more cleanly using a simply ternary operator.

const logLevel = blockWindowHasRequestedWithdraw ? "info" : "debug";

Feel free to ignore as well, this is more personal preference.

improve(liquidator): Send INFO log when WDF is purposefully not activ…

94cc42b

…ated Signed-off-by: Nick Pai <npai.nyc@gmail.com>

nicholaspai requested review from chrismaree and daywiss January 19, 2021 20:05

chrismaree approved these changes Jan 20, 2021

View reviewed changes

daywiss approved these changes Jan 20, 2021

View reviewed changes

nicholaspai and others added 3 commits January 20, 2021 10:15

Update packages/liquidator/src/liquidator.js

93a2ef1

Co-authored-by: Chris Maree <christopher.maree@gmail.com>

change emoji

32e3b94

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

Update liquidationStrategy.js

9851f10

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

nicholaspai requested review from chrismaree and daywiss January 20, 2021 16:02

daywiss approved these changes Jan 20, 2021

View reviewed changes

only send log if RequestWithdrawal was within block window

de8dce2

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

nicholaspai changed the title ~~improve(liquidator): Send INFO log when WDF is purposefully not activated~~ improve(liquidator): Send log when WDF is purposefully not activated Jan 20, 2021

update comments in test

46a3755

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

chrismaree reviewed Jan 21, 2021

View reviewed changes

pack startingBlock and endingBlock into liquidatorConfig

2717599

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

nicholaspai requested a review from chrismaree January 21, 2021 14:12

daywiss approved these changes Jan 21, 2021

View reviewed changes

chrismaree approved these changes Jan 21, 2021

View reviewed changes

redefine liquidatorConfig

39c1912

Signed-off-by: Nick Pai <npai.nyc@gmail.com>

nicholaspai requested review from chrismaree and daywiss January 21, 2021 20:32

chrismaree reviewed Jan 22, 2021

View reviewed changes

nicholaspai merged commit fdc7d3d into master Jan 22, 2021

nicholaspai mentioned this pull request Feb 5, 2021

Bot upgrades related to historical price querying and WDF #2419

Closed

chrismaree deleted the npai/2419 branch December 19, 2022 07:50

Conversation

nicholaspai commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrismaree left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

daywiss 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!

nicholaspai commented Jan 20, 2021

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!

daywiss left a comment

Choose a reason for hiding this comment

Uh oh!

chrismaree left a comment

Choose a reason for hiding this comment

Uh oh!

chrismaree Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholaspai commented Jan 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nicholaspai commented Jan 19, 2021 •

edited

Loading

coveralls commented Jan 19, 2021 •

edited

Loading

chrismaree Jan 21, 2021 •

edited

Loading