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

Support unit parsing in all relevant configs (e.g "10 ether", "200 wei", etc..) #816

Open
iurimatias opened this Issue Sep 12, 2018 · 22 comments

Comments

9 participants
@iurimatias
Member

iurimatias commented Sep 12, 2018

Outline

A lot of configurations in embark need to define an amount in wei. The goal of this task is to support specifying these values as a nice string of the type "<number> <unit>" for e.g "200 finney".
The code for this is already available in embark in the utils class here. The goal is to support this feature in all the required configs.

Acceptance Criteria

  • support "<number> <unit>" config
    ** unit can be: wei, kwei, Kwei, babbage, femtoether, mwei, Mwei, lovelace, picoether, gwei, Gwei, shannon, nanoether, nano, szabo, microether, micro, finney, milliether, milli, ether, kether, grand, mether, gether, tether
    ** in the following fields:
    *** config/blockchain.js : gasPrice
    *** config/contracts.js: gasPrice
    *** any other relevant fields
  • if the config value is a) not a string nor b) the unit is not specified nor c) is a string starting with '0x' then the value should be assumed to be in wei

@iurimatias iurimatias added this to bounty-awaiting-approval in Bounties Sep 12, 2018

@andytudhope andytudhope moved this from bounty-awaiting-approval to bounty (open) in Bounties Sep 12, 2018

@gitcoinbot

This comment has been minimized.

Show comment
Hide comment
@gitcoinbot

gitcoinbot Sep 12, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 60.0 DAI (60.0 USD @ $1.0/DAI) attached to it.

gitcoinbot commented Sep 12, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 60.0 DAI (60.0 USD @ $1.0/DAI) attached to it.

@gitcoinbot

This comment has been minimized.

Show comment
Hide comment
@gitcoinbot

gitcoinbot Sep 12, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work has been started.

These users each claimed they can complete the work by 6 days, 1 hour ago.
Please review their action plans below:

1) Powdurrrr has applied to start work (Funders only: approve worker | reject worker).

Parse the data and compare to static values. Make the wei conversion accordingly, may be best to use a map with a multiplier.

Learn more on the Gitcoin Issue Details page.

2) danlipert has been approved to start work.

I'll be applying the utility function (getWeiBalanceFromString) that converts various Ether values in any standard denomination into wei during the loading of configuration files for contracts, project, blockchain etc. Currently, the configuration files only support wei values, and these need to be converted into wei on the fly so that the configuration files are a bit easier to manage for developers setting up their embark projects.

Learn more on the Gitcoin Issue Details page.

gitcoinbot commented Sep 12, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work has been started.

These users each claimed they can complete the work by 6 days, 1 hour ago.
Please review their action plans below:

1) Powdurrrr has applied to start work (Funders only: approve worker | reject worker).

Parse the data and compare to static values. Make the wei conversion accordingly, may be best to use a map with a multiplier.

Learn more on the Gitcoin Issue Details page.

2) danlipert has been approved to start work.

I'll be applying the utility function (getWeiBalanceFromString) that converts various Ether values in any standard denomination into wei during the loading of configuration files for contracts, project, blockchain etc. Currently, the configuration files only support wei values, and these need to be converted into wei on the fly so that the configuration files are a bit easier to manage for developers setting up their embark projects.

Learn more on the Gitcoin Issue Details page.

@StatusSceptre

This comment has been minimized.

Show comment
Hide comment
@StatusSceptre

StatusSceptre Sep 12, 2018

@kpulkit29 please note that the method to do the conversion is already there

StatusSceptre commented Sep 12, 2018

@kpulkit29 please note that the method to do the conversion is already there

@iurimatias iurimatias moved this from bounty (open) to bounty (in progress) in Bounties Sep 12, 2018

@kpulkit29

This comment has been minimized.

Show comment
Hide comment
@kpulkit29

kpulkit29 Sep 13, 2018

I have gone through the code in util.js can U please give a brief of what file needs to be changed

kpulkit29 commented Sep 13, 2018

I have gone through the code in util.js can U please give a brief of what file needs to be changed

@kpulkit29

This comment has been minimized.

Show comment
Hide comment
@kpulkit29

kpulkit29 Sep 14, 2018

I have seen the method Please explain which file needs to be modified???

kpulkit29 commented Sep 14, 2018

I have seen the method Please explain which file needs to be modified???

@jrainville

This comment has been minimized.

Show comment
Hide comment
@jrainville

jrainville Sep 14, 2018

Member

The changes need to mostly be in core/config.js, but the goal of the bounty is also for you to find if there would be other places that we forgot to specify that would require the conversion too.

Member

jrainville commented Sep 14, 2018

The changes need to mostly be in core/config.js, but the goal of the bounty is also for you to find if there would be other places that we forgot to specify that would require the conversion too.

@StatusSceptre

This comment has been minimized.

Show comment
Hide comment
@StatusSceptre

StatusSceptre Sep 19, 2018

Hi @kpulkit29 is work still being done on this issue from your end? Thanks!

StatusSceptre commented Sep 19, 2018

Hi @kpulkit29 is work still being done on this issue from your end? Thanks!

@gitcoinbot

This comment has been minimized.

Show comment
Hide comment
@gitcoinbot

gitcoinbot Sep 19, 2018

@kpulkit29 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented Sep 19, 2018

@kpulkit29 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@kpulkit29

This comment has been minimized.

Show comment
Hide comment
@kpulkit29

kpulkit29 Sep 19, 2018

Sorry for delay I am having a hard time understanding what exactly needs to be changed

kpulkit29 commented Sep 19, 2018

Sorry for delay I am having a hard time understanding what exactly needs to be changed

@PascalPrecht

This comment has been minimized.

Show comment
Hide comment
@PascalPrecht

PascalPrecht Sep 20, 2018

Member

@kpulkit29 It is indeed a bit hard to follow if you aren't familiar with the codebase (neither am I). However, I did some digging and I think what needs to be done is this:

  • @iurimatias mentioned that a lot of gas related configurations in embark need to be defined in Wei. AFAICT, he's referring to configuration options like gas, gasLimit and targetGasLimit. Notice that those options are used in contracts.json, genesis.json and blockchain.json respectively. Every embark project has these files.

  • Now, you can see that all of these options define their value in different formats, sometimes it's a string ("auto"), sometimes it's a hex string ("0x...") and sometimes it can be a number (800000). All of these values are just different representations. A value can be represented as hex code, binary or decimal number.

  • Specifying values in Wei can be quite cumbersome, considering how huge the numbers can get (1 Ether = 1,000,000,000,000,000,000 Wei (10^18)), that's why programs often us hex codes to make values easier to read, parse and digest. For humans, it'd be even better if one could say "1 ether" instead of 1000000000000000000, or use any other unit that makes sense (Gwei, shannon, finney etc.)

  • It turns out that embark already comes with utility functions to take such a human readable string representation and turn that into the Wei format that embark needs to do its work. One of those functions is getWeiBalanceFromString()

  • So what needs to be done is find the places where those configuration files are being loaded (e.g. here and here) and pipe their gas, gasLimit and targetGasLimit values through the utility function before they get merge into the overall configuration object, which essentially results in users being able to specify their values in human readable formats :)

To be fair, I couldn't find a configuration option for gasPrice in the codebase that is configurable for users/consumers, but I'm sure @iurimatias can shed some light into darkness here.

I hope this makes sense and @iurimatias and @jrainville please correct me here if anything of this is wrong!

Does this help @kpulkit29 ?

Member

PascalPrecht commented Sep 20, 2018

