Fetch deposit BTC address (contract code) #25

liamzebedee · 2019-08-01T19:59:41Z

Refs #14

nkuba · 2019-08-02T11:41:12Z

Shouldn't this PR be linked against #14?

liamzebedee · 2019-08-02T16:19:28Z

This is functioning and ready for review. It just is blocked by #24 being merged, with the artifacts included

nkuba

This is just a partial implementation of what we need to close #14.

Currently, this PR contains the code to retrieve a public key (from tbtc demo script: https://github.com/keep-network/tbtc/blob/master/implementation/demo/2_fetch_public_key.js).
But we also need to add conversion of the key to the BTC address. More details in #25 (comment).

nkuba · 2019-08-03T09:17:09Z

client/src/Deposit.js

-  // 1. Call deposit to create new keep
-  // 2. Watch for ECDSAKeepCreated event from ECDSAKeepFactory contract
-  // 3. call get public key
+export async function initializeDeposit(depositAddress) {


Will we need this function? I think we can remove it.

Yep 💯 I think I removed it in #24

nkuba · 2019-08-03T09:24:32Z

client/src/Deposit.js

 }
+
+export async function waitDepositBTCPublicKey(depositAddress) {


How do we use this function? I can see that in getDepositBTCPublicKey you're finding RegisteredPubkey event.

We should also monitor when the keep is ready with the public key so we can transition from initialize deposit page to retrieve the public key.

I think that for simplification we can just wait for a couple of seconds now (or transition manually on button click) - single signer keep calculates and publishes the key almost immediately.

Later we will need to emit an event from the keep contract and monitor it in the dapp.

Yeah exactly!

So the reason this function is here is to do that. However, due to the current Ganache testchain setup I've got, there's a race between keep-ecdsa emitting the signature and me starting to listen for the event (due to the fact that you can only listen once you have the deposit address).

So to mitigate this, we will have to do some polling instead.

client/src/Deposit.js

nkuba · 2019-08-03T09:33:39Z

client/src/Deposit.js

+
+  console.log(`Call getPublicKey for deposit [${deposit.address}]`)
+
+  let result


We could simplify

let result try { result = await deposit.retrieveSignerPubkey() } catch(err) { console.error(`retrieveSignerPubkey failed: ${err}`) }

to

const result = await deposit.retrieveSignerPubkey() .catch((err)=> { console.error(`retrieveSignerPubkey failed: ${err}`) })

nkuba · 2019-08-05T12:16:09Z

client/src/Deposit.js

  const publicKeyX = eventList[0].args._signingGroupPubkeyX
  const publicKeyY = eventList[0].args._signingGroupPubkeyY

  console.log(`Registered public key:\nX: ${publicKeyX}\nY: ${publicKeyY}`)

-  return [publicKeyX, publicKeyY]
+  const address = publicKeyToP2WPKHaddress(
+    `${publicKeyX.slice(2)}${publicKeyY.slice(2)}`,


What is the result of ${publicKeyX.slice(2)}${publicKeyY.slice(2)}? Why do we do slice(2)?

We should concatenate X and Y coordinates to 64 bytes. Note that if a coordinate's length is lesser than 32-bytes we should pad it with zeros.

Yeah it's to remove the 0x prefix from the strings :)

Maybe prefer a regex in case you aren't getting the string you expect in the future? Also it's self-documenting :)

Haha I think this miiight be a bit of overkill. Considering it's an event emitted from EVM, the value is uint256, so the string should always be the full length and in the 0x format - these assumptions are validated in the test.

Still tried the regex though ;)

('0x'+'0'.repeat(40)).match(/^(?:0x)([a-fA-F0-9]{40})$/)`

We need to add a comment on what's happening and why we slice the values.

On second thought, it's not overkill, I couldn't find any docs from Web3 on events.

How's this for self-documenting code!

let publicKeyX = eventList[0].args._signingGroupPubkeyX let publicKeyY = eventList[0].args._signingGroupPubkeyY publicKeyX = publicKeyX.replace('0x', '') publicKeyY = publicKeyY.replace('0x', '')

liamzebedee · 2019-08-05T16:36:24Z

@nkuba also ready for review

client/src/eth/artifacts/Deposit.json

nkuba · 2019-08-05T20:29:38Z

client/test/DepositTest.js

@@ -1,6 +1,7 @@
 const Web3 = require('web3')

-import { createDeposit, setDefaults } from '../src'
+import { createDeposit, setDefaults, initializeDeposit, getDepositBTCPublicKey, waitDepositBTCPublicKey } from '../src'
+import { setElectrumConfig } from '../src/FundingProof';


We don't need this import, right?

But I do need that linter! Hahaha

nkuba · 2019-08-05T20:30:42Z

client/test/DepositTest.js

@@ -1,6 +1,7 @@
 const Web3 = require('web3')

-import { createDeposit, setDefaults } from '../src'
+import { createDeposit, setDefaults, initializeDeposit, getDepositBTCPublicKey, waitDepositBTCPublicKey } from '../src'


Do we need all the functions imported here? It looks to me that we need to add only getDepositBTCPublicKey to the previous imports.

nkuba · 2019-08-05T20:31:09Z

client/src/Deposit.js

@@ -34,4 +40,68 @@ export async function createDeposit() {
  const depositAddress = logs[0].args.depositCloneAddress

  return depositAddress
+}
+


Remove duplicated empty line.

nkuba · 2019-08-05T20:31:57Z

client/src/Deposit.js

+import {
+  publicKeyToP2WPKHaddress,
+  Network
+} from '../../lib/tbtc-helpers/src/Address'


Shouldn't this be } from 'tbtc-helpers/src/Address'? We have tbtc-helpers as a package dependency.

nkuba · 2019-08-05T20:32:32Z

client/src/Deposit.js

+}
+
+
+// A complement to getDepositBTCPublicKey.


Update to JS docs style we use in other functions.

So if we're not using it maybe we shouldn't commit it? Will we use it for the demo dapp?

@nkuba I'd prefer not to throw away the code if it's going to be useful in immediate future. 😄

In this case (and I know I keep saying this), we are building a library out of the client/ package. As I explained in the comment, the purpose of this function, while not needed yet, will be needed in the production environment.

I would write a test for the function too, so that it's not just dangling there in the code...?

A good practice is to write a code for immediate future but mainnet is not that immediate. Experience shows that such a code is usually dangling, after a month or two, nobody knows why it's there, it gets out of date quickly and it is maintained only to have the compilation passing with no effort in understanding what it really does.

If we were to use this code in a week or two, this would be fine to leave it, but if it's a matter of months, I'd propose to ☠️ it.

Ok in which case I'll remove it.

nkuba · 2019-08-05T20:44:06Z

client/src/Deposit.js

+ * @param {string} depositAddress the address of a Deposit contract
+ * @returns a bech32-encoded Bitcoin address
+ */
+export async function getDepositBTCPublicKey(depositAddress) {


The name getDepositBTCPublicKey is not accurate. We're getting the BTC address from this function.

We already have a function for this purpose in:

tbtc-dapp/client/src/FundingTransaction.js

Line 2 in 3c75e1d

async function getAddress(depositAddress) {

Can we move the implementation to the function I linked?
Actually getBTCadress name would be better than getAddress.
It's up to you if we keep it in Deposit.js or FundingTransaction.js file.

Yep 100% agree - will do.

nkuba · 2019-08-05T20:45:55Z

client/src/Deposit.js

+export async function getDepositBTCPublicKey(depositAddress) {
+  const tbtcSystem = await TBTCSystem.deployed()
+
+  // 1. Request it from the deposit


Let's be more specific and replace it/this in comments with public key and others.

client/src/Deposit.js

nkuba · 2019-08-05T20:54:59Z

client/src/Deposit.js

+    })
+
+  // 2. Parse the logs to get it
+  // we can't get this from result.logs, since it's emitted in another contract


What's more, we should do it as we do here (with fromBlock: '0') because retrieveSignerPubkey might fail because the key was already retrieved (see my previous comment). Let's have it in a comment.

nkuba · 2019-08-05T20:56:38Z

client/src/Deposit.js

+
+  console.log(`Registered public key for deposit ${depositAddress}:\nX: ${publicKeyX}\nY: ${publicKeyY}`)
+
+  const address = publicKeyToP2WPKHaddress(


address -> btcAddress

nkuba · 2019-08-06T12:06:54Z

Let's edit this PR and set base branch to master. We also need to reverse merge master to this branch.

nkuba · 2019-08-06T14:12:56Z

Re: artifacts - I'd prefer to check them in now and remove later? Even though they'll fail due to missing deployments - at least it won't break the build (since they're being require'd).

We will upload the artifacts deployed on our internal testnet in #35. Let's don't commit them in this PR.

liamzebedee · 2019-08-06T14:13:50Z

Ready @nkuba

nkuba · 2019-08-06T14:17:07Z

Let's merge master to this branch and see if CI passes.

nkuba · 2019-08-06T14:56:53Z

client/src/FundingTransaction.js

-  // TODO: Implement:
-  // 1. Get keep public key from the deposit
-  // 2. Calculate P2WPKH address from the key
+


So many blank lines, let's remove them in one of the upcoming PRs.

nkuba · 2019-08-06T14:57:11Z

client/src/FundingTransaction.js

 }

+


Same here about blank lines.

nkuba · 2019-08-06T15:10:06Z

package.json

@@ -16,6 +16,7 @@
    "redux": "^4.0.4",
    "redux-saga": "^1.0.5",
    "tbtc-client": "file:client",
+    "tbtc-helpers": "file:lib/tbtc-helpers",


Do we need this dependency in dapp's package.json? We should be using only tbtc-client.

nkuba · 2019-08-06T15:23:55Z

client/src/FundingTransaction.js

@@ -1,12 +1,61 @@
-const Address = require('tbtc-helpers').Address


CI is failing because of this removal.

pdyraga · 2019-08-06T20:38:45Z

Failed to compile.

./lib/tbtc-helpers/node_modules/bcoin/lib/bcoin.js
Critical dependency: the request of a dependency is an expression

./lib/tbtc-helpers/node_modules/bcoin/node_modules/bcrypto/lib/node/bn.js
SyntaxError: /home/circleci/project/lib/tbtc-helpers/node_modules/bcoin/node_modules/bcrypto/lib/node/bn.js: Identifier directly after number (51:17)

  49 |  */
  50 | 
> 51 | const U8_MAX = (1n << 8n) - 1n;
     |                  ^
  52 | const U16_MAX = (1n << 16n) - 1n;
  53 | const U32_MAX = (1n << 32n) - 1n;
  54 | const U64_MAX = (1n << 64n) - 1n;

./lib/tbtc-helpers/node_modules/bcrypto/lib/node/bn.js
SyntaxError: /home/circleci/project/lib/tbtc-helpers/node_modules/bcrypto/lib/node/bn.js: Identifier directly after number (46:17)

  44 |  */
  45 | 
> 46 | const U8_MAX = (1n << 8n) - 1n;
     |                  ^
  47 | const U16_MAX = (1n << 16n) - 1n;
  48 | const U32_MAX = (1n << 32n) - 1n;
  49 | const U64_MAX = (1n << 64n) - 1n;

liamzebedee · 2019-08-06T20:42:34Z

Yeah @pdyraga see this thread

liamzebedee added 2 commits August 1, 2019 20:29

Export initialiseDeposit, empty test

3c6aa8f

Implement getDepositBTCPublicKey and test

7069859

liamzebedee marked this pull request as ready for review August 2, 2019 11:16

liamzebedee changed the title ~~Put down small bond (contract code)~~ Fetch deposit BTC address (contract code) Aug 2, 2019

Now converting key to BTC address

bbcf7d7

nkuba suggested changes Aug 3, 2019

View reviewed changes

pdyraga assigned nkuba and liamzebedee Aug 4, 2019

nkuba reviewed Aug 5, 2019

View reviewed changes

liamzebedee added 5 commits August 5, 2019 18:01

Merge artifact addresses

2d82278

Simpler try/catch

10a57fa

Doc getDepositBTCPublicKey

9ce980f

Doc watchForDepositBTCPublicKey

328bc85

Add type to param doc

3982aad

Merge master

cda8d02

liamzebedee mentioned this pull request Aug 5, 2019

Fetch deposit btc address (UI integration) #34

Closed

nkuba reviewed Aug 5, 2019

View reviewed changes

client/src/eth/artifacts/Deposit.json Outdated Show resolved Hide resolved

nkuba suggested changes Aug 5, 2019

View reviewed changes

liamzebedee added 3 commits August 6, 2019 13:54

Manual linting

f9a1350

Refactor getDepositBtcAddress

511bcd2

Better self-doc'ing code for hex values in events

cdb5023

liamzebedee added 4 commits August 6, 2019 14:09

Refactor test to match new names

e6e238e

Clearer comments

51b586f

Comments on error handling

339f449

Merge from master

5d07f3d

Remove artifacts

567c7d2

nkuba approved these changes Aug 6, 2019

View reviewed changes

liamzebedee added 3 commits August 6, 2019 16:26

Merge from master

009551e

Fix import path

013eca2

Make index.js the main import

22babe9

nkuba approved these changes Aug 6, 2019

View reviewed changes

liamzebedee added 2 commits August 6, 2019 17:03

Test path issues with CI build

bfda437

Move back index.js to avoid test refactoring

d5819d3

nkuba reviewed Aug 6, 2019

View reviewed changes

Add Address back to imports

030b620

nkuba force-pushed the put-down-small-bond branch from d2d1845 to 030b620 Compare August 6, 2019 15:44

liamzebedee added 8 commits August 6, 2019 18:58

Consistent export syntax

4e327d0

Remove tbtc-helpers, use standard path for index.js

729aa1e

Revert export changes

d6485e5

Remove index.js, import with simple paths

3166d45

Fix bitcoinspv import

37a4c6f

Add index.js, again

fc5d82e

Fix import syntax

cb2b709

Another permutation of "main" and import path

d1d5288

nkuba added 3 commits August 7, 2019 21:23

Add max-old-space-size to build command

93230da

Merge remote-tracking branch 'origin/master' into put-down-small-bond

b9df389

Fix exports in FundingTransaction

a680bec

nkuba merged commit 8690899 into master Aug 7, 2019

nkuba deleted the put-down-small-bond branch August 7, 2019 19:34

		}

		export async function waitDepositBTCPublicKey(depositAddress) {


		console.log(`Call getPublicKey for deposit [${deposit.address}]`)

		let result


		console.log(`Registered public key for deposit ${depositAddress}:\nX: ${publicKeyX}\nY: ${publicKeyY}`)

		const address = publicKeyToP2WPKHaddress(

		@@ -1,12 +1,61 @@
		const Address = require('tbtc-helpers').Address

Fetch deposit BTC address (contract code) #25

Fetch deposit BTC address (contract code) #25

Conversation

liamzebedee commented Aug 1, 2019 • edited Loading

nkuba commented Aug 2, 2019

liamzebedee commented Aug 2, 2019

nkuba left a comment • edited Loading

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

liamzebedee Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamzebedee commented Aug 5, 2019

Choose a reason for hiding this comment

liamzebedee Aug 6, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkuba commented Aug 6, 2019 • edited Loading

nkuba commented Aug 6, 2019

liamzebedee commented Aug 6, 2019

nkuba commented Aug 6, 2019

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

pdyraga commented Aug 6, 2019

liamzebedee commented Aug 6, 2019

liamzebedee commented Aug 1, 2019 •

edited

Loading

nkuba left a comment •

edited

Loading

liamzebedee Aug 6, 2019 •

edited

Loading

liamzebedee Aug 6, 2019 •

edited

Loading

nkuba commented Aug 6, 2019 •

edited

Loading