Fixes ethereum/web3.js#1188 #1191

benjamincburns · 2017-11-22T00:40:46Z

Expands on existing setProvider test, and then uses some inheritance hacks to track web3.eth.Contract.currentProvider and similar per Web3 instance.

coveralls · 2017-11-22T00:47:03Z

Coverage increased (+0.03%) to 85.599% when pulling 3663cb1 on benjamincburns:issue_1188_fix_web3_eth_Contract_shared_state into c4c92c3 on ethereum:1.0.

benjamincburns · 2017-11-22T00:47:33Z

packages/web3-eth/src/index.js

+      Contract.setProvider.apply(this, arguments);
+    };
+
+    util.inherits(ContractWrapper, Contract);


To me, I think this is the most contentious part.

From the current node docs:

Note: Usage of util.inherits() is discouraged. Please use the ES6 class and extends keywords to get language level inheritance support. Also note that the two styles are semantically incompatible.

So, @frozeman my question here is whether we'd rather do things the ES6 way, per the quote above. Is Web3 1.0 babelified? If so, perhaps that'd be more idiomatic.

We are currently working on the 1.0ES6 branch. When we have that we can use it. Currently we do some Babel trans compiling, but I didn’t try if it works, as all the code is es5

Also did you try the standalone lnstantiation? According to the docs?

Would you rather I submitted a PR against that branch? Happy to do so - I think that answers the util.inherit vs class and extend question as well, then?

I didn't try standalone instantiation, as I figured existing tests covered that. This PR definitely won't fix shared state between standalone instances of Contract, however.

If you want to remove that shared state, then I suggest we ditch the dynamic inheritance hacks altogether and go with the usual setProvider thing post construction and revert back to the old contract factory method on web3.eth like in the later pre-1.0 releases.

@frozeman Just tested and this commit cherry-picks cleanly against 1.0ES6 and all tests pass - I'm happy to do that if you like and resubmit?

Important is that standalone still works as expected. We currently don’t have tests for that. Feel free to add some.

Concerning the es6 branch, I will anyway merge this branch back into 1.0ES6 so it should be fine.

@frozeman Provided "as expected" means "as documented," then I'll be happy to add some tests for that.

Otherwise a test like the following will not pass under this branch. Please let me know if fixing this issue is a condition for acceptance.

Contract.setProvider(provider1); let contract1 = new Contract(...); Contract.setProvider(provider2); let contract2 = new Contract(...); assert.deepEqual(provider1, contract1.currentProvider); assert.deepEqual(provider2,contract2.currentProvider);

Actually, forget I asked - I'll add that test, make it pass, and then I think we'll be in good shape. :-)

benjamincburns · 2017-11-22T00:49:00Z

test/setProvider.js

+        assert.equal(lib2.eth.currentProvider.constructor.name, provider3.constructor.name);
+        assert.equal(lib2.eth.net.currentProvider.constructor.name, provider3.constructor.name);
+        assert.equal(lib2.eth.personal.currentProvider.constructor.name, provider3.constructor.name);
+        assert.equal(lib2.eth.Contract.currentProvider.constructor.name, provider3.constructor.name);


Test fails here (line 114) without above changes.

…_web3_eth_Contract_shared_state

benjamincburns · 2017-11-27T19:26:59Z

@frozeman after merging latest 1.0 branch into my PR, I see the following test failure. Not sure why this is happening, however. Would appreciate a pointer.

1) typical usage
       with data
         should deploy a contract, sign transaction, and return contract instance:

      AssertionError: expected 'eth_sendTransaction' to equal 'eth_sendRawTransaction'
      + expected - actual

      -eth_sendTransaction
      +eth_sendRawTransaction

      at test/contract.js:2829:24
      at IpcProvider.FakeIpcProvider.send (test/helpers/FakeIpcProvider.js:53:9)
      at RequestManager.send (packages/web3-core-requestmanager/src/index.js:129:66)
      at sendRequest (packages/web3-core-method/src/index.js:542:42)
      at send (packages/web3-core-method/src/index.js:563:13)
      at Object._executeMethod (packages/web3-eth-contract/src/index.js:888:24)
      at Context.<anonymous> (test/contract.js:2879:16)

benjamincburns · 2017-11-27T21:41:11Z

Broken test is now fixed.

coveralls · 2017-11-27T21:49:07Z

Coverage increased (+0.0004%) to 85.55% when pulling 16087a0 on benjamincburns:issue_1188_fix_web3_eth_Contract_shared_state into 17e1631 on ethereum:1.0.

frozeman · 2017-11-30T12:45:27Z

packages/web3-eth/src/index.js

@@ -138,8 +140,18 @@ var Eth = function Eth() {
    this.personal = new Personal(this.currentProvider);
    this.personal.defaultAccount = this.defaultAccount;

+    var ContractWrapper = function ContractWrapper() {


The problem is that we want the "class" to be "Contract" not "ContractWrapper" when somebody logs the web3.eth.contract object.

I also dopant fully understand this wrapper.

The problem the wrapper solves:

Given two instances of eth, called eth1 and eth2, and two contract instances, contract1, created by calling new eth1.Contract(...) and contract2 created by calling new eth2.Contract(...), we want to ensure that calling eth1.setProvider(...) does not mutate contract2.currentProvider.

The wrapper solves this by giving each instance of eth its own unique contract type, as the contract's provider is stored on its type (hence the need to call Contract.setProvider instead of passing the provider into the contract's constructor).

Edit:

To demonstrate the necessity of the wrapper type, I reverted my changes in packages/web3-eth/src/index.js, and reran tests. The extra checks I added to the setProvider test fail.

2317 passing (21s) 1 failing 1) lib/web3/setProvider Web3 submodules should set the provider using constructor: AssertionError: expected 'IpcProvider' to equal 'HttpProvider' + expected - actual -IpcProvider +HttpProvider at Context.<anonymous> (test/setProvider.js:114:16)

Also, it's trivial to rename the ContractWrapper type to Contract. I can do that if need be. I just wanted to avoid the namespace collision w/ the Contract type defined in the web3-eth-contract module.

Yes please change to Contract

frozeman · 2017-11-30T12:49:33Z

packages/web3-eth/src/index.js

@@ -36,6 +36,8 @@ var Iban = require('web3-eth-iban');
 var Accounts = require('web3-eth-accounts');
 var abi = require('web3-eth-abi');

+var util = require('util');


Is this module available in the browser? As we compile to browser, we need to make sure its available there as well.

Good question. I'm not familiar w/ how you're building for the browser yet, but since util is part of the node runtime, I'd expect most tools like browserify and webpack include shims for it. I'll double check and get back to you.

@frozeman: Answer to your question: It appears that you're using browserify, and it appears (per browserify/browserify#1503) that browserify will pull in the entire util package in this case.

If you'd rather not pull in the entire util module, we can do here what was suggested on that issue, use the inherits package instead of util.inherits. In the ES6 branch you can ditch that as well, and just use the extends keyword.

It would be great if we can avoid using util here and use something natively existing in ES5 already

frozeman · 2017-11-30T12:50:13Z

Thanks. This is a very valuable contribution. But there are still a few things unclear to me. maybe we want to setup a call. Can you reach out to me on gitter?

benjamincburns · 2017-11-30T16:21:32Z

@frozeman Sorry, didn't see your last comment before I started responding above. I reached out on gitter. I'll keep an eye out for your response, but I often miss notifications from there. If I don't respond quickly, feel free to DM me on the ConsenSys Slack as well (I assume you're @frozeman over there as well), as I rarely miss those notifications.

benjamincburns · 2017-11-30T16:49:10Z

@frozeman Since we aren't likely to connect today, I finished responding to your comments above. I'm still happy to have a call on this, but given the timezones and that I'll be traveling all next week, we might not connect for a while.

