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.

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

Add function compareBlockNumbers(a, b) #3540

Closed
wants to merge 4 commits into from
Closed

Add function compareBlockNumbers(a, b) #3540

wants to merge 4 commits into from

Conversation

wbt
Copy link
Contributor

@wbt wbt commented May 25, 2020

Description

Sometimes it is useful to be able to compare block numbers, which is slightly more complicated than usual because of the option for predefined string block numbers.
This PR offers a starting point for a new function which can help with that.
I located it near other functions that seem logically related.

Collaborative development is invited to address any edge case/logical errors missed in the first draft, the following shortcomings, and unchecked boxes below:

  • Tests should be included.
  • BigNumber parameters should probably be considered.

Until such time as this is merged in with web3, the code may be used in a repository that uses web3 under the same license.

Type of change

  • New feature (non-breaking change which adds functionality)

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.
  • I ran npm run test:cov and my test cases cover all the lines and branches of the added code.
  • I ran npm run build-all and tested the resulting files 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.

@coveralls
Copy link

coveralls commented May 25, 2020

Coverage Status

Coverage decreased (-2.4%) to 82.985% when pulling 5c885af on wbt:patch-2 into 0151821 on ethereum:1.x.

@cgewecke
Copy link
Collaborator

This is cool! Can see how it would be really useful. Thanks @wbt.

@ryanio ryanio added 1.x 1.0 related issues Enhancement Includes improvements or optimizations labels Jun 4, 2020
@GregTheGreek
Copy link
Contributor

@wbt any plans to further this?

@wbt
Copy link
Contributor Author

wbt commented Jul 26, 2020

@GregTheGreek I think it would be good for someone else more familiar with the codebase and build process to pick it up from here. (For example, I don't know how to unjam the required stuck tests.) I think this is a useful contribution toward a collaborative development effort, but I don't have the bandwidth to do it all myself at present. Would you like to pick it up?

@GregTheGreek GregTheGreek self-assigned this Aug 11, 2020
@GregTheGreek GregTheGreek mentioned this pull request Aug 11, 2020
14 tasks
@GregTheGreek
Copy link
Contributor

This has been superseded by #3682

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Enhancement Includes improvements or optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants