Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Proxy accounts from the blockchain so that they can be used in the Dapp #1172

Merged
merged 1 commit into from
Dec 14, 2018
Merged

Proxy accounts from the blockchain so that they can be used in the Dapp #1172

merged 1 commit into from
Dec 14, 2018

Conversation

jrainville
Copy link
Collaborator

One of the last steps of the blockchain refactor. Now, with this, in the Dapp, you get all the accounts specified in the blockchain config while getting accounts.

@jrainville jrainville requested a review from a team December 10, 2018 21:26
@michaelsbradleyjr
Copy link
Contributor

26d3ba1 should probably be turned into a separate PR, but for me that branch is still misbehaving on Windows when embark blockchain and embark run are run separately. Likewise, this PR branch has the same problem on my Windows computer, i.e. stopping embark run while it's experiencing a "hanging ping" causes embark blockchain to crash.

alaibe
alaibe reviewed Dec 11, 2018
View reviewed changes
src/lib/modules/blockchain_process/httpProxyOverride.js
@@ -0,0 +1,136 @@
/* global require */
Copy link
Contributor

Choose a reason for hiding this comment

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

It would have been cool to create new file in TS ;)
next time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah forgot. Also, this file is kind of a hack, because it patches the function in http-proxy with a new function from a PR that was never merged.

@michaelsbradleyjr michaelsbradleyjr self-requested a review December 12, 2018 00:21
michaelsbradleyjr
michaelsbradleyjr suggested changes Dec 12, 2018
View reviewed changes
Copy link
Contributor

@michaelsbradleyjr michaelsbradleyjr left a comment

Choose a reason for hiding this comment

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

The changes in 26d3ba1 (my "WIP" commit on this PR branch) are now provided in #1181. So this can be rebased against master (and commits squashed) after #1181 is merged.

@michaelsbradleyjr
Copy link
Contributor

michaelsbradleyjr commented Dec 13, 2018 •
edited
Loading

Now that #1181 is merged, with a rebase against master and some commit cleanup, I think this one will be good to go.

@jrainville jrainville force-pushed the blockchain-accounts-proxied-rev-no-ping branch from f9da633 to 451d1ac Compare December 13, 2018 17:22
@michaelsbradleyjr
Copy link
Contributor

Will need to be revised re: a PR that will land soon removing usage of express and http-proxy-middleware.

@jrainville
Copy link
Collaborator Author

not again 😢

@michaelsbradleyjr
Copy link
Contributor

This is the one: #1195

@jrainville jrainville force-pushed the blockchain-accounts-proxied-rev-no-ping branch from 451d1ac to 84f9199 Compare December 14, 2018 19:32
@jrainville jrainville changed the base branch from master to refactor/blockchain-proxy-round-3 December 14, 2018 19:32
@jrainville jrainville force-pushed the blockchain-accounts-proxied-rev-no-ping branch from 84f9199 to b135683 Compare December 14, 2018 19:35
@jrainville
Copy link
Collaborator Author

Rebased on top of #1195 and changed base to that branch.

@michaelsbradleyjr
Copy link
Contributor

Looks good!

@iurimatias iurimatias merged commit 365f3d9 into refactor/blockchain-proxy-round-3 Dec 14, 2018
@jrainville jrainville deleted the blockchain-accounts-proxied-rev-no-ping branch December 17, 2018 14:00
@jrainville jrainville mentioned this pull request Dec 17, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alaibe alaibe alaibe left review comments

@michaelsbradleyjr michaelsbradleyjr michaelsbradleyjr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jrainville @michaelsbradleyjr @alaibe @iurimatias