Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Commit

Permalink
Merge pull request #537 from keep-network/log-refactor
Browse files Browse the repository at this point in the history 
DepositLog refactor

- Fix `TBTCDepositToken` shadow by removing `TBTCSystem`'s copy
   - Now `TBTCSystem` uses the one inherited from `DepositLog`.
- Remove return value from logging functions.
   - Return values were not used. 
- Logging functions now revert if `approvedToLog` fails. 
   - Returning true/false allowed for non-halting logging fail. Events would
     not be emitted but the `Deposit` contract would continue to function. It
     seems that this is not a real benefit. If `approvedToLog` fails, the logging
     function is not called by the `Deposit` contract and it makes sense to revert.
  • Loading branch information
@Shadowfiend
Shadowfiend authored Mar 27, 2020
2 parents 61e496c + 53e6030 commit caa238b
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 107 deletions.
214 changes: 110 additions & 104 deletions solidity/contracts/DepositLog.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,61 +104,53 @@ contract DepositLog {
// AUTH
//

/// @notice Checks if an address is an allowed logger
/// @notice Checks if an address is an allowed logger.
/// @dev checks tbtcDepositToken to see if the caller represents
/// an existing deposit.
/// We don't require this, so deposits are not bricked if the system borks
/// @param _caller The address of the calling contract
/// @return True if approved, otherwise false
/* solium-disable-next-line no-empty-blocks */
/// We don't require this, so deposits are not bricked if the system borks.
/// @param _caller The address of the calling contract.
/// @return True if approved, otherwise false.
function approvedToLog(address _caller) public view returns (bool) {
return tbtcDepositToken.exists(uint256(_caller));
}

/// @notice Sets the tbtcDepositToken contract.
/// @dev The contract is used by `approvedToLog` to check if the
/// caller is a Deposit contract. This should only be called once.
/// @param _tbtcDepositToken tbtcDepositToken contract.
function setTbtcDepositToken(TBTCDepositToken _tbtcDepositToken) public {
require(
address(tbtcDepositToken) == address(0),
"tbtcDepositToken is already set"
);
tbtcDepositToken = _tbtcDepositToken;
}

//
// Logging
//

/// @notice Fires a Created event
/// @dev We append the sender, which is the deposit contract that called
/// @param _keepAddress The address of the associated keep
/// @return True if successful, else revert
function logCreated(address _keepAddress) public returns (bool) {
if (!approvedToLog(msg.sender)) return false;
/// @notice Fires a Created event.
/// @dev We append the sender, which is the deposit contract that called.
/// @param _keepAddress The address of the associated keep.
/// @return True if successful, else revert.
function logCreated(address _keepAddress) public {
require(
approvedToLog(msg.sender),
"Caller is not approved to log events"
);
emit Created(msg.sender, _keepAddress, block.timestamp);
return true;
}

/// @notice Fires a RedemptionRequested event
/// @dev This is the only event without an explicit timestamp
/// @param _requester The ethereum address of the requester
/// @param _digest The calculated sighash digest
/// @param _utxoSize The size of the utxo in sat
/// @notice Fires a RedemptionRequested event.
/// @dev This is the only event without an explicit timestamp.
/// @param _requester The ethereum address of the requester.
/// @param _digest The calculated sighash digest.
/// @param _utxoSize The size of the utxo in sat.
/// @param _redeemerOutputScript The redeemer's length-prefixed output script.
/// @param _requestedFee The requester or bump-system specified fee
/// @param _outpoint The 36 byte outpoint
/// @return True if successful, else revert
/// @param _requestedFee The requester or bump-system specified fee.
/// @param _outpoint The 36 byte outpoint.
/// @return True if successful, else revert.
function logRedemptionRequested(
address _requester,
bytes32 _digest,
uint256 _utxoSize,
bytes memory _redeemerOutputScript,
uint256 _requestedFee,
bytes memory _outpoint
) public returns (bool) {
if (!approvedToLog(msg.sender)) return false;
) public {
require(
approvedToLog(msg.sender),
"Caller is not approved to log events"
);
emit RedemptionRequested(
msg.sender,
_requester,
Expand All @@ -168,125 +160,139 @@ contract DepositLog {
_requestedFee,
_outpoint
);
return true;
}

/// @notice Fires a GotRedemptionSignature event
/// @dev We append the sender, which is the deposit contract that called
/// @param _digest signed digest
/// @param _r signature r value
/// @param _s signature s value
/// @return True if successful, else revert
/// @notice Fires a GotRedemptionSignature event.
/// @dev We append the sender, which is the deposit contract that called.
/// @param _digest signed digest.
/// @param _r signature r value.
/// @param _s signature s value.
function logGotRedemptionSignature(bytes32 _digest, bytes32 _r, bytes32 _s)
public
returns (bool)
{
if (!approvedToLog(msg.sender)) return false;
require(
approvedToLog(msg.sender),
"Caller is not approved to log events"
);
emit GotRedemptionSignature(
msg.sender,
_digest,
_r,
_s,
block.timestamp
);
return true;
}

/// @notice Fires a RegisteredPubkey event
/// @dev We append the sender, which is the deposit contract that called
/// returns false if not approved, to prevent accidentally halting Deposit
/// @return True if successful, else false
/// @notice Fires a RegisteredPubkey event.
/// @dev We append the sender, which is the deposit contract that called.
function logRegisteredPubkey(
bytes32 _signingGroupPubkeyX,
bytes32 _signingGroupPubkeyY
) public returns (bool) {
if (!approvedToLog(msg.sender)) return false;
) public {
require(
approvedToLog(msg.sender),
"Caller is not approved to log events"
);
emit RegisteredPubkey(
msg.sender,
_signingGroupPubkeyX,
_signingGroupPubkeyY,
block.timestamp
);
return true;
}

/// @notice Fires a SetupFailed event
/// @dev We append the sender, which is the deposit contract that called
/// returns false if not approved, to prevent accidentally halting Deposit
/// @return True if successful, else false
function logSetupFailed() public returns (bool) {
if (!approvedToLog(msg.sender)) return false;
/// @notice Fires a SetupFailed event.
/// @dev We append the sender, which is the deposit contract that called.
function logSetupFailed() public {
require(
approvedToLog(msg.sender),
"Caller is not approved to log events"
);
emit SetupFailed(msg.sender, block.timestamp);
return true;
}

/// @notice Fires a FraudDuringSetup event
/// @dev We append the sender, which is the deposit contract that called
/// returns false if not approved, to prevent accidentally halting Deposit
/// @return True if successful, else false
function logFraudDuringSetup() public returns (bool) {
if (!approvedToLog(msg.sender)) return false;
/// @notice Fires a FraudDuringSetup event.
/// @dev We append the sender, which is the deposit contract that called.
function logFraudDuringSetup() public {
require(
approvedToLog(msg.sender),
"Caller is not approved to log events"
);
emit FraudDuringSetup(msg.sender, block.timestamp);
return true;
}

/// @notice Fires a Funded event
/// @dev We append the sender, which is the deposit contract that called
/// returns false if not approved, to prevent accidentally halting Deposit
/// @return True if successful, else false
function logFunded() public returns (bool) {
if (!approvedToLog(msg.sender)) return false;
/// @notice Fires a Funded event.
/// @dev We append the sender, which is the deposit contract that called.
function logFunded() public {
require(
approvedToLog(msg.sender),
"Caller is not approved to log events"
);
emit Funded(msg.sender, block.timestamp);
return true;
}

/// @notice Fires a CourtesyCalled event
/// @dev We append the sender, which is the deposit contract that called
/// returns false if not approved, to prevent accidentally halting Deposit
/// @return True if successful, else false
function logCourtesyCalled() public returns (bool) {
if (!approvedToLog(msg.sender)) return false;
/// @notice Fires a CourtesyCalled event.
/// @dev We append the sender, which is the deposit contract that called.
function logCourtesyCalled() public {
require(
approvedToLog(msg.sender),
"Caller is not approved to log events"
);
emit CourtesyCalled(msg.sender, block.timestamp);
return true;
}

/// @notice Fires a StartedLiquidation event
/// @dev We append the sender, which is the deposit contract that called
/// @param _wasFraud True if liquidating for fraud
/// @return True if successful, else revert
function logStartedLiquidation(bool _wasFraud) public returns (bool) {
if (!approvedToLog(msg.sender)) return false;
/// @notice Fires a StartedLiquidation event.
/// @dev We append the sender, which is the deposit contract that called.
/// @param _wasFraud True if liquidating for fraud.
function logStartedLiquidation(bool _wasFraud) public {
require(
approvedToLog(msg.sender),
"Caller is not approved to log events"
);
emit StartedLiquidation(msg.sender, _wasFraud, block.timestamp);
return true;
}

/// @notice Fires a Redeemed event
/// @dev We append the sender, which is the deposit contract that called
/// returns false if not approved, to prevent accidentally halting Deposit
/// @return True if successful, else false
function logRedeemed(bytes32 _txid) public returns (bool) {
if (!approvedToLog(msg.sender)) return false;
/// @dev We append the sender, which is the deposit contract that called.
function logRedeemed(bytes32 _txid) public {
require(
approvedToLog(msg.sender),
"Caller is not approved to log events"
);
emit Redeemed(msg.sender, _txid, block.timestamp);
return true;
}

/// @notice Fires a Liquidated event
/// @dev We append the sender, which is the deposit contract that called
/// returns false if not approved, to prevent accidentally halting Deposit
/// @return True if successful, else false
function logLiquidated() public returns (bool) {
if (!approvedToLog(msg.sender)) return false;
/// @dev We append the sender, which is the deposit contract that called.
function logLiquidated() public {
require(
approvedToLog(msg.sender),
"Caller is not approved to log events"
);
emit Liquidated(msg.sender, block.timestamp);
return true;
}

/// @notice Fires a ExitedCourtesyCall event
/// @dev We append the sender, which is the deposit contract that called
/// returns false if not approved, to prevent accidentally halting Deposit
/// @return True if successful, else false
function logExitedCourtesyCall() public returns (bool) {
if (!approvedToLog(msg.sender)) return false;
/// @dev We append the sender, which is the deposit contract that called.
function logExitedCourtesyCall() public {
require(
approvedToLog(msg.sender),
"Caller is not approved to log events"
);
emit ExitedCourtesyCall(msg.sender, block.timestamp);
return true;
}

/// @notice Sets the tbtcDepositToken contract.
/// @dev The contract is used by `approvedToLog` to check if the
/// caller is a Deposit contract. This should only be called once.
/// @param _tbtcDepositTokenAddress The address of the tbtcDepositToken.
function setTbtcDepositToken(TBTCDepositToken _tbtcDepositTokenAddress)
internal
{
require(
address(tbtcDepositToken) == address(0),
"tbtcDepositToken is already set"
);
tbtcDepositToken = _tbtcDepositTokenAddress;
}
}
2 changes: 0 additions & 2 deletions solidity/contracts/system/TBTCSystem.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog {
uint256 pausedTimestamp;
uint256 constant pausedDuration = 10 days;

TBTCDepositToken public tbtcDepositToken;
IBTCETHPriceFeed public priceFeed;
IBondedECDSAKeepVendor public keepVendor;
IRelay public relay;
Expand Down Expand Up @@ -102,7 +101,6 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog {
uint256 _keepSize
) external onlyOwner {
require(!_initialized, "already initialized");
tbtcDepositToken = _tbtcDepositToken;
keepVendor = _keepVendor;
_vendingMachine.setExternalAddresses(
_tbtcToken,
Expand Down
3 changes: 2 additions & 1 deletion solidity/test/DepositFactoryTest.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const {deployAndLinkAll} = require("./helpers/testDeployer.js")
const {contract, web3} = require("@openzeppelin/test-environment")
const {contract, web3, accounts} = require("@openzeppelin/test-environment")
const {states} = require("./helpers/utils.js")
const {BN, constants, expectRevert} = require("@openzeppelin/test-helpers")
const {ZERO_ADDRESS} = constants
Expand Down Expand Up @@ -174,6 +174,7 @@ describe("DepositFactory", async function() {
it("is not affected by state changes to master", async () => {
const keep = await ECDSAKeepStub.new()

await tbtcDepositToken.forceMint(accounts[0], testDeposit.address)
await testDeposit.createNewDeposit(
tbtcSystemStub.address,
tbtcToken.address,
Expand Down
8 changes: 8 additions & 0 deletions solidity/test/DepositUtilsTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ describe("DepositUtils", async function() {
beneficiary = accounts[2]

feeRebateToken.forceMint(beneficiary, web3.utils.toBN(testDeposit.address))
tbtcDepositToken.forceMint(
beneficiary,
web3.utils.toBN(testDeposit.address),
)

await testDeposit.createNewDeposit(
tbtcSystemStub.address,
Expand Down Expand Up @@ -283,6 +287,10 @@ describe("DepositUtils", async function() {

beneficiary = accounts[2]

tbtcDepositToken.forceMint(
beneficiary,
web3.utils.toBN(testDeposit.address),
)
feeRebateToken.forceMint(
beneficiary,
web3.utils.toBN(testDeposit.address),
Expand Down

0 comments on commit caa238b

Please sign in to comment.