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

Implement send LSK with Hardware Wallet Trezor Model T - Closes #1845 #1849 #1847 #1949

Merged
merged 6 commits into from Apr 25, 2019

Conversation

Projects
None yet
3 participants
@osvaldovega
Copy link
Contributor

commented Apr 18, 2019

What issue have I solved?

-- #1845
-- #1847
-- #1849

How have I implemented/fixed it?

For this implementation the same code for send LSK, using HW Ledger Nano S worked with the only difference that for the user of Trezor Model T you need to use the Trezor device ID and for Ledger you need to get the device ID from the key.

In summary for make this to work I had to update the TrezorLogin component to properly update the account reducer to provide the device ID, and with this change now I can use the same functionality as before but using the Trezor ID.

How has this been tested?

  1. Run the app with Electron
  2. Open the developers mode and then add to the local storage one more item with key = trezor and value = true, after this refresh the app.
  3. Login using the Trezor model T
  4. Select the Trezor account
  5. Go to Wallet -> Send LSK
  6. Type any LSK address/account and amount
  7. Go to the confirm screen after press the confirm button
  8. Then here you should check the Trezor device for accept the transaction or cancel it
  9. Then the final screen will show the result of the transaction

Review checklist

osvaldovega added some commits Apr 18, 2019

::

@osvaldovega osvaldovega requested a review from michaeltomasik Apr 18, 2019

@osvaldovega osvaldovega self-assigned this Apr 18, 2019

Show resolved Hide resolved src/components/sendV2/summary/summary.js Outdated
Show resolved Hide resolved src/components/sendV2/summary/summary.js Outdated

const util = require('util');

const { ipc } = window;

const handleConnect = () => {
store.dispatch({

This comment has been minimized.

Copy link
@slaweet

slaweet Apr 18, 2019

Member

I know it's not new in this PR, but IMO we shouldn't use import store from '../store';, because it causes lot of problems with unit tests when it gets imported in completely unrelated unit tests.

So far, the redux architecture we used was that if something needs to dispatch an action, it should be a middleware, and utils should be redux-independent.

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Apr 23, 2019

Author Contributor

ok Vit right now I just put the login in the login middleware file so right now it is better

Show resolved Hide resolved src/utils/hwWallet.js Outdated
Show resolved Hide resolved src/utils/hwWallet.js Outdated

osvaldovega added some commits Apr 21, 2019

label: util.format(HW_MSG.TREZOR_DISCONNECTED, data.model, data.label),
}));
});

ipc.on('trezorButtonCallback', (event, data) => {
store.dispatch(infoToastDisplayed({

This comment has been minimized.

Copy link
@slaweet

slaweet Apr 24, 2019

Member

There are still some more store.dispatchs in this util file. IMO it doesn't make sense why are some ipc.ons here and some in the middleware and all ipc.ons should be together in the middleware.

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Apr 24, 2019

Author Contributor

you were right all the ipc messages were being check on this file now I put it in the hwWallet middleware

store.dispatch({
type: actionTypes.settingsUpdated,
data: { isHarwareWalletConnected: true },
});
store.dispatch(errorToastDisplayed({ label: HW_MSG.LEDGER_CONNECTED }));
store.dispatch(errorToastDisplayed({ label: `${model} connected` }));

This comment has been minimized.

Copy link
@slaweet

slaweet Apr 24, 2019

Member

I would expect green (or neutral) toast here. There is nothing "erroneous" about this event

Suggested change
store.dispatch(errorToastDisplayed({ label: `${model} connected` }));
store.dispatch(successToastDisplayed({ label: `${model} connected` }));

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Apr 24, 2019

Author Contributor

my bad I thought we were using this as default toaster for this but you are right that one is only for error.
I fixed it

@@ -17,30 +14,29 @@ const loginMiddleware = store => next => (action) => { // eslint-disable-line ma
data: { isHarwareWalletConnected: false },
});

ipc.on('ledgerConnected', () => {
ipc.on('hwConnected', (event, { model }) => {

This comment has been minimized.

Copy link
@slaweet

slaweet Apr 24, 2019

Member

IMO it doesn't make sense why listening to hwWallet events is in login as it has nothing to do with login. It should be rather in src/store/middlewares/hwWallet.js

This comment has been minimized.

Copy link
@yasharAyari

yasharAyari Apr 24, 2019

Member

I think it should be part of the login process but it doesn't need to be part of middleware and it should happen inside the login action.

This comment has been minimized.

Copy link
@slaweet

slaweet Apr 24, 2019

Member

Listening to hwConnected event cannot be inside login action. We first need to detect/display that the device was connected, and only then the user can log in.

@@ -17,30 +14,29 @@ const loginMiddleware = store => next => (action) => { // eslint-disable-line ma
data: { isHarwareWalletConnected: false },

This comment has been minimized.

Copy link
@yasharAyari

yasharAyari Apr 24, 2019

Member

you don't need to store isHarwareWalletConnected value in the redux store because it doesn't use in any presetional component. So, dispatching settingsUpdated action here is completely unnecessary.

This comment has been minimized.

Copy link
@slaweet

slaweet Apr 24, 2019

Member

Of course, it is used in a presentational component. Specifically here:

<div className={`${styles.hardwareHolder} ${(this.props.settings && this.props.settings.isHarwareWalletConnected) ? styles.show : ''}`}>

This comment has been minimized.

Copy link
@yasharAyari

yasharAyari Apr 25, 2019

Member

still, it doesn't make sense to store this value in the setting reducer.

This comment has been minimized.

Copy link
@slaweet

slaweet Apr 25, 2019

Member

I agree, but it's way beyond the scope of this PR.

osvaldovega added some commits Apr 24, 2019

@slaweet slaweet requested a review from yasharAyari Apr 24, 2019

@osvaldovega osvaldovega merged commit 0425e51 into development Apr 25, 2019

4 checks passed

Jenkins e2e tests e2e tests passed
Details
Jenkins test deployment Commit was deployed to test
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
security/snyk - package.json (LiskHQ) No manifest changes detected

@osvaldovega osvaldovega deleted the 1845-implement-send-LSK-with-trezor-model-t branch Apr 25, 2019

@slaweet slaweet added the ready label Apr 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.