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

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

Closed
iurimatias opened this issue Sep 12, 2018 · 23 comments

Comments

@iurimatias
Copy link
Collaborator

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
Copy link

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
Copy link

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
Copy link

@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
Copy link

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

@kpulkit29
Copy link

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

@jrainville
Copy link
Collaborator

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
Copy link

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

@gitcoinbot
Copy link

@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
Copy link

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

@0x-r4bbit
Copy link
Contributor

@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
Copy link

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
Copy link

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
Copy link
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!

@0x-r4bbit
Copy link
Contributor

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

@danlipert
Copy link
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.

@gitcoinbot
Copy link

@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
Copy link
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

@gitcoinbot
Copy link

@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
Copy link
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 👍

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

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
Copy link

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
Copy link

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
@0x-r4bbit
Copy link
Contributor

This has landed as 594d132

@0x-r4bbit 0x-r4bbit added this to Done in Embark Nov 14, 2018
@iurimatias iurimatias removed this from bounty (completed) in Bounties Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Embark
  
Done
Development

No branches or pull requests

9 participants