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

Fixes ethereum/web3.js#1188 #1191

Merged
merged 9 commits into from
Dec 21, 2017
Merged

Fixes ethereum/web3.js#1188 #1191

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3663cb1
Fixes ethereum/web3.js#1188
benjamincburns Nov 22, 2017
48b76bd
Exercise standalone Contracts, plus extra set provider tests
benjamincburns Nov 27, 2017
1cb90cf
Merge branch '1.0' of github.com:ethereum/web3.js into issue_1188_fix…
benjamincburns Nov 27, 2017
dea7cfc
forgot to fix up new test during merge
benjamincburns Nov 27, 2017
16087a0
fix broken test
benjamincburns Nov 27, 2017
cfa4982
Merge branch '1.0' into issue_1188_fix_web3_eth_Contract_shared_state
frozeman Dec 11, 2017
7e73f65
Review changes: get rid of util.inherits, rename ContractWrapper to C…
benjamincburns Dec 19, 2017
70869eb
Merge branch 'issue_1188_fix_web3_eth_Contract_shared_state' of githu…
benjamincburns Dec 19, 2017
61a9acf
Merge branch '1.0' into issue_1188_fix_web3_eth_Contract_shared_state
benjamincburns Dec 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions packages/web3-eth-contract/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,16 @@ var Contract = function Contract(jsonInterface, address, options) {
var _this = this,
args = Array.prototype.slice.call(arguments);

if(!(this instanceof Contract)) {
throw new Error('Please use the "new" keyword to instantiate a web3.eth.contract() object!');
}

// sets _requestmanager
core.packageInit(this, [Contract.currentProvider]);
core.packageInit(this, [this.constructor.currentProvider]);

this.clearSubscriptions = this._requestManager.clearSubscriptions;


if(!(this instanceof Contract)) {
throw new Error('Please use the "new" keyword to instantiate a web3.eth.contract() object!');
}

if(!jsonInterface || !(Array.isArray(jsonInterface))) {
throw new Error('You must provide the json interface of the contract when instantiating a contract object.');
Expand Down Expand Up @@ -173,8 +173,8 @@ var Contract = function Contract(jsonInterface, address, options) {
});

// get default account from the Class
var defaultAccount = Contract.defaultAccount;
var defaultBlock = Contract.defaultBlock || 'latest';
var defaultAccount = this.constructor.defaultAccount;
var defaultBlock = this.constructor.defaultBlock || 'latest';

Object.defineProperty(this, 'defaultAccount', {
get: function () {
Expand Down Expand Up @@ -216,9 +216,9 @@ var Contract = function Contract(jsonInterface, address, options) {

Contract.setProvider = function(provider, accounts) {
// Contract.currentProvider = provider;
core.packageInit(Contract, [provider]);
core.packageInit(this, [provider]);

Contract._ethAccounts = accounts;
this._ethAccounts = accounts;
};


Expand Down Expand Up @@ -498,7 +498,8 @@ Contract.prototype.deploy = function(options, callback){
return this._createTxObject.apply({
method: constructor,
parent: this,
deployData: options.data
deployData: options.data,
_ethAccounts: this.constructor._ethAccounts
}, options.arguments);

};
Expand Down Expand Up @@ -695,6 +696,7 @@ Contract.prototype._createTxObject = function _createTxObject(){
txObject.arguments = args || [];
txObject._method = this.method;
txObject._parent = this.parent;
txObject._ethAccounts = this.constructor._ethAccounts || this._ethAccounts;

if(this.deployData) {
txObject._deployData = this.deployData;
Expand Down Expand Up @@ -756,8 +758,8 @@ Contract.prototype._processExecuteArguments = function _processExecuteArguments(
Contract.prototype._executeMethod = function _executeMethod(){
var _this = this,
args = this._parent._processExecuteArguments.call(this, Array.prototype.slice.call(arguments), defer),
defer = promiEvent((args.type !== 'send'));

defer = promiEvent((args.type !== 'send')),
ethAccounts = _this.constructor._ethAccounts || _this._ethAccounts;

// simple return request for batch requests
if(args.generateRequest) {
Expand Down Expand Up @@ -788,7 +790,7 @@ Contract.prototype._executeMethod = function _executeMethod(){
inputFormatter: [formatters.inputCallFormatter],
outputFormatter: utils.hexToNumber,
requestManager: _this._parent._requestManager,
accounts: Contract._ethAccounts, // is eth.accounts (necessary for wallet signing)
accounts: ethAccounts, // is eth.accounts (necessary for wallet signing)
defaultAccount: _this._parent.defaultAccount,
defaultBlock: _this._parent.defaultBlock
})).createFunction();
Expand All @@ -809,7 +811,7 @@ Contract.prototype._executeMethod = function _executeMethod(){
return _this._parent._decodeMethodReturn(_this._method.outputs, result);
},
requestManager: _this._parent._requestManager,
accounts: Contract._ethAccounts, // is eth.accounts (necessary for wallet signing)
accounts: ethAccounts, // is eth.accounts (necessary for wallet signing)
defaultAccount: _this._parent.defaultAccount,
defaultBlock: _this._parent.defaultBlock
})).createFunction();
Expand Down Expand Up @@ -879,7 +881,7 @@ Contract.prototype._executeMethod = function _executeMethod(){
params: 1,
inputFormatter: [formatters.inputTransactionFormatter],
requestManager: _this._parent._requestManager,
accounts: Contract._ethAccounts, // is eth.accounts (necessary for wallet signing)
accounts: _this.constructor._ethAccounts || _this._ethAccounts, // is eth.accounts (necessary for wallet signing)
defaultAccount: _this._parent.defaultAccount,
defaultBlock: _this._parent.defaultBlock,
extraFormatters: extraFormatters
Expand Down
14 changes: 13 additions & 1 deletion packages/web3-eth/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@benjamincburns benjamincburns Nov 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

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 be great if we can avoid using util here and use something natively existing in ES5 already


var getNetworkType = require('./getNetworkType.js');
var formatter = helpers.formatters;

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

var ContractWrapper = function ContractWrapper() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@benjamincburns benjamincburns Nov 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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)

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, 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please change to Contract

Contract.apply(this, arguments);
};

ContractWrapper.setProvider = function() {
Contract.setProvider.apply(this, arguments);
};

util.inherits(ContractWrapper, Contract);
Copy link
Contributor Author

@benjamincburns benjamincburns Nov 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


// add contract
this.Contract = Contract;
this.Contract = ContractWrapper;
this.Contract.defaultAccount = this.defaultAccount;
this.Contract.defaultBlock = this.defaultBlock;
this.Contract.setProvider(this.currentProvider, this.accounts);
Expand Down
Loading