New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parity support (Issue #773) #845

Merged
merged 1 commit into from Oct 12, 2018

Conversation

Projects
None yet
6 participants
@Neurone
Contributor

Neurone commented Sep 15, 2018

Overview

TL;DR
Propostal for #773. I completed the integration of Parity as requested. I used rebase a lot to keep up to date with your changes, so please stop for a while merging other blockchain related PR until this is reviewed.

Review

@vs77bb @iurimatias let me know when you have time to test all this stuff

Cool Spaceship Picture

inna-hansen-pinaple-spaceship

@Neurone Neurone changed the title from Parity support to WIP #773: Parity support Sep 15, 2018

@vs77bb

This comment has been minimized.

Show comment
Hide comment
@vs77bb

vs77bb Sep 18, 2018

Thanks for providing @Neurone 👍

vs77bb commented Sep 18, 2018

Thanks for providing @Neurone 👍

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 18, 2018

Contributor

@vs77bb almost done: last things I'm dealing with are automatic initialization of genesis block and accounts (both quite different from geth). After those I think I can submit a release candidate PR 😄

Contributor

Neurone commented Sep 18, 2018

@vs77bb almost done: last things I'm dealing with are automatic initialization of genesis block and accounts (both quite different from geth). After those I think I can submit a release candidate PR 😄

@Neurone Neurone changed the title from WIP #773: Parity support to RC #773: Parity support Sep 20, 2018

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 20, 2018

Contributor

@vs77bb @iurimatias I completed the integration of Parity as requested. I used rebase a lot to keep up to date with your changes, so please stop for a while merging other blockchain related PR until this is reviewed. Currently Travis complaints are about the lib/modules/webserver/server.js file, that was modified just few hours ago my rebase, so it is included here. Until yesterday all was good with this file (check here: https://github.com/embark-framework/embark/blob/904c716d89143eb8225018363eae823d64441efb/lib/modules/webserver/server.js). Can you check that?

Contributor

Neurone commented Sep 20, 2018

@vs77bb @iurimatias I completed the integration of Parity as requested. I used rebase a lot to keep up to date with your changes, so please stop for a while merging other blockchain related PR until this is reviewed. Currently Travis complaints are about the lib/modules/webserver/server.js file, that was modified just few hours ago my rebase, so it is included here. Until yesterday all was good with this file (check here: https://github.com/embark-framework/embark/blob/904c716d89143eb8225018363eae823d64441efb/lib/modules/webserver/server.js). Can you check that?

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 20, 2018

Contributor

Here my working branch before the last rebase and the webserver merge issue: https://github.com/Neurone/embark/tree/parity-support-review. You can review that to test new Parity functionalities. 😄

Contributor

Neurone commented Sep 20, 2018

Here my working branch before the last rebase and the webserver merge issue: https://github.com/Neurone/embark/tree/parity-support-review. You can review that to test new Parity functionalities. 😄

@Neurone Neurone changed the title from RC #773: Parity support to Parity support (Issue #773) Sep 21, 2018

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 21, 2018

Contributor

@vs77bb @iurimatias Any news? I rebased again and applied some fixes, but still many issues are related to the web server (even the clean develop branch is not stable anymore). Waiting for the situation to be fixed, the best way to test the new features is to use "--noserver" option or to use this branch https://github.com/Neurone/embark/tree/parity-support-review.

Please note the PR is huge, so please not merge other PR until the correct situation is restored.

Contributor

Neurone commented Sep 21, 2018

@vs77bb @iurimatias Any news? I rebased again and applied some fixes, but still many issues are related to the web server (even the clean develop branch is not stable anymore). Waiting for the situation to be fixed, the best way to test the new features is to use "--noserver" option or to use this branch https://github.com/Neurone/embark/tree/parity-support-review.

Please note the PR is huge, so please not merge other PR until the correct situation is restored.

@vs77bb

This comment has been minimized.

Show comment
Hide comment
@vs77bb

vs77bb Sep 25, 2018

@Neurone I heard back from the Embark team that they had an extremely busy weekend, but are hoping to get back to reviewing issues over the next few days. Appreciate your patience here as we move things forward 🙂

vs77bb commented Sep 25, 2018

@Neurone I heard back from the Embark team that they had an extremely busy weekend, but are hoping to get back to reviewing issues over the next few days. Appreciate your patience here as we move things forward 🙂

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 26, 2018

Contributor

@vs77bb No problem. I'll be also very busy starting from the next week (my holiday ends 😄), so let me know if I can do anything else on this to smooth the integration

Contributor

Neurone commented Sep 26, 2018

@vs77bb No problem. I'll be also very busy starting from the next week (my holiday ends 😄), so let me know if I can do anything else on this to smooth the integration

@gitcoinbot gitcoinbot referenced this pull request Sep 26, 2018

Open

Support parity #773

@andremedeiros

This comment has been minimized.

Show comment
Hide comment
@andremedeiros

andremedeiros Sep 27, 2018

Contributor

Hey @Neurone, first of all, awesome spaceship 😉

I tried running your PR, but it fails to start the blockchain (on a fresh setup.) Here's what I get:

~/s/s/embark_demo > dembark blockchain -c parity
===============================================================================
===============================================================================
Embark Blockchain using Parity-Ethereum (https://github.com/paritytech/parity-ethereum)
===============================================================================
===============================================================================
running: parity --version
running: parity --chain=dev --network-id=1337 --base-path=.embark/development/datadir --password=undefined --port=30303 --jsonrpc-port=8555 --jsonrpc-interface=local --jsonrpc-cors=http://localhost:8000,http://localhost:8080,http://embark --jsonrpc-hosts=all --ws-port=8556 --ws-interface=local --ws-origins=http://localhost:8000,http://localhost:8080,http://embark --ws-hosts=all --no-discovery --max-peers=0 --whisper --jsonrpc-apis=web3,eth,pubsub,net,parity,private,parity_pubsub,traces,rpc,shh,shh_pubsub --ws-apis=web3,eth,pubsub,net,parity,private,parity_pubsub,traces,rpc,shh,shh_pubsub,personal --unlock=0x00a329c0648769a73afac7f9381e08fb43dbea72 --gas-floor-target=8000000
parity: 2018-09-27 11:55:36  Starting Parity-Ethereum/v2.0.6-stable-549e202-20180919/x86_64-macos/rustc1.29.0
2018-09-27 11:55:36  Keys path .embark/development/datadir/keys/DevelopmentChain
2018-09-27 11:55:36  DB path .embark/development/datadir/chains/DevelopmentChain/db/1484bce8c021f2ca

parity: 2018-09-27 11:55:36  State DB configuration: fast
2018-09-27 11:55:36  Operating mode: active
undefined Unable to read password file. Ensure it exists and permissions are correct.

parity exited with error code 1

Could you take a look? I haven't touched any configuration, so this should work, I feel.

Contributor

andremedeiros commented Sep 27, 2018

Hey @Neurone, first of all, awesome spaceship 😉

I tried running your PR, but it fails to start the blockchain (on a fresh setup.) Here's what I get:

~/s/s/embark_demo > dembark blockchain -c parity
===============================================================================
===============================================================================
Embark Blockchain using Parity-Ethereum (https://github.com/paritytech/parity-ethereum)
===============================================================================
===============================================================================
running: parity --version
running: parity --chain=dev --network-id=1337 --base-path=.embark/development/datadir --password=undefined --port=30303 --jsonrpc-port=8555 --jsonrpc-interface=local --jsonrpc-cors=http://localhost:8000,http://localhost:8080,http://embark --jsonrpc-hosts=all --ws-port=8556 --ws-interface=local --ws-origins=http://localhost:8000,http://localhost:8080,http://embark --ws-hosts=all --no-discovery --max-peers=0 --whisper --jsonrpc-apis=web3,eth,pubsub,net,parity,private,parity_pubsub,traces,rpc,shh,shh_pubsub --ws-apis=web3,eth,pubsub,net,parity,private,parity_pubsub,traces,rpc,shh,shh_pubsub,personal --unlock=0x00a329c0648769a73afac7f9381e08fb43dbea72 --gas-floor-target=8000000
parity: 2018-09-27 11:55:36  Starting Parity-Ethereum/v2.0.6-stable-549e202-20180919/x86_64-macos/rustc1.29.0
2018-09-27 11:55:36  Keys path .embark/development/datadir/keys/DevelopmentChain
2018-09-27 11:55:36  DB path .embark/development/datadir/chains/DevelopmentChain/db/1484bce8c021f2ca

parity: 2018-09-27 11:55:36  State DB configuration: fast
2018-09-27 11:55:36  Operating mode: active
undefined Unable to read password file. Ensure it exists and permissions are correct.

parity exited with error code 1

Could you take a look? I haven't touched any configuration, so this should work, I feel.

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 27, 2018

Contributor

Hi @andremedeiros, did you recreated the demo? I updated also the template. Old templates does not work until you uprade to the new configuration file (in particular I suppose your blockchain.js configuration file lacks at least the "devPassword" attribute)

Contributor

Neurone commented Sep 27, 2018

Hi @andremedeiros, did you recreated the demo? I updated also the template. Old templates does not work until you uprade to the new configuration file (in particular I suppose your blockchain.js configuration file lacks at least the "devPassword" attribute)

@andremedeiros

This comment has been minimized.

Show comment
Hide comment
@andremedeiros

andremedeiros Sep 27, 2018

Contributor

did you recreated the demo?

I'm an idiot. Done, and done. It seems to work.

We, however, have a feature where, on the right, under "Available Services", it does health checks on the status of the Ethereum client. This seems to be broken with the integration. Could you have a look?

Contributor

andremedeiros commented Sep 27, 2018

did you recreated the demo?

I'm an idiot. Done, and done. It seems to work.

We, however, have a feature where, on the right, under "Available Services", it does health checks on the status of the Ethereum client. This seems to be broken with the integration. Could you have a look?

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 27, 2018

Contributor

I checked also the dashboard and it seems all ok to me (here a snapshot). What's the message on your dashboard? Do you use Parity 2.x?
Please note that currently there a problem with the webserver (not related to this PR), so you need to run the dashboard with "--noserver" option. Or, for now you need to comment the line 42 in lib/modules/webserver/server.js:

this.app.ws('/', function(ws, _req) {
    self.events.on('outputDone', () => {
//      ws.send('outputDone');   // <---- this line causes errors and the process crashes
    });
});

screenshot from 2018-09-27 18-43-36

Contributor

Neurone commented Sep 27, 2018

I checked also the dashboard and it seems all ok to me (here a snapshot). What's the message on your dashboard? Do you use Parity 2.x?
Please note that currently there a problem with the webserver (not related to this PR), so you need to run the dashboard with "--noserver" option. Or, for now you need to comment the line 42 in lib/modules/webserver/server.js:

this.app.ws('/', function(ws, _req) {
    self.events.on('outputDone', () => {
//      ws.send('outputDone');   // <---- this line causes errors and the process crashes
    });
});

screenshot from 2018-09-27 18-43-36

@andremedeiros

This comment has been minimized.

Show comment
Hide comment
@andremedeiros

andremedeiros Sep 27, 2018

Contributor

Here is what I mean. If you do this in develop, it will mark the Ethereum service as red (as it goes away.) In your branch, it switches names, so I suspect there's some more piping that needs doing.

untitled2 2018-09-27 12_58_02

Contributor

andremedeiros commented Sep 27, 2018

Here is what I mean. If you do this in develop, it will mark the Ethereum service as red (as it goes away.) In your branch, it switches names, so I suspect there's some more piping that needs doing.

untitled2 2018-09-27 12_58_02

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 27, 2018

Contributor

I see! Thanks for the gif 😄 I check it now.

Contributor

Neurone commented Sep 27, 2018

I see! Thanks for the gif 😄 I check it now.

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 27, 2018

Contributor

@andremedeiros I checked the behaviour but it is the same on the develop branch. I think it is related to the recent upgrade of the web3 beta release, and in particular in this line

this.events.request("services:register", 'Ethereum', function (cb) {
   async.waterfall([
     function checkNodeConnection(next) {
       if (!self.web3.currentProvider) {    // <----- problem here
         return next(NO_NODE, {name: "No Blockchain node found", status: 'off'});
       }
       next();
     },

     function checkVersion(next) {
       // TODO: web3_clientVersion method is currently not implemented in web3.js 1.0
       self.web3._requestManager.send({method: 'web3_clientVersion', params: []}, (err, version) => {
         if (err) {
           self.isWeb3Ready = false;
           return next(null, {name: "Ethereum node (version unknown)", status: 'on'});
         }

self.web3.currentProvider is not undefined anymore when the provider loose the connection, so the checkNodeConnection passes and the next check is checkVersion, that is not implemented yet so it fails and it alwayes return the message "Ethereum node (version unknown)".

Contributor

Neurone commented Sep 27, 2018

@andremedeiros I checked the behaviour but it is the same on the develop branch. I think it is related to the recent upgrade of the web3 beta release, and in particular in this line

this.events.request("services:register", 'Ethereum', function (cb) {
   async.waterfall([
     function checkNodeConnection(next) {
       if (!self.web3.currentProvider) {    // <----- problem here
         return next(NO_NODE, {name: "No Blockchain node found", status: 'off'});
       }
       next();
     },

     function checkVersion(next) {
       // TODO: web3_clientVersion method is currently not implemented in web3.js 1.0
       self.web3._requestManager.send({method: 'web3_clientVersion', params: []}, (err, version) => {
         if (err) {
           self.isWeb3Ready = false;
           return next(null, {name: "Ethereum node (version unknown)", status: 'on'});
         }

self.web3.currentProvider is not undefined anymore when the provider loose the connection, so the checkNodeConnection passes and the next check is checkVersion, that is not implemented yet so it fails and it alwayes return the message "Ethereum node (version unknown)".

@andremedeiros

This comment has been minimized.

Show comment
Hide comment
@andremedeiros

andremedeiros Sep 27, 2018

Contributor

Ah, you're right. It's not develop, it's master. Something we should look at (cc. @iurimatias)

This is the behaviour I'm talking about. With the blockchain running and when I close the separate blockchain process. I wonder if it's just a matter of rebasing.

Before

After

Contributor

andremedeiros commented Sep 27, 2018

Ah, you're right. It's not develop, it's master. Something we should look at (cc. @iurimatias)

This is the behaviour I'm talking about. With the blockchain running and when I close the separate blockchain process. I wonder if it's just a matter of rebasing.

Before

After

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 27, 2018

Contributor

I see. So the problem is not web3 but how events are managed. In develop the event check:wentOffline:Ethereum does not fire anymore for some reason, so the provider is never set to null, and then it never seems offline anymore.

Contributor

Neurone commented Sep 27, 2018

I see. So the problem is not web3 but how events are managed. In develop the event check:wentOffline:Ethereum does not fire anymore for some reason, so the provider is never set to null, and then it never seems offline anymore.

@andremedeiros

This comment has been minimized.

Show comment
Hide comment
@andremedeiros

andremedeiros Sep 27, 2018

Contributor

Right. But in master it does. I just wanted to make sure that this will also work for Parity when that lands.

Contributor

andremedeiros commented Sep 27, 2018

Right. But in master it does. I just wanted to make sure that this will also work for Parity when that lands.

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 27, 2018

Contributor

Yeah. Maybe some rebase issues like you said. master is currently 118 commits ahead of develop... it seems strange.

Contributor

Neurone commented Sep 27, 2018

Yeah. Maybe some rebase issues like you said. master is currently 118 commits ahead of develop... it seems strange.

@PascalPrecht

This comment has been minimized.

Show comment
Hide comment
@PascalPrecht

PascalPrecht Sep 27, 2018

Member

Yeah. Maybe some rebase issues like you said. master is currently 118 commits ahead of develop... it seems strange.

This is because we recently merged next into master which had a lot of work that was not in develop. The histories diverged at that point. We need to sort this out on our side and get develop straight again, which very likely changes the history of develop as well.

Member

PascalPrecht commented Sep 27, 2018

Yeah. Maybe some rebase issues like you said. master is currently 118 commits ahead of develop... it seems strange.

This is because we recently merged next into master which had a lot of work that was not in develop. The histories diverged at that point. We need to sort this out on our side and get develop straight again, which very likely changes the history of develop as well.

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 28, 2018

Contributor

@PascalPrecht Ok. I heavily worked on develop as requested and this PR is huge, so I fear it will take some time to check the entire PR again. Can you please alert me when the develop branch is in a consistent state and you feel confident that I can proceed? Also, it is perfect if we can coordinate each other so that you stop adding other PRs regarding the blockchain module until this PR is merged.

Contributor

Neurone commented Sep 28, 2018

@PascalPrecht Ok. I heavily worked on develop as requested and this PR is huge, so I fear it will take some time to check the entire PR again. Can you please alert me when the develop branch is in a consistent state and you feel confident that I can proceed? Also, it is perfect if we can coordinate each other so that you stop adding other PRs regarding the blockchain module until this PR is merged.

@PascalPrecht

This comment has been minimized.

Show comment
Hide comment
@PascalPrecht

PascalPrecht Sep 29, 2018

Member

I heavily worked on develop as requested and this PR is huge, so I fear it will take some time to check the entire PR again.

Yes, I agree that this is very unfortunately and ideally we shouldn't rebase branches that other people are working on.

Can you please alert me when the develop branch is in a consistent state and you feel confident that I can proceed?

@Neurone develop has been rebased successfuly on top of latest master a day ago, so you can put your work on top of that. Let me know if you need help rebasing your work.

Also, it is perfect if we can coordinate each other so that you stop adding other PRs regarding the blockchain module until this PR is merged.

As I'm not too familiar with the codebase yet, I can't say how much changes made in the blockchain module interfere with your work. However, I do think that once your PR is rebased again, it shouldn't be too hard to keep it up to date, even if develop keeps evolving in the blockchain module, as following rebase will be rather small.

Member

PascalPrecht commented Sep 29, 2018

I heavily worked on develop as requested and this PR is huge, so I fear it will take some time to check the entire PR again.

Yes, I agree that this is very unfortunately and ideally we shouldn't rebase branches that other people are working on.

Can you please alert me when the develop branch is in a consistent state and you feel confident that I can proceed?

@Neurone develop has been rebased successfuly on top of latest master a day ago, so you can put your work on top of that. Let me know if you need help rebasing your work.

Also, it is perfect if we can coordinate each other so that you stop adding other PRs regarding the blockchain module until this PR is merged.

As I'm not too familiar with the codebase yet, I can't say how much changes made in the blockchain module interfere with your work. However, I do think that once your PR is rebased again, it shouldn't be too hard to keep it up to date, even if develop keeps evolving in the blockchain module, as following rebase will be rather small.

Neurone added a commit to Neurone/embark that referenced this pull request Sep 29, 2018

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 29, 2018

Contributor

@PascalPrecht @andremedeiros I rebased, fixed and submitted the PR again. Feel free to test it again. 🤞

PS: For convenience, I referenced all currently opened PRs that involves blockchain's module in some way.

Contributor

Neurone commented Sep 29, 2018

@PascalPrecht @andremedeiros I rebased, fixed and submitted the PR again. Feel free to test it again. 🤞

PS: For convenience, I referenced all currently opened PRs that involves blockchain's module in some way.

@PascalPrecht

This comment has been minimized.

Show comment
Hide comment
@PascalPrecht

PascalPrecht Sep 29, 2018

Member

@Neurone Thanks for your effort on rebasing your work. We'll review this upcoming week. Just be aware that chances are high we might want to get those commits "in shape" before this gets merged. As in, there's many fixes in tests etc. that are probably better shipped with the commit that introduces the feature.

PS: For convenience, I referenced all currently opened PRs that involves blockchain's module in some way.

I think that this wasn't necessary as

a) some of those PRs don't really touch the code that this PR touches and therefore simply confuses other contributors (see: #883 (comment)) UPDATE: the comment has been removed by the author

b) like I've mentioned in #845 (comment), even if other PRs touch the same code that this PR touches and get merged earlier, it won't be too hard to keep your PR up-to-date (also because most of those other changes are rather small).

I do understand your concerns, especially because this PR turns out to be quite big, but we can't just simply stop merging other work because of that :)

And as always, we're here to help. So if any work or collaboration within version control turns out to be too challenging for a contributor, just let us know and we can do it together.

Member

PascalPrecht commented Sep 29, 2018

@Neurone Thanks for your effort on rebasing your work. We'll review this upcoming week. Just be aware that chances are high we might want to get those commits "in shape" before this gets merged. As in, there's many fixes in tests etc. that are probably better shipped with the commit that introduces the feature.

PS: For convenience, I referenced all currently opened PRs that involves blockchain's module in some way.

I think that this wasn't necessary as

a) some of those PRs don't really touch the code that this PR touches and therefore simply confuses other contributors (see: #883 (comment)) UPDATE: the comment has been removed by the author

b) like I've mentioned in #845 (comment), even if other PRs touch the same code that this PR touches and get merged earlier, it won't be too hard to keep your PR up-to-date (also because most of those other changes are rather small).

I do understand your concerns, especially because this PR turns out to be quite big, but we can't just simply stop merging other work because of that :)

And as always, we're here to help. So if any work or collaboration within version control turns out to be too challenging for a contributor, just let us know and we can do it together.

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Sep 29, 2018

Contributor

a) some of those PRs don't really touch the code that this PR touches and therefore simply confuses other contributors (see: #883 (comment)) UPDATE: the comment has been removed by the author

Yeah, you are right: after a rebase many of those PRs will be fine but I preferred to flag them to avoid merging conflicts. My intention was to help, sorry for the confusion.

b) like I've mentioned in #845 (comment), even if other PRs touch the same code that this PR touches and get merged earlier, it won't be too hard to keep your PR up-to-date (also because most of those other changes are rather small).

Small changes in the old version can bring to very different changes in the new one. I needed developing again (not simply merging) some PRs of others (i.e. send ready only when the proxy is started, start HTTP and WS proxies individually, async setupProxy, etc.). Just trying to limit future rework.

I do understand your concerns, especially because this PR turns out to be quite big, but we can't just simply stop merging other work because of that :)

I'm not asking to stop merging other's work, just to make other contributors to work on the new version as soon as possible. 😃 I think everyone will benefit about that because it means less work for everybody.

In any case, this is all IMHO and these are only suggestions: your repository, your rules. Just let me know when you think this feature is tested enough and ready to be merged, and I'll check it and fix it again as needed.

Contributor

Neurone commented Sep 29, 2018

a) some of those PRs don't really touch the code that this PR touches and therefore simply confuses other contributors (see: #883 (comment)) UPDATE: the comment has been removed by the author

Yeah, you are right: after a rebase many of those PRs will be fine but I preferred to flag them to avoid merging conflicts. My intention was to help, sorry for the confusion.

b) like I've mentioned in #845 (comment), even if other PRs touch the same code that this PR touches and get merged earlier, it won't be too hard to keep your PR up-to-date (also because most of those other changes are rather small).

Small changes in the old version can bring to very different changes in the new one. I needed developing again (not simply merging) some PRs of others (i.e. send ready only when the proxy is started, start HTTP and WS proxies individually, async setupProxy, etc.). Just trying to limit future rework.

I do understand your concerns, especially because this PR turns out to be quite big, but we can't just simply stop merging other work because of that :)

I'm not asking to stop merging other's work, just to make other contributors to work on the new version as soon as possible. 😃 I think everyone will benefit about that because it means less work for everybody.

In any case, this is all IMHO and these are only suggestions: your repository, your rules. Just let me know when you think this feature is tested enough and ready to be merged, and I'll check it and fix it again as needed.

@PascalPrecht

This comment has been minimized.

Show comment
Hide comment
@PascalPrecht

PascalPrecht Sep 30, 2018

Member

Yeah, you are right: after a rebase many of those PRs will be fine but I preferred to flag them to avoid merging conflicts. My intention was to help, sorry for the confusion.

Absolutely, and we appreciate that!

Just trying to limit future rework.

Fully understood!

I'm not asking to stop merging other's work, just to make other contributors to work on the new version as soon as possible. 😃

Yes, I might have not managed to express myself correctly. Let's see how this one goes. Hope we'll land this soon!

Member

PascalPrecht commented Sep 30, 2018

Yeah, you are right: after a rebase many of those PRs will be fine but I preferred to flag them to avoid merging conflicts. My intention was to help, sorry for the confusion.

Absolutely, and we appreciate that!

Just trying to limit future rework.

Fully understood!

I'm not asking to stop merging other's work, just to make other contributors to work on the new version as soon as possible. 😃

Yes, I might have not managed to express myself correctly. Let's see how this one goes. Hope we'll land this soon!

@jrainville

Very nice job. Some small things need to be fixed

Show resolved Hide resolved .jshintrc
Show resolved Hide resolved lib/modules/blockchain_process/blockchain.js
Show resolved Hide resolved lib/modules/blockchain_process/blockchain.js
Show resolved Hide resolved lib/modules/blockchain_process/blockchain.js
Show resolved Hide resolved lib/modules/blockchain_process/blockchain.js
Show resolved Hide resolved lib/modules/blockchain_process/parityClient.js
Show resolved Hide resolved lib/modules/whisper/index.js
Show resolved Hide resolved lib/modules/whisper/index.js
Show resolved Hide resolved lib/modules/whisper/js/embarkjs.js
Show resolved Hide resolved lib/modules/whisper/js/embarkjs.js
@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Oct 4, 2018

Contributor

@jrainville I completed the modifications you requested. Let me know if there's something else to do.

Contributor

Neurone commented Oct 4, 2018

@jrainville I completed the modifications you requested. Let me know if there's something else to do.

@jrainville

One last change. I think the rest is gucci

@jrainville

Looks good for me. You'll just have to do one last rebase

Neurone added a commit to Neurone/embark that referenced this pull request Oct 10, 2018

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Oct 10, 2018

Contributor

@jrainville Rebased.

Contributor

Neurone commented Oct 10, 2018

@jrainville Rebased.

@PascalPrecht

This comment has been minimized.

Show comment
Hide comment
@PascalPrecht

PascalPrecht Oct 10, 2018

Member

@Neurone 👏👏👏👏

This gets probably super annoying but there's one last thing we'd like to ask you to do:

Your PR currently has 39 commits out of which 3 are not from you (probably due to other history rebases) and a lot of the others are work-in-process commits or fixes for bugs that have been introduced in previous commits of the same PR.

Since this is a single feature, it probably makes sense to take all of your feature related commits (including the WIPs and fixes etc) and squash them into a single commit. It's also fine to end up with mulitiple commits if it makes sense semantically.

Just make sure that none of the commits in this PR introduce any sort of "invalid" state. Or in other words, if you remove commits one by one starting with the latest, everything should still build and be valid (no lint errors, no compile errors, travis happy).

Let me know if you need help with that!

Member

PascalPrecht commented Oct 10, 2018

@Neurone 👏👏👏👏

This gets probably super annoying but there's one last thing we'd like to ask you to do:

Your PR currently has 39 commits out of which 3 are not from you (probably due to other history rebases) and a lot of the others are work-in-process commits or fixes for bugs that have been introduced in previous commits of the same PR.

Since this is a single feature, it probably makes sense to take all of your feature related commits (including the WIPs and fixes etc) and squash them into a single commit. It's also fine to end up with mulitiple commits if it makes sense semantically.

Just make sure that none of the commits in this PR introduce any sort of "invalid" state. Or in other words, if you remove commits one by one starting with the latest, everything should still build and be valid (no lint errors, no compile errors, travis happy).

Let me know if you need help with that!

Add support for Parity
Addons
 - New chain initialization and genesis management
 - Option to choose client to use
 - Option to "ping forever" for Geth
 - Creation and unlock of accounts at client's start
 - Utility to fund accounts with ethers
 - Miner settings inside the ethereum client
 - Workaround to CORS problem: origin is now http://embark
 - Several double callback's checks

Updates
 - Boilerplate, templates, configuration files and demo stuff
 - Messages and i18n strings
 - Tests

Fixes
 - Geth client now uses miner.gastarget instead of the deprecated targetGasLimit
 - Workaround for shh_version with Parity

Reworks of other PRs into the new code
 - Included delayed proxy
 - Send ready only when the proxy is started
 - Start HTTP and WS proxies individually
 - Async setupProxy
 - Fixed datadir for GethMiner
@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Oct 10, 2018

Contributor

@PascalPrecht I rebased to have a single commit. I changed the message log to contain all relevant addons and changes made by the commit.

Contributor

Neurone commented Oct 10, 2018

@PascalPrecht I rebased to have a single commit. I changed the message log to contain all relevant addons and changes made by the commit.

@iurimatias

This comment has been minimized.

Show comment
Hide comment
@iurimatias

iurimatias Oct 12, 2018

Member

Hi @Neurone , this branch is currently crashing for me with the default embark run command, see screenshot below:

screen shot 2018-10-12 at 12 47 16 pm

Steps:

  1. cd test_apps/test_app
  2. ../../embark run --nodashboard

notes:

  • I do not have the parity client installed
  • I have geth 1.8.12-stable installed
Member

iurimatias commented Oct 12, 2018

Hi @Neurone , this branch is currently crashing for me with the default embark run command, see screenshot below:

screen shot 2018-10-12 at 12 47 16 pm

Steps:

  1. cd test_apps/test_app
  2. ../../embark run --nodashboard

notes:

  • I do not have the parity client installed
  • I have geth 1.8.12-stable installed
@iurimatias

This comment has been minimized.

Show comment
Hide comment
@iurimatias

iurimatias Oct 12, 2018

Member

@Neurone sorry it has just been called to my attention this are options from the latest geth. I'll update and retry

Member

iurimatias commented Oct 12, 2018

@Neurone sorry it has just been called to my attention this are options from the latest geth. I'll update and retry

@iurimatias iurimatias merged commit bad9b97 into embark-framework:develop Oct 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Neurone Neurone deleted the Neurone:parity-support branch Oct 12, 2018

@PascalPrecht

This comment has been minimized.

Show comment
Hide comment
@PascalPrecht

PascalPrecht Oct 15, 2018

Member

@iurimatias I think we need some documentation for this no?

Member

PascalPrecht commented Oct 15, 2018

@iurimatias I think we need some documentation for this no?

@Neurone

This comment has been minimized.

Show comment
Hide comment
@Neurone

Neurone Oct 15, 2018

Contributor

For users, these are the key points I think should be explained in the documentation:

  • Parity should be already installed
  • Supported version: 2.0.x or above
  • Configuration in config/blockchain.js is the same for both clients (actual config values are converted to corresponding parity commands) with these notes:
    • ethereumClientName is the old geth_bin. It can can be geth, parity, or whatever you called the binary file to start the client (default: geth)
    • datadir: ".embark/development/datadir" Data directory for the databases and keystore (Geth 1.8.15 and Parity 2.0.4 can use the same base folder, till now they does not conflict each other)
    • account.devPassword: "config/development/devpassword" // [Parity-only] File with a void line needed to unlock the Parity dev account
    • You can find a new privateparitynetsample config in demo and boilerplate. with a reference to a new genesis config file specific for Parity

For developers, you can find a summary of the modifications in the commit's log and other useful informations in the comments, and in particular:

  • New chain initialization and genesis management
  • Option to choose client to use
  • Option to "ping forever" for Geth
  • Creation and unlock of accounts at client's start
  • Utility to fund accounts with ethers
  • Miner settings inside the ethereum client
  • Workaround to CORS problem: origin is now http://embark

Updates

  • Boilerplate, templates, configuration files and demo stuff
  • Messages and i18n strings
  • Tests

Fixes

  • Geth client now uses miner.gastarget instead of the deprecated targetGasLimit
  • Workaround for shh_version with Parity
Contributor

Neurone commented Oct 15, 2018

For users, these are the key points I think should be explained in the documentation:

  • Parity should be already installed
  • Supported version: 2.0.x or above
  • Configuration in config/blockchain.js is the same for both clients (actual config values are converted to corresponding parity commands) with these notes:
    • ethereumClientName is the old geth_bin. It can can be geth, parity, or whatever you called the binary file to start the client (default: geth)
    • datadir: ".embark/development/datadir" Data directory for the databases and keystore (Geth 1.8.15 and Parity 2.0.4 can use the same base folder, till now they does not conflict each other)
    • account.devPassword: "config/development/devpassword" // [Parity-only] File with a void line needed to unlock the Parity dev account
    • You can find a new privateparitynetsample config in demo and boilerplate. with a reference to a new genesis config file specific for Parity

For developers, you can find a summary of the modifications in the commit's log and other useful informations in the comments, and in particular:

  • New chain initialization and genesis management
  • Option to choose client to use
  • Option to "ping forever" for Geth
  • Creation and unlock of accounts at client's start
  • Utility to fund accounts with ethers
  • Miner settings inside the ethereum client
  • Workaround to CORS problem: origin is now http://embark

Updates

  • Boilerplate, templates, configuration files and demo stuff
  • Messages and i18n strings
  • Tests

Fixes

  • Geth client now uses miner.gastarget instead of the deprecated targetGasLimit
  • Workaround for shh_version with Parity
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment