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

Make a deposit (contract code) #24

Merged
merged 20 commits into from
Aug 5, 2019
Merged

Make a deposit (contract code) #24

merged 20 commits into from
Aug 5, 2019

Conversation

liamzebedee
Copy link
Contributor

@liamzebedee liamzebedee commented Aug 1, 2019

Implements the Web3/contract side of creating a deposit using DepositFactory. Part of the client/ package.

Checks in the contract artifacts of tbtc.

Blockers: dApp integration blocked by #18

Refs: #12

@pdyraga pdyraga requested a review from nkuba August 2, 2019 09:13
@liamzebedee liamzebedee marked this pull request as ready for review August 2, 2019 11:16
Copy link
Member

@nkuba nkuba left a comment

Choose a reason for hiding this comment

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

We should commit only the contracts artifacts that we use (TBTCSystem, TBTCToken, KeepBridge, DepositFactory, Deposit, did I miss something?). We don't need to add all the artifacts from tbtc

}


Copy link
Member

Choose a reason for hiding this comment

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

Just one empty line, please.


let electrumConfig

export function setElectrumConfig(config) {
Copy link
Member

Choose a reason for hiding this comment

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

We will need to update this part after #21 is merged.

Copy link
Contributor Author

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.

#21 is merged to master. Can you please merge master to this branch and see if the changes to this file are still necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah @nkuba will do 😄

@@ -0,0 +1,7 @@
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to commit this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's good for reproducibility? Would we be able to find a better way to communicate what's happening?

Copy link
Member

Choose a reason for hiding this comment

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

For now, we will just commit artifacts manually as a workaround. And use the ones deployed to the internal testnet. We need to mention it in the readme and find a proper way later. I don't think we need to add this copy-artifacts.sh file, especially that it may not work due to a hardcoded path to different repo ../../../../tbtc/implementation/build/contracts/*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, perhaps we could still save some time by having a script, but passing in the path of where the contracts are as an argument. What do you think of that?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but we need to add big TODO as this is just for development and remove it as soon as we have a final solution for artifacts downloading.

web3 = new Web3(new Web3.providers.HttpProvider('http://localhost:8545'));
await setDefaults(web3)

// setElectrumConfig({
Copy link
Member

Choose a reason for hiding this comment

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

Remove commented code.


expect(await deposit.getCurrentState()).to.eq.BN('1')
})

Copy link
Member

Choose a reason for hiding this comment

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

Remove duplicated empty lines.

package.json Outdated
@@ -10,6 +11,13 @@
"lib/tbtc-helpers"
],
"devDependencies": {
"bn-chai": "^1.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this all dependencies be added to client/package.json?

package.json Outdated
@@ -1,6 +1,7 @@
{
"name": "tbtc-dapp",
"scripts": {
"install": "subpkg run install",
Copy link
Member

Choose a reason for hiding this comment

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

This won't work unless we define install script in each of the submodules.

➜  tbtc-dapp git:(put-down-small-bond) ✗ npm run install

> tbtc-dapp@ install /Users/jakub/workspace/tbtc-dapp
> subpkg run install

Running run install for 2 packages...
Package tbtc-client ...
npm ERR! missing script: install

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/jakub/.npm/_logs/2019-08-05T10_59_04_931Z-debug.log
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! tbtc-dapp@ install: `subpkg run install`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the tbtc-dapp@ install script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/jakub/.npm/_logs/2019-08-05T10_59_04_959Z-debug.log

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woah - I think I might've missed this from debugging. Will remove.

Copy link
Member

Choose a reason for hiding this comment

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

Let's don't add it. We need to remove this whole submodule thing probably since we've got dapp in the root. It was initially added to simplify tests execution but from now we will handle tests separately per module in the CI.

@liamzebedee
Copy link
Contributor Author

Ok @nkuba manually tested after merging master, it passes. Let's merge?

@@ -3,21 +3,35 @@
"version": "0.0.1",
"description": "",
"scripts": {
"test": "mocha --timeout 5000",
"test": "CONFIG_FILE=${npm_package_config_file} mocha -r @babel/register --timeout 5000",
Copy link
Member

Choose a reason for hiding this comment

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

We no longer need to pass CONFIG_FILE=${npm_package_config_file} in the command.
This line should be:
"test": "mocha -r @babel/register --timeout 5000",

@@ -3,21 +3,35 @@
"version": "0.0.1",
"description": "",
"scripts": {
"test": "mocha --timeout 5000",
"test": "CONFIG_FILE=${npm_package_config_file} mocha -r @babel/register --timeout 5000",
"test:debug": "CONFIG_FILE=${npm_package_config_file} node --inspect node_modules/.bin/mocha -r @babel/register --timeout 5000",
Copy link
Member

Choose a reason for hiding this comment

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

We no longer need to pass CONFIG_FILE=${npm_package_config_file} in the command.
This line should be:
"test:debug": "node --inspect node_modules/.bin/mocha -r @babel/register --timeout 5000",

"js:lint": "eslint .",
"js:lint:fix": "eslint --fix ."
},
"main": "src/index.js",
"config": {
Copy link
Member

Choose a reason for hiding this comment

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

This config was removed in master. We should remove it also from this PR.

@@ -64,5 +64,6 @@ async function calculateAndSubmitFundingProof(electrumClient, txID, fundingOutpu
}

module.exports = {
setElectrumConfig,
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this change in this PR, right?

Copy link
Contributor Author

@liamzebedee liamzebedee Aug 5, 2019

Choose a reason for hiding this comment

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

Nope, will remove


describe("Ethereum helpers", async () => {
before(async () => {
Web3.providers.HttpProvider.prototype.sendAsync = Web3.providers.HttpProvider.prototype.send
Copy link
Member

Choose a reason for hiding this comment

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

What's this magic? Why do we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhhh I accidentally removed the comment on this.

This is an issue with an incompatibility between Web3 1.0 and Truffle's usage of Web3's API's. I will re-document this.
web3/web3.js#1119 for an explainer -

@liamzebedee
Copy link
Contributor Author

@nkuba take another look!

@nkuba
Copy link
Member

nkuba commented Aug 5, 2019

I'm merging this PR with failing CI check because CI is not ready yet.

@nkuba nkuba merged commit 3c75e1d into master Aug 5, 2019
@mhluongo mhluongo deleted the make-a-deposit branch July 15, 2020 12:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants