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.

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

Truffle migrations verbosity and fixed invalid JSON rpc #416

Merged
merged 14 commits into from
Nov 4, 2019

Conversation

kendricktan
Copy link
Contributor

https://github.com/omisego/devops/issues/190

What this commit contains:

  1. More verbosity while deploying from truffle (easier to see where did it screw up)
  2. Fixed invalid JSON RPC error on Infura using infura in truffle and get Error: Invalid JSON RPC response: "" trufflesuite/truffle#852 (comment)
  3. Added some warning signs during the initial deployments

);
const paymentToPaymentV2Condition = await PaymentOutputToPaymentTxCondition.deployed();
Copy link
Contributor

@boolafish boolafish Oct 31, 2019

Choose a reason for hiding this comment

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

ah, so you can deploy same contract multiple times by deployer.deply() just when you try to get it back by deployed() you will get the latest one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap you can, you save the instance in memory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also <>.new() deploys the contracts silently whereas deployer.deploy(<>) does it verbosely for some reason.....

@kendricktan kendricktan requested a review from boolafish October 31, 2019 04:10
@@ -2,6 +2,9 @@ require('dotenv').config(); // auto parse env variables from '.env' file

const HDWalletProvider = require('@truffle/hdwallet-provider');

const infuraUrl = `${process.env.INFURA_URL || 'https://infura.io/v3/'}/${process.env.INFURA_API_KEY}`;
const cleanInfuraUrl = infuraUrl.replace(/([^:])(\/\/+)/g, '$1/');
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to keep the previous comment of Replace double // or is it obvious? (not to me but might just be me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes actually keep the comment, it's definitely not obvious

Copy link
Contributor

Choose a reason for hiding this comment

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

reminder: the comment is not added yet

0, 3,
);
},
// Jesus fuck why can't this be a function
Copy link
Contributor

Choose a reason for hiding this comment

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

lol do we want to keep the f word XDD

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
Contributor

Choose a reason for hiding this comment

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

Might want to remove the J word as well. Lets try not to offend anyone... 😄

@@ -0,0 +1,8248 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to support both npm and yarn? They might end up locking different version and bring confusion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I'll remove it. Let's stick with npm for now

// Jesus fuck why can't this be a function
provider: new HDWalletProvider(
[
process.env.DEPLOYER_PRIVATEKEY || '0'.repeat(64),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand, why we would need empty 0s here?

Copy link
Contributor Author

@kendricktan kendricktan Oct 31, 2019

Choose a reason for hiding this comment

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

It fails on tests if the private keys are undefined since infuras wallet provider is not a function, but rather instanciates a class on runtime. This instantiation reads the keys and trys to derive it but immediately throws an error because the keys are undefined. To circumvent this I've made the default keys to be zeros

Copy link
Contributor

@boolafish boolafish left a comment

Choose a reason for hiding this comment

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

Aside from adding comment on the regrex, LGTM

@@ -2,6 +2,9 @@ require('dotenv').config(); // auto parse env variables from '.env' file

const HDWalletProvider = require('@truffle/hdwallet-provider');

const infuraUrl = `${process.env.INFURA_URL || 'https://infura.io/v3/'}/${process.env.INFURA_API_KEY}`;
const cleanInfuraUrl = infuraUrl.replace(/([^:])(\/\/+)/g, '$1/');
Copy link
Contributor

Choose a reason for hiding this comment

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

reminder: the comment is not added yet

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.

3 participants