diff --git a/CHANGELOG.md b/CHANGELOG.md index 992f3d3..757102c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## v4.9.1 +### Prudentia +#### Controllers +- Allow pausing updates for a token without a configuration: This allows us to manually push rates (which requires a config) before any updates occur. +- Add another kickback prevention to PidController#manuallyPushRate and PidController unpausing. + ## v4.9.0 ### Dependencies - Upgrade adrastia-core to v4.9.0. diff --git a/contracts/rates/RateController.sol b/contracts/rates/RateController.sol index 3329e8b..fcdb5d0 100644 --- a/contracts/rates/RateController.sol +++ b/contracts/rates/RateController.sol @@ -192,13 +192,8 @@ abstract contract RateController is ERC165, HistoricalRates, IRateComputer, IUpd checkSetUpdatesPaused(); BufferMetadata storage meta = rateBufferMetadata[token]; - if (meta.maxSize == 0) { - // Uninitialized buffer means that the rate config is missing - // It doesn't make sense to pause updates if they can't occur in the first place - revert MissingConfig(token); - } - uint16 flags = rateBufferMetadata[token].flags; + uint16 flags = meta.flags; bool currentlyPaused = (flags & PAUSE_FLAG_MASK) != 0; if (currentlyPaused != paused) { @@ -208,7 +203,7 @@ abstract contract RateController is ERC165, HistoricalRates, IRateComputer, IUpd flags &= ~PAUSE_FLAG_MASK; } - rateBufferMetadata[token].flags = flags; + meta.flags = flags; emit PauseStatusChanged(token, paused); diff --git a/contracts/rates/computers/MutatedValueComputer.sol b/contracts/rates/computers/MutatedValueComputer.sol index de8d648..3d7f0d3 100644 --- a/contracts/rates/computers/MutatedValueComputer.sol +++ b/contracts/rates/computers/MutatedValueComputer.sol @@ -127,11 +127,11 @@ abstract contract MutatedValueComputer is IERC165, IRateComputer { } /** - * @notice Returns the mutated value for a given token. + * @notice Returns the raw value for a given token which will later be mutated. * @dev This is an internal virtual function that must be implemented by the derived contract to provide the - * specific logic for extracting the mutated value for the token. - * @param token The token address for which the mutated value should be computed. - * @return The mutated value for the given token. + * specific logic for extracting the raw value for the token. + * @param token The token address for which the raw value should be computed. + * @return The raw value for the given token. */ function getValue(address token) internal view virtual returns (uint256); diff --git a/contracts/rates/controllers/PidController.sol b/contracts/rates/controllers/PidController.sol index 71b0e5c..c8ed8d7 100644 --- a/contracts/rates/controllers/PidController.sol +++ b/contracts/rates/controllers/PidController.sol @@ -198,6 +198,12 @@ abstract contract PidController is RateController { } function _manuallyPushRate(address token, uint64 target, uint64 current, uint256 amount) internal virtual override { + if (pidData[token].config.kPDenominator == 0) { + // We're missing a PID config, so we can't manually push a rate. + // We require the PID config to be set so that we can protect against large jumps in the output rate. + revert MissingPidConfig(token); + } + super._manuallyPushRate(token, target, current, amount); // Reinitialize the PID controller to avoid a large jump in the output rate with the next update. @@ -249,7 +255,8 @@ abstract contract PidController is RateController { /// @dev Initializes the PID controller state for a given token. /// @param token The address of the token for which to initialize the PID controller. function initializePid(address token) internal virtual { - PidState storage pidState = pidData[token].state; + PidData storage pid = pidData[token]; + PidState storage pidState = pid.state; BufferMetadata storage meta = rateBufferMetadata[token]; (int256 input, int256 err) = getSignedInputAndError(token); @@ -259,7 +266,23 @@ abstract contract PidController is RateController { if (meta.size > 0) { // We have a past rate, so set the iTerm to it to avoid a large jump. // We don't need to clamp this because the last rate was already clamped. - pidState.iTerm = int256(uint256(getLatestRate(token).current)); + + // Calculate the pTerm, if the PID config is set + PidConfig memory pidConfig = pid.config; + int256 pTerm = pidConfig.kPDenominator == 0 + ? int256(0) + : calculatePTerm( + input, + err, + pidConfig.kPNumerator, + pidConfig.kPDenominator, + pidConfig.proportionalOnMeasurement + ); + + // Subtract the pTerm from the output rate to get the iTerm. We subtract the pTerm to prevent a large jump + // in the output rate with the next update (as something close to it will likely be added to the iTerm with + // the next update to calculate the next rate). + pidState.iTerm = int256(uint256(getLatestRate(token).current)) - pTerm; } } @@ -306,6 +329,20 @@ abstract contract PidController is RateController { return int256(uint256(clamped)); } + function calculatePTerm( + int256 input, + int256 err, + int32 kPNumerator, + uint32 kPDenominator, + bool proportionalOnMeasurement + ) internal pure virtual returns (int256) { + if (proportionalOnMeasurement) { + return -(int256(kPNumerator) * input) / int256(uint256(kPDenominator)); + } else { + return (int256(kPNumerator) * err) / int256(uint256(kPDenominator)); + } + } + function computeNextPidRate( address token ) internal view virtual returns (uint64 target, uint64 current, PidState memory newPidState) { @@ -336,12 +373,13 @@ abstract contract PidController is RateController { (int256 input, int256 err) = getSignedInputAndError(token); // Compute proportional - int256 pTerm; - if (pidConfig.proportionalOnMeasurement) { - pTerm = -(int256(pidConfig.kPNumerator) * input) / int256(uint256(pidConfig.kPDenominator)); - } else { - pTerm = (int256(pidConfig.kPNumerator) * err) / int256(uint256(pidConfig.kPDenominator)); - } + int256 pTerm = calculatePTerm( + input, + err, + pidConfig.kPNumerator, + pidConfig.kPDenominator, + pidConfig.proportionalOnMeasurement + ); // Compute integral int256 previousITerm = pidState.iTerm; diff --git a/package.json b/package.json index 4d7c0a5..bef256c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@adrastia-oracle/adrastia-periphery", - "version": "4.9.0", + "version": "4.9.1", "main": "index.js", "author": "TRILEZ SOFTWARE INC.", "license": "BUSL-1.1", diff --git a/test/rates/rate-controller.js b/test/rates/rate-controller.js index fe3b615..a6b9d58 100644 --- a/test/rates/rate-controller.js +++ b/test/rates/rate-controller.js @@ -3310,6 +3310,39 @@ function describePidControllerUpdateTests(deployFunc, getController) { expect(currentRate).to.equal(startingRate); }); + it("Manually pushing a rate does not result in kickback with the next update", async function () { + const controller = await getController(); + + const period = await controller.period(); + + // Set input and target to the same 90% (using 8 decimals of input precision) + const targetRate = ethers.utils.parseUnits("0.90", 8); + const inputRate = ethers.utils.parseUnits("0.10", 8); + await controller.setTarget(GRT, targetRate); + await controller.setInput(GRT, inputRate); + + const startingRate = ethers.utils.parseUnits("0.234", 8); + await controller.manuallyPushRate(GRT, startingRate, startingRate, 1); + + // Advance the block time by the period + await timeAndMine.increaseTime(period.toNumber()); + + // Update the rate + await controller.update(ethers.utils.defaultAbiCoder.encode(["address"], [GRT])); + + // Get the current rate + const currentRate = await controller.computeRate(GRT); + + // Calculate expected rate: The proportional component should not kick + const error = targetRate.sub(inputRate); + const expectedChange = error.mul(DEFAULT_PID_CONFIG.kINumerator).div(DEFAULT_PID_CONFIG.kIDenominator); + const expectedRate = startingRate.add(expectedChange); + + // Confirm that the new rate equals the manually pushed rate + // If the PID controller is not reinitialized, the rate will be the same as rate before the manually pushed rate (0) + expect(currentRate).to.equal(expectedRate); + }); + it("It works with a transformer", async function () { const deployment1 = await deployFunc(); const deployment2 = await deployFunc(); @@ -4128,6 +4161,7 @@ function createDescribeCanUpdateTests(isPidController) { function describeTests( contractName, deployFunc, + setDefaultConfigFunc, describeComputeRateTests, describeNeedsUpdateTests, describeUpdateTests, @@ -4250,12 +4284,16 @@ function describeTests( await expect(controller.connect(signer).setUpdatesPaused(GRT, true)).to.not.be.reverted; }); - it("Should revert if the token is missing a config", async function () { - await expect(controller.setUpdatesPaused(USDC, true)).to.be.revertedWith("MissingConfig").withArgs(USDC); + it("Should not revert if the token is missing a config", async function () { + await expect(controller.setUpdatesPaused(USDC, true)) + .to.emit(controller, "PauseStatusChanged") + .withArgs(USDC, true); - // Sanity check that we can successfully call the function if we have the config - await controller.setConfig(USDC, DEFAULT_CONFIG); - await expect(controller.setUpdatesPaused(USDC, true)).to.not.be.reverted; + // Sanity check that the changes were made + expect(await controller.areUpdatesPaused(USDC)).to.equal(true); + + // Sanity check that USDC is missing a config + await expect(controller.getConfig(USDC)).to.be.revertedWith("MissingConfig").withArgs(USDC); }); it("Should emit an event when the updates are paused", async function () { @@ -5659,7 +5697,7 @@ function describeTests( await controller.grantRole(UPDATE_PAUSE_ADMIN_ROLE, signer.address); // Set config for GRT - await controller.setConfig(GRT, DEFAULT_CONFIG); + await setDefaultConfigFunc(controller, GRT); }); it("Should revert if the caller does not have the ADMIN role", async function () { @@ -5690,11 +5728,11 @@ function describeTests( const rate = ethers.utils.parseUnits("0.1234", 18); await expect(controller.manuallyPushRate(USDC, rate, rate, 1)) - .to.be.revertedWith("MissingConfig") + .to.be.revertedWith(/MissingPidConfig|MissingConfig/) .withArgs(USDC); // Sanity check that it works if we set a config - await controller.setConfig(USDC, DEFAULT_CONFIG); + await setDefaultConfigFunc(controller, USDC); await expect(controller.manuallyPushRate(USDC, rate, rate, 1)).to.not.be.reverted; }); @@ -5811,9 +5849,19 @@ async function initializePidController(controller) { await controller.setPidConfig(GRT, DEFAULT_PID_CONFIG); } +async function setDefaultStandardConfig(controller, token) { + await controller.setConfig(token, DEFAULT_CONFIG); +} + +async function setDefaultPidConfig(controller, token) { + await controller.setConfig(token, DEFAULT_CONFIG); + await controller.setPidConfig(token, DEFAULT_PID_CONFIG); +} + describeTests( "RateController", deployStandardController, + setDefaultStandardConfig, describeStandardControllerComputeRateTests, createDescribeStandardControllerNeedsUpdateTests(false, undefined, undefined), createDescribeStandardControllerUpdateTests(undefined, true, undefined), @@ -5822,6 +5870,7 @@ describeTests( describeTests( "PidController", deployPidController, + setDefaultPidConfig, describePidControllerComputeRateTests, createDescribeStandardControllerNeedsUpdateTests( true,