Consistent deployment gas price #87

juniset · 2020-04-03T11:24:26Z

Making gasPrice consistent across all deployment scripts. The gas price can be set in the .env file as DEPLOYER_GAS_PRICE. If not present the value of 20 Gwei will be used by default.

…ripts

olivdb · 2020-04-07T09:54:25Z

deployment/7_upgrade_1_6.js

-      `Set ${deploymentWallet.address} as the manager of the TokenPriceProvider`);
-  }
-  const TokenPriceProviderSetKyberNetworkTx = await TokenPriceProviderWrapper.contract.setKyberNetwork("0x0000000000000000000000000000000000000000");
+  const TokenPriceProviderAddManagerTx = await TokenPriceProviderWrapper.contract.addManager(deploymentWallet.address, { gasPrice });


I noticed that you left out my if (!await TokenPriceProviderWrapper.contract.managers(deploymentWallet.address)) { but I think it's ok. Not really needed.

I think I dropped it by mistake when doing the rebase. I will add it again.

olivdb · 2020-04-07T10:09:41Z

utils/multisigexecutor.js

@@ -32,7 +33,7 @@ class MultisigExecutor {
      signature = ethers.utils.joinSignature(split);

      // Call "execute" on the Multisig wallet with data and signatures
-      const executeTransaction = await this._multisigWrapper.contract.execute(contractAddress, 0, data, signature, { gasLimit: 2000000 });
+      const executeTransaction = await this._multisigWrapper.contract.execute(contractAddress, 0, data, signature, this._overrides);


There is a multisig transaction that made the deployment script (silently) fail on CI: https://app.circleci.com/pipelines/github/argentlabs/argent-contracts/451/workflows/5f21b4a6-2bfe-491f-8113-527c50ddf6f0/jobs/485

I wonder if this could be because you have reduced the gasLimit from 2M to 300k here.

Hmm, I did not see any fail transactions when running the deployment scripts. I guess we set it to 2M for a reason but it feels a bit too much. I've set it to 1M and confirmed that the deployment scripts work.

elenadimitrova

In general we should avoid using the .contract on etherlime contract wrappers as that points to the underlying ethers.js contract which for gasPrice handling differs in behaviour, in that it doesn't use the etherlime provider set default option. All .contract references in deployment scripts and otherwise can be replaced by direct calls on the wrapper, with the exception of encoding call data we use in some tests.

I am not in favour of passing gasPrice explicitly as it will be forgotten in some calls and we have no way of verifying it apart from trawling through the deployment test to watch for 2 GWei transactions.

This again raises the question of migrating to truffle which has a much clearer gasPrice handling than this.

elenadimitrova · 2020-04-07T13:50:59Z

deployment/7_upgrade_1_6.js

-  const TokenPriceProviderSetKyberNetworkTx = await TokenPriceProviderWrapper.contract.setKyberNetwork("0x0000000000000000000000000000000000000000");
+
+  const TokenPriceProviderSetKyberNetworkTx = await TokenPriceProviderWrapper.contract.setKyberNetwork("0x0000000000000000000000000000000000000000",
+    { gasPrice });


In this script further up, there need to be updates to the MakerRegistryWrapper to use gasPrice explicitly as well, atm it uses 2 Gwei if you look in the deployment scripts.

Same for the TokenPriceProviderWrapper.contract.revokeManager below.

Yep, I forgot those 2 when rebasing my PR. Done.

elenadimitrova · 2020-04-07T14:00:10Z

utils/deploy-manager.js


 const defaultConfigs = {
-  gasPrice: 20000000000, // 20 Gwei
+  gasPrice: utils.bigNumberify(process.env.DEPLOYER_GAS_PRICE || 20000000000),


Can this be in config rather than env vars? We have settings->deployer configuration in there already.

No because the value to use depends on the network status (typically the fast value on https://ethgasstation.info/) and we want to set it at the beginning of a deployment.

elenadimitrova · 2020-04-07T14:12:32Z

deployment/1_setup_test_environment.js

  const KyberWrapper = await deployer.deploy(Kyber);
  const ERC20Wrapper = await deployer.deploy(ERC20, {}, [KyberWrapper.contractAddress], TEST_ERC20_SUPPLY, TEST_ERC20_DECIMALS);

-  const addToken = await KyberWrapper.contract.addToken(ERC20Wrapper.contractAddress, TEST_ERC20_RATE, TEST_ERC20_DECIMALS);
+  const addToken = await KyberWrapper.contract.addToken(ERC20Wrapper.contractAddress, TEST_ERC20_RATE, TEST_ERC20_DECIMALS, { gasPrice });


Initializing UniswapFactory step is also missing gasPrice here.

juniset · 2020-04-07T15:10:01Z

In general we should avoid using the .contract on etherlime contract wrappers as that points to the underlying ethers.js contract which for gasPrice handling differs in behaviour, in that it doesn't use the etherlime provider set default option. All .contract references in deployment scripts and otherwise can be replaced by direct calls on the wrapper, with the exception of encoding call data we use in some tests.

I am not in favour of passing gasPrice explicitly as it will be forgotten in some calls and we have no way of verifying it apart from trawling through the deployment test to watch for 2 GWei transactions.

I considered both approaches and decided to use the contract object of ethers.js because it gives us more flexibility. In particular the wrapper object of etherlime doesn't give us any control on the gasLimit used for a transaction and we've had failing transactions in deployment because the gas was not correctly estimated. This is the reason we are already using the contract object everywhere in the scripts.

By the way I explained these 2 options on the call last week and thought we had agreed to use the contract object instead of the wrapper. I probably should have explained the tradeoff better.

This again raises the question of migrating to truffle which has a much clearer gasPrice handling than this.

If that's the case and we endup rewriting all the scripts for truffle anyway then maybe which option we choose (the contract or the wrapper) doesn't really matter as long as we are consistent.

elenadimitrova

Sorry I can't remember hearing about the etherlime wrapper object passing a bad gasLimit on transactions. Happy as I can be for now.

juniset requested a review from elenadimitrova April 3, 2020 11:24

elenadimitrova assigned juniset Apr 3, 2020

juniset requested a review from olivdb April 6, 2020 10:37

juniset added 4 commits April 6, 2020 17:54

making gasPrice consistent across all transactions of deployment sc…

c8e711d

…ripts

fixing bug in 7_upgrade_1_6.js

e65715c

fixing js linting

b28f287

more js linting

be87cf3

juniset force-pushed the maintenance/deployment_gasPrice branch from ee486f1 to be87cf3 Compare April 6, 2020 16:01

olivdb reviewed Apr 7, 2020

View reviewed changes

addressing review comments

6767c92

olivdb approved these changes Apr 7, 2020

View reviewed changes

elenadimitrova reviewed Apr 7, 2020

View reviewed changes

addressing more review comments

e3587fc

elenadimitrova approved these changes Apr 7, 2020

View reviewed changes

elenadimitrova changed the title ~~Maintenance/deployment gas price~~ Consistent deployment gas price Apr 7, 2020

juniset merged commit eb67742 into develop Apr 8, 2020

elenadimitrova deleted the maintenance/deployment_gasPrice branch April 8, 2020 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Consistent deployment gas price #87

Consistent deployment gas price #87

Uh oh!

juniset commented Apr 3, 2020 •

edited

Loading

Uh oh!

olivdb Apr 7, 2020

Uh oh!

juniset Apr 7, 2020

Uh oh!

olivdb Apr 7, 2020 •

edited

Loading

Uh oh!

juniset Apr 7, 2020 •

edited

Loading

Uh oh!

elenadimitrova left a comment

Uh oh!

elenadimitrova Apr 7, 2020

Uh oh!

elenadimitrova Apr 7, 2020

Uh oh!

juniset Apr 7, 2020

Uh oh!

elenadimitrova Apr 7, 2020

Uh oh!

juniset Apr 7, 2020

Uh oh!

elenadimitrova Apr 7, 2020

Uh oh!

juniset Apr 7, 2020

Uh oh!

juniset commented Apr 7, 2020

Uh oh!

elenadimitrova left a comment

Uh oh!

Uh oh!

Consistent deployment gas price #87

Consistent deployment gas price #87

Uh oh!

Conversation

juniset commented Apr 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olivdb Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juniset Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elenadimitrova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juniset commented Apr 7, 2020

Uh oh!

elenadimitrova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

juniset commented Apr 3, 2020 •

edited

Loading

olivdb Apr 7, 2020 •

edited

Loading

juniset Apr 7, 2020 •

edited

Loading