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-594] Enable sending of native tokens for hardware wallets #2446

Conversation

nikolaglumac
Copy link
Contributor

@nikolaglumac nikolaglumac commented Mar 9, 2021

This PR enables sending of native tokens for hardware wallets.
JIRA ticket

Todos

  • Implement "unformatted amount" view on the send-form "Confirmation" dialog
  • Enable hardware wallet sending on the API level
  • Explicitly order the token bundle entries before passing them to Ledger, making sure to replicate the logic of the wallet backend which uses the canonical ordering @tomislavhoracek

Screenshots

Hardware wallets

Screenshot 2021-03-11 at 09 20 08
Screenshot 2021-03-11 at 09 20 39
Screenshot 2021-03-11 at 09 20 07
Screenshot 2021-03-11 at 09 20 37

Software wallets

Screenshot 2021-03-11 at 09 19 54
Screenshot 2021-03-11 at 09 20 27
Screenshot 2021-03-11 at 09 19 50
Screenshot 2021-03-11 at 09 20 25


Testing Checklist

  • Slack QA thread
  • Test sending native tokens with both Ledger and Trezor wallets on all platforms
  • Test regular transactions without native tokens with both Ledger and Trezor wallets on all platforms.
  • Test delegation with both Ledger and Trezor wallets on all platforms.

Test Scenarios

Scenario 1 - Send ADA + 1 Native token with Hardware Wallet

Given a |Hardware Wallet| is added to Daedalus
And a Shelley wallet is added to Daedalus
And both wallets are synced
And the |Hardware Wallet| balance is >=2
And the |Hardware Wallet| has native tokens
When I access the "Send" screen for the |Hardware Wallet|
And I paste an address from the Shelley wallet on the "Receiver" input
And I insert a valid amount on the "Amount" input
And I add a token to the transaction
And I click on "Next" and confirm the transaction
And I plug, unlock and export the public key on the device
And confirm the transaction on the dialog
Then the transaction is performed
And the transaction can be visible on both wallets
And transaction is labeled "Multiple tokens sent" in my transaction history

Scenario 2 - Send ADA + multiple Native tokens with Hardware Wallet

Given a |Hardware Wallet| is added to Daedalus
And a Shelley wallet is added to Daedalus
And both wallets are synced
And the |Hardware Wallet| balance is >=2
And the |Hardware Wallet| has native tokens
When I access the "Send" screen for the |Hardware Wallet|
And I paste an address from the Shelley wallet on the "Receiver" input
And I insert a valid amount on the "Amount" input
And I add multiple tokens to the transaction
And I click on "Next" and confirm the transaction
And I plug, unlock and export the public key on the device
And confirm the transaction on the dialog
Then the transaction is performed
And the transaction can be visible on both wallets
And transaction is labeled "Multiple tokens sent" in my transaction history

Scenario 3 - Send ADA from Hardware Wallet

Given a |Hardware Wallet| is added to Daedalus
And a Shelley wallet is added to Daedalus
And both wallets are synced
And the |Hardware Wallet| balance is >=2
When I access the "Send" screen for the |Hardware Wallet|
And I paste an address from the Shelley wallet on the "Receiver" input
And I insert a valid amount on the "Amount" input
And I click on "Next" and confirm the transaction
And I plug, unlock and export the public key on the device
And confirm the transaction on the dialog
Then the transaction is performed
And the transaction can be visible on both wallets

Scenario 4 - Delegate Hardware Wallet

Given a |Hardware Wallet| with at least 10 ADA is added
And both Daedalus and the wallet are synced
And the "Delegation Center" screen on "Staking Center" is displayed
When I click on "Delegate" button for the wallet
And I follow the instruction on the wizard to delegate the wallet
And I plug, unlock and export the public key on the device
And confirm the transaction on the dialog
Then the wallet is displayed as delegated to the new stake pool in the epoch number = current epoch + 2

|Hardware Wallet|
Trezor T
Ledger Nano S
Ledger Nano X


Review Checklist

