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

implement approvedToLog check #467

Merged
merged 8 commits into from
Mar 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions implementation/contracts/DepositLog.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
pragma solidity ^0.5.10;

import {TBTCDepositToken} from "./system/TBTCDepositToken.sol";


contract DepositLog {
/*
Expand All @@ -10,6 +12,10 @@ contract DepositLog {
Everyone should be able to ENTIRELY rely on log messages
*/

// `TBTCDepositToken` mints a token for every new Deposit.
// If a token exists for a given ID, we know it is a valid Deposit address.
TBTCDepositToken tbtcDepositToken;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔shadows TBTCSystem.tbtcDepositToken. will raise a DeclarationError with solc >= 0.6 due to re-declaration in same class.


// This event is fired when we init the deposit
event Created(
address indexed _depositContractAddress,
Expand Down Expand Up @@ -99,15 +105,26 @@ contract DepositLog {
//

/// @notice Checks if an address is an allowed logger
/// @dev Calls the system to check if the caller is a Deposit
/// @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 */
Copy link

@tintinweb tintinweb Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now obsolete 🙌 , solium stmt can be removed

function approvedToLog(address _caller) public pure returns (bool) {
/* TODO: auth via system */
_caller;
return true;
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 _tbtcDepositTokenAddress The address of the tbtcDepositToken.
function setTbtcDepositToken(address _tbtcDepositTokenAddress) public {
Copy link

@tintinweb tintinweb Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be internal - otherwise anyone could call this before TBTCSystem.initialize is called.

it is not very elegant though that this is needed in the first place, see comment on shadowed variable - TBTCDepositToken is also available in the system contract and we will end up with the same var in two scopes holding the same value.

it would be good to find a way to deduplicate this but I understand that moving TBTCDepositToken to DepositLog is counter-intuitive. Another option would be to move DepositLog to TBTCSystem and provide a IDepositLog interface for use with other contracts (i.e. OutsourceDepositLogging).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be internal

indeed >.<

it is not very elegant though that this is needed in the first place

Agree here, There's definitely a cleaner way to do this

require(
address(tbtcDepositToken) == address(0),
"tbtcDepositToken is already set"
);
tbtcDepositToken = TBTCDepositToken(_tbtcDepositTokenAddress);
}

//
Expand Down
1 change: 1 addition & 0 deletions implementation/contracts/system/TBTCSystem.sol
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog {
_keepThreshold,
_keepSize
);
setTbtcDepositToken(_tbtcDepositToken);
_initialized = true;
allowNewDeposits = true;
}
Expand Down
5 changes: 0 additions & 5 deletions implementation/contracts/test/deposit/TBTCSystemStub.sol
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,4 @@ contract TBTCSystemStub is TBTCSystem {
function requestNewKeep(uint256, uint256, uint256) external payable returns (address _keepAddress) {
return keepAddress;
}

// override parent
function approvedToLog(address) public pure returns (bool) {
return true;
}
}