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

Use specific contract types #542

Merged
merged 13 commits into from
Mar 26, 2020
Merged

Use specific contract types #542

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1f5c278
Use TBTCDepositToken instead of address
Mar 25, 2020
3e6b63a
Use contract over addresses types where reasonable
Mar 25, 2020
3303dc5
TBTCSystem address-> contract type where reasonable
Mar 25, 2020
2577752
Use contract types direclty in parameters
Mar 25, 2020
54b1567
Update ITBTCSystem interface
Mar 25, 2020
9878ca5
Use ITBTCSystem directly from DepositUtils
Mar 25, 2020
ef8c467
Make contract types public
Mar 25, 2020
41104d0
Contract type for Function parameters
Mar 25, 2020
fc03c4f
Avoid unnecessary double-case
Mar 26, 2020
9f31086
distributeERC20Reward takes IERC20
Mar 26, 2020
08bba81
vendingMachine -> vendingMachineAddress
Mar 26, 2020
38e0b59
Update capitalization for consistency
Mar 26, 2020
6756361
Under distributeERC20Reward Interface change
Mar 26, 2020
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
21 changes: 13 additions & 8 deletions solidity/contracts/deposit/Deposit.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ import {DepositUtils} from "./DepositUtils.sol";
import {DepositFunding} from "./DepositFunding.sol";
import {DepositRedemption} from "./DepositRedemption.sol";
import {DepositStates} from "./DepositStates.sol";
import {ITBTCSystem} from "../interfaces/ITBTCSystem.sol";
import {IERC721} from "openzeppelin-solidity/contracts/token/ERC721/IERC721.sol";
import {TBTCToken} from "../system/TBTCToken.sol";
import {FeeRebateToken} from "../system/FeeRebateToken.sol";

import "../system/DepositFactoryAuthority.sol";

