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

Refactor IKeep/KeepBridge #268

Merged
merged 50 commits into from
Sep 30, 2019
Merged
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
9867947
Pull keep-tecdsa contracts as npm dependency
nkuba Aug 30, 2019
64778d8
Initialize TBCSystem with Keep Registry address
nkuba Aug 30, 2019
6d53832
Move requestNewKeep from IKeep to TBTCSystem
nkuba Aug 30, 2019
18a8e3a
Update deploy script with system initialization
nkuba Aug 30, 2019
c5094a4
Update tests and test contracts
nkuba Aug 30, 2019
de8d48a
Remove getKeepPubkey from IKeep interface
nkuba Sep 3, 2019
9c58fab
Update tests after getKeepPubkey change
nkuba Sep 3, 2019
c683879
Rename getKeepPubkeyResult -> getKeepPublicKey
nkuba Sep 3, 2019
6f272de
Extract digest approval from IKeep
nkuba Sep 4, 2019
66ff1d8
Update tests after digest approval refactoring
nkuba Sep 4, 2019
cac0213
Keep approveDigest function not to return boolean
nkuba Sep 4, 2019
a96a3fe
Package lock for latest keep-tecdsa version
nkuba Sep 5, 2019
4e23d40
Use keep-tecdsa interfaces for interaction
nkuba Sep 5, 2019
c2550be
Remove KeepBridge
nkuba Sep 5, 2019
40cacc4
Move IKeep interface
nkuba Sep 5, 2019
ec3ee18
Update deployment scripts
nkuba Sep 5, 2019
fd61c97
Stub functions to implement keep interfaces
nkuba Sep 5, 2019
cdc5f41
Stub for IKeep functions
nkuba Sep 5, 2019
57efbcb
Replaced (set|get)KeepInfo with separate functions
nkuba Sep 5, 2019
1b29c2d
Update setExteroriorAddresses to setExteriorAddresses
nkuba Sep 5, 2019
c215b42
Update order of functions in TestDeposit
nkuba Sep 5, 2019
eb592c3
Remove KeepBridgeTest
nkuba Sep 5, 2019
cbb2e67
Update DepositFactory tests
nkuba Sep 5, 2019
af677ca
Update Keep stub handling in tests
nkuba Sep 5, 2019
0f48473
Update get/set Keep info in tests
nkuba Sep 5, 2019
ed528b0
Update demo script to remove KeepBridge
nkuba Sep 5, 2019
8ee4484
Change approveDigest to internal
nkuba Sep 10, 2019
9c4cac0
approvedDigests bytes -> bytes32
nkuba Sep 10, 2019
33811d3
Get public key directly without getKeepPublicKey
nkuba Sep 10, 2019
4fb3778
IKeep -> IBondedECDSAKeep
nkuba Sep 10, 2019
ec3b7a5
Docs update
nkuba Sep 10, 2019
a04b5e7
Merge remote-tracking branch 'origin/master' into refactor-keep-bridge
nkuba Sep 10, 2019
f985dc7
Fix failinf deposit factory test
nkuba Sep 11, 2019
b35a401
Update tests for payment forwarding in Deposit Factory
nkuba Sep 12, 2019
64a54b7
Update approvedDigests documentation
nkuba Sep 18, 2019
2e208d9
Make KeepVendorStub test more dynamic
nkuba Sep 18, 2019
6560d55
Merge remote-tracking branch 'origin/master' into refactor-keep-bridge
nkuba Sep 19, 2019
e5de8e7
Update tbtcSystem initialization in test
nkuba Sep 19, 2019
1cb90a8
Update approveDigests map docs
nkuba Sep 22, 2019
675fa69
Use expect tp compare BN in DepositUtilsTest
nkuba Sep 22, 2019
3440afb
Use new for TBTCSystem test deployment
nkuba Sep 22, 2019
d507ac7
Initialize value for keep in keep vendor stub
nkuba Sep 22, 2019
5832ca7
Replace external systems addresses with sample values
nkuba Sep 26, 2019
b51bffe
Merge remote-tracking branch 'origin/master' into refactor-keep-bridge
nkuba Sep 26, 2019
4ea17c4
Break long lines in TBTCSystem
nkuba Sep 26, 2019
5b6cdd0
npm dependency to @keep-network/keep-tecdsa
nkuba Sep 29, 2019
aef6b32
Update path to imported contracts from keep-tecdsa
nkuba Sep 29, 2019
eb5fc4b
CircleCI: Authenticate to github npm registry
nkuba Sep 29, 2019
d9cf615
Use gituhub repo for @keep-network scope only
nkuba Sep 29, 2019
e9d3724
Update comments
nkuba Sep 30, 2019
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
2 changes: 0 additions & 2 deletions implementation/contracts/deposit/Deposit.sol
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,11 @@ contract Deposit {
function createNewDeposit(
address _TBTCSystem,
address _TBTCToken,
address _KeepBridge,
uint256 _m,
uint256 _n
) public payable returns (bool) {
self.TBTCSystem = _TBTCSystem;
self.TBTCToken = _TBTCToken;
self.KeepBridge = _KeepBridge;
self.createNewDeposit(_m, _n);
return true;
}
Expand Down
30 changes: 12 additions & 18 deletions implementation/contracts/deposit/DepositFunding.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ pragma solidity ^0.5.10;
import {SafeMath} from "bitcoin-spv/contracts/SafeMath.sol";
import {BytesLib} from "bitcoin-spv/contracts/BytesLib.sol";
import {BTCUtils} from "bitcoin-spv/contracts/BTCUtils.sol";

import {IECDSAKeep} from "keep-tecdsa/solidity/contracts/api/IECDSAKeep.sol";
Copy link
Member

Choose a reason for hiding this comment

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

❤️

Copy link
Member

Choose a reason for hiding this comment

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

I wonder - can we tweak it in #57 to have it exported to some other directory structure? For example, to have here just:

keep-tecdsa/api/IECDSAKeep.sol

?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to do that, but unfortunately, due to npm limitations, we would need to change the structure to have api directory in the root dir of keep-tecdsa or do some magic scripting workaround. Let's discuss it in keep-network/keep-ecdsa#57 if you think it's needed.


import {TBTCToken} from "../system/TBTCToken.sol";
import {IKeep} from "../interfaces/IKeep.sol";
import {TBTCSystem} from "../system/TBTCSystem.sol";
import {DepositUtils} from "./DepositUtils.sol";
import {DepositLiquidation} from "./DepositLiquidation.sol";
import {DepositStates} from "./DepositStates.sol";
Expand Down Expand Up @@ -39,17 +42,6 @@ library DepositFunding {
_d.signingGroupPubkeyY = bytes32(0);
}

/// @notice get the signer pubkey for our keep
/// @dev calls out to the keep contract, should get 64 bytes back
/// @return the 64 byte pubkey
function getKeepPubkeyResult(DepositUtils.Deposit storage _d) public view returns (bytes memory) {
IKeep _keep = IKeep(_d.KeepBridge);
bytes memory _pubkey = _keep.getKeepPubkey(_d.keepAddress);
/* solium-disable-next-line */
require(_pubkey.length == 64, "public key not set or not 64-bytes long");
return _pubkey;
}

/// @notice The system can spin up a new deposit
/// @dev This should be called by an approved contract, not a developer
/// @param _d deposit storage pointer
Expand All @@ -63,10 +55,10 @@ library DepositFunding {
) public returns (bool) {
require(_d.inStart(), "Deposit setup already requested");

IKeep _keep = IKeep(_d.KeepBridge);

// TODO: Whole value is forwarded to TBTC System, but should be partially
Copy link
Contributor

Choose a reason for hiding this comment

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

value is partially forwarded to TBTC System and partially stored as funder bond in the deposit https://github.com/keep-network/tbtc/issues/279.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually currently it's stored in deposit as funder bond but we need to transfer part of it to the keep as per #297. I've updated the comment.

// stored as funder's bond in the deposit https://github.com/keep-network/tbtc/issues/279.
/* solium-disable-next-line value-in-payable */
_d.keepAddress = _keep.requestNewKeep.value(msg.value)(_m, _n); // kinda gross but
_d.keepAddress = TBTCSystem(_d.TBTCSystem).requestNewKeep.value(msg.value)(_m, _n); // kinda gross but
_d.signingGroupRequestedAt = block.timestamp;

_d.setAwaitingSignerSetup();
Expand Down Expand Up @@ -150,10 +142,12 @@ library DepositFunding {
/// @return True if successful, otherwise revert
function retrieveSignerPubkey(DepositUtils.Deposit storage _d) public {
require(_d.inAwaitingSignerSetup(), "Not currently awaiting signer setup");
bytes memory _keepResult = getKeepPubkeyResult(_d);

_d.signingGroupPubkeyX = _keepResult.slice(0, 32).toBytes32();
_d.signingGroupPubkeyY = _keepResult.slice(32, 32).toBytes32();
bytes memory _publicKey = IECDSAKeep(_d.keepAddress).getPublicKey();
require(_publicKey.length == 64, "public key not set or not 64-bytes long");

_d.signingGroupPubkeyX = _publicKey.slice(0, 32).toBytes32();
_d.signingGroupPubkeyY = _publicKey.slice(32, 32).toBytes32();
require(_d.signingGroupPubkeyY != bytes32(0) && _d.signingGroupPubkeyX != bytes32(0), "Keep returned bad pubkey");
_d.fundingProofTimerStart = block.timestamp;

Expand Down
4 changes: 2 additions & 2 deletions implementation/contracts/deposit/DepositLiquidation.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {BTCUtils} from "bitcoin-spv/contracts/BTCUtils.sol";
import {DepositStates} from "./DepositStates.sol";
import {DepositUtils} from "./DepositUtils.sol";
import {TBTCConstants} from "./TBTCConstants.sol";
import {IKeep} from "../interfaces/IKeep.sol";
import {IBondedECDSAKeep} from "../external/IBondedECDSAKeep.sol";
import {OutsourceDepositLogging} from "./OutsourceDepositLogging.sol";
import {TBTCToken} from "../system/TBTCToken.sol";

Expand Down Expand Up @@ -42,7 +42,7 @@ library DepositLiquidation {
bytes32 _signedDigest,
bytes memory _preimage
) public returns (bool _isFraud) {
IKeep _keep = IKeep(_d.KeepBridge);
IBondedECDSAKeep _keep = IBondedECDSAKeep(_d.keepAddress);
return _keep.submitSignatureFraud(_d.keepAddress, _v, _r, _s, _signedDigest, _preimage);
}

Expand Down
28 changes: 16 additions & 12 deletions implementation/contracts/deposit/DepositRedemption.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import {BytesLib} from "bitcoin-spv/contracts/BytesLib.sol";
import {ValidateSPV} from "bitcoin-spv/contracts/ValidateSPV.sol";
import {CheckBitcoinSigs} from "bitcoin-spv/contracts/SigCheck.sol";
import {DepositUtils} from "./DepositUtils.sol";
import {IKeep} from "../interfaces/IKeep.sol";
import {IBondedECDSAKeep} from "../external/IBondedECDSAKeep.sol";
import {IECDSAKeep} from "keep-tecdsa/solidity/contracts/api/IECDSAKeep.sol";
import {DepositStates} from "./DepositStates.sol";
import {OutsourceDepositLogging} from "./OutsourceDepositLogging.sol";
import {TBTCConstants} from "./TBTCConstants.sol";
Expand All @@ -33,19 +34,20 @@ library DepositRedemption {
address _tbtcTokenAddress = _d.TBTCToken;
TBTCToken _tbtcToken = TBTCToken(_tbtcTokenAddress);

IKeep _keep = IKeep(_d.KeepBridge);
IBondedECDSAKeep _keep = IBondedECDSAKeep(_d.keepAddress);

_tbtcToken.approve(_d.KeepBridge, DepositUtils.signerFee());
_tbtcToken.approve(_d.keepAddress, DepositUtils.signerFee());
_keep.distributeERC20ToKeepGroup(_d.keepAddress, _tbtcTokenAddress, DepositUtils.signerFee());
}

/// @notice approves a digest for signing by our keep group
/// @dev calls out to the keep contract
/// @param _digest the digest to approve
/// @return true if approved, otherwise revert
function approveDigest(DepositUtils.Deposit storage _d, bytes32 _digest) public returns (bool) {
IKeep _keep = IKeep(_d.KeepBridge);
return _keep.approveDigest(_d.keepAddress, _digest);
/// @notice Approves digest for signing by a keep
/// @dev Calls given keep to sign the digest. Records a current timestamp
/// for given digest
/// @param _digest Digest to approve
function approveDigest(DepositUtils.Deposit storage _d, bytes32 _digest) internal {
IECDSAKeep(_d.keepAddress).sign(_digest);

_d.approvedDigests[_digest] = block.timestamp;
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 practice defensive programming in our codebase (thinking about @pdyraga's comment earlier 😉)? Should we assert the digest hasn't already been approved? ie:

require(_d.approvedDigests[_digest] == 0x0, "digest was already approved")

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about this. It's an internal function and we use it in a specific context of redemption.

}

function redemptionTBTCBurn(DepositUtils.Deposit storage _d) private {
Expand Down Expand Up @@ -93,7 +95,8 @@ library DepositRedemption {
_d.initialRedemptionFee = _requestedFee;
_d.withdrawalRequestTime = block.timestamp;
_d.lastRequestedDigest = _sighash;
require(approveDigest(_d, _sighash), "Keep returned false");

approveDigest(_d, _sighash);

_d.setAwaitingWithdrawalSignature();
_d.logRedemptionRequested(
Expand Down Expand Up @@ -168,7 +171,8 @@ library DepositRedemption {
// Ratchet the signature and redemption proof timeouts
_d.withdrawalRequestTime = block.timestamp;
_d.lastRequestedDigest = _sighash;
require(approveDigest(_d, _sighash), "Keep returned false");

approveDigest(_d, _sighash);
NicholasDotSol marked this conversation as resolved.
Show resolved Hide resolved

// Go back to waiting for a signature
_d.setAwaitingWithdrawalSignature();
Expand Down
26 changes: 15 additions & 11 deletions implementation/contracts/deposit/DepositUtils.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {BytesLib} from "bitcoin-spv/contracts/BytesLib.sol";
import {TBTCConstants} from "./TBTCConstants.sol";
import {ITBTCSystem} from "../interfaces/ITBTCSystem.sol";
import {IERC721} from "../interfaces/IERC721.sol";
import {IKeep} from "../interfaces/IKeep.sol";
import {IBondedECDSAKeep} from "../external/IBondedECDSAKeep.sol";
import {TBTCToken} from "../system/TBTCToken.sol";

library DepositUtils {
Expand All @@ -24,7 +24,6 @@ library DepositUtils {
// SET DURING CONSTRUCTION
address TBTCSystem;
address TBTCToken;
address KeepBridge;
uint8 currentState;

// SET ON FRAUD
Expand All @@ -51,6 +50,11 @@ library DepositUtils {
bytes8 utxoSizeBytes; // LE uint. the size of the deposit UTXO in satoshis
uint256 fundedAt; // timestamp when funding proof was received
bytes utxoOutpoint; // the 36-byte outpoint of the custodied UTXO

/// @notice Map of timestamps for transaction digests approved for signing
/// @dev Holds a timestamp from the moment when the transaction digest
/// was approved for signing
mapping (bytes32 => uint256) approvedDigests;
liamzebedee marked this conversation as resolved.
Show resolved Hide resolved
}

/// @notice Gets the current block difficulty
Expand Down Expand Up @@ -363,7 +367,7 @@ library DepositUtils {
/// @dev Calls the keep contract to do so
/// @return The amount of bonded ETH in wei
function fetchBondAmount(Deposit storage _d) public view returns (uint256) {
IKeep _keep = IKeep(_d.KeepBridge);
IBondedECDSAKeep _keep = IBondedECDSAKeep(_d.keepAddress);
return _keep.checkBondAmount(_d.keepAddress);
}

Expand All @@ -374,13 +378,13 @@ library DepositUtils {
return abi.encodePacked(_b).reverseEndianness().bytesToUint();
}

/// @notice determines whether a digest has been approved for our keep group
/// @dev calls out to the keep contract, storing a 256bit int costs the same as a bool
/// @param _digest the digest to check approval time for
/// @return the time it was approved. 0 if unapproved
/// @notice Gets timestamp of digest approval for signing
Copy link
Contributor

Choose a reason for hiding this comment

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

fix up formatting :)

/// @dev Identifies entry in the recorded approvals by keep ID and digest pair
/// @param _digest Digest to check approval for
/// @return Timestamp from the moment of recording the digest for signing.
/// Returns 0 if the digest was not approved for signing
function wasDigestApprovedForSigning(Deposit storage _d, bytes32 _digest) public view returns (uint256) {
IKeep _keep = IKeep(_d.KeepBridge);
return _keep.wasDigestApprovedForSigning(_d.keepAddress, _digest);
return _d.approvedDigests[_digest];
}

/// @notice Looks up the deposit beneficiary by calling the tBTC system
Expand All @@ -406,7 +410,7 @@ library DepositUtils {
/// @return the amount of ether seized
function seizeSignerBonds(Deposit storage _d) public returns (uint256) {
uint256 _preCallBalance = address(this).balance;
IKeep _keep = IKeep(_d.KeepBridge);
IBondedECDSAKeep _keep = IBondedECDSAKeep(_d.keepAddress);
_keep.seizeSignerBonds(_d.keepAddress);
uint256 _postCallBalance = address(this).balance;
require(_postCallBalance > _preCallBalance, "No funds received, unexpected");
Expand All @@ -428,7 +432,7 @@ library DepositUtils {
/// @return true if successful, otherwise revert
function pushFundsToKeepGroup(Deposit storage _d, uint256 _ethValue) public returns (bool) {
require(address(this).balance >= _ethValue, "Not enough funds to send");
IKeep _keep = IKeep(_d.KeepBridge);
IBondedECDSAKeep _keep = IBondedECDSAKeep(_d.keepAddress);
return _keep.distributeEthToKeepGroup.value(_ethValue)(_d.keepAddress);
}
}
Original file line number Diff line number Diff line change
@@ -1,33 +1,8 @@
pragma solidity ^0.5.10;

/**
* @title Keep interface
*/

interface IKeep {

// request a new m-of-n group
// should return a unique keep contract address
function requestNewKeep(uint256 _m, uint256 _n) external payable returns (address _keepAddress);

// get the result of a keep formation
// should return a 64 byte packed pubkey (x and y)
// error if not ready yet
function getKeepPubkey(address _keepAddress) external view returns (bytes memory);

/// @notice Approves digest for signing.
/// @param _keepAddress Keep address
/// @param _digest Digest to sign
/// @return True if successful.
function approveDigest(address _keepAddress, bytes32 _digest) external returns (bool _success);

/// @notice Gets timestamp of digest approval for signing.
/// @param _keepAddress Keep address
/// @param _digest Digest to sign
/// @return Timestamp from the moment of recording the digest for signing.
/// Returns 0 if the digest was not recorded for signing for the given keep.
function wasDigestApprovedForSigning(address _keepAddress, bytes32 _digest) external view returns (uint256);

// TODO: This is a contract holding functions which are required to be implemented
Copy link
Contributor

Choose a reason for hiding this comment

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

comment is a little confusing, what about:

// TODO: This is an interface holding functions which are required to be implemented on keep side

// on keep side and added to IBondedECDSAKeep interface.
interface IBondedECDSAKeep {
// Expected behavior:
// Error if not fraud
// Return true if fraud
Expand All @@ -47,7 +22,7 @@ interface IKeep {

// Allow sending tokens to a keep group
// Useful for sending signers their TBTC
// The Keep contract should call transferFrom on the token contrcact
// The Keep contract should call transferFrom on the token contract
function distributeERC20ToKeepGroup(address _keepAddress, address _asset, uint256 _value) external returns (bool);

// returns the amount of the keep's ETH bond in wei
Expand Down
Loading