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

[FEATURE] Smart Gas Price #381

Merged
merged 5 commits into from
Sep 30, 2019
Merged

[FEATURE] Smart Gas Price #381

merged 5 commits into from
Sep 30, 2019

Conversation

eswarasai
Copy link
Contributor

@eswarasai eswarasai commented Sep 12, 2019

Ref: DigixGlobal/governance-ui#44 DigixGlobal/governance-ui#46

Description

Added a Smart Gas Price component with Advanced options mode to be able to switch fast between the amount of gas price user would like to pay based on transaction times whose values are being fetched directly from ETHGasStation API.

Current behaviour just had a slider for gas price without any advanced options to quickly select it.

Test Plan

  • Once you have dev setup up and running, open the app and login using Import JSON option
  • After logging in, click on Lock DGD button on the Header and enter the amount you'd like to lock
  • In the transaction details and signing screen, you should now see options to select the gas price similar to the design mentioned in the issue description

Copy link
Contributor

@mandres-digix mandres-digix left a comment

Choose a reason for hiding this comment

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

Hey @eswarasai, thanks for taking this on! I was able to run this on my machine, and the design looks good! 👍

To help you along, I commented on the issue and described the general flow of gasPrice and how it moves from the governance-ui-components code to the governance-ui code, so you can transfer the design from the Lock DGD overlay to the transaction modal. Please have a look at the comment first.

This would be my advice for working on the task, although if you find a better way of doing things, feel free to do so!

In general, you will be working on the TransactionSigningOverlay component from governance-ui. You will want to replace the contents of the renderAdvancedTab method, which renders the Advanced tab and the slider for changing the gas price. You can replace its contents with the design you just did for the gas price options in this PR, and then call setGasPrice when a user selects the gas price.

You can also rename it to something more descriptive (renderGasPriceOptions or something similar). Even better, you can probably create a separate component for this and just import it to TransactionSigningOverlay.

In governance-ui-components (this repo), maybe we can set DEFAULT_GAS_PRICE in src/constants.js to "FAST" and use that value in TransactionSigningOverlay to pre-select the default gas price option for the user.

Hopefully this makes the code flow clearer and help you out with your changes. Just comment and tag me here on github if you have any questions regarding this task!

P.S. we just updated the PR template for this repo. If you could, kindly update the PR description using the template in .github/PULL_REQUEST_TEMPLATE.md. We will be updating the template for governance-ui as well, so please look out for it!

src/components/common/blocks/gas-price/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mandres-digix mandres-digix left a comment

Choose a reason for hiding this comment

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

Hey @eswarasai. Since we're keeping the component here and just importing it to governance-ui, I just have some quick comments on the code here.

src/components/common/blocks/gas-price/index.js Outdated Show resolved Hide resolved
src/components/common/blocks/gas-price/style.js Outdated Show resolved Hide resolved
src/components/common/elements/slider/style.css Outdated Show resolved Hide resolved
Copy link
Contributor

@mandres-digix mandres-digix left a comment

Choose a reason for hiding this comment

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

