Onboarding: add time remaining and progress ring #1210

e18r · 2019-12-09T16:31:27Z

Fixes AutarkLabs#29
Fixes AutarkLabs#30
Fixes AutarkLabs#31
Fixes #1064

Adds a loading state for transactions by subscribing to the transactionHash web3 event.
Consumes EthGasStation API to obtain an estimate of the confirmation delay for each transaction.
Shows the time remaining in hours, minutes or seconds when the transaction is broadcasted.
Created a new Progress Ring component to show the progress using the remaining time estimation.

Note: The estimaton times are only available for mainnet, so they are also used in other environments, creating very wrong estimates in these environments, as can be seen in the animated GIF above (it lasts like 8 minutes)

Progress ring inspired by http://firchow.net/css3-prozentanzeige-kreis/

ottodevs

Great job @e18r !!! The PR seems really promising!!

I just found some inconsistencies and I experienced several potential issues during my initial tests locally and using the automated now.sh deployment, could you please take a look to the following?

The template deployment fails on first transaction when using Fundraising or OpenEnterprise templates, on the other hand, it completes the single transaction for the rest of templates.
The gas estimation fallback is wrong locally, so metamask confirmation shows up with an empty Gas Limit field, this is maybe related to an small typo on the ethgasstation fetch line.
It does not show the "Organization Ready" banner after successfully approve the tx
It does not redirect to the new DAO as expected after that step as well
No animation on the progress ring (maybe related to the ethgasstation bug I mentioned)
The Info panel should be a Warning message; it is not implemented with the correct style (yellow bg) as Javier and Paty suggested in Animate signature progress and add time remaining AutarkLabs/aragon#29 I am missing some newer directions on that?
It does not show any kind of time estimation (again, maybe this is result of failing to fetch estimations because of wrong URL in the fetch request).

Please don't hesitate to ask if you need further clarification on any of the items 👍🍀

ottodevs · 2019-12-17T06:37:29Z

