Skip to content
This repository has been archived by the owner on Oct 19, 2023. It is now read-only.

feature(component): Show account balance and feedback when wallet balance is too low to create transaction #95

Merged
merged 32 commits into from
Aug 29, 2019
Merged

feature(component): Show account balance and feedback when wallet balance is too low to create transaction #95

merged 32 commits into from
Aug 29, 2019

Conversation

thefiend
Copy link
Contributor

@thefiend thefiend commented Jul 3, 2019 •
edited
Loading

  • Account balance shown in the navigation bar header and refreshes when there are any transactions made in blockchain.

Shows notification when:

  • insufficient account balance when the account does not have enough Ethers to proceed with the transaction
  • user has rejected transaction
  • transaction has successfully been processed

@thefiend thefiend self-assigned this Jul 3, 2019
@Nebulis
Copy link
Contributor

Nebulis commented Jul 5, 2019

any expctation from this ? 😬

@thefiend
Copy link
Contributor Author

thefiend commented Jul 5, 2019 •
edited
Loading

any expctation from this ? 😬

  • Moved the wallet address and network selector to a navigation bar.
  • Added the account balance which gets updated with every new block added into the network.
  • Added validation to check if admin's account balance meets the minimum required amount to create the transaction

This is for issue #58 number 2.

thefiend added 7 commits July 8, 2019 14:33
@thefiend
Merge branch 'fix/up-admin-page-ui' of https://github.com/OpenCerts/a…
602af69 
…dmin-website into feature/show-account-balance

* 'fix/up-admin-page-ui' of https://github.com/OpenCerts/admin-website:
  Fixed changed of name from isValidHash to isValidCertificateHash
  Changed console.error to use logger error instead
  Disabled console logging in eslint
  Removed unused imports
  Fixed input and button tests
  Added tests for isValidCertificateHash function
  Used isEmpty from lodash and show message only when there is message
  Update logger.js
  Removed console.log
  Moved functions to class functions and cleanup code
  Fixed eslint alert for popup window
  Updated Store Deploy Block, Store Issue Block and Store Revoke Block to use new utility validation methods
  Removed unnecessary babel-eslint
  Removed pageLoaderBase
  Moved validators to utils.js and added validation to StoreIssue and StoreRevoke
  Cleanup
  Added store deploy validation
  Imported React in AdminContainer
  Added wallet address not found page

# Conflicts:
#	src/components/AdminContainer.js
@thefiend
Amended max width of admin panel
07a404d
@thefiend
Merge branch 'master' of https://github.com/OpenCerts/admin-website i…
bfae028 
…nto feature/show-account-balance

* 'master' of https://github.com/OpenCerts/admin-website:
  Changed error messages
@thefiend
Fixed navbar mobile responsiveness
10d5f57
@thefiend
Updated to load account balance when a new block is found
21b13e4
@thefiend
Added check if wallet has sufficient balance before allowing transact…
e1b746c 
…ion.
@thefiend
Added toastify notification
5ca0404
@thefiend thefiend changed the title feature(component): Show account balance feature(component): Show account balance and feedback when wallet balance is too low to create transaction] Jul 10, 2019
@thefiend thefiend changed the title feature(component): Show account balance and feedback when wallet balance is too low to create transaction] feature(component): Show account balance and feedback when wallet balance is too low to create transaction Jul 10, 2019
@thefiend thefiend mentioned this pull request Jul 10, 2019
Open
@thefiend thefiend requested a review from yehjxraymond July 16, 2019 03:13
@thefiend
Merge branch 'master' of https://github.com/OpenCerts/admin-website i…
01dcde2 
…nto feature/show-account-balance

* 'master' of https://github.com/OpenCerts/admin-website:
  Revert "Added redirect to home screen if no wallet provider selected"
  Added redirect to home screen if no wallet provider selected
  Fixed wallet message bug
  Added module testing and integration testing into travis

