Use specific contract types #542

NicholasDotSol · 2020-03-25T18:44:00Z

closes: #507

Rather than storing addresses and then casting to the known contract type, it’s better to use the best type available so the compiler can check for type safety.

Only TBTCDepositToken should be changed since it is the only contract used. The rest are just address placeholders for init funcitons.

FeeRebateToken not using IERC721 since the interface does not expose an exists() function

Shadowfiend

Left a few questions/notes.

solidity/contracts/deposit/Deposit.sol

solidity/contracts/deposit/DepositFunding.sol

solidity/contracts/deposit/DepositRedemption.sol

Shadowfiend · 2020-03-25T19:42:10Z

solidity/contracts/deposit/DepositUtils.sol

@@ -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)))));


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?

Yeah that might be a good idea.

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

solidity/contracts/system/TBTCSystem.sol

Instead of casting the parameter adress, accept Contract type directly

Add functions needed to create a new Deposit.

The interface should contain all function sneeded to create the Deposit.

solidity/contracts/proxy/DepositFactory.sol

Shadowfiend · 2020-03-25T21:49:56Z

solidity/contracts/proxy/DepositFactory.sol

-    address public feeRebateToken;
+    TBTCSystem public tbtcSystem;
+    TBTCToken public tbtcToken;
+    FeeRebateToken public feeRebateToken;
    address public vendingMachine;


Should we hit this one?

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.

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.

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?

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.

Shadowfiend · 2020-03-25T21:50:20Z

solidity/contracts/interfaces/ITBTCSystem.sol

+    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);


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

Explicit visibility for consistency

When a contract has use beyon it's address, Pass the contract type isntead of the address.

solidity/contracts/system/TBTCSystem.sol

Use `this` instead of casting twice to get to the same result.

Take an explicit contract type over an address

We're not using the contract instance, clarify that we are only using the address

solidity/contracts/deposit/Deposit.sol

solidity/contracts/test/deposit/TestDeposit.sol

Shadowfiend · 2020-03-26T20:59:26Z

solidity/contracts/test/keep/ECDSAKeepStub.sol

@@ -35,7 +36,7 @@ contract ECDSAKeepStub is IBondedECDSAKeep {
        // solium-disable-previous-line no-empty-blocks
    }

-    function distributeERC20Reward(address _asset, uint256 _value) external {
+    function distributeERC20Reward(IERC20 _asset, uint256 _value) external {


This won't be enough, we have to play this out over both repos. Let's do that in a separate PR.

This won't be enough

We just need an interface change from keep-ecdsa. This should be enough for Deposit

we have to play this out over both repos

Okay with moving to separate PR to unblock

Well, the interface change will also require changing the implementors of the interface, etc. It's not going to be huge, just needs a little more finesse :)

Changing distributeERC20Reward to accept IERC20 param Requires some dependency syncing Revert the change for now

Shadowfiend

Ok let's do ittt.

NicholasDotSol added 3 commits March 25, 2020 13:25

Use TBTCDepositToken instead of address

1f5c278

Only TBTCDepositToken should be changed since it is the only contract used. The rest are just address placeholders for init funcitons.

Use contract over addresses types where reasonable

3e6b63a

FeeRebateToken not using IERC721 since the interface does not expose an exists() function

TBTCSystem address-> contract type where reasonable

3303dc5

NicholasDotSol requested a review from Shadowfiend March 25, 2020 18:44

Shadowfiend reviewed Mar 25, 2020

View reviewed changes

NicholasDotSol added 3 commits March 25, 2020 16:02

Use contract types direclty in parameters

2577752

Instead of casting the parameter adress, accept Contract type directly

Update ITBTCSystem interface

54b1567

Add functions needed to create a new Deposit.

Use ITBTCSystem directly from DepositUtils

9878ca5

The interface should contain all function sneeded to create the Deposit.

Shadowfiend reviewed Mar 25, 2020

View reviewed changes

solidity/contracts/proxy/DepositFactory.sol Outdated Show resolved Hide resolved

Shadowfiend reviewed Mar 25, 2020

View reviewed changes

NicholasDotSol added 2 commits March 25, 2020 18:04

Make contract types public

ef8c467

Explicit visibility for consistency

Contract type for Function parameters

41104d0

When a contract has use beyon it's address, Pass the contract type isntead of the address.

NicholasDotSol force-pushed the contract-types branch from dd39eaf to 41104d0 Compare March 25, 2020 23:21

Shadowfiend reviewed Mar 26, 2020

View reviewed changes

solidity/contracts/system/TBTCSystem.sol Outdated Show resolved Hide resolved

NicholasDotSol added 3 commits March 26, 2020 15:30

Avoid unnecessary double-case

fc03c4f

Use `this` instead of casting twice to get to the same result.

distributeERC20Reward takes IERC20

9f31086

Take an explicit contract type over an address

vendingMachine -> vendingMachineAddress

08bba81

We're not using the contract instance, clarify that we are only using the address

Shadowfiend reviewed Mar 26, 2020

View reviewed changes

solidity/contracts/deposit/Deposit.sol Outdated Show resolved Hide resolved

Shadowfiend reviewed Mar 26, 2020

View reviewed changes

solidity/contracts/test/deposit/TestDeposit.sol Outdated Show resolved Hide resolved

Shadowfiend reviewed Mar 26, 2020

View reviewed changes

NicholasDotSol added 2 commits March 26, 2020 16:25

Update capitalization for consistency

38e0b59

Under distributeERC20Reward Interface change

6756361

Changing distributeERC20Reward to accept IERC20 param Requires some dependency syncing Revert the change for now

Shadowfiend approved these changes Mar 26, 2020

View reviewed changes

Shadowfiend merged commit c397657 into master Mar 26, 2020

Shadowfiend deleted the contract-types branch March 26, 2020 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use specific contract types #542

Use specific contract types #542

NicholasDotSol commented Mar 25, 2020

Shadowfiend left a comment

Shadowfiend Mar 25, 2020

NicholasDotSol Mar 25, 2020

Shadowfiend Mar 25, 2020

Shadowfiend Mar 25, 2020

NicholasDotSol Mar 25, 2020

Shadowfiend Mar 25, 2020

NicholasDotSol Mar 25, 2020

Shadowfiend Mar 26, 2020

NicholasDotSol Mar 26, 2020

Shadowfiend Mar 25, 2020

Shadowfiend Mar 26, 2020

NicholasDotSol Mar 26, 2020 •

edited

Loading

NicholasDotSol Mar 26, 2020

Shadowfiend Mar 26, 2020

Shadowfiend left a comment

Use specific contract types #542

Use specific contract types #542

Conversation

NicholasDotSol commented Mar 25, 2020

Shadowfiend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicholasDotSol Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shadowfiend left a comment

Choose a reason for hiding this comment

NicholasDotSol Mar 26, 2020 •

edited

Loading