poanetwork / token-wizard

(Discontinued) TokenWizard is an DApp to create and manage crowdsale and token contracts using a simple UI
MIT License
381 stars 216 forks source link

(Fix) Step4 - transaction is generated after each page refreshing #1164

Closed fernandomg closed 5 years ago

fernandomg commented 5 years ago

Closes #1113

Based on what was discussed in the issue, each step of the deployment process was divided into steps, each of which is properly updated and serve for traceability of every tx status.

If a tx is not signed (confirmation pending), and for instance, the user refreshes the page; that tx will be considered as Lost. In this scenario, a modal asking the user to reject/cancel the current pending tx will be displayed.

If the page is refreshed after signing the tx, and the transactionHash was stored (transaction pending of being mined); the wizard will update the tx status based on the information retrieved from the blockchain based on the transactionHash stored.

NOTE: Despite the intention of this PR is to prevent multiple txs being queued, TW cannot prevent the user from cheating the wizard and clicking 'Continue' even if the Lost tx wasn't rejected in the wallet. Thus this situation may lead to having more than one tx pending in the wallet (you can see that behavior being simulated in the last step of the deployment in the video).

This is a video where I tried to generate all possible scenarios to make the deployment process fail: https://drive.google.com/file/d/1653l5Xq3qdTwEFQbfbUrF0Np0J5oRcRX/view

PS: Do not evaluate the styles, I just did structural changes to facilitate the redesign work. Thus, styles will be changed in the redesign process.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 3235


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/utils/alerts.js 0 1 0.0%
src/components/stepFour/utils.js 0 2 0.0%
src/utils/blockchainHelpers.js 4 16 25.0%
src/components/Common/TxProgressStatus.js 0 14 0.0%
src/components/stepFour/index.js 0 33 0.0%
src/stores/DeploymentStore.js 1 42 2.38%
<!-- Total: 11 114 9.65% -->
Files with Coverage Reduction New Missed Lines %
src/stores/DeploymentStore.js 1 6.16%
src/utils/blockchainHelpers.js 3 9.27%
<!-- Total: 4 -->
Totals Coverage Status
Change from base Build 3221: -0.5%
Covered Lines: 892
Relevant Lines: 3949

💛 - Coveralls
BlackDuckCoPilot commented 5 years ago

Black Duck Security Report

Merging #1164 into 2.0 will not change security risk.

Click here to see full report

dennis00010011b commented 5 years ago

If user rejected transaction in a wallet for some reason then there is no way to restore this transaction unless to reload the page. I think there is should be more understandable way to restore transaction. It could be:

  1. In modal add button restore transaction in addition to button skip transaction OR/AND
  2. If user select No in alert Skip transaction then transaction will be constructed again @fernandomg thoughts?
fernandomg commented 5 years ago

@dennis00010011b, good point

I'd rather prefer the idea of adding a button by the skip transaction button.

Will try to implement it.

Thanks!

fernandomg commented 5 years ago

@dennis00010011b Now a button retry transaction will appear next to the skip transaction option.

recover-from-rejection

dennis00010011b commented 5 years ago

@fernandomg App crashed if user attempt to contribute

screen shot 2018-10-07 at 1 27 18 pm

Steps:

  1. Create Minted crowdsale, 1 tier , no whitelist, mincap = 0( I tried with ganache, Sokol, Ropsten)
  2. Open contribution page and buy any valid amount. App is crashed after submitting transaction
fernandomg commented 5 years ago

App crashed if user attempt to contribute

@dennis00010011b, fixed. Thanks

dennis00010011b commented 5 years ago

@fernandomg Crowdsale initialized with incorrect start/end time if 2nd transaction is skipped and retried. Steps:

  1. Start creating crowdsale: Minted, no whitelist , 1 tier. Keep start end time by default
  2. Reject and retry 2nd transaction In Step 4
  3. Finish deployment

    1. Observe start/end time on Publish, Manage, Contribution pages Actual result:
      • time values is incorrect (converted to UTC and set as local time)

    Sokol, 0xac93a5c5414d9dff190094d686b00d6464dcb613

https://www.useloom.com/share/4e6d8da4a28b470abfea1e931b58e629

fernandomg commented 5 years ago

Crowdsale initialized with incorrect start/end time if 2nd transaction is skipped and retried.

@dennis00010011b, tricky thing you found. Where you in GMT -7 by chance? Clearly, there's an offset of +7 in the values, except for the summary in step four, that is +14.

I'll check that tomorrow. Thanks

dennis00010011b commented 5 years ago

Where you in GMT -7 by chance?

@fernandomg Yes, my local machine time was PDT( Pacific Daylight Time) = GMT-7

fernandomg commented 5 years ago

Crowdsale initialized with incorrect start/end time if 2nd transaction is skipped and retried.

@dennis00010011b fixed.

Please take special care on this change review, as I modified part of how the dates are handled.

I didn't find any issue. Dates are being displayed in UTC+0 in the publish screen, and in the downloaded files. Manage screen shows dates in local TZ.

@mariano-aguero Your feedback is really appreciated


The issue was that when a tx is being constructed, a method is called which modified the dates stored in the tierStore... that is not a problem if the tx happens only one time. But in the scenario that Dennis found, if the user retries the tx, the date already modified is modified once again, and it will continue happening for every repeat of a tx construction.