# Conflicts:
#	pages/deploy.js
#	src/components/AdminContainer.js
yehjxraymond
yehjxraymond reviewed Jul 16, 2019
View reviewed changes
src/sagas/admin.js Outdated
) {
toast.error("Transaction was rejected.");
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Problem is this promise will always run resolve...

You can either:

  1. move the resolve into this else statement so that it's not executed AFTER reject (i dont likt this)
  2. swap the position of the if statement on top with the reject and return reject(err) instead so the rest of the code does not execute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@thefiend thefiend requested review from yehjxraymond and Nebulis July 17, 2019 03:32
@thefiend
Merge branch 'master' of https://github.com/OpenCerts/admin-website i…
8db6096 
…nto feature/show-account-balance

* 'master' of https://github.com/OpenCerts/admin-website:
  Removed unused favicon.ico
  Fixed not able to redeploy after rejecting a transaction
  Added favicon and changed page title
  Updates to !networkId
  Fixed linter issues
  Check if networkID null before running getNetworkId
  Removed unused network proptype
  Update network ID when in deploy page
@Nebulis
Copy link
Contributor

Nebulis commented Aug 26, 2019

image

  • it feels nicer if every h3 items is having pl2 on it
    image
  • I'm wondering if h3 elements are really needed (i.e do we need to display Current Account / Current Balance / Waller Provider). It feels like a title could be enough to explain what it is, and values look self identifiable by themself => information sounds not that helpful to me and takes a lot of space
  • the link to the account store address display "click to copy" but it actually redirect to etherscan

Nebulis
Nebulis reviewed Aug 26, 2019
View reviewed changes
src/reducers/admin.js Outdated Show resolved Hide resolved
@Nebulis
Copy link
Contributor

Nebulis commented Aug 26, 2019

The deploying toast is really weird

image

It doesnt wait for the deployment to be done which makes me feel like: "did it work ? what happened ?"

In the end what's the goal with toaster ? there is already a loading indicator on the deploy button ?

Nebulis
Nebulis reviewed Aug 26, 2019
View reviewed changes
src/sagas/admin.js Outdated
@@ -53,17 +81,24 @@ function sendTxWrapper({ txObject, gasPrice, gasLimit, fromAddress }) {
(err, res) => {
// callback passed into eth.contract.send() to get the txhash
if (err) {
reject(err);
if (
err.message ===
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this message comes from ? It looks like a weak comparison (i.e may easily break)

Why not always displaying the toaster toast.error("Transaction was rejected.");?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

err is returned message from Web3 and not all error messages are caused by rejected transactions. So it is necessary to only show the message when the transaction has actually been rejected.

Copy link
Contributor

Choose a reason for hiding this comment

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

does this handle ledger as well ?

Copy link
Contributor

Choose a reason for hiding this comment

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

as per @rjchow 💡

let's use a different check: err.message.includes('User denied transaction signature.')
Some sources
https://ethereum.stackexchange.com/questions/45836/elegant-way-to-map-web3-js-error-messages#comment54609_45848
https://github.com/blocknative/assist/blob/afbf565dda006158f6f1397d8ea645fa6d307eff/src/js/helpers/utilities.js#L134

What about ledger ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and works for ledger too in latest commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment why we are doing this, it's not obvious at all =)

@thefiend
Realigned navigation bar headers and removed accountBalance checking …
ba7538d 
…in front end and directly using the values in redux to check
@thefiend
Copy link
Contributor Author

thefiend commented Aug 26, 2019 •
edited
Loading

image

  • it feels nicer if every h3 items is having pl2 on it
    image
  • I'm wondering if h3 elements are really needed (i.e do we need to display Current Account / Current Balance / Waller Provider). It feels like a title could be enough to explain what it is, and values look self identifiable by themself => information sounds not that helpful to me and takes a lot of space
  • the link to the account store address display "click to copy" but it actually redirect to etherscan

Updated.

Discussed with the rest on point 3 and have come to the conclusion that it is better to have the headers as it might be confusing for some of the users if we do not label them.

@thefiend thefiend requested a review from Nebulis August 29, 2019 02:02
@thefiend thefiend merged commit 68e1e99 into master Aug 29, 2019
@thefiend thefiend deleted the feature/show-account-balance branch August 29, 2019 06:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Nebulis Nebulis Nebulis approved these changes

@sumitnitsurat sumitnitsurat Awaiting requested review from sumitnitsurat

@rjchow rjchow Awaiting requested review from rjchow

@yehjxraymond yehjxraymond Awaiting requested review from yehjxraymond

Assignees

@thefiend thefiend

Labels
enhancement New feature or request Review Me Topic: Frontend/UI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thefiend @Nebulis @yehjxraymond