-
Notifications
You must be signed in to change notification settings - Fork 40
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanntt thissss.
My only concern here: are we sure we've covered all events by tests, to make sure we don't suddenly lose a bunch of events with this access control?
Also, let's file an issue here so we can flag it for the Diligence folks. |
@Shadowfiend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎸
@@ -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.
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
@@ -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.
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.
/// @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.
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
).
There was a problem hiding this comment.
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
|
closes: #477
Checks that a caller is a Deposit address by calling
TBTCDepositToken.exists
.setTbtcDepositToken
sets up thetbtcDepositToken
contract. This setup is performed duringTBTCSystem.initialize
Another 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: