Consensys / truffle-security

MythX smart contract security verification plugin for Truffle Framework
https://mythx.io
124 stars 28 forks source link

In progress bar distinguish initial delay from rest? #95

Closed rocky closed 5 years ago

rocky commented 5 years ago

The progress bars are fantastic Something that might make them even more awesome would be to distinguish the part where we are in the inital delay versus the part after that. One possibility is another color, another is a different symbol than '*'.

This way people can gauge whether and how to adjust the initial delay. Thoughts @tagomaru ?

tagomaru commented 5 years ago

@rocky OK, I will think about it, however I am not clear about initialDelay yet.

According to Improving Polling Response, it is a parameter that armlet waits before attempting its first status pool.

However I could not find whether this parameter affects to it...

https://github.com/ConsenSys/armlet/blob/master/index.js#L118

If the above is false, as a next step, polling seems to start at the below.

https://github.com/ConsenSys/armlet/blob/master/index.js#L128

I expected that it waits until initialDelay time passes before the below step, but there is no wating.

https://github.com/ConsenSys/armlet/blob/master/lib/analysisPoller.js#L177

Could I ask you where and how the initialDelay works as Improving Polling Response says?

rocky commented 5 years ago

We want to encourage good guessing of initial delay. And in IDE's we probably will be using historical data to do so.

The following is from README.md. Does this help?

There are two time parameters, given in milliseconds, that change how quickly a analysis result is reported back:

The initial delay is the minimum amount of time that this library waits before attempting its first status poll.

Note however that if a request has been cached, then results come back immediately and no status polling is needed. (The server caches previous analysis runs; it takes into account the data passed to it, the analysis mode, and the back-end versions of components used to provide the analysis.)

The maximum delay is the maximum amount of time we will wait for an analysis to complete. Note, however, that if the processing has not finished when this timeout is reached, it may still be running on the server side. Therefore when a timeout occurs, you will get back a UUID which can subsequently be used to get status and results.

The closer these two parameters are to the actual time range that is needed by analysis, the faster the response will get reported back after completion on the server end.

tagomaru commented 5 years ago

@rocky

The initial delay is the minimum amount of time that this library waits before attempting its first status poll.

Sorry, but still strange for me.

According to this, do you mean that initialDelay parameter is not for first waiting time itself but for parameter to affect first waiting time? (it also affects all of the waiting times, though)

rocky commented 5 years ago

That code occurs only after we discover the result is not cached and have waited for the initialDelay has been waited for.

Suppose you have 7 polling opportunites that look like this:

!  !    !    !     !       !               !

And you want to minimize the number of polls and the number of delay after a MythX job completes? Suppose based on past history the completion time is in this range:

                          =============

If you fit the two together you would get:

!  !    !    !     !       !               !
                          ============

If instead you added an initial delay (to the polls the white space before the first !) you would get:

                      !  !    !    !     !       !               !
                      ============
tagomaru commented 5 years ago

@rocky Yeah, I understood that, but what i cannot understand is

have waited for the initialDelay has been waited for.

Which code works for this?

tagomaru commented 5 years ago

Accoding to this, there seems to be no wating during initialDealy.

rocky commented 5 years ago

Ah - yes you are correct - that is a bug! there should be an initial delay done in analysisPoller and that is not there.

tagomaru commented 5 years ago

Oh,,, will i fix it?

tagomaru commented 5 years ago

By the way, if this is fixed, the number of polling except fist one should be changed from 10 to 9? The image is like below.

  1. First waiting time is initialDelay.
  2. The rest waiting times are calculated base on this.

I thinks sqrt(385) should be changed to sqrt(285).

rocky commented 5 years ago

Oh,,, will i fix it?

If you would - then please do.

The 385 is for 10 and then 285 is for 9. There is nothing magic about either 9+1 or 10+1. I had thought for analysis in associating maxumum number of polls per analysis request 10 might be an easy number and it looks like right now I have 11. So sure if in that you want to go with 9 and 285, that's better.

tagomaru commented 5 years ago

@rocky OK, thanks for your comment. I will think about it and fix it soon.

tagomaru commented 5 years ago

OK, i will work on it at a later date. Howerver, by this code, it may not make sense if refresh token occurs, since it waits again during intial delay time.

rocky commented 5 years ago

So that's a bug and we should fix. Note however that after the refresh is done there won't be another one for a day. So as a problem, it is not a big one.

tagomaru commented 5 years ago

Yes. anyway after i work on this, users can notice whehter initial delay passed or not, which means users can notice whether the first response status is 'Finished' or not.

nbanmp commented 5 years ago

Closing as this is very low priority. If anyone believes this is worth implementing, please reopen and explain.