implement approvedToLog check #467
Conversation
Calls `TBTCDepositToken.exists` to ensure that the caller is a minted TDT. TBTCDepositToken is set via one-time function setTbtcDepositToken
Set via TBTCSystemStub.initialize()
Regular approvedToLog can be used instead of the override for better testing accuracy
|
Also, let's file an issue here so we can flag it for the Diligence folks. |
@Shadowfiend |
| @@ -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 */ | |||
There was a problem hiding this comment.
now obsolete 🙌 , solium stmt can be removed
| @@ -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; | |||
There was a problem hiding this comment.
🤔shadows TBTCSystem.tbtcDepositToken. will raise a DeclarationError with solc >= 0.6 due to re-declaration in same class.
| /// @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 { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
|
closes: #477
Checks that a caller is a Deposit address by calling
TBTCDepositToken.exists.setTbtcDepositTokensets up thetbtcDepositTokencontract. This setup is performed duringTBTCSystem.initializeAnother approach could be to use a constructor and handle deployment ordering, but the Deposit is not operational until TBTCSystem.initialize has been called, and no events are expected before that call.
Waiting on: