This repository has been archived by the owner on Mar 28, 2023. It is now read-only.
Refactor IKeep/KeepBridge #268
Merged
Conversation
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
nkuba
force-pushed
the
refactor-keep-bridge
branch
from
5f9ecfe to
2431a7f
Compare
We install keep-tecdas contracts via npm. This will allow us to remove interfaces we created before for communication with keeps.
We need to initialize TBTCSystem with Keep Registry address for communication with keep.
Function was moved as we are getting rid of IKeep interface. We will call keep registry contract directly from TBTCSystem.
Removed function to get keep public key from IKeep and KeepBridge. We call keep directly from the deposit.
Moved approveDigest and wasDigestApprovedForSigning from IKeep interaface and KeepBrdige implementation directly to deposit contrac where we call Keep and store list of approved digests in deposit.
Keep function approveDigest won't return boolean, we no longer require it's result to be equal true.
keep-tecdsa exposes interfaces for communication with contracts. Here we use this interfaces.
We remove Keep Bridge and interact with keep contracts directly.
This interface remains until all functions are implemented properly within keep and we can communicate directly with the interface exposed from keep. For now this is just a stub.
Splitet setKeepInfo and getKeepInfo into separate functions to simplify tests.
Updated name of the function and removed KeepBridge address from it which is no longer used.
nkuba
force-pushed
the
refactor-keep-bridge
branch
from
5c0ff17 to
ed528b0
Compare
liamzebedee
previously requested changes
Sep 23, 2019
|
Hey, what's the status here? If we resolve conflicts are we ready to merge? |
|
Yep @nkuba, all okay my end! |
nkuba
force-pushed
the
refactor-keep-bridge
branch
from
e6a27c3 to
b51bffe
Compare
|
Done, the merge was painfull but I sorted it out. Waiting for final approvals. |
Contracts we use from keep-tecdsa project are now published to github package registry. Here we update dependency to use it. We also update .npmrc to point to github registry to be able to download dependencies from there.
nkuba
force-pushed
the
refactor-keep-bridge
branch
3 times, most recently
from
5f1a088 to
467a944
Compare
We need to access private packages from Github Package Registry, here we set it up for Circle CI with a GITHUB_TOKEN.
nkuba
force-pushed
the
refactor-keep-bridge
branch
from
467a944 to
eb5fc4b
Compare
nkuba
force-pushed
the
refactor-keep-bridge
branch
from
93777c3 to
d9cf615
Compare
| /// @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.
fix up formatting :)
| /// 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 |
There was a problem hiding this comment.
comment is a little confusing, what about:
// TODO: This is an interface holding functions which are required to be implemented on keep side
| @@ -63,10 +55,11 @@ 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 | |||
There was a problem hiding this comment.
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.
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.
NicholasDotSol
approved these changes
Sep 30, 2019
pdyraga
dismissed
liamzebedee’s stale review
Approved in the comment, all questions addressed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
In this PR we move functions implemented in a KeepBridge contract responsible for communication with keep to be directly executed by a deposit.
We pull interfaces from
keep-tecdsato communicate with the contracts deployed as part of keep system.There still remained
IKeepinterface which holds functions which are not yet implemented on the keep side. As it's done this interface should be replaced byIECDSAKeepcalls.Closes: #221