Basics

  • PR has been assigned and has appropriate labels (feature/bug/chore, release-x.x.x)
  • PR is updated to the most recent version of the target branch (and there are no conflicts)
  • PR has a good description that summarizes all changes
  • PR has default-sized Daedalus window screenshots or animated GIFs of important UI changes:
    • In English
    • In Japanese
  • CHANGELOG entry has been added to the top of the appropriate section (Features, Fixes, Chores) and is linked to the correct PR on GitHub
  • Automated tests: All acceptance and unit tests are passing (yarn test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn package / CI builds)
  • There are no flow errors or warnings (yarn flow:test)
  • There are no lint errors or warnings (yarn lint)
  • There are no prettier errors or warnings (yarn prettier:check)
  • There are no missing translations (running yarn manage:translations produces no changes)
  • Text changes are proofread and approved (Jane Wild / Amy Reeve)
  • Japanese text changes are proofread and approved (Junko Oda)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (yarn storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

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

Testing

  • New feature/change is covered by acceptance tests
  • New feature/change is manually tested and approved by QA team
  • 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 the PR
  • Delete the source branch
  • Move the ticket to done column on the YouTrack board
  • Update Slack QA thread by marking it with a green checkmark

Copy link
Contributor

@alexander-rukin alexander-rukin left a comment

Choose a reason for hiding this comment

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

Perfect! As always

@tomislavhoracek
Copy link
Contributor

tomislavhoracek commented Mar 9, 2021

@nikolaglumac Logic for signing and sending NT with hardware wallets is done (except for specific edge case we faced).

@ManusMcCole
Copy link

ManusMcCole commented Mar 10, 2021

Hi @tomislavhoracek. Tested on Testnet build 17053

When I attempt to send all of my balance of testcoin away from both Trezor or Ledger Daedalus goes all white and I get this error on the console see screenshot. Only happens when testcoin balance after transaction is 0

Also on the Trezor device when attempting to send Native Tokens the token comes up as Asset 1 name (Ascii): and on Ledger it shows up as "Asset name is empty"

@tomislavhoracek
Copy link
Contributor

@ManusMcCole I will check now

@ManusMcCole
Copy link

ManusMcCole commented Mar 10, 2021

Hi @tomislavhoracek

There is one scenario I am concerned with Ledger(tested so far only on Mac). If the user decides to press the back button on Daedalus for a transaction(doesnt matter if Native Assets are involved in the transaction) when the confirming the transaction process is underway on the device at any step the transaction confirming dialog stays alive on the Device. On Trezor it goes away immediately. The confirmation dialog stays on the device. Once the user attempted to sign another transaction they must continue confirming the past transaction even though its been cancelled in Daedalus. Once that is done the new transaction confirmation dialog is visible on the device. Once the user confirms the details for the new transaction a transaction confirmation error appears. See video
Points to note.1.There is only ever one hardware wallet device connected to the machine at any one time
2.There is a Ledger, Trezor and 1 software wallet present in Daedalus

Also with James coin it shows up the Asset name with the (Ascii): part also visible.Im not sure if this expected. I thought i would add just incase. See screenshot

@gabriela-ponce
Copy link

gabriela-ponce commented Mar 10, 2021

@tomislavhoracek

Transactions without NT work as expected so far (I struggled with Daedalus not detecting the device, it happened on production too). I got these results after sending NT from Ledger S:

  • 1st try: "An error occurred" after confirming with the device and clicking "Send". The transaction was performed anyway.
    Check the screenshot of the error, of the transaction and the logs.

  • 2nd: Blank screen after confirming with the device and clicking "Send". The transaction was performed anyway.
    Check the screenshot of the error, of the transaction and the logs.

  • 3rd: Loading for over 5 mins and Daedalus froze. The transaction wasn't performed.
    Check this video.

  • Also, the tooltip for "Unformatted amount" looks wrong in both languages. Check screenshot 1 and 2.

  • I reproduced the same issue @miorsufianiohk mentioned on the thread, regarding the asset name. Check this picture.

I'll continue testing transactions with NT and let you know if I reproduce any of these again or if I find something else 👍

@ManusMcCole
Copy link

Hi @tomislavhoracek and @nikolaglumac. The error we discussed on the call for Ledger when a transaction is cancelled is as predicted present in production. I will open up a ticket

@nikolaglumac
Copy link
Contributor Author

@gabriela-ponce your test results are not valid anymore - please use the latest build. Thanks!

@nikolaglumac
Copy link
Contributor Author

ASCII name is now implemented!
The “HEX” value gets copied and you can select and manually copy the ASCII value.

Screenshot 2021-03-10 at 14 37 53
Screenshot 2021-03-10 at 14 37 58

Copy link

@miorsufianiohk miorsufianiohk left a comment

Choose a reason for hiding this comment

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

LGTM on Nano X. Tested on 17061. Great job @nikolaglumac @tomislavhoracek 👍

@gabriela-ponce
Copy link

@nikolaglumac @tomislavhoracek These issues don't reproduce in 17061 👍

The tooltip still is wrong in both languages. Check the new screenshot.

Copy link

@ManusMcCole ManusMcCole left a comment

Choose a reason for hiding this comment

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

Great work @tomislavhoracek 👍 Tested on build 17061 Trezor Model T Testnet

Copy link

@miorsufianiohk miorsufianiohk left a comment

Choose a reason for hiding this comment

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

Looks good on 17065 but the additional right paddings on both SW and HW English translation make the display look a bit odd.
Screenshot 2021-03-10 at 16 50 13

Copy link

@miorsufianiohk miorsufianiohk left a comment

Choose a reason for hiding this comment

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

LGTM on 17068. Great work @nikolaglumac 👍

@nikolaglumac nikolaglumac merged commit da2e98f into develop Mar 11, 2021
@iohk-bors iohk-bors bot deleted the feature/ddw-594-implement-unformatted-value-view-for-native-tokens branch March 11, 2021 09:31
@nikolaglumac nikolaglumac added release-4.0.2-FC3 Daedalus Flight and removed ⏳release-vNext labels Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants