This repository has been archived by the owner on Mar 28, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 42
Refactor IKeep/KeepBridge #268
Merged
Merged
Changes from 49 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 64778d8
Initialize TBCSystem with Keep Registry address
nkuba 6d53832
Move requestNewKeep from IKeep to TBTCSystem
nkuba 18a8e3a
Update deploy script with system initialization
nkuba c5094a4
Update tests and test contracts
nkuba de8d48a
Remove getKeepPubkey from IKeep interface
nkuba 9c58fab
Update tests after getKeepPubkey change
nkuba c683879
Rename getKeepPubkeyResult -> getKeepPublicKey
nkuba 6f272de
Extract digest approval from IKeep
nkuba 66ff1d8
Update tests after digest approval refactoring
nkuba cac0213
Keep approveDigest function not to return boolean
nkuba a96a3fe
Package lock for latest keep-tecdsa version
nkuba 4e23d40
Use keep-tecdsa interfaces for interaction
nkuba c2550be
Remove KeepBridge
nkuba 40cacc4
Move IKeep interface
nkuba ec3ee18
Update deployment scripts
nkuba fd61c97
Stub functions to implement keep interfaces
nkuba cdc5f41
Stub for IKeep functions
nkuba 57efbcb
Replaced (set|get)KeepInfo with separate functions
nkuba 1b29c2d
Update setExteroriorAddresses to setExteriorAddresses
nkuba c215b42
Update order of functions in TestDeposit
nkuba eb592c3
Remove KeepBridgeTest
nkuba cbb2e67
Update DepositFactory tests
nkuba af677ca
Update Keep stub handling in tests
nkuba 0f48473
Update get/set Keep info in tests
nkuba ed528b0
Update demo script to remove KeepBridge
nkuba 8ee4484
Change approveDigest to internal
nkuba 9c4cac0
approvedDigests bytes -> bytes32
nkuba 33811d3
Get public key directly without getKeepPublicKey
nkuba 4fb3778
IKeep -> IBondedECDSAKeep
nkuba ec3b7a5
Docs update
nkuba a04b5e7
Merge remote-tracking branch 'origin/master' into refactor-keep-bridge
nkuba f985dc7
Fix failinf deposit factory test
nkuba b35a401
Update tests for payment forwarding in Deposit Factory
nkuba 64a54b7
Update approvedDigests documentation
nkuba 2e208d9
Make KeepVendorStub test more dynamic
nkuba 6560d55
Merge remote-tracking branch 'origin/master' into refactor-keep-bridge
nkuba e5de8e7
Update tbtcSystem initialization in test
nkuba 1cb90a8
Update approveDigests map docs
nkuba 675fa69
Use expect tp compare BN in DepositUtilsTest
nkuba 3440afb
Use new for TBTCSystem test deployment
nkuba d507ac7
Initialize value for keep in keep vendor stub
nkuba 5832ca7
Replace external systems addresses with sample values
nkuba b51bffe
Merge remote-tracking branch 'origin/master' into refactor-keep-bridge
nkuba 4ea17c4
Break long lines in TBTCSystem
nkuba 5b6cdd0
npm dependency to @keep-network/keep-tecdsa
nkuba aef6b32
Update path to imported contracts from keep-tecdsa
nkuba eb5fc4b
CircleCI: Authenticate to github npm registry
nkuba d9cf615
Use gituhub repo for @keep-network scope only
nkuba e9d3724
Update comments
nkuba File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,54 +18,72 @@ jobs: | |
lint_js: | ||
executor: docker-node | ||
steps: | ||
- checkout | ||
- run: | ||
name: Lint JS | ||
command: | | ||
cd implementation/ | ||
npm install | ||
npm run js:lint | ||
- checkout | ||
- run: | ||
name: Authenticate GitHub Package Registry | ||
working_directory: ~/project/implementation | ||
command: echo "//npm.pkg.github.com/:_authToken=$GITHUB_TOKEN" >> .npmrc | ||
- run: | ||
name: Lint JS | ||
working_directory: ~/project/implementation | ||
command: | | ||
npm install | ||
npm run js:lint | ||
lint_solidity: | ||
executor: docker-node | ||
steps: | ||
- checkout | ||
- run: | ||
name: Lint Solidity | ||
working_directory: ~/project/implementation | ||
command: | | ||
set -ex | ||
npm install | ||
npm run sol:lint | ||
- checkout | ||
- run: | ||
name: Authenticate GitHub Package Registry | ||
working_directory: ~/project/implementation | ||
command: echo "//npm.pkg.github.com/:_authToken=$GITHUB_TOKEN" >> .npmrc | ||
- run: | ||
name: Lint Solidity | ||
working_directory: ~/project/implementation | ||
command: | | ||
set -ex | ||
npm install | ||
npm run sol:lint | ||
unit_test_contracts: | ||
executor: docker-node | ||
steps: | ||
- checkout | ||
- run: sudo npm install -g [email protected] | ||
- run: | ||
name: Running testrpc | ||
command: ganache-cli | ||
background: true | ||
- run: | ||
name: Run NPM tests | ||
working_directory: ~/project/implementation/contracts | ||
command: npm install && npm run test | ||
- checkout | ||
- run: sudo npm install -g [email protected] | ||
- run: | ||
name: Running testrpc | ||
working_directory: ~/project/implementation | ||
command: ganache-cli | ||
background: true | ||
- run: | ||
name: Authenticate GitHub Package Registry | ||
working_directory: ~/project/implementation | ||
command: echo "//npm.pkg.github.com/:_authToken=$GITHUB_TOKEN" >> .npmrc | ||
- run: | ||
name: Run NPM tests | ||
working_directory: ~/project/implementation | ||
command: npm install && npm run test | ||
integration_test_contracts: | ||
executor: docker-node | ||
steps: | ||
- checkout | ||
- run: sudo npm install -g [email protected] | ||
- run: | ||
name: Running testrpc | ||
command: ganache-cli | ||
background: true | ||
- run: | ||
name: Deploy Uniswap | ||
working_directory: ~/project/implementation/scripts | ||
command: ./deploy_uniswap.sh | ||
- run: | ||
name: Run NPM tests | ||
working_directory: ~/project/implementation/contracts | ||
command: npm install && npm run integration-test | ||
- checkout | ||
- run: sudo npm install -g [email protected] | ||
- run: | ||
name: Running testrpc | ||
working_directory: ~/project/implementation | ||
command: ganache-cli | ||
background: true | ||
- run: | ||
name: Authenticate GitHub Package Registry | ||
working_directory: ~/project/implementation | ||
command: echo "//npm.pkg.github.com/:_authToken=$GITHUB_TOKEN" >> .npmrc | ||
- run: | ||
name: Deploy Uniswap | ||
working_directory: ~/project/implementation/scripts | ||
command: ./deploy_uniswap.sh | ||
- run: | ||
name: Run NPM tests | ||
working_directory: ~/project/implementation | ||
command: npm install && npm run integration-test | ||
generate_pngs: | ||
docker: | ||
- image: keepnetwork/texlive:15 | ||
|
@@ -208,21 +226,21 @@ jobs: | |
image: initcontainer-provision-tbtc-maintainers | ||
tag: latest | ||
publish_contract_data: | ||
executor: gcp-cli/default | ||
steps: | ||
- attach_workspace: | ||
at: /tmp/tbtc | ||
- gcp-cli/install | ||
- gcp-cli/initialize: | ||
google-project-id: GOOGLE_PROJECT_ID | ||
google-compute-zone: GOOGLE_COMPUTE_ZONE_A | ||
# This param doesn't actually set anything, leaving here as a reminder to check when they fix it. | ||
gcloud-service-key: GCLOUD_SERVICE_KEY | ||
- run: | ||
name: Upload contract data | ||
command: | | ||
cd /tmp/tbtc/contracts | ||
gsutil -m cp * gs://keep-dev-contract-data/tbtc | ||
executor: gcp-cli/default | ||
steps: | ||
- attach_workspace: | ||
at: /tmp/tbtc | ||
- gcp-cli/install | ||
- gcp-cli/initialize: | ||
google-project-id: GOOGLE_PROJECT_ID | ||
google-compute-zone: GOOGLE_COMPUTE_ZONE_A | ||
# This param doesn't actually set anything, leaving here as a reminder to check when they fix it. | ||
gcloud-service-key: GCLOUD_SERVICE_KEY | ||
- run: | ||
name: Upload contract data | ||
command: | | ||
cd /tmp/tbtc/contracts | ||
gsutil -m cp * gs://keep-dev-contract-data/tbtc | ||
workflows: | ||
version: 2 | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
@keep-network:registry=https://npm.pkg.github.com/keep-network |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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-network/keep-tecdsa/contracts/api/IECDSAKeep.sol"; | ||
import {DepositStates} from "./DepositStates.sol"; | ||
import {OutsourceDepositLogging} from "./OutsourceDepositLogging.sol"; | ||
import {TBTCConstants} from "./TBTCConstants.sol"; | ||
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
@@ -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( | ||
|
@@ -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(); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 "openzeppelin-solidity/contracts/token/ERC721/IERC721.sol"; | ||
import {IKeep} from "../interfaces/IKeep.sol"; | ||
import {IBondedECDSAKeep} from "../external/IBondedECDSAKeep.sol"; | ||
import {TBTCToken} from "../system/TBTCToken.sol"; | ||
|
||
library DepositUtils { | ||
|
@@ -24,7 +24,6 @@ library DepositUtils { | |
// SET DURING CONSTRUCTION | ||
address TBTCSystem; | ||
address TBTCToken; | ||
address KeepBridge; | ||
uint8 currentState; | ||
|
||
// SET ON FRAUD | ||
|
@@ -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 | ||
|
@@ -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); | ||
} | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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"); | ||
|
@@ -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); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
value is partially forwarded to TBTC System and partially stored as funder bond in the deposit https://github.com/keep-network/tbtc/issues/279.
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.
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.