Skip to content

Conversation

@frangio
Copy link
Contributor

@frangio frangio commented Nov 28, 2018

Fixes #1516
Fixes #1340

It seems like the creation of an HDWalletProvider object was blocking Truffle from exiting. I'm guessing this was because of some pending promise, probably related to a network connection that is started. This was completely unnecessary for the general use case of compiling or running the tests against Ganache, so I made the creation of this provider lazy using a JavaScript getter as seen here.

To be honest, I haven't tested the provider defined in this way. :-) I just know that it fixes the problem we were having.

@frangio frangio added this to the v2.1 milestone Nov 28, 2018
@frangio frangio requested a review from nventuro November 28, 2018 23:08
@spalladino
Copy link
Contributor

@frangio note that you can assign a function that returns the provider to the provider entry in the truffle config file, there is no need to create a custom getter. However, keep in mind that this may cause issues with nonce subproviders if you are using the hdwallet provider (see ConsenSys-archive/truffle-hdwallet-provider#65).

@frangio
Copy link
Contributor Author

frangio commented Nov 28, 2018

@spalladino Oh cool! Thanks, I'll use that feature.

@nventuro
Copy link
Contributor

Yay, it works! From my local testing this also seems to fix #1340. How did you figure it out?

Regarding the fix, do we actually need to have the ropsten network defined? I'd remove the whole thing and drop the truffle-hdwallet-provider dependency, though it may be required for some obscure feature that I'm unaware of.

@frangio
Copy link
Contributor Author

frangio commented Nov 29, 2018

I removed most contracts and it still wasn't working and I just figured that the config was the only other thing that could be failing because I was told that other projects weren't having this problem.

Do we need any network other than ganache? I think we used them for EthPM deployment, but that wan't working anyway (though it may be fixable by now, see #824).

@frangio
Copy link
Contributor Author

frangio commented Nov 29, 2018

If you're sure that it fixes #1340 feel free to add the Fixes annotation to the description, but I don't have the context for that other issue.

@nventuro
Copy link
Contributor

In that case I'd remove the ropsten config, and add it back when (if) the fix the EthPM issue, or any other issue that requires it.

@frangio
Copy link
Contributor Author

frangio commented Nov 29, 2018

Also removed the ganache network which was identical to development.

nventuro
nventuro previously approved these changes Nov 29, 2018
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

Nice! Can we also get rid of truffle-hdwallet-provider?

@frangio
Copy link
Contributor Author

frangio commented Nov 29, 2018

Seems so!

@frangio frangio requested a review from nventuro November 29, 2018 20:37
@nventuro
Copy link
Contributor

Looks like we have an implicit dependency on ethereumjs-util, can you explicitly install it?

@frangio frangio force-pushed the fix-truffle-termination branch from e81a895 to 49bc316 Compare December 1, 2018 21:26
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

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

Awesome! 🎉

@frangio frangio merged commit e7d6e86 into OpenZeppelin:master Dec 2, 2018
@frangio frangio deleted the fix-truffle-termination branch December 2, 2018 03:10
frangio added a commit to frangio/openzeppelin-contracts that referenced this pull request Dec 11, 2018
frangio added a commit that referenced this pull request Dec 12, 2018
* remove note that was fixed in #1526

* add build script

* add prepack script

* remove custom compilation steps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

truffle compile doesn't exit Figure out why the test suite doesn't finish

3 participants