Ledger integration milestone 2

[TobiaszCudnik]

Next milestone for the ledger integration should include:

• merge LedgerStore and LedgerHub
• remove LedgerChannel in favor of LedgerStore.isOpen
• deviceId should be an observable
• when signing a transaction one have to do it twice
  • first one times out, although the ledger UI guides as expected Ledger U2F timeout issue #224 (support page)

[roosmaa]

Please can you include motivation behind those desired changes?

Based on that list alone I'm confident this would be going three steps backwards. So here are some comments regarding the items about why they are as they are.

Store is redundant, hub is enough

The Store, Channel and Hub are there to hide the internals from outside world. No need to give us the chance (in the future) to attempt poking internal things that are not safe to be poked.

use only one transport and channel

The main reason why the Channel abstraction exists is to make UI layer code a lot simpler to deal with. You create a channel for a component and when that component gets destroyed you simply close that channel, you don't have to worry about the pending async tasks as the LedgerHub takes care of disposing of them cleanly.

deviceId should be an observable

But it already is: https://github.com/RiseVision/rise-react-wallet/blob/master/src/stores/ledger.ts#L150 (it propagates through via the getters in Channel and Store).

replace promises with async

I'm pretty confident you cannot replace promises with async and get codebase that's easier digest than the current one. The executioner needs to be able to trigger the start of a Ledger task and inject the ping task into the queue. Without promises that isn't really possible. Currently async is used where promises aren't absolutely necessary.

[TobiaszCudnik]

Please can you include motivation behind those desired changes?

I will provide detailed info once I assign the ticket and start working on it. Overall the implementation seems too complex, not encapsulated enough and ideally we'd depend on some external module(s).

The Store, Channel and Hub are there to hide the internals from outside world.

I'm pretty convinced store & hub can be merged for now.

The main reason why the Channel abstraction exists is to make UI layer code a lot simpler to deal with.

Could be simplified from the consumer perspective.

deviceId should be an observable

But it already is: https://github.com/RiseVision/rise-react-wallet/blob/master/src/stores/ledger.ts#L150 (it propagates through via the getters in Channel and Store).

Not in the store, which is the point of @observables.

this would be going three steps backwards

Theres nothing wrong in going backwards to simplify things, as long as they work equally well.

Also, blocked by #121.

[roosmaa]

Ledger things have undergone a refactor, so closing this issue.

[TobiaszCudnik]

Ledger's refactoring was related to supporting ledger-node-hid module and only partially addressed this ticket. Re opening.

I've updated the todo list in the ticket's description to reflect the current situation.