Add flag 'ignoreMaxPriceIfNeeded' #3127
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3127 +/- ##
===================================================
- Coverage 57.35266% 57.34447% -0.00819%
===================================================
Files 92 92
Lines 15797 15801 +4
===================================================
+ Hits 9060 9061 +1
- Misses 6133 6136 +3
Partials 604 604
Continue to review full report in Codecov by Sentry.
|
cmd/livepeer/starter/starter.go
Outdated
| @@ -204,6 +205,7 @@ func DefaultLivepeerConfig() LivepeerConfig { | |||
| defaultMaxTotalEV := "20000000000000" | |||
| defaultDepositMultiplier := 1 | |||
| defaultMaxPricePerUnit := "0" | |||
| defaultEnforceMaxPrice := false | |||
There was a problem hiding this comment.
I think conceptually it would make more sense for the max price to be enforced by default (equivalent to having true here [1]). That because the non-enforcing behavior can be very misleading for independent node operators, being a behavior that only really makes sense for Studio right now that prefers to overpay than to deny service. On the Studio side, we can toggle the flag in our deployment easily enough.
[1] Even though we could just change the default to true, it is always cumbersome to have boolean flags that default to true since you can't disable them as easily as enabling (e.g. just --enforce-max-price), and sometimes you can't disable them at all (mist haha). So my suggestion is to have an "inverse" flag here, which actually means enabling the feature of ignoring the max price if no Os match. WDYT of --ignore-max-price-if-needed, defaulting to false, or sth like that?
There was a problem hiding this comment.
I think conceptually it would make more sense for the max price to be enforced by default (equivalent to having
truehere [1]). That because the non-enforcing behavior can be very misleading for independent node operators, being a behavior that only really makes sense for Studio right now that prefers to overpay than to deny service. On the Studio side, we can toggle the flag in our deployment easily enough.[1] Even though we could just change the default to
true, it is always cumbersome to have boolean flags that default to true since you can't disable them as easily as enabling (e.g. just--enforce-max-price), and sometimes you can't disable them at all (mist haha). So my suggestion is to have an "inverse" flag here, which actually means enabling the feature of ignoring the max price if no Os match. WDYT of--ignore-max-price-if-needed, defaulting tofalse, or sth like that?
I'm ok with this. @rickstaa wdyt?
There was a problem hiding this comment.
I agree with Victor's reasoning here
There was a problem hiding this comment.
@thomshutt I already discussed this with @victorges async and I am onboard with the suggested changes. Feel free to merge with the suggested changes 👍. Thanks for creating this pull request.
There was a problem hiding this comment.
Updated. @thomshutt please approve the PR and I'll merge
leszko
force-pushed
the
rafal/add-flag-enforce-max-price
branch
from
8ae8632 to
8c83921
Compare
cmd/livepeer/starter/starter.go
Outdated
| @@ -294,6 +296,7 @@ func DefaultLivepeerConfig() LivepeerConfig { | |||
| MaxTotalEV: &defaultMaxTotalEV, | |||
| DepositMultiplier: &defaultDepositMultiplier, | |||
| MaxPricePerUnit: &defaultMaxPricePerUnit, | |||
| IgnoreMaxPriceIfNeeded: &defaultEnforceMaxPrice, | |||
There was a problem hiding this comment.
@leszko looks good to me thanks! However is there a reason the default parameter name was not refactored while the test was?
There was a problem hiding this comment.
Ohh, sorry. Updated now. Also updated the PR description. PTAL.
This flag decides what to do when all Os have a price above the
maxPricePerPixeldefined by B:true=> Use an O with the price abovemaxPricePerPixelfalse=> Do not use any O (do not work)