src/onboarding/Create/Create.js

    return deployTransactions.map(({ name }, index) => ({
      name,
      status: status(index),
+      dateStart: submittedTransactions[index]
+        ? submittedTransactions[index].dateStart
+        : undefined,
+      gasPrice: submittedTransactions[index]
+        ? submittedTransactions[index].gasPrice
+        : undefined,


This seems much more clear way to express the same logic, and seems a bit less cumbersome and readable, also notably shorter :

const result = deployTransactions.map(({name}, index) => { const { dateStart, gasPrice } = submittedTransactions[index] const status = status(index) return {dateStart, gasPrice, name, status} } return result

Thank you, I was struggling with that part.

I ended up doing something different:

let deployment = { name, status: status(index), } if (submittedTransactions[index]) { const { dateStart, gasPrice } = submittedTransactions[index] deployment = { ...deployment, dateStart, gasPrice } } return deployment

ottodevs · 2019-12-17T06:38:45Z

src/onboarding/Create/Create.js

-    const { signed, errored } = transactionProgress
+    const { submitted, signed, errored } = transactionProgress
    const status = index => {
      if (errored !== -1 && index >= errored) {
        return TRANSACTION_STATUS_ERROR
      }
-      if (index === signed) {
-        return TRANSACTION_STATUS_PENDING
-      }
-      if (index < signed) {
+      if (index <= signed) {
        return TRANSACTION_STATUS_SUCCESS
      }
+      if (index <= submitted) {
+        return TRANSACTION_STATUS_PENDING
+      }
      return TRANSACTION_STATUS_UPCOMING
    }


These conditions seem a bit convoluted and hard to follow/read, I suggest coming up with a more readable way to ease debugging or maintaining it in case of errors or future modifications

I thought about this a lot. The underlying problem is the way these indexes work, and that's something that was there before I arrived. I'm merely extending that functionality (and making it probably more complex along the way).

I thought about changing this to an object where the keys were the transaction IDs and the values were the status of each transaction. However, it seemed hard to make sure I wasn't breaking anything in the process, and that's why I backed off.

But I'm gonna think about this more. Any suggestions welcome.

I've decided to leave them like that for now. It really would require a level of refactoring that exceeds this PR.

ottodevs · 2019-12-17T06:39:43Z

src/onboarding/Create/Create.js

+                    ...prev,
+                    [i]: {
+                      dateStart: new Date(),
+                      gasPrice: submittedTransaction.gasPrice / 1000000000,


I suggest using 1e9 instead of 1000000000, for readability and even using a constant instead.

Or even better: fromWei

ottodevs · 2019-12-17T06:51:16Z

src/onboarding/Deployment/DeploymentStepsItem.js

+
+  useMemo(() => {
+    if (gasPrice === undefined) return
+    fetch('https:ethgasstation.info/json/predictTable.json')


It seems we are missing the slashes after https: protocol, causing errors in runtime, because the URL ends up being added to the deployment address:
https://aragon-g00c0o0hh.now.sh/ethgasstation.info/json/predictTable.json

Suggested change

fetch('https:ethgasstation.info/json/predictTable.json')

fetch('https://ethgasstation.info/json/predictTable.json')

😕 Thank you

ottodevs · 2019-12-17T06:54:41Z

src/onboarding/Deployment/DeploymentStepsItem.js

+  )
+
+  useMemo(() => {
+    if (gasPrice === undefined) return


We could add here an extra check as a fallback mechanism for rinkeby, localhost or anything different from mainnet, to set a different remaining time for those cases.

What would that be? I feel like the mainnet estimate is still the best prediction available, even for Rinkeby or local.

Well, I guess local network is not considered that important in the client, as it is for example tagged as "wrong network" when selected in metamask, so it is probably not worth the effort to reach accurate times there.

ottodevs · 2019-12-17T06:57:28Z

src/onboarding/Deployment/DeploymentStepsItem.js

+      const time = (dateEnd - now) / 1000
+      const unit = time < 60 ? 'sec' : time < 3600 ? 'min' : 'hr'
+      const amount = Math.floor(
+        unit === 'sec' ? time : unit === 'min' ? 1 + time / 60 : 1 + time / 3600
+      )


Are we sure this is really needed if we are using date-fns? Maybe you might want to use some of these methods

These methods still seem in development, but I'm gonna take a look. Thanks for the suggestion.

We may also want to take a look at dayjs to see if they have something similar. We're trying to move away from date-fns.

I haven't found any methods in DayJS that give us what we want. I'm leaving this like this for now.

It seems straightforward by enabling dayjs embedded plugin for that, we were talking about this the other day, but the change seems still pending

The result differs a bit from the design - and it looks OK, so I think it can work. However, now the transaction status line is prone to occupying two lines:

What say you, @javieralaves?

@e18r Green light on my end, this is fine

e18r · 2019-12-18T22:13:22Z

Hi @ottodevs, thank you so much for your review. Answering to your general remarks:

The template deployment fails on first transaction when using Fundraising or OpenEnterprise templates, on the other hand, it completes the single transaction for the rest of templates.

solved!

The gas estimation fallback is wrong locally, so metamask confirmation shows up with an empty Gas Limit field, this is maybe related to an small typo on the ethgasstation fetch line.

yes, it was that typo.

It does not show the "Organization Ready" banner after successfully approve the tx

solved!

It does not redirect to the new DAO as expected after that step as well

solved!

No animation on the progress ring (maybe related to the ethgasstation bug I mentioned)

yes

The Info panel should be a Warning message; it is not implemented with the correct style (yellow bg) as Javier and Paty suggested in AutarkLabs#29 I am missing some newer directions on that?

See #1211

It does not show any kind of time estimation (again, maybe this is result of failing to fetch estimations because of wrong URL in the fetch request).

yes

Please don't re-review yet; I still have to address your code-oriented remarks.

Fixes #29 * Adds a loading state for transactions by subscribing to the 'transactionHash' web3 event. * Consumes EthGasStation API to obtain an estimate of the confirmation delay for each transaction. * Shows the time remaining when the transaction is broadcasted * Created a new Progress Ring component to show the progress using the remaining time estimation **Note**: this PR was not tested for templates with multiple transactions.

@ottodevs

Tested with the Open Enterprise template and addressed comments by @ottodevs: * Transactions don't fail anymore * The DAO gets created at the end * Added a new state, "signig", which corresponds to the "waiting for signature" state * Used `fromWei(value, 'gwei')` instead of dividing by 1000000000 * Made more readable some parts of the code * Solved weird typo on the EthGasStation URL * Used `.attrs` in the HalfRing styled component to make it more performant

ottodevs

Thanks @e18r some minor comments, mostly conventions that may not apply to this repo, but good to know, it was not clear by your last comment if that was ready for re-review, please ping me when it is 👍 🥇

ottodevs · 2020-01-02T01:13:08Z

src/symbols.js

 export const TRANSACTION_STATUS_PENDING = Symbol('TRANSACTION_STATUS_PENDING')
 export const TRANSACTION_STATUS_SUCCESS = Symbol('TRANSACTION_STATUS_SUCCESS')
-export const TRANSACTION_STATUS_UPCOMING = Symbol('TRANSACTION_STATUS_UPCOMING')
+export const TRANSACTION_STATUS_ERROR = Symbol('TRANSACTION_STATUS_ERROR')


These symbols were alphabetically sorted, is there a good reason to change that sort?

I think it makes sense to sort these symbols according to the order in which they occur in the transaction lifecycle. This helps understand what each of these symbols stands for:

Upcoming - Waiting for a previous transaction to finish

Signing - Waiting for the user to sign the transaction through the Web3 provider

Pending - Waiting for the transaction to be mined in the blockchain

Success - The transaction was mined

Error - There was an error

ottodevs · 2020-01-02T01:13:39Z

src/prop-types.js

  TRANSACTION_STATUS_PENDING,
  TRANSACTION_STATUS_SUCCESS,
-  TRANSACTION_STATUS_UPCOMING,
+  TRANSACTION_STATUS_ERROR,


These array members were alphabetically sorted, is there a good reason to change that sort?

See my answer to your similar question at #1210 (comment)

ottodevs · 2020-01-02T01:14:33Z

src/prop-types.js

  TRANSACTION_STATUS_PENDING,
  TRANSACTION_STATUS_SUCCESS,
-  TRANSACTION_STATUS_UPCOMING,
+  TRANSACTION_STATUS_ERROR,


I don't think this change is relevant for the purpose of the PR, but if there is a good reason for this, I'd love some help to understand it

See my answer to your similar question at #1210 (comment)

ottodevs · 2020-01-02T01:17:56Z

src/onboarding/Deployment/DeploymentStepsItem.js

+  height: ${({ diameter }) => diameter}px;
+  border: 2px solid ${({ theme }) => theme.selected};
+  border-radius: 50%;
+  clip-path: inset(0px ${({ diameter }) => diameter / 2}px 0px 0px);


It is considered a standard convention to remove the unit for zero values:

Suggested change

clip-path: inset(0px ${({ diameter }) => diameter / 2}px 0px 0px);

clip-path: inset(0 ${({ diameter }) => diameter / 2}px 0 0);

Same for lines 235, 241, 242, 252

ottodevs · 2020-01-02T01:20:38Z

src/onboarding/Deployment/DeploymentStepsPanel.js

+          {transactionsStatus.map(
+            ({ name, status, dateStart, gasPrice }, index) => (
+              <DeploymentStepsItem
+                key={index}


It would improve the readability if the props are ordered alphabetically, it is also considered a good convention, feel free to skip this change if you think the oposite way

ottodevs · 2020-01-02T01:23:50Z

src/onboarding/Deployment/DeploymentStepsItem.js

+  )
+
+  useMemo(() => {
+    if (gasPrice === undefined) return


Well, I guess local network is not considered that important in the client, as it is for example tagged as "wrong network" when selected in metamask, so it is probably not worth the effort to reach accurate times there.

ottodevs · 2020-01-02T01:27:00Z

src/onboarding/Deployment/DeploymentStepsItem.js

+      const time = (dateEnd - now) / 1000
+      const unit = time < 60 ? 'sec' : time < 3600 ? 'min' : 'hr'
+      const amount = Math.floor(
+        unit === 'sec' ? time : unit === 'min' ? 1 + time / 60 : 1 + time / 3600
+      )


It seems straightforward by enabling dayjs embedded plugin for that, we were talking about this the other day, but the change seems still pending

ottodevs · 2020-01-02T01:28:06Z

src/onboarding/Create/Create.js

  TRANSACTION_STATUS_PENDING,
  TRANSACTION_STATUS_SUCCESS,
-  TRANSACTION_STATUS_UPCOMING,
+  TRANSACTION_STATUS_ERROR,


It does not seem a big deal, but why breaking the alphabetic sort here?

See my answer to your similar question at #1210 (comment)

e18r · 2020-01-02T15:19:35Z

@ottodevs Thank you for your second feedback. I will address your comments, as well as the DayJS comment from the last review. I'll let you know as soon as I'm done.

Now using the `dayjs` library instead of `date-fns`, both for adding minutes to a date, and for displaying the remaining time.

Removed the units from zeroes in CSS values

Props sorted alphabetically

e18r · 2020-01-02T18:19:41Z

@ottodevs this PR is ready for a further review. Thank you.

Solved a linting error

stale · 2020-07-31T17:42:34Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for contributing to Aragon! 🦅

sohkai · 2020-08-03T10:00:40Z

Hi @e18r @ottodevs, sorry for taking an incredibly long time to review this change.

Given the current gas market and how wildly it fluctuates (as well as the general amount of network congestion), we've decided to leave this responsibility out to wallets.

Sorry again for not being able to merge this sooner, when it would have been much more useful!

vercel bot temporarily deployed to staging December 9, 2019 16:31 Inactive

ottodevs suggested changes Dec 17, 2019

View reviewed changes

e18r force-pushed the onboarding-time-remaining branch from ccdf731 to 994b25c Compare December 17, 2019 15:58

vercel bot temporarily deployed to staging December 17, 2019 15:58 Inactive

e18r force-pushed the onboarding-time-remaining branch from 994b25c to 48d812b Compare December 18, 2019 16:14

vercel bot temporarily deployed to staging December 18, 2019 16:14 Inactive

e18r changed the title ~~Onboarding: add time remaining and progress ring~~ [WIP] Onboarding: add time remaining and progress ring Dec 18, 2019

vercel bot temporarily deployed to staging December 18, 2019 21:26 Inactive

e18r force-pushed the onboarding-time-remaining branch from 0cbfa63 to 34f4d7f Compare December 18, 2019 22:54

vercel bot temporarily deployed to staging December 18, 2019 22:54 Inactive

e18r added 2 commits December 18, 2019 17:55

e18r force-pushed the onboarding-time-remaining branch from 34f4d7f to b795801 Compare December 18, 2019 22:55

vercel bot temporarily deployed to staging December 18, 2019 22:55 Inactive

e18r changed the title ~~[WIP] Onboarding: add time remaining and progress ring~~ Onboarding: add time remaining and progress ring Dec 19, 2019

e18r requested a review from sohkai December 19, 2019 00:01

ottodevs suggested changes Jan 2, 2020

View reviewed changes

e18r added 3 commits January 2, 2020 12:43

Onboarding: add time remaining and progress ring - dayjs

6c2c424

Now using the `dayjs` library instead of `date-fns`, both for adding minutes to a date, and for displaying the remaining time.

Onboarding: add time remaining and progress ring - unitless zeroes

8541cfd

Removed the units from zeroes in CSS values

Onboarding: add time remaining and progress ring - sorted props

48ff9d5

Props sorted alphabetically

vercel bot requested a deployment to Preview January 2, 2020 18:18 Pending

vercel bot deployed to Preview January 2, 2020 18:22 View deployment

Onboarding: add time remaining and progress ring - linting error

1776fa6

Solved a linting error

vercel bot deployed to Preview January 2, 2020 19:24 View deployment

stale bot added the abandoned label Feb 2, 2020

aragon deleted a comment from stale bot Feb 2, 2020

stale bot removed the abandoned label Feb 2, 2020

stale bot added the abandoned label Mar 3, 2020

aragon deleted a comment from stale bot Mar 3, 2020

stale bot removed the abandoned label Mar 3, 2020

stale bot added the abandoned label Apr 2, 2020

sohkai removed the abandoned label Apr 8, 2020

sohkai mentioned this pull request Apr 12, 2020

Onboarding: indicate that the client is waiting for a signature #1064

Open

stale bot added the abandoned label May 12, 2020

aragon deleted a comment from stale bot May 12, 2020

stale bot removed the abandoned label May 12, 2020

aragon deleted a comment from stale bot May 12, 2020

stale bot added the abandoned label Jun 12, 2020

aragon deleted a comment from stale bot Jun 12, 2020

stale bot removed the abandoned label Jun 12, 2020

sohkai requested a review from bpierre July 1, 2020 16:07

stale bot added the abandoned label Jul 31, 2020

sohkai closed this Aug 3, 2020

sohkai deleted the onboarding-time-remaining branch August 3, 2020 10:00

	fetch('https:ethgasstation.info/json/predictTable.json')
	fetch('https://ethgasstation.info/json/predictTable.json')

	clip-path: inset(0px ${({ diameter }) => diameter / 2}px 0px 0px);
	clip-path: inset(0 ${({ diameter }) => diameter / 2}px 0 0);

Onboarding: add time remaining and progress ring #1210

Onboarding: add time remaining and progress ring #1210

Conversation

e18r commented Dec 9, 2019 • edited by sohkai Loading

ottodevs left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e18r commented Dec 18, 2019

ottodevs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e18r Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e18r Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e18r commented Jan 2, 2020

e18r commented Jan 2, 2020

stale bot commented Jul 31, 2020

sohkai commented Aug 3, 2020

e18r commented Dec 9, 2019 •

edited by sohkai

Loading

ottodevs left a comment •

edited

Loading

e18r Jan 2, 2020 •

edited

Loading

e18r Jan 2, 2020 •

edited

Loading