DigixGlobal / governance-ui-components

Governance UI reusable components library
Other
3 stars 2 forks source link

[FEATURE] Smart Gas Price #381

Closed eswarasai closed 5 years ago

eswarasai commented 5 years ago

Ref: https://github.com/DigixGlobal/governance-ui/issues/44 https://github.com/DigixGlobal/governance-ui/pull/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

mikej-digix commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

ohh. right. apologies. looking into different row. :+1:

mikej-digix commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@eswarasai thanks. confirmed fixed the above issue raised.

mikej-digix commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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