From eb7328a18ab97d453d9048d3afc94309b7b4f91e Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Thu, 5 Mar 2020 09:24:17 -0600 Subject: [PATCH 01/10] Time delay Governable value changes The time delay ensures that users have more information regarding the state of the contract. This is to avoid situatoins where the state of a Deposit is unpredictable due to governance changes earlier within the same block. --- .../contracts/system/TBTCSystem.sol | 187 ++++++++++++++---- 1 file changed, 154 insertions(+), 33 deletions(-) diff --git a/implementation/contracts/system/TBTCSystem.sol b/implementation/contracts/system/TBTCSystem.sol index fca3fc666..3b585a4bb 100644 --- a/implementation/contracts/system/TBTCSystem.sol +++ b/implementation/contracts/system/TBTCSystem.sol @@ -24,6 +24,15 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { using SafeMath for uint256; + event LotSizesUpdateStarted(uint256[] _lotSizes, uint256 _timestamp); + event SignerFeeDivisorUpdateStarted(uint256 _signerFeeDivisor, uint256 _timestamp); + event CollateralizationThresholdsUpdateStarted( + uint128 _initialCollateralizedPercent, + uint128 _undercollateralizedThresholdPercent, + uint128 _severelyUndercollateralizedThresholdPercent, + uint256 _timestamp + ); + event LotSizesUpdated(uint256[] _lotSizes); event AllowNewDepositsUpdated(bool _allowNewDeposits); event SignerFeeDivisorUpdated(uint256 _signerFeeDivisor); @@ -49,6 +58,18 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { uint128 private severelyUndercollateralizedThresholdPercent = 110; // percent uint256[] lotSizesSatoshis = [10**5, 10**6, 10**7, 2 * 10**7, 5 * 10**7, 10**8]; // [0.001, 0.01, 0.1, 0.2, 0.5, 1.0] BTC + uint256 constant governanceTimeDelay = 1 hours; + + uint256 private signerFeeDivisorChangeInitiated; + uint256 private lotSizesChangeInitiated; + uint256 private collateralizationThresholdsChangeInitiated; + + uint256 private newSignerFeeDivisor; + uint256[] newLotSizesSatoshis; + uint128 private newInitialCollateralizedPercent; + uint128 private newUndercollateralizedThresholdPercent; + uint128 private newSeverelyUndercollateralizedThresholdPercent; + constructor(address _priceFeed, address _relay) public { priceFeed = _priceFeed; relay = _relay; @@ -125,57 +146,50 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { pausedDuration.sub(block.timestamp.sub(pausedTimestamp)); } + /// @notice Gets the system signer fee divisor. + /// @return The signer fee divisor. + function getSignerFeeDivisor() external view returns (uint256) { return signerFeeDivisor; } + /// @notice Set the system signer fee divisor. + /// @dev This can be finalized by callingm `finalizeSignerFeeDivisorUpdate` + /// Anytime after `governanceTimeDelay` has elapsed. /// @param _signerFeeDivisor The signer fee divisor. - function setSignerFeeDivisor(uint256 _signerFeeDivisor) + function beginSignerFeeDivisorUpdate(uint256 _signerFeeDivisor) external onlyOwner { require(_signerFeeDivisor > 9, "Signer fee divisor must be greater than 9, for a signer fee that is <= 10%."); - signerFeeDivisor = _signerFeeDivisor; - emit SignerFeeDivisorUpdated(_signerFeeDivisor); + newSignerFeeDivisor = _signerFeeDivisor; + signerFeeDivisorChangeInitiated = block.timestamp; + emit SignerFeeDivisorUpdateStarted(_signerFeeDivisor, block.timestamp); } - /// @notice Gets the system signer fee divisor. - /// @return The signer fee divisor. - function getSignerFeeDivisor() external view returns (uint256) { return signerFeeDivisor; } - /// @notice Set the allowed deposit lot sizes. /// @dev Lot size array should always contain 10**8 satoshis (1BTC value) + /// This can be finalized by calling `finalizeLotSizesUpdate` + /// Anytime after `governanceTimeDelay` has elapsed. /// @param _lotSizes Array of allowed lot sizes. - function setLotSizes(uint256[] calldata _lotSizes) external onlyOwner { + function beginLotSizesUpdate(uint256[] calldata _lotSizes) + external onlyOwner + { for( uint i = 0; i < _lotSizes.length; i++){ if (_lotSizes[i] == 10**8){ lotSizesSatoshis = _lotSizes; - emit LotSizesUpdated(_lotSizes); + emit LotSizesUpdateStarted(_lotSizes, block.timestamp); + newLotSizesSatoshis = _lotSizes; + lotSizesChangeInitiated = block.timestamp; return; } } revert("Lot size array must always contain 1BTC"); } - /// @notice Gets the allowed lot sizes - /// @return Uint256 array of allowed lot sizes - function getAllowedLotSizes() external view returns (uint256[] memory){ - return lotSizesSatoshis; - } - - /// @notice Check if a lot size is allowed. - /// @param _lotSizeSatoshis Lot size to check. - /// @return True if lot size is allowed, false otherwise. - function isAllowedLotSize(uint256 _lotSizeSatoshis) external view returns (bool){ - for( uint i = 0; i < lotSizesSatoshis.length; i++){ - if (lotSizesSatoshis[i] == _lotSizeSatoshis){ - return true; - } - } - return false; - } - /// @notice Set the system collateralization levels + /// @dev This can be finalized by calling `finalizeCollateralizationThresholdsUpdate` + /// Anytime after `governanceTimeDelay` has elapsed. /// @param _initialCollateralizedPercent default signing bond percent for new deposits /// @param _undercollateralizedThresholdPercent first undercollateralization trigger /// @param _severelyUndercollateralizedThresholdPercent second undercollateralization trigger - function setCollateralizationThresholds( + function beginCollateralizationThresholdsUpdate( uint128 _initialCollateralizedPercent, uint128 _undercollateralizedThresholdPercent, uint128 _severelyUndercollateralizedThresholdPercent @@ -192,16 +206,93 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { _undercollateralizedThresholdPercent > _severelyUndercollateralizedThresholdPercent, "Severe undercollateralized threshold must be < undercollateralized threshold" ); - initialCollateralizedPercent = _initialCollateralizedPercent; - undercollateralizedThresholdPercent = _undercollateralizedThresholdPercent; - severelyUndercollateralizedThresholdPercent = _severelyUndercollateralizedThresholdPercent; - emit CollateralizationThresholdsUpdated( + + newInitialCollateralizedPercent = _initialCollateralizedPercent; + newUndercollateralizedThresholdPercent = _undercollateralizedThresholdPercent; + newSeverelyUndercollateralizedThresholdPercent = _severelyUndercollateralizedThresholdPercent; + collateralizationThresholdsChangeInitiated = block.timestamp; + emit CollateralizationThresholdsUpdateStarted( _initialCollateralizedPercent, _undercollateralizedThresholdPercent, - _severelyUndercollateralizedThresholdPercent + _severelyUndercollateralizedThresholdPercent, + block.timestamp ); } + /// @notice Finish setting the system signer fee divisor. + /// @dev `beginSignerFeeDivisorUpdate` must be called first, once `governanceTimeDelay` + /// has passed, this function can be called to set the signer fee divisor to the + /// value set in `beginSignerFeeDivisorUpdate` + function finalizeSignerFeeDivisorUpdate() + external onlyOwner + { + require( + block.timestamp.sub(signerFeeDivisorChangeInitiated) >= governanceTimeDelay, + "Timer not elapsed, or change not initiated" + ); + signerFeeDivisor = newSignerFeeDivisor; + emit SignerFeeDivisorUpdated(newSignerFeeDivisor); + newSignerFeeDivisor = 0; + signerFeeDivisorChangeInitiated = 0; + } + /// @notice Finish setting the accepted system lot sizes. + /// @dev `beginLotSizesUpdate` must be called first, once `governanceTimeDelay` + /// has passed, this function can be called to set the lot sizes to the + /// value set in `beginLotSizesUpdate` + function finalizeLotSizesUpdate() external onlyOwner { + require( + block.timestamp.sub(lotSizesChangeInitiated) >= governanceTimeDelay, + "Timer not elapsed, or change not initiated" + ); + lotSizesSatoshis = newLotSizesSatoshis; + emit LotSizesUpdated(newLotSizesSatoshis); + lotSizesChangeInitiated = 0; + newLotSizesSatoshis.length = 0; + } + + /// @notice Gets the allowed lot sizes + /// @return Uint256 array of allowed lot sizes + function getAllowedLotSizes() external view returns (uint256[] memory){ + return lotSizesSatoshis; + } + + /// @notice Check if a lot size is allowed. + /// @param _lotSizeSatoshis Lot size to check. + /// @return True if lot size is allowed, false otherwise. + function isAllowedLotSize(uint256 _lotSizeSatoshis) external view returns (bool){ + for( uint i = 0; i < lotSizesSatoshis.length; i++){ + if (lotSizesSatoshis[i] == _lotSizeSatoshis){ + return true; + } + } + return false; + } + + /// @notice Finish setting the system collateralization levels + /// @dev `beginCollateralizationThresholdsUpdate` must be called first, once `governanceTimeDelay` + /// has passed, this function can be called to set the collateralization thresholds to the + /// value set in `beginCollateralizationThresholdsUpdate` + function finalizeCollateralizationThresholdsUpdate() external onlyOwner { + require( + block.timestamp.sub(collateralizationThresholdsChangeInitiated) >= governanceTimeDelay, + "Timer not elapsed, or change not initiated" + ); + initialCollateralizedPercent = newInitialCollateralizedPercent; + undercollateralizedThresholdPercent = newUndercollateralizedThresholdPercent; + severelyUndercollateralizedThresholdPercent = newSeverelyUndercollateralizedThresholdPercent; + + emit CollateralizationThresholdsUpdated( + newInitialCollateralizedPercent, + newUndercollateralizedThresholdPercent, + newSeverelyUndercollateralizedThresholdPercent + ); + + newInitialCollateralizedPercent = 0; + newUndercollateralizedThresholdPercent = 0; + newSeverelyUndercollateralizedThresholdPercent = 0; + collateralizationThresholdsChangeInitiated = 0; + } + /// @notice Get the system undercollateralization level for new deposits function getUndercollateralizedThresholdPercent() external view returns (uint128) { return undercollateralizedThresholdPercent; @@ -217,6 +308,36 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { return initialCollateralizedPercent; } + /// @notice Get the time remaining until the collateralization thresholds can be updated. + function getRemainingCollateralizationUpdateTime() external view returns (uint256) { + require(collateralizationThresholdsChangeInitiated > 0, "Update not initiated"); + + uint256 elapsed = block.timestamp.sub(collateralizationThresholdsChangeInitiated); + return (elapsed >= governanceTimeDelay)? + 0: + governanceTimeDelay.sub(elapsed); + } + + /// @notice Get the time remaining until the lot sizes can be updated. + function getRemainingLotSizesUpdateTime() external view returns (uint256) { + require(lotSizesChangeInitiated > 0, "Update not initiated"); + + uint256 elapsed = block.timestamp.sub(lotSizesChangeInitiated); + return (elapsed >= governanceTimeDelay)? + 0: + governanceTimeDelay.sub(elapsed); + } + + /// @notice Get the time remaining until the signer fee divisor can be updated. + function geRemainingSignerFeeDivisorUpdateTime() external view returns (uint256) { + require(signerFeeDivisorChangeInitiated > 0, "Update not initiated"); + + uint256 elapsed = block.timestamp.sub(signerFeeDivisorChangeInitiated); + return (elapsed >= governanceTimeDelay)? + 0: + governanceTimeDelay.sub(elapsed); + } + // Price Feed /// @notice Get the price of one satoshi in wei. From 3375a2ac971578b02ba49af596fa19e94833c0d8 Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Thu, 5 Mar 2020 09:25:03 -0600 Subject: [PATCH 02/10] Test governable value time delay --- implementation/test/TBTCSystemTest.js | 202 +++++++++++++++++++++----- 1 file changed, 164 insertions(+), 38 deletions(-) diff --git a/implementation/test/TBTCSystemTest.js b/implementation/test/TBTCSystemTest.js index 901175d96..45a843c73 100644 --- a/implementation/test/TBTCSystemTest.js +++ b/implementation/test/TBTCSystemTest.js @@ -5,7 +5,7 @@ const { restoreSnapshot, } = require("../testHelpers/helpers/snapshot.js") const {accounts, contract, web3} = require("@openzeppelin/test-environment") -const {BN, expectRevert} = require("@openzeppelin/test-helpers") +const {BN, expectRevert, expectEvent} = require("@openzeppelin/test-helpers") const {expect} = require("chai") const TBTCSystem = contract.fromArtifact("TBTCSystem") @@ -69,55 +69,181 @@ describe("TBTCSystem", async function() { }) }) - describe("setSignerFeeDivisor", async () => { - it("sets the signer fee", async () => { - await tbtcSystem.setSignerFeeDivisor(new BN("201")) + describe("update Signer fee", async () => { + describe("beginSignerFeeDivisorUpdate", async () => { + const newFee = new BN("201") + it("executes and fires SignerFeeDivisorUpdateStarted event", async () => { + receipt = await tbtcSystem.beginSignerFeeDivisorUpdate(newFee) - const signerFeeDivisor = await tbtcSystem.getSignerFeeDivisor() - expect(signerFeeDivisor).to.eq.BN(new BN("201")) + expectEvent(receipt, "SignerFeeDivisorUpdateStarted", { + _signerFeeDivisor: newFee, + }) + }) + + it("reverts if msg.sender != owner", async () => { + await expectRevert.unspecified( + tbtcSystem.beginSignerFeeDivisorUpdate(newFee, { + from: accounts[1], + }), + "", + ) + }) + + it("reverts if fee divisor is smaller than 10", async () => { + await expectRevert( + tbtcSystem.beginSignerFeeDivisorUpdate(new BN("9")), + "Signer fee divisor must be greater than 9, for a signer fee that is <= 10%.", + ) + }) }) - it("reverts if msg.sender != owner", async () => { - await expectRevert.unspecified( - tbtcSystem.setSignerFeeDivisor(new BN("201"), { - from: accounts[1], - }), - "", - ) + describe("finalizeSignerFeeDivisorUpdate", async () => { + it("reverts if the governance timer has not elapsed", async () => { + await expectRevert( + tbtcSystem.finalizeSignerFeeDivisorUpdate(), + "Timer not elapsed, or change not initiated", + ) + }) + + it("updates signer fee and fires SignerFeeDivisorUpdated event", async () => { + const remainingTime = await tbtcSystem.geRemainingSignerFeeDivisorUpdateTime() + + await increaseTime(remainingTime.toNumber() + 1) + + receipt = await tbtcSystem.finalizeSignerFeeDivisorUpdate() + + expectEvent(receipt, "SignerFeeDivisorUpdated", { + _signerFeeDivisor: new BN("201"), + }) + }) }) }) - describe("setLotSizes", async () => { - it("sets a different lot size array", async () => { - const blockNumber = await web3.eth.getBlock("latest").number - const lotSizes = [10 ** 8, 10 ** 6] - await tbtcSystem.setLotSizes(lotSizes) + describe("update lot sizes", async () => { + const lotSizes = [new BN(10 ** 8), new BN(10 ** 6)] + describe("beginLotSizesUpdate", async () => { + it("executes and emits a LotSizesUpdateStarted event", async () => { + const block = await web3.eth.getBlock("latest") + const receipt = await tbtcSystem.beginLotSizesUpdate(lotSizes) + expectEvent(receipt, "LotSizesUpdateStarted", {}) + expect(receipt.logs[0].args[0][0]).to.eq.BN(lotSizes[0]) + expect(receipt.logs[0].args[0][1]).to.eq.BN(lotSizes[1]) + expect(receipt.logs[0].args[1]).to.eq.BN(block.timestamp) + }) - const eventList = await tbtcSystem.getPastEvents("LotSizesUpdated", { - fromBlock: blockNumber, - toBlock: "latest", + it("reverts if lot size array is empty", async () => { + const lotSizes = [] + await expectRevert( + tbtcSystem.beginLotSizesUpdate(lotSizes), + "Lot size array must always contain 1BTC", + ) + }) + + it("reverts if lot size array does not contain a 1BTC lot size", async () => { + const lotSizes = [10 ** 7] + await expectRevert( + tbtcSystem.beginLotSizesUpdate(lotSizes), + "Lot size array must always contain 1BTC", + ) }) - expect(eventList.length).to.equal(1) - expect(eventList[0].returnValues._lotSizes).to.eql([ - "100000000", - "1000000", - ]) // deep equality check }) - it("reverts if lot size array is empty", async () => { - const lotSizes = [] - await expectRevert( - tbtcSystem.setLotSizes(lotSizes), - "Lot size array must always contain 1BTC", - ) + describe("finalizeLotSizesUpdate", async () => { + it("reverts if the governance timer has not elapsed", async () => { + await expectRevert( + tbtcSystem.finalizeLotSizesUpdate(), + "Timer not elapsed, or change not initiated", + ) + }) + + it("updates lot sizes and fires LotSizesUpdated event", async () => { + const remainingTime = await tbtcSystem.getRemainingLotSizesUpdateTime() + + await increaseTime(remainingTime.toNumber() + 1) + + receipt = await tbtcSystem.finalizeLotSizesUpdate() + + expectEvent(receipt, "LotSizesUpdated", {}) + expect(receipt.logs[0].args._lotSizes[0]).to.eq.BN(lotSizes[0]) + expect(receipt.logs[0].args._lotSizes[1]).to.eq.BN(lotSizes[1]) + }) }) + }) - it("reverts if lot size array does not contain a 1BTC lot size", async () => { - const lotSizes = [10 ** 7] - await expectRevert( - tbtcSystem.setLotSizes(lotSizes), - "Lot size array must always contain 1BTC", - ) + describe("update collateralization thresholds", async () => { + const initialPercent = new BN("150") + const undercollateralizedPercent = new BN("130") + const severelyUndercollateralizedPercent = new BN("120") + describe("beginCollateralizationThresholdsUpdate", async () => { + it("executes and fires CollateralizationThresholdsUpdateStarted event", async () => { + receipt = await tbtcSystem.beginCollateralizationThresholdsUpdate( + initialPercent, + undercollateralizedPercent, + severelyUndercollateralizedPercent, + ) + + expectEvent(receipt, "CollateralizationThresholdsUpdateStarted", { + _initialCollateralizedPercent: initialPercent, + _undercollateralizedThresholdPercent: undercollateralizedPercent, + _severelyUndercollateralizedThresholdPercent: severelyUndercollateralizedPercent, + }) + }) + + it("reverts if Initial collateralized percent > 300", async () => { + await expectRevert( + tbtcSystem.beginCollateralizationThresholdsUpdate( + new BN("301"), + new BN("130"), + new BN("120"), + ), + "Initial collateralized percent must be <= 300%", + ) + }) + + it("reverts if Undercollateralized threshold > initial collateralize percent", async () => { + await expectRevert( + tbtcSystem.beginCollateralizationThresholdsUpdate( + new BN("150"), + new BN("160"), + new BN("120"), + ), + "Undercollateralized threshold must be < initial collateralized percent", + ) + }) + + it("reverts if Severe undercollateralized threshold > undercollateralized threshold", async () => { + await expectRevert( + tbtcSystem.beginCollateralizationThresholdsUpdate( + new BN("150"), + new BN("130"), + new BN("131"), + ), + "Severe undercollateralized threshold must be < undercollateralized threshold", + ) + }) + }) + + describe("finalizeCollateralizationThresholdsUpdate", async () => { + it("reverts if the governance timer has not elapsed", async () => { + await expectRevert( + tbtcSystem.finalizeCollateralizationThresholdsUpdate(), + "Timer not elapsed, or change not initiated", + ) + }) + + it("updates collateralization thresholds and fires CollateralizationThresholdsUpdated event", async () => { + const remainingTime = await tbtcSystem.getRemainingCollateralizationUpdateTime() + + await increaseTime(remainingTime.toNumber() + 1) + + receipt = await tbtcSystem.finalizeCollateralizationThresholdsUpdate() + + expectEvent(receipt, "CollateralizationThresholdsUpdated", { + _initialCollateralizedPercent: initialPercent, + _undercollateralizedThresholdPercent: undercollateralizedPercent, + _severelyUndercollateralizedThresholdPercent: severelyUndercollateralizedPercent, + }) + }) }) }) From b0b8b5fd578580ebd66134b6c20f24b4bd58e2a7 Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Fri, 6 Mar 2020 09:18:11 -0600 Subject: [PATCH 03/10] Extract delay check to a modifier --- .../contracts/system/TBTCSystem.sol | 42 +++++++++++-------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/implementation/contracts/system/TBTCSystem.sol b/implementation/contracts/system/TBTCSystem.sol index 3b585a4bb..e1e6fcc83 100644 --- a/implementation/contracts/system/TBTCSystem.sol +++ b/implementation/contracts/system/TBTCSystem.sol @@ -151,8 +151,8 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { function getSignerFeeDivisor() external view returns (uint256) { return signerFeeDivisor; } /// @notice Set the system signer fee divisor. - /// @dev This can be finalized by callingm `finalizeSignerFeeDivisorUpdate` - /// Anytime after `governanceTimeDelay` has elapsed. + /// @dev This can be finalized by calling `finalizeSignerFeeDivisorUpdate` + /// Anytime after `governanceTimeDelay` has elapsed. /// @param _signerFeeDivisor The signer fee divisor. function beginSignerFeeDivisorUpdate(uint256 _signerFeeDivisor) external onlyOwner @@ -219,17 +219,25 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { ); } + modifier onlyAfterDelay(uint256 _changeInitializedTimestamp) { + require(_changeInitializedTimestamp > 0, "Change not initiated"); + require( + block.timestamp.sub(_changeInitializedTimestamp) >= + governanceTimeDelay, + "Timer not elapsed" + ); + _; + } + /// @notice Finish setting the system signer fee divisor. /// @dev `beginSignerFeeDivisorUpdate` must be called first, once `governanceTimeDelay` /// has passed, this function can be called to set the signer fee divisor to the /// value set in `beginSignerFeeDivisorUpdate` function finalizeSignerFeeDivisorUpdate() - external onlyOwner + external + onlyOwner + onlyAfterDelay(signerFeeDivisorChangeInitiated) { - require( - block.timestamp.sub(signerFeeDivisorChangeInitiated) >= governanceTimeDelay, - "Timer not elapsed, or change not initiated" - ); signerFeeDivisor = newSignerFeeDivisor; emit SignerFeeDivisorUpdated(newSignerFeeDivisor); newSignerFeeDivisor = 0; @@ -239,11 +247,11 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { /// @dev `beginLotSizesUpdate` must be called first, once `governanceTimeDelay` /// has passed, this function can be called to set the lot sizes to the /// value set in `beginLotSizesUpdate` - function finalizeLotSizesUpdate() external onlyOwner { - require( - block.timestamp.sub(lotSizesChangeInitiated) >= governanceTimeDelay, - "Timer not elapsed, or change not initiated" - ); + function finalizeLotSizesUpdate() + external + onlyOwner + onlyAfterDelay(lotSizesChangeInitiated) { + lotSizesSatoshis = newLotSizesSatoshis; emit LotSizesUpdated(newLotSizesSatoshis); lotSizesChangeInitiated = 0; @@ -272,11 +280,11 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { /// @dev `beginCollateralizationThresholdsUpdate` must be called first, once `governanceTimeDelay` /// has passed, this function can be called to set the collateralization thresholds to the /// value set in `beginCollateralizationThresholdsUpdate` - function finalizeCollateralizationThresholdsUpdate() external onlyOwner { - require( - block.timestamp.sub(collateralizationThresholdsChangeInitiated) >= governanceTimeDelay, - "Timer not elapsed, or change not initiated" - ); + function finalizeCollateralizationThresholdsUpdate() + external + onlyOwner + onlyAfterDelay(collateralizationThresholdsChangeInitiated) { + initialCollateralizedPercent = newInitialCollateralizedPercent; undercollateralizedThresholdPercent = newUndercollateralizedThresholdPercent; severelyUndercollateralizedThresholdPercent = newSeverelyUndercollateralizedThresholdPercent; From 7e64610ae1a5cbf4a0f648efc8ff31b9f7191a80 Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Fri, 6 Mar 2020 09:26:18 -0600 Subject: [PATCH 04/10] Update system tests Uninitiated change and unelapsed timer are separate test cases now. --- implementation/test/TBTCSystemTest.js | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/implementation/test/TBTCSystemTest.js b/implementation/test/TBTCSystemTest.js index 45a843c73..a2243868a 100644 --- a/implementation/test/TBTCSystemTest.js +++ b/implementation/test/TBTCSystemTest.js @@ -101,7 +101,7 @@ describe("TBTCSystem", async function() { it("reverts if the governance timer has not elapsed", async () => { await expectRevert( tbtcSystem.finalizeSignerFeeDivisorUpdate(), - "Timer not elapsed, or change not initiated", + "Timer not elapsed", ) }) @@ -116,6 +116,13 @@ describe("TBTCSystem", async function() { _signerFeeDivisor: new BN("201"), }) }) + + it("reverts if a change has not been initiated", async () => { + await expectRevert( + tbtcSystem.finalizeSignerFeeDivisorUpdate(), + "Change not initiated", + ) + }) }) }) @@ -152,7 +159,7 @@ describe("TBTCSystem", async function() { it("reverts if the governance timer has not elapsed", async () => { await expectRevert( tbtcSystem.finalizeLotSizesUpdate(), - "Timer not elapsed, or change not initiated", + "Timer not elapsed", ) }) @@ -167,6 +174,13 @@ describe("TBTCSystem", async function() { expect(receipt.logs[0].args._lotSizes[0]).to.eq.BN(lotSizes[0]) expect(receipt.logs[0].args._lotSizes[1]).to.eq.BN(lotSizes[1]) }) + + it("reverts if a change has not been initiated", async () => { + await expectRevert( + tbtcSystem.finalizeLotSizesUpdate(), + "Change not initiated", + ) + }) }) }) @@ -227,7 +241,7 @@ describe("TBTCSystem", async function() { it("reverts if the governance timer has not elapsed", async () => { await expectRevert( tbtcSystem.finalizeCollateralizationThresholdsUpdate(), - "Timer not elapsed, or change not initiated", + "Timer not elapsed", ) }) @@ -244,6 +258,13 @@ describe("TBTCSystem", async function() { _severelyUndercollateralizedThresholdPercent: severelyUndercollateralizedPercent, }) }) + + it("reverts if a change has not been initiated", async () => { + await expectRevert( + tbtcSystem.finalizeCollateralizationThresholdsUpdate(), + "Change not initiated", + ) + }) }) }) From bce465c4fa63a199fb57d686e223abcd3b32d0ed Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Fri, 6 Mar 2020 10:22:46 -0600 Subject: [PATCH 05/10] Check values after update finalization Ensure the values are changes correctly --- implementation/test/TBTCSystemTest.js | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/implementation/test/TBTCSystemTest.js b/implementation/test/TBTCSystemTest.js index a2243868a..9ab3ff527 100644 --- a/implementation/test/TBTCSystemTest.js +++ b/implementation/test/TBTCSystemTest.js @@ -10,7 +10,8 @@ const {expect} = require("chai") const TBTCSystem = contract.fromArtifact("TBTCSystem") -describe("TBTCSystem", async function() { +// eslint-disable-next-line no-only-tests/no-only-tests +describe.only("TBTCSystem", async function() { let tbtcSystem let ecdsaKeepFactory @@ -69,7 +70,7 @@ describe("TBTCSystem", async function() { }) }) - describe("update Signer fee", async () => { + describe("update signer fee", async () => { describe("beginSignerFeeDivisorUpdate", async () => { const newFee = new BN("201") it("executes and fires SignerFeeDivisorUpdateStarted event", async () => { @@ -112,9 +113,12 @@ describe("TBTCSystem", async function() { receipt = await tbtcSystem.finalizeSignerFeeDivisorUpdate() + const signerFeeDivisor = await tbtcSystem.getSignerFeeDivisor.call() + expectEvent(receipt, "SignerFeeDivisorUpdated", { _signerFeeDivisor: new BN("201"), }) + expect(signerFeeDivisor).to.eq.BN(new BN("201")) }) it("reverts if a change has not been initiated", async () => { @@ -170,9 +174,13 @@ describe("TBTCSystem", async function() { receipt = await tbtcSystem.finalizeLotSizesUpdate() + const currentLotSizes = await tbtcSystem.getAllowedLotSizes.call() + expectEvent(receipt, "LotSizesUpdated", {}) expect(receipt.logs[0].args._lotSizes[0]).to.eq.BN(lotSizes[0]) expect(receipt.logs[0].args._lotSizes[1]).to.eq.BN(lotSizes[1]) + expect(currentLotSizes[0]).to.eq.BN(lotSizes[0]) + expect(currentLotSizes[1]).to.eq.BN(lotSizes[1]) }) it("reverts if a change has not been initiated", async () => { @@ -252,11 +260,21 @@ describe("TBTCSystem", async function() { receipt = await tbtcSystem.finalizeCollateralizationThresholdsUpdate() + const initial = await tbtcSystem.getInitialCollateralizedPercent.call() + const undercollateralized = await tbtcSystem.getUndercollateralizedThresholdPercent.call() + const severelyUndercollateralized = await tbtcSystem.getSeverelyUndercollateralizedThresholdPercent.call() + expectEvent(receipt, "CollateralizationThresholdsUpdated", { _initialCollateralizedPercent: initialPercent, _undercollateralizedThresholdPercent: undercollateralizedPercent, _severelyUndercollateralizedThresholdPercent: severelyUndercollateralizedPercent, }) + + expect(initialPercent).to.eq.BN(initial) + expect(undercollateralizedPercent).to.eq.BN(undercollateralized) + expect(severelyUndercollateralizedPercent).to.eq.BN( + severelyUndercollateralized, + ) }) it("reverts if a change has not been initiated", async () => { From e5cd8426350949d6c5bea46ba129ff7f4f31786c Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Fri, 6 Mar 2020 10:25:07 -0600 Subject: [PATCH 06/10] Extract common getRemainingUpdateTerm functionality getRemainingUpdateTime functions share most of their functinality. Extract that functionality into an internal function. --- .../contracts/system/TBTCSystem.sol | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/implementation/contracts/system/TBTCSystem.sol b/implementation/contracts/system/TBTCSystem.sol index e1e6fcc83..fd9c14366 100644 --- a/implementation/contracts/system/TBTCSystem.sol +++ b/implementation/contracts/system/TBTCSystem.sol @@ -318,34 +318,27 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { /// @notice Get the time remaining until the collateralization thresholds can be updated. function getRemainingCollateralizationUpdateTime() external view returns (uint256) { - require(collateralizationThresholdsChangeInitiated > 0, "Update not initiated"); - - uint256 elapsed = block.timestamp.sub(collateralizationThresholdsChangeInitiated); - return (elapsed >= governanceTimeDelay)? - 0: - governanceTimeDelay.sub(elapsed); + return getRemainingChangeTime(collateralizationThresholdsChangeInitiated); } /// @notice Get the time remaining until the lot sizes can be updated. function getRemainingLotSizesUpdateTime() external view returns (uint256) { - require(lotSizesChangeInitiated > 0, "Update not initiated"); - - uint256 elapsed = block.timestamp.sub(lotSizesChangeInitiated); - return (elapsed >= governanceTimeDelay)? - 0: - governanceTimeDelay.sub(elapsed); + return getRemainingChangeTime(lotSizesChangeInitiated); } /// @notice Get the time remaining until the signer fee divisor can be updated. function geRemainingSignerFeeDivisorUpdateTime() external view returns (uint256) { - require(signerFeeDivisorChangeInitiated > 0, "Update not initiated"); + return getRemainingChangeTime(signerFeeDivisorChangeInitiated); + } - uint256 elapsed = block.timestamp.sub(signerFeeDivisorChangeInitiated); + /// @notice Get the time remaining until the function parameter timer value can be updated. + function getRemainingChangeTime(uint256 _changeTimestamp) internal view returns (uint256){ + require(_changeTimestamp > 0, "Update not initiated"); + uint256 elapsed = block.timestamp.sub(_changeTimestamp); return (elapsed >= governanceTimeDelay)? 0: governanceTimeDelay.sub(elapsed); } - // Price Feed /// @notice Get the price of one satoshi in wei. From d7adb82af186a62a14f3759884a7ae963c7436ae Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Fri, 6 Mar 2020 11:25:52 -0600 Subject: [PATCH 07/10] test getRemainingUpdateTime functions --- .../contracts/system/TBTCSystem.sol | 5 + implementation/test/TBTCSystemTest.js | 126 +++++++++++++++++- 2 files changed, 129 insertions(+), 2 deletions(-) diff --git a/implementation/contracts/system/TBTCSystem.sol b/implementation/contracts/system/TBTCSystem.sol index fd9c14366..2dfbd5af4 100644 --- a/implementation/contracts/system/TBTCSystem.sol +++ b/implementation/contracts/system/TBTCSystem.sol @@ -339,6 +339,11 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { 0: governanceTimeDelay.sub(elapsed); } + + function getGovernanceTimeDelay() public view returns (uint256) { + return governanceTimeDelay; + } + // Price Feed /// @notice Get the price of one satoshi in wei. diff --git a/implementation/test/TBTCSystemTest.js b/implementation/test/TBTCSystemTest.js index 9ab3ff527..c8090cbfa 100644 --- a/implementation/test/TBTCSystemTest.js +++ b/implementation/test/TBTCSystemTest.js @@ -10,8 +10,7 @@ const {expect} = require("chai") const TBTCSystem = contract.fromArtifact("TBTCSystem") -// eslint-disable-next-line no-only-tests/no-only-tests -describe.only("TBTCSystem", async function() { +describe("TBTCSystem", async function() { let tbtcSystem let ecdsaKeepFactory @@ -70,6 +69,129 @@ describe.only("TBTCSystem", async function() { }) }) + describe("geRemainingSignerFeeDivisorUpdateTime", async () => { + let totalDelay + + before(async () => { + totalDelay = await tbtcSystem.getGovernanceTimeDelay.call() + }) + + beforeEach(async () => { + await createSnapshot() + }) + + afterEach(async () => { + await restoreSnapshot() + }) + + it("reverts if update has not been initiated", async () => { + await expectRevert( + tbtcSystem.geRemainingSignerFeeDivisorUpdateTime.call(), + "Update not initiated", + ) + }) + + it("returns total delay if no time has passed ", async () => { + await tbtcSystem.beginSignerFeeDivisorUpdate(new BN("200")) + const remaining = await tbtcSystem.geRemainingSignerFeeDivisorUpdateTime.call() + expect(remaining).to.eq.BN(totalDelay) + }) + + it("returns the correct remaining time", async () => { + await tbtcSystem.beginSignerFeeDivisorUpdate(new BN("200")) + const expectedRemaining = 100 + await increaseTime(totalDelay.toNumber() - expectedRemaining) + + const remaining = await tbtcSystem.geRemainingSignerFeeDivisorUpdateTime.call() + expect(remaining).to.eq.BN(expectedRemaining) + }) + }) + + describe("getRemainingLotSizesUpdateTime", async () => { + let totalDelay + const lotSizes = [new BN(10 ** 8), new BN(10 ** 6)] + + before(async () => { + totalDelay = await tbtcSystem.getGovernanceTimeDelay.call() + }) + + beforeEach(async () => { + await createSnapshot() + }) + + afterEach(async () => { + await restoreSnapshot() + }) + + it("reverts if update has not been initiated", async () => { + await expectRevert( + tbtcSystem.getRemainingLotSizesUpdateTime.call(), + "Update not initiated", + ) + }) + + it("returns total delay if no time has passed ", async () => { + await tbtcSystem.beginLotSizesUpdate(lotSizes) + const remaining = await tbtcSystem.getRemainingLotSizesUpdateTime.call() + expect(remaining).to.eq.BN(totalDelay) + }) + + it("returns the correct remaining time", async () => { + await tbtcSystem.beginLotSizesUpdate(lotSizes) + const expectedRemaining = 100 + await increaseTime(totalDelay.toNumber() - expectedRemaining) + + const remaining = await tbtcSystem.getRemainingLotSizesUpdateTime.call() + expect(remaining).to.eq.BN(expectedRemaining) + }) + }) + + describe("getRemainingCollateralizationUpdateTime", async () => { + let totalDelay + + before(async () => { + totalDelay = await tbtcSystem.getGovernanceTimeDelay.call() + }) + + beforeEach(async () => { + await createSnapshot() + }) + + afterEach(async () => { + await restoreSnapshot() + }) + + it("reverts if update has not been initiated", async () => { + await expectRevert( + tbtcSystem.getRemainingCollateralizationUpdateTime.call(), + "Update not initiated", + ) + }) + + it("returns total delay if no time has passed ", async () => { + await tbtcSystem.beginCollateralizationThresholdsUpdate( + new BN("150"), + new BN("130"), + new BN("120"), + ) + const remaining = await tbtcSystem.getRemainingCollateralizationUpdateTime.call() + expect(remaining).to.eq.BN(totalDelay) + }) + + it("returns the correct remaining time", async () => { + await tbtcSystem.beginCollateralizationThresholdsUpdate( + new BN("150"), + new BN("130"), + new BN("120"), + ) + const expectedRemaining = 100 + await increaseTime(totalDelay.toNumber() - expectedRemaining) + + const remaining = await tbtcSystem.getRemainingCollateralizationUpdateTime.call() + expect(remaining).to.eq.BN(expectedRemaining) + }) + }) + describe("update signer fee", async () => { describe("beginSignerFeeDivisorUpdate", async () => { const newFee = new BN("201") From 596088b01e579462cab83ec606b3896e4f0fde6f Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Mon, 9 Mar 2020 19:44:05 -0500 Subject: [PATCH 08/10] Test Value update init override Ensure the update overrides the old value and resets the timestamp. Time tests have a grac eperiod of 1 second to account for unexpected time incrememnts due due to computation time. --- implementation/test/TBTCSystemTest.js | 64 +++++++++++++++++++++++++-- 1 file changed, 61 insertions(+), 3 deletions(-) diff --git a/implementation/test/TBTCSystemTest.js b/implementation/test/TBTCSystemTest.js index c8090cbfa..1fb94d0b9 100644 --- a/implementation/test/TBTCSystemTest.js +++ b/implementation/test/TBTCSystemTest.js @@ -103,7 +103,9 @@ describe("TBTCSystem", async function() { await increaseTime(totalDelay.toNumber() - expectedRemaining) const remaining = await tbtcSystem.geRemainingSignerFeeDivisorUpdateTime.call() - expect(remaining).to.eq.BN(expectedRemaining) + expect([expectedRemaining, expectedRemaining + 1]).to.include.toString( + remaining.toNumber(), + ) }) }) @@ -142,7 +144,9 @@ describe("TBTCSystem", async function() { await increaseTime(totalDelay.toNumber() - expectedRemaining) const remaining = await tbtcSystem.getRemainingLotSizesUpdateTime.call() - expect(remaining).to.eq.BN(expectedRemaining) + expect([expectedRemaining, expectedRemaining + 1]).to.include.toString( + remaining.toNumber(), + ) }) }) @@ -188,7 +192,9 @@ describe("TBTCSystem", async function() { await increaseTime(totalDelay.toNumber() - expectedRemaining) const remaining = await tbtcSystem.getRemainingCollateralizationUpdateTime.call() - expect(remaining).to.eq.BN(expectedRemaining) + expect([expectedRemaining, expectedRemaining + 1]).to.include.toString( + remaining.toNumber(), + ) }) }) @@ -196,11 +202,25 @@ describe("TBTCSystem", async function() { describe("beginSignerFeeDivisorUpdate", async () => { const newFee = new BN("201") it("executes and fires SignerFeeDivisorUpdateStarted event", async () => { + receipt = await tbtcSystem.beginSignerFeeDivisorUpdate(new BN("200")) + + expectEvent(receipt, "SignerFeeDivisorUpdateStarted", { + _signerFeeDivisor: new BN("200"), + }) + }) + + it("overrides previous update and resets timer", async () => { receipt = await tbtcSystem.beginSignerFeeDivisorUpdate(newFee) + const remainingTime = await tbtcSystem.geRemainingSignerFeeDivisorUpdateTime.call() + const totalDelay = await tbtcSystem.getGovernanceTimeDelay.call() expectEvent(receipt, "SignerFeeDivisorUpdateStarted", { _signerFeeDivisor: newFee, }) + expect([ + remainingTime.toString(), + remainingTime.toString() - 1, + ]).to.include(totalDelay.toString()) }) it("reverts if msg.sender != owner", async () => { @@ -256,12 +276,29 @@ describe("TBTCSystem", async function() { const lotSizes = [new BN(10 ** 8), new BN(10 ** 6)] describe("beginLotSizesUpdate", async () => { it("executes and emits a LotSizesUpdateStarted event", async () => { + const testSizes = [new BN(10 ** 8), new BN(10 ** 6)] + const block = await web3.eth.getBlock("latest") + const receipt = await tbtcSystem.beginLotSizesUpdate(testSizes) + expectEvent(receipt, "LotSizesUpdateStarted", {}) + expect(receipt.logs[0].args[0][0]).to.eq.BN(testSizes[0]) + expect(receipt.logs[0].args[0][1]).to.eq.BN(testSizes[1]) + expect(receipt.logs[0].args[1]).to.eq.BN(block.timestamp) + }) + + it("overrides previous update and resets timer", async () => { const block = await web3.eth.getBlock("latest") const receipt = await tbtcSystem.beginLotSizesUpdate(lotSizes) + const remainingTime = await tbtcSystem.getRemainingLotSizesUpdateTime.call() + const totalDelay = await tbtcSystem.getGovernanceTimeDelay.call() + expectEvent(receipt, "LotSizesUpdateStarted", {}) expect(receipt.logs[0].args[0][0]).to.eq.BN(lotSizes[0]) expect(receipt.logs[0].args[0][1]).to.eq.BN(lotSizes[1]) expect(receipt.logs[0].args[1]).to.eq.BN(block.timestamp) + expect([ + remainingTime.toString(), + remainingTime.toString() - 1, + ]).to.include(totalDelay.toString()) }) it("reverts if lot size array is empty", async () => { @@ -320,17 +357,38 @@ describe("TBTCSystem", async function() { const severelyUndercollateralizedPercent = new BN("120") describe("beginCollateralizationThresholdsUpdate", async () => { it("executes and fires CollateralizationThresholdsUpdateStarted event", async () => { + receipt = await tbtcSystem.beginCollateralizationThresholdsUpdate( + new BN("213"), + new BN("156"), + new BN("128"), + ) + + expectEvent(receipt, "CollateralizationThresholdsUpdateStarted", { + _initialCollateralizedPercent: new BN("213"), + _undercollateralizedThresholdPercent: new BN("156"), + _severelyUndercollateralizedThresholdPercent: new BN("128"), + }) + }) + + it("overrides previous update and resets timer", async () => { receipt = await tbtcSystem.beginCollateralizationThresholdsUpdate( initialPercent, undercollateralizedPercent, severelyUndercollateralizedPercent, ) + const remainingTime = await tbtcSystem.getRemainingCollateralizationUpdateTime.call() + const totalDelay = await tbtcSystem.getGovernanceTimeDelay.call() + expectEvent(receipt, "CollateralizationThresholdsUpdateStarted", { _initialCollateralizedPercent: initialPercent, _undercollateralizedThresholdPercent: undercollateralizedPercent, _severelyUndercollateralizedThresholdPercent: severelyUndercollateralizedPercent, }) + expect([ + remainingTime.toString(), + remainingTime.toString() - 1, + ]).to.include(totalDelay.toString()) }) it("reverts if Initial collateralized percent > 300", async () => { From 3e8687e81c886168b78f13a2a041a36562176a44 Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Tue, 10 Mar 2020 13:01:36 -0500 Subject: [PATCH 09/10] governanceTimeDelay Update From 1 hour to 6 --- implementation/contracts/system/TBTCSystem.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/implementation/contracts/system/TBTCSystem.sol b/implementation/contracts/system/TBTCSystem.sol index 3620e89f8..1160321fd 100644 --- a/implementation/contracts/system/TBTCSystem.sol +++ b/implementation/contracts/system/TBTCSystem.sol @@ -58,7 +58,7 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog { uint128 private severelyUndercollateralizedThresholdPercent = 110; // percent uint256[] lotSizesSatoshis = [10**5, 10**6, 10**7, 2 * 10**7, 5 * 10**7, 10**8]; // [0.001, 0.01, 0.1, 0.2, 0.5, 1.0] BTC - uint256 constant governanceTimeDelay = 1 hours; + uint256 constant governanceTimeDelay = 6 hours; uint256 private signerFeeDivisorChangeInitiated; uint256 private lotSizesChangeInitiated; From bdb417d5bc78a895ce26251a57f16835dd5ebbf2 Mon Sep 17 00:00:00 2001 From: NicholasDotSol Date: Tue, 10 Mar 2020 13:15:13 -0500 Subject: [PATCH 10/10] Add grace of 1 block to timestamp check Adding the timestamp grace allows the test to be off by 1 due to execution time pushing the timestamp up by 1 unpredictably --- implementation/test/TBTCSystemTest.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/implementation/test/TBTCSystemTest.js b/implementation/test/TBTCSystemTest.js index 1fb94d0b9..0652ba3c1 100644 --- a/implementation/test/TBTCSystemTest.js +++ b/implementation/test/TBTCSystemTest.js @@ -282,7 +282,10 @@ describe("TBTCSystem", async function() { expectEvent(receipt, "LotSizesUpdateStarted", {}) expect(receipt.logs[0].args[0][0]).to.eq.BN(testSizes[0]) expect(receipt.logs[0].args[0][1]).to.eq.BN(testSizes[1]) - expect(receipt.logs[0].args[1]).to.eq.BN(block.timestamp) + expect([ + receipt.logs[0].args[1].toString(), + receipt.logs[0].args[1].toString() - 1, + ]).to.include(block.timestamp.toString()) }) it("overrides previous update and resets timer", async () => { @@ -294,7 +297,10 @@ describe("TBTCSystem", async function() { expectEvent(receipt, "LotSizesUpdateStarted", {}) expect(receipt.logs[0].args[0][0]).to.eq.BN(lotSizes[0]) expect(receipt.logs[0].args[0][1]).to.eq.BN(lotSizes[1]) - expect(receipt.logs[0].args[1]).to.eq.BN(block.timestamp) + expect([ + receipt.logs[0].args[1].toString(), + receipt.logs[0].args[1].toString() - 1, + ]).to.include(block.timestamp.toString()) expect([ remainingTime.toString(), remainingTime.toString() - 1,