Please be aware that this is a very important issue for Ganache (our tests can't be updated to 1.0 without this fix or lots of code duplication). I won't be even the slightest bit offended (in fact, I'd be delighted) if you wanted to reject this PR and redo it yourself in some way that's more aligned to your vision of how these things should fit together, especially if that's the quicker path to getting #1188 closed.

frozeman

Sorry for the delay. And thank you for that contributions, its very valuable. Just make sure to change the things i requested now

coveralls · 2017-12-11T10:25:29Z

Coverage increased (+0.0004%) to 85.55% when pulling cfa4982 on benjamincburns:issue_1188_fix_web3_eth_Contract_shared_state into dbf7bfc on ethereum:1.0.

…ontract

…b.com:benjamincburns/web3.js into issue_1188_fix_web3_eth_Contract_shared_state

benjamincburns · 2017-12-19T09:55:10Z

@frozeman I too am sorry for the delay! 😊

Unless I missed something, your requests were that I

Rename the proxied ContractWrapper type to, simply, Contract (I assume to avoid potentially breaking type checks which relied on that name)
Ditch util.inherits in favour of proper ES5 inheritance.

Both are done in the latest push. Also thanks for pushing me to ditch util.inherits - I'd apparently leaned on it as a crutch for too long and never bothered to learn the proper way to do prototype chaining!

coveralls · 2017-12-19T09:57:57Z

Coverage increased (+0.0001%) to 85.671% when pulling 61a9acf on benjamincburns:issue_1188_fix_web3_eth_Contract_shared_state into dd9d540 on ethereum:1.0.

coveralls · 2017-12-19T10:03:42Z

Coverage increased (+0.0001%) to 85.671% when pulling 61a9acf on benjamincburns:issue_1188_fix_web3_eth_Contract_shared_state into dd9d540 on ethereum:1.0.

* Fixes web3#1188 * Exercise standalone Contracts, plus extra set provider tests * forgot to fix up new test during merge * fix broken test * Review changes: get rid of util.inherits, rename ContractWrapper to Contract

Fixes web3#1188

3663cb1

benjamincburns mentioned this pull request Nov 22, 2017

Web3 1.0 instances seem to share state (provider overwritten) #1188

Closed

benjamincburns commented Nov 22, 2017

View reviewed changes

benjamincburns added 3 commits November 27, 2017 13:50

Exercise standalone Contracts, plus extra set provider tests

48b76bd

Merge branch '1.0' of github.com:ethereum/web3.js into issue_1188_fix…

1cb90cf

…_web3_eth_Contract_shared_state

forgot to fix up new test during merge

dea7cfc

fix broken test

16087a0

benjamincburns mentioned this pull request Nov 28, 2017

Add Websocket support trufflesuite/ganache#14

Closed

frozeman reviewed Nov 30, 2017

View reviewed changes

Merge branch '1.0' into issue_1188_fix_web3_eth_Contract_shared_state

cfa4982

frozeman reviewed Dec 11, 2017

View reviewed changes

benjamincburns added 2 commits December 19, 2017 22:48

Review changes: get rid of util.inherits, rename ContractWrapper to C…

7e73f65

…ontract

Merge branch 'issue_1188_fix_web3_eth_Contract_shared_state' of githu…

70869eb

…b.com:benjamincburns/web3.js into issue_1188_fix_web3_eth_Contract_shared_state

Merge branch '1.0' into issue_1188_fix_web3_eth_Contract_shared_state

61a9acf

frozeman approved these changes Dec 21, 2017

View reviewed changes

frozeman merged commit 6cd3885 into web3:1.0 Dec 21, 2017

mjhm mentioned this pull request Dec 27, 2017

"Provider not set or invalid." Error after transition from 1.0.0-beta.24 to 1.0.0-beta.27 #1253

Closed

cgewecke mentioned this pull request Jan 11, 2020

Add ganache-core E2E test #3304

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes ethereum/web3.js#1188 #1191

Fixes ethereum/web3.js#1188 #1191

benjamincburns commented Nov 22, 2017

coveralls commented Nov 22, 2017 •

edited

Loading

benjamincburns Nov 22, 2017 •

edited

Loading

frozeman Nov 22, 2017

frozeman Nov 22, 2017

benjamincburns Nov 22, 2017

benjamincburns Nov 23, 2017

frozeman Nov 27, 2017

benjamincburns Nov 27, 2017

benjamincburns Nov 27, 2017

benjamincburns Nov 22, 2017 •

edited

Loading

benjamincburns commented Nov 27, 2017

benjamincburns commented Nov 27, 2017

coveralls commented Nov 27, 2017 •

edited

Loading

frozeman Nov 30, 2017

benjamincburns Nov 30, 2017 •

edited

Loading

benjamincburns Nov 30, 2017

frozeman Dec 11, 2017

frozeman Nov 30, 2017

benjamincburns Nov 30, 2017

benjamincburns Nov 30, 2017 •

edited

Loading

frozeman Dec 11, 2017

frozeman commented Nov 30, 2017

benjamincburns commented Nov 30, 2017

benjamincburns commented Nov 30, 2017 •

edited

Loading

frozeman left a comment

coveralls commented Dec 11, 2017 •

edited

Loading

benjamincburns commented Dec 19, 2017

coveralls commented Dec 19, 2017 •

edited

Loading

coveralls commented Dec 19, 2017 •

edited

Loading

Fixes ethereum/web3.js#1188 #1191

Fixes ethereum/web3.js#1188 #1191

Conversation

benjamincburns commented Nov 22, 2017

coveralls commented Nov 22, 2017 • edited Loading

benjamincburns Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamincburns Nov 22, 2017 • edited Loading

Choose a reason for hiding this comment

benjamincburns commented Nov 27, 2017

benjamincburns commented Nov 27, 2017

coveralls commented Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

benjamincburns Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamincburns Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frozeman commented Nov 30, 2017

benjamincburns commented Nov 30, 2017

benjamincburns commented Nov 30, 2017 • edited Loading

frozeman left a comment

Choose a reason for hiding this comment

coveralls commented Dec 11, 2017 • edited Loading

benjamincburns commented Dec 19, 2017

coveralls commented Dec 19, 2017 • edited Loading

coveralls commented Dec 19, 2017 • edited Loading

coveralls commented Nov 22, 2017 •

edited

Loading

benjamincburns Nov 22, 2017 •

edited

Loading

benjamincburns Nov 22, 2017 •

edited

Loading

coveralls commented Nov 27, 2017 •

edited

Loading

benjamincburns Nov 30, 2017 •

edited

Loading

benjamincburns Nov 30, 2017 •

edited

Loading

benjamincburns commented Nov 30, 2017 •

edited

Loading

coveralls commented Dec 11, 2017 •

edited

Loading

coveralls commented Dec 19, 2017 •

edited

Loading

coveralls commented Dec 19, 2017 •

edited

Loading