@kpulkit29 It is indeed a bit hard to follow if you aren't familiar with the codebase (neither am I). However, I did some digging and I think what needs to be done is this:

  • @iurimatias mentioned that a lot of gas related configurations in embark need to be defined in Wei. AFAICT, he's referring to configuration options like gas, gasLimit and targetGasLimit. Notice that those options are used in contracts.json, genesis.json and blockchain.json respectively. Every embark project has these files.

  • Now, you can see that all of these options define their value in different formats, sometimes it's a string ("auto"), sometimes it's a hex string ("0x...") and sometimes it can be a number (800000). All of these values are just different representations. A value can be represented as hex code, binary or decimal number.

  • Specifying values in Wei can be quite cumbersome, considering how huge the numbers can get (1 Ether = 1,000,000,000,000,000,000 Wei (10^18)), that's why programs often us hex codes to make values easier to read, parse and digest. For humans, it'd be even better if one could say "1 ether" instead of 1000000000000000000, or use any other unit that makes sense (Gwei, shannon, finney etc.)

  • It turns out that embark already comes with utility functions to take such a human readable string representation and turn that into the Wei format that embark needs to do its work. One of those functions is getWeiBalanceFromString()

  • So what needs to be done is find the places where those configuration files are being loaded (e.g. here and here) and pipe their gas, gasLimit and targetGasLimit values through the utility function before they get merge into the overall configuration object, which essentially results in users being able to specify their values in human readable formats :)

To be fair, I couldn't find a configuration option for gasPrice in the codebase that is configurable for users/consumers, but I'm sure @iurimatias can shed some light into darkness here.

I hope this makes sense and @iurimatias and @jrainville please correct me here if anything of this is wrong!

Does this help @kpulkit29 ?

@Destiner

This comment has been minimized.

Show comment
Hide comment
@Destiner

Destiner Sep 24, 2018

As far as I understand, the first step is to find which config values can represent Ether values.
I did a quick research on what that values might be, and here's what I found:

  • There are 6 config files: blockchain.js, communication.js, contracts.js, namesystem.js, storage.js, and webserver.js.
  • We need to check every field that might be set as Ether in config files to make sure we didn't miss any.
  • For config/communation.js and config/storage.js, there are Communation and Storage docs pages, from which we can see that there is no such fields.
  • For config/blockchain.js and config/contracts.js, there are at least gasPrice (as noted in the issue) and account.balance (found in template examples) fields, but there might be others. Couldn't find a full list of possible config parameters.
  • For namesystem.js and webserver.js, there are neither docs, nor any relevant fields in templates/, and I guess these two can't contain such fields, as they are not related to Ethereum itself.

I also found that for account.balance of config/blockchain.js, there is already an example of using getWeiBalanceFromString. I guess we can use as a reference for other fields.

Destiner commented Sep 24, 2018

As far as I understand, the first step is to find which config values can represent Ether values.
I did a quick research on what that values might be, and here's what I found:

  • There are 6 config files: blockchain.js, communication.js, contracts.js, namesystem.js, storage.js, and webserver.js.
  • We need to check every field that might be set as Ether in config files to make sure we didn't miss any.
  • For config/communation.js and config/storage.js, there are Communation and Storage docs pages, from which we can see that there is no such fields.
  • For config/blockchain.js and config/contracts.js, there are at least gasPrice (as noted in the issue) and account.balance (found in template examples) fields, but there might be others. Couldn't find a full list of possible config parameters.
  • For namesystem.js and webserver.js, there are neither docs, nor any relevant fields in templates/, and I guess these two can't contain such fields, as they are not related to Ethereum itself.

I also found that for account.balance of config/blockchain.js, there is already an example of using getWeiBalanceFromString. I guess we can use as a reference for other fields.

@vs77bb

This comment has been minimized.

Show comment
Hide comment
@vs77bb

vs77bb Sep 24, 2018

@StatusSceptre Looks like @Destiner has provided a good approach here; mind approving on Gitcoin to allow him to 'Start Work'?

vs77bb commented Sep 24, 2018

@StatusSceptre Looks like @Destiner has provided a good approach here; mind approving on Gitcoin to allow him to 'Start Work'?

@danlipert

This comment has been minimized.

Show comment
Hide comment
@danlipert

danlipert Sep 25, 2018

Contributor

@iurimatias I've taken some time today to work on this issue and altered the existing tests to test the unit conversion - let me know if you think this type of test change should stay in: danlipert@33b8b00 Thanks!

Contributor

danlipert commented Sep 25, 2018

@iurimatias I've taken some time today to work on this issue and altered the existing tests to test the unit conversion - let me know if you think this type of test change should stay in: danlipert@33b8b00 Thanks!

@PascalPrecht

This comment has been minimized.

Show comment
Hide comment
@PascalPrecht

PascalPrecht Sep 25, 2018

Member

@danlipert would you mind sending a PR so we can review and comment right there? :)

Member

PascalPrecht commented Sep 25, 2018

@danlipert would you mind sending a PR so we can review and comment right there? :)

@danlipert

This comment has been minimized.

Show comment
Hide comment
@danlipert

danlipert Sep 26, 2018

Contributor

@PascalPrecht here it is: #910 still a WIP but wanted to see if I could get some feedback on the test change before proceeding.

Contributor

danlipert commented Sep 26, 2018

@PascalPrecht here it is: #910 still a WIP but wanted to see if I could get some feedback on the test change before proceeding.

@gitcoinbot

This comment has been minimized.

Show comment
Hide comment
@gitcoinbot

gitcoinbot Sep 30, 2018

@danlipert Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented Sep 30, 2018

@danlipert Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@PascalPrecht PascalPrecht added the bounty label Oct 1, 2018

@danlipert

This comment has been minimized.

Show comment
Hide comment
@danlipert

danlipert Oct 1, 2018

Contributor

@danlipert Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

Yes - sorry for the delay! We got hit by a typhoon here this weekend but I'll have the updated PR soon

Contributor

danlipert commented Oct 1, 2018

@danlipert Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

Yes - sorry for the delay! We got hit by a typhoon here this weekend but I'll have the updated PR soon

@gitcoinbot

This comment has been minimized.

Show comment
Hide comment
@gitcoinbot

gitcoinbot Oct 4, 2018

@danlipert Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented Oct 4, 2018

@danlipert Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@danlipert

This comment has been minimized.

Show comment
Hide comment
@danlipert

danlipert Oct 5, 2018

Contributor

@danlipert Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

Yup! Will have this wrapped up soon, was just waiting for some feedback on moving the final wei conversion into the config file. Looks like nobody has any issues with that so I'll make that change and get this squashed and rebased 👍

Contributor

danlipert commented Oct 5, 2018

@danlipert Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

Yup! Will have this wrapped up soon, was just waiting for some feedback on moving the final wei conversion into the config file. Looks like nobody has any issues with that so I'll make that change and get this squashed and rebased 👍

@iurimatias iurimatias moved this from bounty (in progress) to bounty (changes requested) in Bounties Oct 5, 2018

@gitcoinbot

This comment has been minimized.

Show comment
Hide comment
@gitcoinbot

gitcoinbot Oct 9, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work for 60.0 DAI (60.0 USD @ $1.0/DAI) has been submitted by:

  1. @danlipert

@StatusSceptre please take a look at the submitted work:


gitcoinbot commented Oct 9, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work for 60.0 DAI (60.0 USD @ $1.0/DAI) has been submitted by:

  1. @danlipert

@StatusSceptre please take a look at the submitted work:


@gitcoinbot

This comment has been minimized.

Show comment
Hide comment
@gitcoinbot

gitcoinbot Oct 9, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work for 60.0 DAI (60.0 USD @ $1.0/DAI) has been submitted by:

  1. @danlipert

@StatusSceptre please take a look at the submitted work:


gitcoinbot commented Oct 9, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work for 60.0 DAI (60.0 USD @ $1.0/DAI) has been submitted by:

  1. @danlipert

@StatusSceptre please take a look at the submitted work:


@iurimatias iurimatias moved this from bounty (changes requested) to bounty (merged/work approved) in Bounties Oct 18, 2018

@gitcoinbot

This comment has been minimized.

Show comment
Hide comment
@gitcoinbot

gitcoinbot Oct 18, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


The funding of 60.0 DAI (60.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @danlipert.

gitcoinbot commented Oct 18, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


The funding of 60.0 DAI (60.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @danlipert.

@StatusSceptre StatusSceptre moved this from bounty (merged/work approved) to bounty (completed) in Bounties Oct 18, 2018

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