node(governor): refactor dayLength and dailyLimit fields and change wording
#4431
Conversation
johnsaigle
added
governor
quality
johnsaigle
force-pushed
the
gov-daylength-refactor
branch
from
5d423e0 to
f9bd686
Compare
johnsaigle
requested review from
SEJeff,
djb15,
mdulin2 and
pleasew8t
as code owners
node/pkg/governor/governor.go
Outdated
| emitterAddr vaa.Address | ||
| // The amount of notional value that can leave the chain during the period | ||
| // defined by the sliding window. | ||
| dailyLimit uint64 |
There was a problem hiding this comment.
Makes sense to change this variable name too imo
There was a problem hiding this comment.
@djb15 I changed it to USDLimit and updated all the code and documentation references.
There was a problem hiding this comment.
This will be breaking change so I updated the PR description.
If this ends up not being worth it I'm fine to leave dailyLimit as-is and just remove the day length field.
There was a problem hiding this comment.
Ahhh ok yeah if Bruce and co don't think it's worth making a breaking change here then I agree you can revert back to the commit pre this change
There was a problem hiding this comment.
I think it's reasonable to change it if you want. Anyone who uses it will just need to make a change whenever they decide to pick up a new version of the package. 🤷
johnsaigle
changed the title
dayLength and dailyLimit fields and change wording
|
For historical reference, But I guess we don't really do that much governor testing in tilt, so it doesn't matter. |
johnsaigle
force-pushed
the
gov-daylength-refactor
branch
from
03cc0b7 to
b642527
Compare
- Remove the daylength field configured on the Governor in favour of using a constant. This was always configured to be the same value, and it unclear conceptually to change the length of minutes in a day, when in fact we want to change the duration of a sliding window of time. - Change wording in code comments to refer to a sliding window. This more accurate reflects that the Governor works with a rolling window rather than as discrete calendar days of 24h. - Use separate constants for mainnet and devnet deployments - Removed redundant time.Duration calls
johnsaigle
force-pushed
the
gov-daylength-refactor
branch
from
b642527 to
5f4f35f
Compare
|
Adding draft mode until I get around to fixing the conflicts |
daylengthfield configured on the Governor in favour of using a constant. This was always configured to be the same value, and it unclear conceptually to change the length of minutes in a day, when in fact we want to change the duration of a sliding window of time.dailyLimittoUSDLimitand add code comments explaining the relationship to the sliding window. Update all code and documentation references accordingly.Note: because
dailyLimitis an exported field, and is included in log ouput, this likely constitutes a breaking change for packages that import the Governor code as well as for monitoring tools. They will need to update their code to use the new variable name.