Skip to content
This repository was archived by the owner on Mar 5, 2025. It is now read-only.

fixes defaultBlock property handling#3247

Merged
nivida merged 7 commits into1.xfrom
issue/3244
Nov 29, 2019
Merged

fixes defaultBlock property handling#3247
nivida merged 7 commits into1.xfrom
issue/3244

Conversation

@nivida
Copy link
Contributor

@nivida nivida commented Nov 28, 2019

Description

Fixes #3244

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success and extended the tests if necessary.
  • I ran npm run build-all and tested the resulting file/'s from dist folder in a browser.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have tested my code on the live network.

@nivida nivida added Bug Addressing a bug 1.x 1.0 related issues labels Nov 28, 2019
@nivida nivida marked this pull request as ready for review November 28, 2019 09:17
@nivida nivida requested a review from cgewecke November 28, 2019 09:17
@coveralls
Copy link

coveralls commented Nov 28, 2019

Coverage Status

Coverage remained the same at 84.453% when pulling ddf979f on issue/3244 into 02b695a on 1.x.

@nivida nivida added the Review Needed Maintainer(s) need to review label Nov 28, 2019
Copy link
Collaborator

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

LGTM. Only thing that might be helpful is to add a regression test for this that uses the reproduction case in #3244.

// Regression test for #3244, should not throw.
it('formats the default block correctly', async function(){
    const accounts = await web3.eth.getAccounts();
    const latestBlock = await web3.eth.getBlockNumber();
    web3.eth.defaultBlock = latestBlock - 1
    await web3.eth.getBalance(accounts[0])
})

@nivida
Copy link
Contributor Author

nivida commented Nov 29, 2019

Only thing that might be helpful is to add a regression test for this that uses the reproduction case in #3244.

I've added an additional test case to the eth.getBalance unit test which does cover the mentioned defaultBlock case. (number -> hex)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

1.x 1.0 related issues Bug Addressing a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

eth.defaultBlock must be set to hex of number not regular number

3 participants