Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

On calls, pull defaultBlock from web3 #2964

Merged
merged 3 commits into from
Apr 21, 2020
Merged

On calls, pull defaultBlock from web3 #2964

merged 3 commits into from
Apr 21, 2020

Conversation

wbt
Copy link
Contributor

@wbt wbt commented Apr 15, 2020

Issue:

Function calls using @truffle/contract (.call()) act as if web3.eth.defaultBlock == 'latest', even when it isn't.

The workaround is to add a parameter at the end of the parameter list for every call() which is web3.eth.defaultBlock.

This is not intuitive, not consistent with a prior major version, and the change does not appear easy to find in documentation. It's also not clear that this is the best choice for behavior.

I think the behaviour is due to the line: let defaultBlock = "latest";

That was added in this commit by @cgewecke back on Jun 10, 2018, but I can't really find why; I don't see any associated PR/discussion.

This PR proposes changing that line to let defaultBlock = this.web3.eth.defaultBlock; but hasn't been tested. The intended result includes:

  • Backwards compatibility: if you are using the workaround that you currently have to use, or if you are leaving the defaultBlock as 'latest' all the time, no behavior will change and no code changes are required.
  • If someone is changing web3.eth.defaultBlock, calls should respect that.

instead of always using 'latest.' See PR for detail.
Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

I think the build is failing because of this inside function() {}, but that's easily corrected. I also suggested a bit of defensive coding with the || "latest", just to be on the safe side.

Apart from that, I think this change makes sense and shouldn't cause problems! Will approve once tests pass. Thanks for this!

packages/contract/lib/execute.js Outdated Show resolved Hide resolved
@gnidan
Copy link
Contributor

gnidan commented Apr 15, 2020

@eggplantzzz if you have a second, could you get your eyes on this too? Would just like another opinion!

@gnidan gnidan requested a review from eggplantzzz April 15, 2020 23:27
in case defaultBlock is undefined, per suggestion by @gnidan, though seems likely unnecessary
@wbt
Copy link
Contributor Author

wbt commented Apr 16, 2020

Note that it's not 100% clear to me, having not yet fully tested this, whether changes to the global web3.eth.defaultBlock are reflected in the web3 referenced in the "this"/constructor object. If not, I think the edit would probably do no harm, but also not accomplish the intended goal.

@gnidan
Copy link
Contributor

gnidan commented Apr 16, 2020

whether changes to the global web3.eth.defaultBlock are reflected in the web3

My guess is that almost certainly not, since @truffle/contract constructs its own web3

@wbt
Copy link
Contributor Author

wbt commented Apr 16, 2020

In that case, this should probably be considered an Issue report which had a specific suggested fix but where that suggestion was likely ultimately insufficient by itself.

Copy link
Contributor

@eggplantzzz eggplantzzz left a comment

Choose a reason for hiding this comment

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

This looks like it should be fine to add to me

@eggplantzzz eggplantzzz merged commit 1f1fd56 into trufflesuite:develop Apr 21, 2020
@wbt wbt deleted the patch-2 branch April 22, 2020 13:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants