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

Jump to bottom

[web3] Updated web3.utils.toWei() Unit list and added default Unit value #27528

Merged
4 commits merged into from
Jul 30, 2018
Merged

[web3] Updated web3.utils.toWei() Unit list and added default Unit value #27528

4 commits merged into from
Jul 30, 2018

Conversation

zlumer
Copy link
Contributor

@zlumer zlumer commented Jul 24, 2018

This is a duplicate of #27137 that was unfortunately closed by bot.

I've added one more commit to better match official web3 docs.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

@typescript-bot
Copy link
Contributor

typescript-bot commented Jul 24, 2018 •
edited
Loading

@zlumer Thank you for submitting this PR!

🔔 @simon-jentzsch @nitzantomer @zurbo @yxliang01 @phra @naddison36 @icaroharry @linusnorton @jpeletier @anneau @matrushka @andrevmatos @levino - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@levino
Copy link
Contributor

levino commented Jul 24, 2018

Good stuff. Will review tomorrow.

levino
levino suggested changes Jul 26, 2018
View reviewed changes
Copy link
Contributor

@levino levino 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 "type check of the returned value" can be done more elegantly. See remark.

types/web3/index.d.ts
@@ -13,6 +13,7 @@
// Baris Gumustas <https://github.com/matrushka>
// André Vitor de Lima Matos <https://github.com/andrevmatos>
// Levin Keller <https://github.com/levino>
// Dmitry Radkovskiy <https://github.com/zlumer>
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair.

types/web3/utils.d.ts
| "finney"
| "milliether"
Copy link
Contributor

Choose a reason for hiding this comment

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

For reference https://web3js.readthedocs.io/en/1.0/web3-utils.html?highlight=toWei#id76
List goes not alphabetically but by the number of trailing zeros.

types/web3/utils.d.ts
@@ -66,6 +71,7 @@ export default interface Utils {
toDecimal(val: any): number;
toHex(val: any): string;
toUtf8(val: any): string;
toWei(val: string | number | BigNumber, unit: Unit): string | BigNumber;
toWei(val: string | number, unit?: Unit): string;
toWei(val: BigNumber, unit?: Unit): BigNumber;
Copy link
Contributor

Choose a reason for hiding this comment

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

Reference for overloading: https://web3js.readthedocs.io/en/1.0/web3-utils.html?highlight=toWei#id77

types/web3/web3-tests.ts Outdated

const weiStr = web3.utils.toWei("100", "gwei");
weiStr.endsWith(weiStr);
const weiBn = web3.utils.toWei(web3.utils.toBN("1"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest

const weiBn: BigNumber = web3.utils.toWei(web3.utils.toBN("1"));

instead of line 22-23. What do you say?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's awesome, I don't even know why I didn't do it this way in the first place. Great idea.

@typescript-bot typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed Awaiting reviewer feedback labels Jul 26, 2018
@typescript-bot
Copy link
Contributor

@zlumer One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Jul 26, 2018
@levino
Copy link
Contributor

levino commented Jul 30, 2018

@zlumer Do you wanna change it?

@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Jul 30, 2018
@zlumer
Copy link
Contributor Author

zlumer commented Jul 30, 2018

@levino updated. Let me know if there are more possible improvements.

@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Jul 30, 2018
@typescript-bot
Copy link
Contributor

🔔 @levino - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

levino
levino approved these changes Jul 30, 2018
View reviewed changes
Copy link
Contributor

@levino levino left a comment

Choose a reason for hiding this comment

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

I like it.

@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express labels Jul 30, 2018
@typescript-bot
Copy link
Contributor

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@ghost ghost merged commit e4902dd into DefinitelyTyped:master Jul 30, 2018
briandk pushed a commit to briandk/DefinitelyTyped that referenced this pull request Aug 7, 2018
@zlumer @briandk
[web3] Updated web3.utils.toWei() Unit list and added default Unit …
2683f10 
…value (DefinitelyTyped#27528)

* Updated `web3.utils.toWei()` Unit list and added default Unit value

* edit tests for tslint

* web3: reordered ether Unit list to match official docs

* web3 tests improvement
This was referenced Jan 26, 2019
Closed
Closed
@joshstevens19 joshstevens19 mentioned this pull request Jan 26, 2019
Merged
14 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@levino levino levino approved these changes

Assignees
No one assigned
Labels
Owner Approved A listed owner of this package signed off on the pull request. Revision needed This PR needs code changes before it can be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zlumer @typescript-bot @levino