-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Minimize changes to test/contract.js (#3190) #3303
Conversation
var eth = new Eth(provider); | ||
const contract = new eth.Contract(abi, address, options); | ||
//eth.setProvider(provider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call of setProvider
is not required because the constructor
of the Eth
module does set the provider already as expected. The tests fail because the added validations to the FakeIpcProvider
aren't in the correct order (I will check that closer). But we can say that the logic behind setProvider
is still correct or the test case on line 3112 would fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it seems possible everything is ok but it would be good to certain as well :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgewecke I've checked the issue we have with the additional call of eth.setProvider
closer. Because we pass the exact same provider again with setProvider
does it register the data
listener again and the eth_getTransactionReceipt
call will get executed more than required. We could implement the removal of all listeners on setProvider
to prevent this behaviour but this would probably break existing DApps which do register event listeners before setProvider
got called.
After all, if we add an explanatory note to the setProvider
method to inform the user about this behaviour can we ignore it. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - that sounds reasonable...I'm going to walk the code again as you describe just to satisfy my paranoia. The way the test set's itself up with with two provider settings doesn't seem very normal anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cgewecke Great! 👌
… if someone does passes the exact same provider instance again to the setProvider method of the current module
@cgewecke I've extended the note of the |
Description
PR targets #3190
(Part of ongoing #3190 review. Opening for further discussion.)
PR reverts all the changes to test/contract.js except the three necessary to get the tests passing.
Diff vs. 1.x looks like this:
Many tests fail unless the call to eth.setProvider is commented out in the test fixture. Why?
https://github.com/ethereum/web3.js/blob/5e1aec2f07c82d412123229bef8a865142a8668a/test/contract.js#L242-L244