/// @title Deposit.
Expand Down Expand Up @@ -104,19 +109,19 @@ contract Deposit is DepositFactoryAuthority {
/// (10**7 satoshi == 0.1 BTC == 0.1 TBTC).
/// @return True if successful, otherwise revert.
function createNewDeposit(
address _TBTCSystem,
address _TBTCToken,
address _TBTCDepositToken,
address _FeeRebateToken,
ITBTCSystem _TBTCSystem,
TBTCToken _TBTCToken,
IERC721 _TBTCDepositToken,
FeeRebateToken _FeeRebateToken,
address _VendingMachine,
uint256 _m,
uint256 _n,
uint256 _lotSizeSatoshis
) public onlyFactory payable returns (bool) {
self.TBTCSystem = _TBTCSystem;
self.TBTCToken = _TBTCToken;
self.TBTCDepositToken = _TBTCDepositToken;
self.FeeRebateToken = _FeeRebateToken;
self.tbtcSystem = (_TBTCSystem);
self.tbtcToken = (_TBTCToken);
self.tbtcDepositToken = (_TBTCDepositToken);
self.feeRebateToken = (_FeeRebateToken);
self.VendingMachine = _VendingMachine;
self.createNewDeposit(_m, _n, _lotSizeSatoshis);
return true;
Expand Down
2 changes: 1 addition & 1 deletion solidity/contracts/deposit/DepositFunding.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ library DepositFunding {
uint256 _n,
uint256 _lotSizeSatoshis
) public returns (bool) {
TBTCSystem _system = TBTCSystem(_d.TBTCSystem);
TBTCSystem _system = TBTCSystem(address(_d.tbtcSystem));
Shadowfiend marked this conversation as resolved.
Show resolved Hide resolved

require(_system.getAllowNewDeposits(), "Opening new deposits is currently disabled.");
require(_d.inStart(), "Deposit setup already requested");
Expand Down
8 changes: 3 additions & 5 deletions solidity/contracts/deposit/DepositLiquidation.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,16 +167,14 @@ library DepositLiquidation {
// send the TBTC to the TDT holder. If the TDT holder is the Vending Machine, burn it to maintain the peg.
address tdtHolder = _d.depositOwner();

TBTCToken _tbtcToken = TBTCToken(_d.TBTCToken);

uint256 lotSizeTbtc = _d.lotSizeTbtc();
require(_tbtcToken.balanceOf(msg.sender) >= lotSizeTbtc, "Not enough TBTC to cover outstanding debt");
require(_d.tbtcToken.balanceOf(msg.sender) >= lotSizeTbtc, "Not enough TBTC to cover outstanding debt");

if(tdtHolder == _d.VendingMachine){
_tbtcToken.burnFrom(msg.sender, lotSizeTbtc); // burn minimal amount to cover size
_d.tbtcToken.burnFrom(msg.sender, lotSizeTbtc); // burn minimal amount to cover size
}
else{
_tbtcToken.transferFrom(msg.sender, tdtHolder, lotSizeTbtc);
_d.tbtcToken.transferFrom(msg.sender, tdtHolder, lotSizeTbtc);
}

// Distribute funds to auction buyer
Expand Down
24 changes: 9 additions & 15 deletions solidity/contracts/deposit/DepositRedemption.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,10 @@ library DepositRedemption {
/// @notice Pushes signer fee to the Keep group by transferring it to the Keep address.
/// @dev Approves the keep contract, then expects it to call transferFrom.
function distributeSignerFee(DepositUtils.Deposit storage _d) internal {
address _tbtcTokenAddress = _d.TBTCToken;
TBTCToken _tbtcToken = TBTCToken(_tbtcTokenAddress);

IBondedECDSAKeep _keep = IBondedECDSAKeep(_d.keepAddress);

_tbtcToken.approve(_d.keepAddress, _d.signerFee());
_keep.distributeERC20Reward(_tbtcTokenAddress, _d.signerFee());
_d.tbtcToken.approve(_d.keepAddress, _d.signerFee());
_keep.distributeERC20Reward(address(_d.tbtcToken), _d.signerFee());
Shadowfiend marked this conversation as resolved.
Show resolved Hide resolved
}

/// @notice Closes keep associated with the deposit.
Expand All @@ -61,7 +58,6 @@ library DepositRedemption {
/// @notice Handles TBTC requirements for redemption.
/// @dev Burns or transfers depending on term and supply-peg impact.
function performRedemptionTBTCTransfers(DepositUtils.Deposit storage _d) internal {
TBTCToken _tbtc = TBTCToken(_d.TBTCToken);
address tdtHolder = _d.depositOwner();
address vendingMachine = _d.VendingMachine;

Expand All @@ -75,7 +71,7 @@ library DepositRedemption {
}
// if we owe > 0 & < signerfee, msg.sender is TDT owner but not FRT holder.
if(tbtcOwed <= signerFee){
_tbtc.transferFrom(msg.sender, address(this), tbtcOwed);
_d.tbtcToken.transferFrom(msg.sender, address(this), tbtcOwed);
return;
}
// Redemmer always owes a full TBTC for at-term redemption.
Expand All @@ -86,16 +82,16 @@ library DepositRedemption {
// Vending Machine-owned TDTs have been used to mint TBTC,
// and we should always burn a full TBTC to redeem the deposit.
if(tdtHolder == vendingMachine){
_tbtc.burnFrom(msg.sender, tbtcLot);
_d.tbtcToken.burnFrom(msg.sender, tbtcLot);
}
// if signer fee is not escrowed, escrow and it here and send the rest to TDT owner
else if(_tbtc.balanceOf(address(this)) < signerFee){
_tbtc.transferFrom(msg.sender, address(this), signerFee);
_tbtc.transferFrom(msg.sender, tdtHolder, tbtcLot.sub(signerFee));
else if(_d.tbtcToken.balanceOf(address(this)) < signerFee){
_d.tbtcToken.transferFrom(msg.sender, address(this), signerFee);
_d.tbtcToken.transferFrom(msg.sender, tdtHolder, tbtcLot.sub(signerFee));
}
// tansfer a full TBTC to TDT owner if signerFee is escrowed
else{
_tbtc.transferFrom(msg.sender, tdtHolder, tbtcLot);
_d.tbtcToken.transferFrom(msg.sender, tdtHolder, tbtcLot);
}
return;
}
Expand Down Expand Up @@ -162,9 +158,7 @@ library DepositRedemption {
bytes memory _redeemerOutputScript,
address payable _finalRecipient
) public {
IERC721 _tbtcDepositToken = IERC721(_d.TBTCDepositToken);

_tbtcDepositToken.transferFrom(msg.sender, _finalRecipient, uint256(address(this)));
_d.tbtcDepositToken.transferFrom(msg.sender, _finalRecipient, uint256(address(this)));

_requestRedemption(_d, _outputValueBytes, _redeemerOutputScript, _finalRecipient);
}
Expand Down
33 changes: 13 additions & 20 deletions solidity/contracts/deposit/DepositUtils.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@ library DepositUtils {
struct Deposit {

// SET DURING CONSTRUCTION
address TBTCSystem;
address TBTCToken;
address TBTCDepositToken;
address FeeRebateToken;
ITBTCSystem tbtcSystem;
TBTCToken tbtcToken;
IERC721 tbtcDepositToken;
FeeRebateToken feeRebateToken;
address VendingMachine;
uint256 lotSizeSatoshis;
uint8 currentState;
Expand Down Expand Up @@ -74,16 +74,14 @@ library DepositUtils {
/// @dev Calls the light relay and gets the current block difficulty.
/// @return The difficulty.
function currentBlockDifficulty(Deposit storage _d) public view returns (uint256) {
ITBTCSystem _sys = ITBTCSystem(_d.TBTCSystem);
return _sys.fetchRelayCurrentDifficulty();
return _d.tbtcSystem.fetchRelayCurrentDifficulty();
}

/// @notice Gets the previous block difficulty.
/// @dev Calls the light relay and gets the previous block difficulty.
/// @return The difficulty.
function previousBlockDifficulty(Deposit storage _d) public view returns (uint256) {
ITBTCSystem _sys = ITBTCSystem(_d.TBTCSystem);
return _sys.fetchRelayPreviousDifficulty();
return _d.tbtcSystem.fetchRelayPreviousDifficulty();
}

/// @notice Evaluates the header difficulties in a proof.
Expand Down Expand Up @@ -310,8 +308,7 @@ library DepositUtils {
/// @dev Polls the price feed via the system contract.
/// @return The current price of 1 sat in wei.
function fetchBitcoinPrice(Deposit storage _d) public view returns (uint256) {
ITBTCSystem _sys = ITBTCSystem(_d.TBTCSystem);
return _sys.fetchBitcoinPrice();
return _d.tbtcSystem.fetchBitcoinPrice();
}

/// @notice Fetches the Keep's bond amount in wei.
Expand Down Expand Up @@ -342,10 +339,9 @@ library DepositUtils {
/// @return The current token holder if the Token exists.
/// address(0) if the token does not exist.
function feeRebateTokenHolder(Deposit storage _d) public view returns (address payable) {
FeeRebateToken _feeRebateToken = FeeRebateToken(_d.FeeRebateToken);
address tokenHolder;
if(_feeRebateToken.exists(uint256(address(this)))){
tokenHolder = address(uint160(_feeRebateToken.ownerOf(uint256(address(this)))));
if(_d.feeRebateToken.exists(uint256(address(this)))){
tokenHolder = address(uint160(_d.feeRebateToken.ownerOf(uint256(address(this)))));
}
return address(uint160(tokenHolder));
}
Expand All @@ -354,8 +350,7 @@ library DepositUtils {
/// @dev We cast the address to a uint256 to match the 721 standard.
/// @return The current deposit beneficiary.
function depositOwner(Deposit storage _d) public view returns (address payable) {
IERC721 _tbtcDepositToken = IERC721(_d.TBTCDepositToken);
return address(uint160(_tbtcDepositToken.ownerOf(uint256(address(this)))));
return address(uint160(_d.tbtcDepositToken.ownerOf(uint256(address(this)))));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there room here for an overload of ownerAddressOf that takes a Deposit and returns an address and does all of the weird type conversions for us as needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that might be a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not worth cycles unless you're motivated, but yeah, seems cleaner.

}

/// @notice Deletes state after termination of redemption process.
Expand Down Expand Up @@ -394,16 +389,14 @@ library DepositUtils {
/// @notice Distributes the fee rebate to the Fee Rebate Token owner.
/// @dev Whenever this is called we are shutting down.
function distributeFeeRebate(Deposit storage _d) internal {
TBTCToken _tbtc = TBTCToken(_d.TBTCToken);

address rebateTokenHolder = feeRebateTokenHolder(_d);

// We didn't escrow a rebate if the redeemer is also the Fee Rebate Token holder
if(_d.redeemerAddress == rebateTokenHolder) return;

// pay out the rebate if it is available
if(_tbtc.balanceOf(address(this)) >= signerFee(_d)) {
_tbtc.transfer(rebateTokenHolder, signerFee(_d));
if(_d.tbtcToken.balanceOf(address(this)) >= signerFee(_d)) {
_d.tbtcToken.transfer(rebateTokenHolder, signerFee(_d));
}
}

Expand All @@ -430,7 +423,7 @@ library DepositUtils {
return fee;
}
}
uint256 contractTbtcBalance = TBTCToken(_d.TBTCToken).balanceOf(address(this));
uint256 contractTbtcBalance = _d.tbtcToken.balanceOf(address(this));
if(contractTbtcBalance < fee) {
return fee.sub(contractTbtcBalance);
}
Expand Down
24 changes: 12 additions & 12 deletions solidity/contracts/deposit/OutsourceDepositLogging.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ library OutsourceDepositLogging {
/// msg.sender will be the calling Deposit's address.
/// @param _keepAddress The address of the associated keep.
function logCreated(DepositUtils.Deposit storage _d, address _keepAddress) external {
DepositLog _logger = DepositLog(_d.TBTCSystem);
DepositLog _logger = DepositLog(address(_d.tbtcSystem));
_logger.logCreated(_keepAddress);
}

Expand All @@ -34,7 +34,7 @@ library OutsourceDepositLogging {
uint256 _requestedFee,
bytes memory _outpoint
) public { // not external to allow bytes memory output scripts
DepositLog _logger = DepositLog(_d.TBTCSystem);
DepositLog _logger = DepositLog(address(_d.tbtcSystem));
_logger.logRedemptionRequested(
_redeemer,
_digest,
Expand All @@ -57,7 +57,7 @@ library OutsourceDepositLogging {
bytes32 _r,
bytes32 _s
) external {
DepositLog _logger = DepositLog(_d.TBTCSystem);
DepositLog _logger = DepositLog(address(_d.tbtcSystem));
_logger.logGotRedemptionSignature(
_digest,
_r,
Expand All @@ -72,7 +72,7 @@ library OutsourceDepositLogging {
bytes32 _signingGroupPubkeyX,
bytes32 _signingGroupPubkeyY
) external {
DepositLog _logger = DepositLog(_d.TBTCSystem);
DepositLog _logger = DepositLog(address(_d.tbtcSystem));
_logger.logRegisteredPubkey(
_signingGroupPubkeyX,
_signingGroupPubkeyY);
Expand All @@ -81,57 +81,57 @@ library OutsourceDepositLogging {
/// @notice Fires a SetupFailed event.
/// @dev The logger is on a system contract, so all logs from all deposits are from the same address.
function logSetupFailed(DepositUtils.Deposit storage _d) external {
DepositLog _logger = DepositLog(_d.TBTCSystem);
DepositLog _logger = DepositLog(address(_d.tbtcSystem));
_logger.logSetupFailed();
}

/// @notice Fires a FraudDuringSetup event.
/// @dev The logger is on a system contract, so all logs from all deposits are from the same address.
function logFraudDuringSetup(DepositUtils.Deposit storage _d) external {
DepositLog _logger = DepositLog(_d.TBTCSystem);
DepositLog _logger = DepositLog(address(_d.tbtcSystem));
_logger.logFraudDuringSetup();
}

/// @notice Fires a Funded event.
/// @dev The logger is on a system contract, so all logs from all deposits are from the same address.
function logFunded(DepositUtils.Deposit storage _d) external {
DepositLog _logger = DepositLog(_d.TBTCSystem);
DepositLog _logger = DepositLog(address(_d.tbtcSystem));
_logger.logFunded();
}

/// @notice Fires a CourtesyCalled event.
/// @dev The logger is on a system contract, so all logs from all deposits are from the same address.
function logCourtesyCalled(DepositUtils.Deposit storage _d) external {
DepositLog _logger = DepositLog(_d.TBTCSystem);
DepositLog _logger = DepositLog(address(_d.tbtcSystem));
_logger.logCourtesyCalled();
}

/// @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(DepositUtils.Deposit storage _d, bool _wasFraud) external {
DepositLog _logger = DepositLog(_d.TBTCSystem);
DepositLog _logger = DepositLog(address(_d.tbtcSystem));
_logger.logStartedLiquidation(_wasFraud);
}

/// @notice Fires a Redeemed event.
/// @dev The logger is on a system contract, so all logs from all deposits are from the same address.
function logRedeemed(DepositUtils.Deposit storage _d, bytes32 _txid) external {
DepositLog _logger = DepositLog(_d.TBTCSystem);
DepositLog _logger = DepositLog(address(_d.tbtcSystem));
_logger.logRedeemed(_txid);
}

/// @notice Fires a Liquidated event.
/// @dev The logger is on a system contract, so all logs from all deposits are from the same address.
function logLiquidated(DepositUtils.Deposit storage _d) external {
DepositLog _logger = DepositLog(_d.TBTCSystem);
DepositLog _logger = DepositLog(address(_d.tbtcSystem));
_logger.logLiquidated();
}

/// @notice Fires a ExitedCourtesyCall event.
/// @dev The logger is on a system contract, so all logs from all deposits are from the same address.
function logExitedCourtesyCall(DepositUtils.Deposit storage _d) external {
DepositLog _logger = DepositLog(_d.TBTCSystem);
DepositLog _logger = DepositLog(address(_d.tbtcSystem));
_logger.logExitedCourtesyCall();
}
}
6 changes: 6 additions & 0 deletions solidity/contracts/interfaces/ITBTCSystem.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,10 @@ interface ITBTCSystem {
function fetchRelayPreviousDifficulty() external view returns (uint256);

function getInitialCollateralizedPercent() external view returns (uint128);
function getAllowNewDeposits() external view returns (bool);
function isAllowedLotSize(uint256 _lotSizeSatoshis) external view returns (bool);
function requestNewKeep(uint256 _m, uint256 _n, uint256 _bond) external payable returns (address);
function getSignerFeeDivisor() external view returns (uint256);
function getUndercollateralizedThresholdPercent() external view returns (uint128);
function getSeverelyUndercollateralizedThresholdPercent() external view returns (uint128);
Copy link
Contributor

Choose a reason for hiding this comment

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

Arguably our docs should be here rather than on TBTCSystem, but I need to see how our documentation generator is operating.

}
22 changes: 12 additions & 10 deletions solidity/contracts/proxy/DepositFactory.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ pragma solidity ^0.5.10;
import "./CloneFactory.sol";
import "../deposit/Deposit.sol";
import "../system/TBTCSystem.sol";
import "../system/TBTCToken.sol";
import "../system/FeeRebateToken.sol";
import "../system/TBTCSystemAuthority.sol";
import {TBTCDepositToken} from "../system/TBTCDepositToken.sol";

Expand All @@ -18,10 +20,10 @@ contract DepositFactory is CloneFactory, TBTCSystemAuthority{
// Holds the address of the deposit contract
// which will be used as a master contract for cloning.
address payable public masterDepositAddress;
address public tbtcSystem;
address public tbtcToken;
address public tbtcDepositToken;
address public feeRebateToken;
TBTCDepositToken tbtcDepositToken;
TBTCSystem public tbtcSystem;
TBTCToken public tbtcToken;
FeeRebateToken public feeRebateToken;
address public vendingMachine;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we hit this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only use the address and never call any VendingMachine contract functions.
Hitting this would be counterproductive since we would just have to cast it back to use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use the address? createNewDeposit should likewise take a VendingMachine, right? Generally eschewing the type check is a bad idea, and if we have to cast to a looser type at the use point, that's fine---otherwise you're leaving on the table the capacity of the compiler to bonk you when you mishandle some point in the initialization chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we use the address?

It's just used for redemption logic:
if(tdtHolder == vendingMachine){...}

Worth noting, It's introduced into the system as a contract type via tbtcSystem.initialize().
Then it's passed along and stored as an address. Might be worth renaming it to vendingMachineAddress to avoid confusion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this. Makes it clear that we're really only interested in the address.

Also makes it clear this is a dirty dirty hack and in another life where we had more time we'd try to get around it haha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok awesome

uint256 public keepThreshold;
uint256 public keepSize;
Expand All @@ -34,7 +36,7 @@ contract DepositFactory is CloneFactory, TBTCSystemAuthority{
/// @param _masterDepositAddress The address of the master deposit contract.
/// @param _tbtcSystem Address of system contract.
/// @param _tbtcToken Address of TBTC token contract.
/// @param _depositOwnerToken Address of the Deposit Owner Token contract.
/// @param _tbtcDepositToken Address of the TBTC Deposit Token contract.
/// @param _feeRebateToken Address of the Fee Rebate Token contract.
/// @param _vendingMachine Address of the Vending Machine contract.
/// @param _keepThreshold Minimum number of honest keep members.
Expand All @@ -43,17 +45,17 @@ contract DepositFactory is CloneFactory, TBTCSystemAuthority{
address payable _masterDepositAddress,
address _tbtcSystem,
address _tbtcToken,
address _depositOwnerToken,
address _tbtcDepositToken,
address _feeRebateToken,
address _vendingMachine,
uint256 _keepThreshold,
uint256 _keepSize
) public onlyTbtcSystem {
masterDepositAddress = _masterDepositAddress;
tbtcSystem = _tbtcSystem;
tbtcToken = _tbtcToken;
tbtcDepositToken = _depositOwnerToken;
feeRebateToken = _feeRebateToken;
tbtcDepositToken = TBTCDepositToken(_tbtcDepositToken);
tbtcSystem = TBTCSystem(_tbtcSystem);
Shadowfiend marked this conversation as resolved.
Show resolved Hide resolved
tbtcToken = TBTCToken(_tbtcToken);
feeRebateToken = FeeRebateToken(_feeRebateToken);
vendingMachine = _vendingMachine;
keepThreshold = _keepThreshold;
keepSize = _keepSize;
Expand Down
Loading