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: send UX enhancements (ready for review) #1766

Merged
merged 8 commits into from
Dec 31, 2018

Conversation

comountainclimber
Copy link
Member

@comountainclimber comountainclimber commented Dec 24, 2018

send-enhancements-3

TODO:

  • Fix bug preventing correct hash from being generated during contract invocation

  • Fix error messaging in Send timeout to direct users to take a look at activity (separate PR)

  • Add number of pending tx on top of Activity logo based on spunky state (separate PR)

What current issue(s) from Trello/Github does this address?
Solves at least part of #1741 and a lot of problems that arise during periods of network instability ie what exactly is the status of my transaction

What problem does this PR solve?
This creates an internal storage log of "pending" transactions that the user has attempted to broadcast to the network. We generate a transaction ID locally before broadcasting to a node and then use this ID to poll the blockchain for confirmations/transaction status. Once the transaction reaches 40 10 confirmations OR the neoscan API responds with this transaction id we purge the id from local storage.

How did you solve this problem?
By generating the transaction locally before making the request to the node and then storing the determined tx id in local storage. UPDATE: It is not currently possible (that I know of) to take the an InvocationTransaction script and reverse engineer it to calculate the exact outputs as an alternative I decided to pass around the send entries array and conditionally use its value instead in the event of the transaction being an invocation.

How did you make sure your solution works?
Manual testing - incoming unit and integration tests. (separate PR)

Are there any special changes in the code that we should be aware of?

  • Adds images to the Activity page 🎉
  • Updates neon-js to 3.11.9  and installs via npm instead of github
  • Sets timeout of RPC requests to 60000MS instead of the default 30000

Is there anything else we should know?
NOTE: for some reason the hashes I am generating for tokens (not GAS or NEO) are not creating the correct hash and I am actively investigating

  • Unit tests written?

@coveralls
Copy link

coveralls commented Dec 24, 2018

Pull Request Test Coverage Report for Build 5581

  • 48 of 148 (32.43%) changed or added relevant lines in 15 files are covered.
  • 62 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-0.5%) to 40.146%

Changes Missing Coverage Covered Lines Changed/Added Lines %
app/components/Blockchain/Transaction/ReceiveAbstract.jsx 4 5 80.0%
app/components/TransactionHistory/TransactionHistoryPanel/Transactions.jsx 2 3 66.67%
app/actions/transactionHistoryActions.js 0 2 0.0%
app/components/Blockchain/Transaction/Transaction.jsx 9 11 81.82%
app/components/Blockchain/Transaction/SendAbstract.jsx 0 3 0.0%
app/components/TransactionHistory/TransactionHistoryPanel/TransactionHistoryPanel.jsx 8 12 66.67%
app/components/Blockchain/Transaction/PendingAbstract.jsx 0 6 0.0%
app/util/findAndReturnTokenInfo.js 2 14 14.29%
app/modules/transactions.js 4 20 20.0%
app/actions/pendingTransactionActions.js 12 65 18.46%
Files with Coverage Reduction New Missed Lines %
app/modules/transactions.js 1 10.11%
app/components/Blockchain/Transaction/Transaction.jsx 1 62.75%
app/containers/Send/Send.jsx 1 54.68%
app/actions/transactionHistoryActions.js 2 5.26%
app/containers/LoginPrivateKey/LoginPrivateKey.jsx 4 44.44%
app/components/Modals/SendModal/ReadCode/ReadCode.jsx 6 0.0%
app/components/Modals/SendModal/SendModal.jsx 6 0.0%
app/components/QrCodeScanner/QrCodeScanner.jsx 41 2.25%
Totals Coverage Status
Change from base Build 5552: -0.5%
Covered Lines: 1065
Relevant Lines: 2427

💛 - Coveralls

@ranbena
Copy link
Contributor

ranbena commented Dec 26, 2018

@comountainclimber I have a few preliminary questions and suggestions:

  1. UX - After confirming a send transaction, the user is prompted "TRANSFER COMPLETED" although it's actually pending. Perhaps update this?

  2. How do you determine a stale transaction?

  3. Possible to merge <PendingTransaction /> into <Transaction /> instead of being entirely separate html implementations?
    I was thinking of splitting <Transaction /> into types <TxRecieve />, <TxClaim />, <TxSend /> and now <TxPending /> utilizing Flow's implements feature perhaps. Wdyt?

  4. Would be great to disconnect these into separate "prep" PRs:

    • util/findAndReturnTokenInfo
    • Add asset image
    • @cityofzion/neon-js

@comountainclimber
Copy link
Member Author

Thanks for taking a look @ranbena in regards to your points:
1.) I want to update all the error messaging in a separate PR as to to not bloat this one any further
2.) A transaction is currently "stale" if the RPC server responds with an error otherwise it was never successfully posted - perhaps this logic needs to be updated 🤷‍♂️
3.) I like this plan and will go ahead and implement
4.) I think at this point it might be more trouble than its worth breaking this one up any further but will go ahead and add any additional feature work to separate PRs:

  • send messaging enhancements / error messaging improvements
  • UI enhancements to activity page
  • Unit and integration tests

@dvd-schwrtz
Copy link

I know you are still working on this but the code looks good so far

@comountainclimber comountainclimber changed the title Feature: better Send UX (work in progress do not merge) Feature: send UX enhancements (ready for review) Dec 30, 2018
@ranbena
Copy link
Contributor

ranbena commented Dec 30, 2018

Super feature @comountainclimber! Mazal Tov 🍾

Copy link
Contributor

@ranbena ranbena left a comment

Choose a reason for hiding this comment

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

Other than a few requested changes - good to go

@comountainclimber
Copy link
Member Author

Thanks for the review @ranbena and @dvd-schwrtz 👍

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.

4 participants