Please update your PR description with a link to the governance-ui PR (when it's done) and provide a test plan for manual QA.

Use this as reference: https://github.com/DigixGlobal/governance-ui-components/edit/develop/.github/PULL_REQUEST_TEMPLATE.md

@eswarasai eswarasai changed the title [feature] Smart Gas Price [FEATURE] Smart Gas Price Sep 18, 2019
Copy link
Contributor

@mandres-digix mandres-digix left a comment

Choose a reason for hiding this comment

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

Was able to run it on my machine, but needed to do a quick fix for the TimeText component. Please add the fix to the PR, thanks!

Screenshot from 2019-09-19 16-12-08

src/components/common/blocks/gas-price/index.js Outdated Show resolved Hide resolved
@mikej-digix
Copy link
Contributor

mikej-digix commented Sep 19, 2019

@eswarasai @mandres-digix how we could handle the gas value on the details section? it doesn't fetch the selected gas option to the details section

image

Gas value on details section shows 1000000 but on the selected gas option, it is 25 GWEI

@eswarasai
Copy link
Contributor Author

eswarasai commented Sep 19, 2019

@mikej-digix - It's the gas price actually and you can see the value 250000000000000 in the first row of the table. I made sure to handle that behaviour 😅

Edit: That piece of code is actually in the governance-ui

@mikej-digix
Copy link
Contributor

ohh. right. apologies. looking into different row. 👍

@mikej-digix
Copy link
Contributor

mikej-digix commented Sep 22, 2019

hi @eswarasai , tried to test your PR but there is a console error showing up when using ledger/trezor wallet when signing the transaction. see attached image

image

image

@eswarasai
Copy link
Contributor Author

@mikej-digix - Just pushed a fix for this. I guess the behaviour is hiding the gas price selection component for other wallets which I've missed earlier. Thanks for letting me know 🙂

@mikej-digix
Copy link
Contributor

@eswarasai thanks. confirmed fixed the above issue raised.

@mikej-digix
Copy link
Contributor

@eswarasai can we also apply the changes for metamask wallet. Target for the metamask is not to show the metamask pop up until user click the Sign Transaction button on the modal similar to trezor/ledger implementation.

@eswarasai
Copy link
Contributor Author

@mikej-digix - I haven't changed the Sign Transaction behaviour for any of the wallets. I remember that we discussed about Metamask and we wouldn't need Smart Gas Price feature in this scenario as users can do that within Metamask.

Let me know if you'd like me to integrate the gas price options in case of Metamask as well. If not, then it doesn't make sense to display the transaction signing modal and having user to click on the Sign Tranasaction button unnecessarily.

cc: @mandres-digix @roynalnaruto

mikej-digix pushed a commit to DigixGlobal/governance-ui that referenced this pull request Sep 30, 2019
Ref: #44 DigixGlobal/governance-ui-components#381

Description
Added a Smart Gas Price component with Advanced options mode to be able to switch fast between the amount of gas price user would like to pay based on transaction times whose values are being fetched directly from ETHGasStation API.

Current behaviour just had a slider for gas price without any advanced options to quickly select it.

Test Plan
Before firing up the dev env, please make sure you also have the governance-ui-components PR is linked with this one during setup
Once you have dev setup up and running, open the app and login using Import JSON option
After logging in, click on Lock DGD button on the Header and enter the amount you'd like to lock
In the transaction details and signing screen, you should now see options to select the gas price similar to the design mentioned in the issue description
@mikej-digix
Copy link
Contributor

mikej-digix commented Sep 30, 2019

iteration: 1
status: PASSED
envrionment: LOCAL

Test Condition:
WHEN user clicks any action on proposal page ON locking_phase THEN user should be able to see the notice overlay
image

@mikej-digix mikej-digix merged commit 58eaef3 into DigixGlobal:develop Sep 30, 2019
mikej-digix pushed a commit to DigixGlobal/governance-ui that referenced this pull request Sep 30, 2019
Ref: #44 DigixGlobal/governance-ui-components#381

Description
Added a Smart Gas Price component with Advanced options mode to be able to switch fast between the amount of gas price user would like to pay based on transaction times whose values are being fetched directly from ETHGasStation API.

Current behaviour just had a slider for gas price without any advanced options to quickly select it.

Test Plan
Before firing up the dev env, please make sure you also have the governance-ui-components PR is linked with this one during setup
Once you have dev setup up and running, open the app and login using Import JSON option
After logging in, click on Lock DGD button on the Header and enter the amount you'd like to lock
In the transaction details and signing screen, you should now see options to select the gas price similar to the design mentioned in the issue description
mikej-digix pushed a commit that referenced this pull request Sep 30, 2019
    Ref: DigixGlobal/governance-ui#44 DigixGlobal/governance-ui#46

    Description
    Added a Smart Gas Price component with Advanced options mode to be able to switch fast between the amount of gas price user would like to pay based on transaction times whose values are being fetched directly from ETHGasStation API.

    Current behaviour just had a slider for gas price without any advanced options to quickly select it.

    Test Plan
    Once you have dev setup up and running, open the app and login using Import JSON option
    After logging in, click on Lock DGD button on the Header and enter the amount you'd like to lock
    In the transaction details and signing screen, you should now see options to select the gas price similar to the design mentioned in the issue description
mikej-digix pushed a commit that referenced this pull request Sep 30, 2019
    Ref: DigixGlobal/governance-ui#44 DigixGlobal/governance-ui#46

    Description
    Added a Smart Gas Price component with Advanced options mode to be able to switch fast between the amount of gas price user would like to pay based on transaction times whose values are being fetched directly from ETHGasStation API.

    Current behaviour just had a slider for gas price without any advanced options to quickly select it.

    Test Plan
    Once you have dev setup up and running, open the app and login using Import JSON option
    After logging in, click on Lock DGD button on the Header and enter the amount you'd like to lock
    In the transaction details and signing screen, you should now see options to select the gas price similar to the design mentioned in the issue description
mikej-digix pushed a commit to DigixGlobal/governance-ui that referenced this pull request Sep 30, 2019
Ref: #44 DigixGlobal/governance-ui-components#381

Description
Added a Smart Gas Price component with Advanced options mode to be able to switch fast between the amount of gas price user would like to pay based on transaction times whose values are being fetched directly from ETHGasStation API.

Current behaviour just had a slider for gas price without any advanced options to quickly select it.

Test Plan
Before firing up the dev env, please make sure you also have the governance-ui-components PR is linked with this one during setup
Once you have dev setup up and running, open the app and login using Import JSON option
After logging in, click on Lock DGD button on the Header and enter the amount you'd like to lock
In the transaction details and signing screen, you should now see options to select the gas price similar to the design mentioned in the issue description
mikej-digix pushed a commit that referenced this pull request Oct 8, 2019
    Ref: DigixGlobal/governance-ui#44 DigixGlobal/governance-ui#46

    Description
    Added a Smart Gas Price component with Advanced options mode to be able to switch fast between the amount of gas price user would like to pay based on transaction times whose values are being fetched directly from ETHGasStation API.

    Current behaviour just had a slider for gas price without any advanced options to quickly select it.

    Test Plan
    Once you have dev setup up and running, open the app and login using Import JSON option
    After logging in, click on Lock DGD button on the Header and enter the amount you'd like to lock
    In the transaction details and signing screen, you should now see options to select the gas price similar to the design mentioned in the issue description
mikej-digix pushed a commit to DigixGlobal/governance-ui that referenced this pull request Oct 8, 2019
Ref: #44 DigixGlobal/governance-ui-components#381

Description
Added a Smart Gas Price component with Advanced options mode to be able to switch fast between the amount of gas price user would like to pay based on transaction times whose values are being fetched directly from ETHGasStation API.

Current behaviour just had a slider for gas price without any advanced options to quickly select it.

Test Plan
Before firing up the dev env, please make sure you also have the governance-ui-components PR is linked with this one during setup
Once you have dev setup up and running, open the app and login using Import JSON option
After logging in, click on Lock DGD button on the Header and enter the amount you'd like to lock
In the transaction details and signing screen, you should now see options to select the gas price similar to the design mentioned in the issue description
mikej-digix pushed a commit that referenced this pull request Oct 8, 2019
    Ref: DigixGlobal/governance-ui#44 DigixGlobal/governance-ui#46

    Description
    Added a Smart Gas Price component with Advanced options mode to be able to switch fast between the amount of gas price user would like to pay based on transaction times whose values are being fetched directly from ETHGasStation API.

    Current behaviour just had a slider for gas price without any advanced options to quickly select it.

    Test Plan
    Once you have dev setup up and running, open the app and login using Import JSON option
    After logging in, click on Lock DGD button on the Header and enter the amount you'd like to lock
    In the transaction details and signing screen, you should now see options to select the gas price similar to the design mentioned in the issue description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants