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

[DDW-575] Handle 'transaction is too big' transaction error #1308

Conversation

thedanheller
Copy link
Contributor

@thedanheller thedanheller commented Feb 7, 2019

This PR implements a handle for the 'transaction is too big' error in the "fee calculation:" and "create transaction" actions.

This error happens the user making the transaction has their amount distributed among too many addresses, therefore the transaction needs to use too many input addresses.

Todo:

  • Implement the error handle for the TooBigTransaction error message
  • Final English copy: Amount too big due to wallet fragmentation. Learn more. - @darko-mijic
  • Japanese translation: ウォレットのフラグメンテーションの為、額が大きすぎます。もっと詳しく知る - @nikolaglumac
  • Replace all mentions of Ada with ada - it should NOT be written with a capital A - @daniloprates

Note: The text part Learn more should be a link to:

Screenshot:

screen shot 2019-02-11 at 13 09 09


Review Checklist:

Basics

  • PR is updated to the most recent version of target branch (and there are no conflicts)
  • PR has good description that summarizes all changes and shows some screenshots or animated GIFs of important UI changes
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub
  • Automated tests: All acceptance tests are passing (yarn run test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn run dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn run package / CI builds)
  • There are no flow errors or warnings (yarn run flow:test)
  • There are no lint errors or warnings (yarn run lint)
  • Text changes are proofread and approved (Jane Wild)
  • There are no missing translations (running yarn run manage:translations produces no changes)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (yarn run storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly documented and commented
  • Code is properly typed with flow
  • React components are split-up enough to avoid unnecessary re-rendering
  • Any code that only works in Electron is neatly separated from components

Testing

  • New feature / change is covered by acceptance tests
  • All existing acceptance tests are still up-to-date
  • New feature / change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review:

  • Merge PR
  • Delete source branch
  • Move ticket to done on the Youtrack board

@nikolaglumac
Copy link
Contributor

@daniloprates here's Japanese translation:

Amount too big due to wallet fragmentation. Learn more.
ウォレットのフラグメンテーションの為、額が大きすぎます。もっと詳しく知る

...the last part もっと詳しく知る should be linked.

@thedanheller
Copy link
Contributor Author

@DominikGuzei @MarcusHurney for this task it was implemented a way to add links to a LocalizableError. This is how it was done:

@thedanheller
Copy link
Contributor Author

For the "Replace all mentions of Ada with ada" part, this part should remain with uppercase letters:

image

Copy link
Contributor

@MarcusHurney MarcusHurney left a comment

Choose a reason for hiding this comment

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

@daniloprates
I'd like to suggest moving the FormattedHTMLMessageWithLink.js file from source/renderer/app/utils into source/renderer/app/components/widgets since it's a component.

@DominikGuzei
Copy link
Member

@daniloprates can you tell us an easy way how to test this error?

@thedanheller
Copy link
Contributor Author

source/renderer/app/components/widgets

@MarcusHurney makes total sense. Done.

@thedanheller
Copy link
Contributor Author

thedanheller commented Feb 13, 2019

@daniloprates can you tell us an easy way how to test this error?

Replace the try/catch on calculateTransactionFee and createTransaction by throw new TooBigTransactionError();

@MarcusHurney
Copy link
Contributor

@daniloprates
I pushed a commit fixing the import path that was off after moving the FormattedHTMLMessageWithLink.js component to the /widgets folder.

Outside of that little fix, the error message looks like it is working correctly when I tested it in calculateTransactionFee and createTransaction. The only flaw I found is in the styling of the error message (with a link) when it is rendered in Japanese within the WalletSendConfirmationDialog.js
The text is longer in Kanji than in English, so the link wraps to the next line. Here is a screenshot:

screen shot 2019-02-13 at 10 55 47 pm

In WalletSendForm.js the styling is fine in Japanese 👌:

screen shot 2019-02-13 at 10 57 23 pm

Copy link
Contributor

@MarcusHurney MarcusHurney left a comment

Choose a reason for hiding this comment

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

@daniloprates @DominikGuzei
One more thing, I this PR needs to target the release branch release/0.13.0, which does not exist yet. I am going to create it from develop, and then this PR needs to point at it.

@darko-mijic
Copy link
Contributor

@MarcusHurney, we need DDW-565 completed for 0.13.0. Maybe we should wait while both this PR and that one are merged before creating the release branch.

@DominikGuzei
Copy link
Member

@daniloprates @MarcusHurney im wondering why the error should be shown on the confirmation dialog? Shouldn't the submit button be disabled if the error is shown on the form (and thus the user would never get to the confirmation dialog?)

Copy link
Member

@DominikGuzei DominikGuzei left a comment

Choose a reason for hiding this comment

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

Works great for me! 🎉 (i couldn't reproduce the issue with the confirmation dialog @MarcusHurney has mentioned)

@thedanheller
Copy link
Contributor Author

image

I could reproduce the issue in the calculation part. But there's not much to do, here, from our side. I've tried letter-spacing: -.7px, but it looked awful in English. Maybe ask the Japanese translator a shorter sentence or open a card to improve the error position and accommodate longer messages.

@DominikGuzei DominikGuzei dismissed MarcusHurney’s stale review February 14, 2019 18:54

This PR should target develop so thats fine

@nikolaglumac
Copy link
Contributor

@daniloprates @DominikGuzei @MarcusHurney is this one ready to be merged?

@DominikGuzei
Copy link
Member

@nikolaglumac yes i think so, the PR itself works correctly - the only thing that Danilo did since last week was to fix the ADA renaming stuff … not sure how critical those updates are but it seems like his last three commits have been dedicated to iron out the last issues.

@nikolaglumac
Copy link
Contributor

Thanks @DominikGuzei - I will review it again.

@nikolaglumac nikolaglumac merged commit 2b5e7e7 into develop Feb 18, 2019
@iohk-bors iohk-bors bot deleted the feature/ddw-575-handle-transaction-is-too-big-transaction-error branch February 18, 2019 11:13
@nikolaglumac nikolaglumac mentioned this pull request Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants